Message-ID: <ZeYWKVpeFm1+4mlT@p14s>
Date: Mon, 4 Mar 2024 11:42:49 -0700
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: abdellatif.elkhlifi@....com
Cc: Bjorn Andersson <andersson@...nel.org>,
	Rob Herring <robh+dt@...nel.org>, Liviu Dudau <liviu.dudau@....com>,
	Sudeep Holla <sudeep.holla@....com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>, Drew.Reed@....com,
	Adam.Johnston@....com, linux-arm-kernel@...ts.infradead.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver

Good day Abdellatif,

On Fri, Mar 01, 2024 at 04:42:25PM +0000, abdellatif.elkhlifi@....com wrote:
> From: Abdellatif El Khlifi <abdellatif.elkhlifi@....com>
> 
> introduce remoteproc support for Arm remote processors
> 
> The supported remote processors are those that come with a reset
> control register and a reset status register. The driver allows to
> switch on or off the remote processor.
> 
> The current use case is Corstone-1000 External System (Cortex-M3).
> 
> The driver can be extended to support other remote processors
> controlled with a reset control and a reset status registers.
> 
> The driver also supports control of multiple remote processors at the
> same time.
> 
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@....com>
> ---
>  MAINTAINERS                    |   6 +
>  drivers/remoteproc/Kconfig     |  18 ++
>  drivers/remoteproc/Makefile    |   1 +
>  drivers/remoteproc/arm_rproc.c | 395 +++++++++++++++++++++++++++++++++
>  4 files changed, 420 insertions(+)
>  create mode 100644 drivers/remoteproc/arm_rproc.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d1052fa6a69..54d6a40feea5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1764,6 +1764,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/interrupt-controller/arm,vic.yaml
>  F:	drivers/irqchip/irq-vic.c
>  
> +ARM REMOTEPROC DRIVER
> +M:	Abdellatif El Khlifi <abdellatif.elkhlifi@....com>
> +L:	linux-remoteproc@...r.kernel.org
> +S:	Maintained
> +F:	drivers/remoteproc/arm_rproc.c
> +

Humm... I'm not sure this is needed for now.  You'll be CC'ed in future postings
anyway if someone changes this drivers.

>  ARM SMC WATCHDOG DRIVER
>  M:	Julius Werner <jwerner@...omium.org>
>  R:	Evan Benn <evanbenn@...omium.org>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 48845dc8fa85..57fbac454a5d 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -365,6 +365,24 @@ config XLNX_R5_REMOTEPROC
>  
>  	  It's safe to say N if not interested in using RPU r5f cores.
>  
> +config ARM_REMOTEPROC
> +	tristate "Arm remoteproc support"
> +	depends on HAS_IOMEM && ARM64
> +	default n
> +	help
> +	  Say y here to support Arm remote processors via the remote
> +	  processor framework.
> +
> +	  The supported processors are those that come with a reset control register
> +	  and a reset status register. The design can be extended to support different
> +	  processors meeting these requirements.
> +	  The driver also supports control of multiple remote cores at the same time.
> +
> +	  Supported remote cores:
> +	      Corstone-1000 External System (Cortex-M3)
> +

Please remove.  The descrition in the Kconfig file should be related to a family
of device and the specific model number found in the driver.  

> +	  It's safe to say N here.
> +
>  endif # REMOTEPROC
>  
>  endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 91314a9b43ce..73126310835b 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -39,3 +39,4 @@ obj-$(CONFIG_STM32_RPROC)		+= stm32_rproc.o
>  obj-$(CONFIG_TI_K3_DSP_REMOTEPROC)	+= ti_k3_dsp_remoteproc.o
>  obj-$(CONFIG_TI_K3_R5_REMOTEPROC)	+= ti_k3_r5_remoteproc.o
>  obj-$(CONFIG_XLNX_R5_REMOTEPROC)	+= xlnx_r5_remoteproc.o
> +obj-$(CONFIG_ARM_REMOTEPROC)		+= arm_rproc.o
> diff --git a/drivers/remoteproc/arm_rproc.c b/drivers/remoteproc/arm_rproc.c
> new file mode 100644
> index 000000000000..6afa78ae7ad3
> --- /dev/null
> +++ b/drivers/remoteproc/arm_rproc.c
> @@ -0,0 +1,395 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Arm Limited and/or its affiliates <open-source-office@....com>
> + *
> + * Authors:
> + *   Abdellatif El Khlifi <abdellatif.elkhlifi@....com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/firmware.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +
> +#include "remoteproc_internal.h"
> +
> +/**
> + * struct arm_rproc_reset_cfg - remote processor reset configuration
> + * @ctrl_reg: address of the control register
> + * @state_reg: address of the reset status register
> + */
> +struct arm_rproc_reset_cfg {
> +	void __iomem *ctrl_reg;
> +	void __iomem *state_reg;
> +};
> +
> +struct arm_rproc;
> +

This is useless, please remove.

> +/**
> + * struct arm_rproc_dcfg - Arm remote processor configuration

Configuration?  This looks more like operations to me.  Please consider
renaming.

> + * @stop: stop callback function
> + * @start: start callback function
> + */
> +struct arm_rproc_dcfg {
> +	int (*stop)(struct rproc *rproc);
> +	int (*start)(struct rproc *rproc);
> +};
> +
> +/**
> + * struct arm_rproc - Arm remote processor instance
> + * @rproc: rproc handler
> + * @core_dcfg: device configuration pointer
> + * @reset_cfg: reset configuration registers
> + */
> +struct arm_rproc {
> +	struct rproc				*rproc;
> +	const struct arm_rproc_dcfg		*core_dcfg;
> +	struct arm_rproc_reset_cfg		reset_cfg;
> +};
> +
> +/* Definitions for Arm Corstone-1000 External System */
> +
> +#define EXTSYS_RST_CTRL_CPUWAIT			BIT(0)
> +#define EXTSYS_RST_CTRL_RST_REQ			BIT(1)
> +
> +#define EXTSYS_RST_ACK_MASK				GENMASK(2, 1)
> +#define EXTSYS_RST_ST_RST_ACK(x)			\
> +				((u8)(FIELD_GET(EXTSYS_RST_ACK_MASK, (x))))
> +
> +#define EXTSYS_RST_ACK_NO_RESET_REQ			(0x0)
> +#define EXTSYS_RST_ACK_NOT_COMPLETE			(0x1)
> +#define EXTSYS_RST_ACK_COMPLETE			(0x2)
> +#define EXTSYS_RST_ACK_RESERVED			(0x3)
> +
> +#define EXTSYS_RST_ACK_POLL_TRIES			(3)
> +#define EXTSYS_RST_ACK_POLL_TIMEOUT			(1000)

On my side most of the values above came out misaligned.

> +
> +/**
> + * arm_rproc_start_cs1000_extsys() - custom start function
> + * @rproc: pointer to the remote processor object
> + *
> + * Start function for Corstone-1000 External System.
> + * Allow the External System core start execute instructions.
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_start_cs1000_extsys(struct rproc *rproc)
> +{
> +	struct arm_rproc *priv = rproc->priv;
> +	u32 ctrl_reg;
> +
> +	/* CPUWAIT signal of the External System is de-asserted */
> +	ctrl_reg = readl(priv->reset_cfg.ctrl_reg);
> +	ctrl_reg &= ~EXTSYS_RST_CTRL_CPUWAIT;
> +	writel(ctrl_reg, priv->reset_cfg.ctrl_reg);
> +
> +	return 0;
> +}
> +
> +/**
> + * arm_rproc_cs1000_extsys_poll_rst_ack() - poll RST_ACK bits
> + * @rproc: pointer to the remote processor object
> + * @exp_ack: expected bits value
> + * @rst_ack: bits value read
> + *
> + * Tries to read RST_ACK bits until the timeout expires.
> + * EXTSYS_RST_ACK_POLL_TRIES tries are made,
> + * every EXTSYS_RST_ACK_POLL_TIMEOUT milliseconds.
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_cs1000_extsys_poll_rst_ack(struct rproc *rproc,
> +						u8 exp_ack, u8 *rst_ack)
> +{
> +	struct arm_rproc *priv = rproc->priv;
> +	struct device *dev = rproc->dev.parent;
> +	u32 state_reg;
> +	int tries = EXTSYS_RST_ACK_POLL_TRIES;
> +	unsigned long timeout;
> +
        struct device *dev = rproc->dev.parent;
	struct arm_rproc *priv = rproc->priv;
	int tries = EXTSYS_RST_ACK_POLL_TRIES;
	unsigned long timeout;
	u32 state_reg;

> +	do {
> +		state_reg = readl(priv->reset_cfg.state_reg);
> +		*rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> +
> +		if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> +			dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> +				*rst_ack);
> +			return -EINVAL;
> +		}
> +
> +		/* expected ACK value read */
> +		if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))

I'm not sure why the second condition in this if() statement is needed.  As far
as I can tell the first condition will trigger and the second one won't be
reached.

> +			return 0;
> +
> +		timeout = msleep_interruptible(EXTSYS_RST_ACK_POLL_TIMEOUT);
> +
> +		if (timeout) {
> +			dev_err(dev, "polling RST_ACK  aborted\n");
> +			return -ECONNABORTED;
> +		}
> +	} while (--tries);
> +
> +	dev_err(dev, "polling RST_ACK timed out\n");
> +
> +	return -ETIMEDOUT;
> +}
> +
> +/**
> + * arm_rproc_stop_cs1000_extsys() - custom stop function
> + * @rproc: pointer to the remote processor object
> + *
> + * Reset all logic within the External System, the core will be in a halt state.
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_stop_cs1000_extsys(struct rproc *rproc)
> +{
> +	struct arm_rproc *priv = rproc->priv;
> +	struct device *dev = rproc->dev.parent;
> +	u32 ctrl_reg;
> +	u8 rst_ack, req_status;
> +	int ret;
> +

	struct device *dev = rproc->dev.parent;
	struct arm_rproc *priv = rproc->priv;
	u8 rst_ack, req_status;
	u32 ctrl_reg;
	int ret;

> +	ctrl_reg = readl(priv->reset_cfg.ctrl_reg);
> +	ctrl_reg |= EXTSYS_RST_CTRL_RST_REQ;
> +	writel(ctrl_reg, priv->reset_cfg.ctrl_reg);
> +
> +	ret = arm_rproc_cs1000_extsys_poll_rst_ack(rproc,
> +						   EXTSYS_RST_ACK_COMPLETE |
> +						   EXTSYS_RST_ACK_NOT_COMPLETE,
> +						   &rst_ack);
> +	if (ret)
> +		return ret;
> +
> +	req_status = rst_ack;
> +
> +	ctrl_reg = readl(priv->reset_cfg.ctrl_reg);
> +	ctrl_reg &= ~EXTSYS_RST_CTRL_RST_REQ;
> +	writel(ctrl_reg, priv->reset_cfg.ctrl_reg);
> +
> +	ret = arm_rproc_cs1000_extsys_poll_rst_ack(rproc, 0, &rst_ack);
> +	if (ret)
> +		return ret;
> +
> +	if (req_status == EXTSYS_RST_ACK_COMPLETE) {
> +		dev_dbg(dev, "the requested reset has been accepted\n");

Please remove

> +		return 0;
> +	}
> +
> +	dev_err(dev, "the requested reset has been denied\n");

There is enough error reporting in arm_rproc_cs1000_extsys_poll_rst_ack(),
please remove.

> +	return -EACCES;
> +}
> +
> +static const struct arm_rproc_dcfg arm_rproc_cfg_corstone1000_extsys = {
> +	.stop          = arm_rproc_stop_cs1000_extsys,
> +	.start         = arm_rproc_start_cs1000_extsys,
> +};
> +
> +/**
> + * arm_rproc_stop() - Stop function for rproc_ops
> + * @rproc: pointer to the remote processor object
> + *
> + * Calls the stop() callback of the remote core
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_stop(struct rproc *rproc)
> +{
> +	struct arm_rproc *priv = rproc->priv;
> +
> +	return priv->core_dcfg->stop(rproc);
> +}
> +
> +/**
> + * arm_rproc_start() - Start function for rproc_ops
> + * @rproc: pointer to the remote processor object
> + *
> + * Calls the start() callback of the remote core
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_start(struct rproc *rproc)
> +{
> +	struct arm_rproc *priv = rproc->priv;
> +
> +	return priv->core_dcfg->start(rproc);
> +}
> +
> +/**
> + * arm_rproc_parse_fw() - Parse firmware function for rproc_ops
> + * @rproc: pointer to the remote processor object
> + * @fw: pointer to the firmware
> + *
> + * Does nothing currently.
> + *
> + * Return:
> + *
> + * 0 for success.
> + */
> +static int arm_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> +{
> +	return 0;
> +}
> +
> +/**
> + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> + * @rproc: pointer to the remote processor object
> + * @fw: pointer to the firmware
> + *
> + * Does nothing currently.
> + *
> + * Return:
> + *
> + * 0 for success.
> + */
> +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> +{

What is the point of doing rproc_of_parse_firmware() if the firmware image is
not loaded to memory?  Does the remote processor have some kind of default ROM
image to run if it doesn't find anything in memory?

> +	return 0;
> +}
> +
> +static const struct rproc_ops arm_rproc_ops = {
> +	.start		= arm_rproc_start,
> +	.stop		= arm_rproc_stop,
> +	.load		= arm_rproc_load,
> +	.parse_fw	= arm_rproc_parse_fw,
> +};
> +
> +/**
> + * arm_rproc_probe() - the platform device probe
> + * @pdev: the platform device
> + *
> + * Read from the device tree the properties needed to setup
> + * the reset and comms for the remote processor.
> + * Also, allocate a rproc device and register it with the remoteproc subsystem.
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_probe(struct platform_device *pdev)
> +{
> +	const struct arm_rproc_dcfg *core_dcfg;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct arm_rproc *priv;
> +	struct rproc *rproc;
> +	const char *fw_name;
> +	int ret;
> +	struct resource *res;
> +
> +	core_dcfg = of_device_get_match_data(dev);
> +	if (!core_dcfg)
> +		return -ENODEV;
> +
> +	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> +	if (ret) {
> +		dev_err(dev,
> +			"can't parse firmware-name from device tree (%pe)\n",
> +			ERR_PTR(ret));
> +		return ret;

Please replace with dev_err_probe().

> +	}
> +
> +	dev_dbg(dev, "firmware-name: %s\n", fw_name);
> +
> +	rproc = rproc_alloc(dev, np->name, &arm_rproc_ops, fw_name,
> +			    sizeof(*priv));

Using devm_rproc_alloc() would make this driver even more simple.

> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	priv = rproc->priv;
> +	priv->rproc = rproc;
> +	priv->core_dcfg = core_dcfg;
> +
> +	res = platform_get_resource_byname(pdev,
> +					   IORESOURCE_MEM, "reset-control");
> +	priv->reset_cfg.ctrl_reg = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->reset_cfg.ctrl_reg)) {
> +		ret = PTR_ERR(priv->reset_cfg.ctrl_reg);
> +		dev_err(dev,
> +			"can't map the reset-control register (%pe)\n",
> +			ERR_PTR((unsigned long)priv->reset_cfg.ctrl_reg));

dev_err_probe()

> +		goto err_free_rproc;

This isn't needed after moving to devm_rproc_alloc().

> +	} else {
> +		dev_dbg(dev, "reset-control: %p\n", priv->reset_cfg.ctrl_reg);
> +	}
> +
> +	res = platform_get_resource_byname(pdev,
> +					   IORESOURCE_MEM, "reset-status");
> +	priv->reset_cfg.state_reg = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->reset_cfg.state_reg)) {
> +		ret = PTR_ERR(priv->reset_cfg.state_reg);
> +		dev_err(dev,
> +			"can't map the reset-status register (%pe)\n",
> +			ERR_PTR((unsigned long)priv->reset_cfg.state_reg));
> +		goto err_free_rproc;

Same comments as above.

> +	} else {
> +		dev_dbg(dev, "reset-status: %p\n",
> +			priv->reset_cfg.state_reg);

This isn't adding anything valuable, please remove.

> +	}
> +
> +	platform_set_drvdata(pdev, rproc);
> +
> +	ret = rproc_add(rproc);

devm_rproc_add()

> +	if (ret) {
> +		dev_err(dev, "can't add remote processor (%pe)\n",
> +			ERR_PTR(ret));
> +		goto err_free_rproc;
> +	} else {
> +		dev_dbg(dev, "remote processor added\n");

Please remove.

> +	}
> +
> +	return 0;
> +
> +err_free_rproc:
> +	rproc_free(rproc);
> +
> +	return ret;
> +}
> +
> +/**
> + * arm_rproc_remove() - the platform device remove
> + * @pdev: the platform device
> + *
> + * Delete and free the resources used.
> + */
> +static void arm_rproc_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +
> +	rproc_del(rproc);
> +	rproc_free(rproc);
> +}

This shouldn't be needed anymore after using devm_rproc_alloc() and
devm_rproc_add().

> +
> +static const struct of_device_id arm_rproc_of_match[] = {
> +	{ .compatible = "arm,corstone1000-extsys", .data = &arm_rproc_cfg_corstone1000_extsys },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, arm_rproc_of_match);
> +
> +static struct platform_driver arm_rproc_driver = {
> +	.probe = arm_rproc_probe,
> +	.remove_new = arm_rproc_remove,
> +	.driver = {
> +		.name = "arm-rproc",
> +		.of_match_table = arm_rproc_of_match,
> +	},
> +};
> +module_platform_driver(arm_rproc_driver);
> +

I am echoing Krzysztof view about how generic this driver name is.  This has to
be related to a family of processors or be made less generic in some way.  Have
a look at what TI did for their K3 lineup [1] - I would like to see the same
thing done here.

Thanks,
Mathieu

[1]. https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/ti_k3_dsp_remoteproc.c#L898


> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Arm Remote Processor Control Driver");
> +MODULE_AUTHOR("Abdellatif El Khlifi <abdellatif.elkhlifi@....com>");
> -- 
> 2.25.1
> 

Powered by blists - more mailing lists