[1/3] remoteproc: Add Arm remoteproc driver

Commit Message

Abdellatif El Khlifi March 1, 2024, 4:42 p.m. UTC

From: Abdellatif El Khlifi <[e-mail]>

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 <[e-mail]>
---
 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

Comments

Rob Herring (Arm) March 4, 2024, 6:30 p.m. UTC | #1

On Fri, Mar 01, 2024 at 04:42:25PM +0000, [e-mail] wrote:
> From: Abdellatif El Khlifi <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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 <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +F:	drivers/remoteproc/arm_rproc.c
> +
>  ARM SMC WATCHDOG DRIVER
>  M:	Julius Werner <[e-mail]>
>  R:	Evan Benn <[e-mail]>
> 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"

Too generic of a name. It should say Corstone or Corstone-1000. Here and 
everywhere you use just 'Arm'.

> +	depends on HAS_IOMEM && ARM64

depends on ARM64 || (HAS_IOMEM && COMPILE_TEST)

That gets us wider build coverage. You should check at least x86 
allmodconfig passes.

> +	default n

The default is already n, so drop.

> +	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)
> +
> +	  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 <[e-mail]>

We don't normally put OSO email in here.

> + *
> + * Authors:
> + *   Abdellatif El Khlifi <[e-mail]>

That's recorded in the commit message and by git, so no need to put it 
in the file.

> + */

Mathieu Poirier March 4, 2024, 6:42 p.m. UTC | #2

Good day Abdellatif,

On Fri, Mar 01, 2024 at 04:42:25PM +0000, [e-mail] wrote:
> From: Abdellatif El Khlifi <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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 <[e-mail]>
> +L:	[e-mail]
> +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 <[e-mail]>
>  R:	Evan Benn <[e-mail]>
> 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 <[e-mail]>
> + *
> + * Authors:
> + *   Abdellatif El Khlifi <[e-mail]>
> + */
> +
> +#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 <[e-mail]>");
> -- 
> 2.25.1
>

Abdellatif El Khlifi March 7, 2024, 7:40 p.m. UTC | #3

Hi Mathieu,

> > +	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.

The second condition takes care of the following: exp_ack and  *rst_ack are both 0.
This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
we expect the RST_ACK to be 00 afterwards.

> > +/**
> > + * 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?

Yes, the remote processor has a default FW image already loaded by default.

rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
provided.

Please correct me if I'm wrong.

[1]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/remoteproc/remoteproc_core.c#L1947
[2]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/firmware_loader/main.c#L863

> > +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.

Thank you, I'll take care of that and of all the other comments made.

Cheers,
Abdellatif

Mathieu Poirier March 8, 2024, 4:44 p.m. UTC | #4

On Thu, 7 Mar 2024 at 12:40, Abdellatif El Khlifi
<[e-mail]> wrote:
>
> Hi Mathieu,
>
> > > +   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.
>
> The second condition takes care of the following: exp_ack and  *rst_ack are both 0.
> This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
> we expect the RST_ACK to be 00 afterwards.
>

This is the kind of conditions that definitely deserve documentation.
Please split the conditions in two different if() statements and add a
comment to explain what is going on.

> > > +/**
> > > + * 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?
>
> Yes, the remote processor has a default FW image already loaded by default.
>

That too would have mandated a comment - otherwise people looking at
the code are left wondering, as I did.

> rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
> provided.
>
> Please correct me if I'm wrong.

You are correct, the remoteproc subsystem expects a firmware image to
be provided _and_ loaded into memory.  Providing a dummy image just to
get the remote processor booted is a hack, but simply because the
subsystem isn't tailored to handle this use case.  So I am left
wondering what the plans are for this driver, i.e is this a real
scenario that needs to be addressed or just an initial patchset to get
a foundation for the driver.

In the former case we need to start talking about refactoring the
subsystem so that it properly handles remote processors that don't
need a firmware image.  In the latter case I'd rather see a patchset
where the firmware image is loaded into RAM.

>
> [1]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/remoteproc/remoteproc_core.c#L1947
> [2]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/firmware_loader/main.c#L863
>
> > > +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.
>
> Thank you, I'll take care of that and of all the other comments made.
>
> Cheers,
> Abdellatif

Abdellatif El Khlifi March 11, 2024, 11:44 a.m. UTC | #5

Hi Mathieu,

On Fri, Mar 08, 2024 at 09:44:26AM -0700, Mathieu Poirier wrote:
> On Thu, 7 Mar 2024 at 12:40, Abdellatif El Khlifi
> <[e-mail]> wrote:
> >
> > Hi Mathieu,
> >
> > > > +   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.
> >
> > The second condition takes care of the following: exp_ack and  *rst_ack are both 0.
> > This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
> > we expect the RST_ACK to be 00 afterwards.
> >
> 
> This is the kind of conditions that definitely deserve documentation.
> Please split the conditions in two different if() statements and add a
> comment to explain what is going on.

Thanks, I'll address that.

> 
> > > > +/**
> > > > + * 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?
> >
> > Yes, the remote processor has a default FW image already loaded by default.
> >
> 
> That too would have mandated a comment - otherwise people looking at
> the code are left wondering, as I did.
> 
> > rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
> > provided.
> >
> > Please correct me if I'm wrong.
> 
> You are correct, the remoteproc subsystem expects a firmware image to
> be provided _and_ loaded into memory.  Providing a dummy image just to
> get the remote processor booted is a hack, but simply because the
> subsystem isn't tailored to handle this use case.  So I am left
> wondering what the plans are for this driver, i.e is this a real
> scenario that needs to be addressed or just an initial patchset to get
> a foundation for the driver.
> 
> In the former case we need to start talking about refactoring the
> subsystem so that it properly handles remote processors that don't
> need a firmware image.  In the latter case I'd rather see a patchset
> where the firmware image is loaded into RAM.

This is an initial patchset for allowing to turn on and off the remote processor.
The FW is already loaded before the Corstone-1000 SoC is powered on and this
is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
(emulator).

The plan for the driver is as follows:

Step 1: provide a foundation driver capable of turning the core on/off

Step 2: provide mailbox support for comms

Step 3: provide FW reload capability

Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can share memory with
the remote core.

I'm happy to provide more explanation in the commit log to reflect this status.

Is it OK that we go with step 1 as a foundation please ?

Cheers
Abdellatif

Mathieu Poirier March 12, 2024, 4:29 p.m. UTC | #6

On Mon, 11 Mar 2024 at 05:44, Abdellatif El Khlifi
<[e-mail]> wrote:
>
> Hi Mathieu,
>
> On Fri, Mar 08, 2024 at 09:44:26AM -0700, Mathieu Poirier wrote:
> > On Thu, 7 Mar 2024 at 12:40, Abdellatif El Khlifi
> > <[e-mail]> wrote:
> > >
> > > Hi Mathieu,
> > >
> > > > > +   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.
> > >
> > > The second condition takes care of the following: exp_ack and  *rst_ack are both 0.
> > > This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
> > > we expect the RST_ACK to be 00 afterwards.
> > >
> >
> > This is the kind of conditions that definitely deserve documentation.
> > Please split the conditions in two different if() statements and add a
> > comment to explain what is going on.
>
> Thanks, I'll address that.
>
> >
> > > > > +/**
> > > > > + * 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?
> > >
> > > Yes, the remote processor has a default FW image already loaded by default.
> > >
> >
> > That too would have mandated a comment - otherwise people looking at
> > the code are left wondering, as I did.
> >
> > > rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
> > > provided.
> > >
> > > Please correct me if I'm wrong.
> >
> > You are correct, the remoteproc subsystem expects a firmware image to
> > be provided _and_ loaded into memory.  Providing a dummy image just to
> > get the remote processor booted is a hack, but simply because the
> > subsystem isn't tailored to handle this use case.  So I am left
> > wondering what the plans are for this driver, i.e is this a real
> > scenario that needs to be addressed or just an initial patchset to get
> > a foundation for the driver.
> >
> > In the former case we need to start talking about refactoring the
> > subsystem so that it properly handles remote processors that don't
> > need a firmware image.  In the latter case I'd rather see a patchset
> > where the firmware image is loaded into RAM.
>
> This is an initial patchset for allowing to turn on and off the remote processor.
> The FW is already loaded before the Corstone-1000 SoC is powered on and this
> is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> (emulator).
>

From the above I take it that booting with a preloaded firmware is a
scenario that needs to be supported and not just a temporary stage.

> The plan for the driver is as follows:
>
> Step 1: provide a foundation driver capable of turning the core on/off
>
> Step 2: provide mailbox support for comms
>
> Step 3: provide FW reload capability
>

What happens when a user wants to boot the remote processor with the
firmware provided on the file system rather than the one preloaded
into memory?  Furthermore, how do we account for scenarios where the
remote processor goes from running a firmware image on the file system
to the firmware image loaded by an external entity?  Is this a valid
scenario?

> Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can share memory with
> the remote core.
>
> I'm happy to provide more explanation in the commit log to reflect this status.
>
> Is it OK that we go with step 1 as a foundation please ?
>

First let's clarify all the scenarios that need to be supported.  From
there I will advise on how to proceed and what modifications to the
subsystem's core should be made, if need be.

> Cheers
> Abdellatif

Abdellatif El Khlifi March 12, 2024, 5:32 p.m. UTC | #7

Hi Mathieu,

On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > This is an initial patchset for allowing to turn on and off the remote processor.
> > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > (emulator).
> >
> >From the above I take it that booting with a preloaded firmware is a
> scenario that needs to be supported and not just a temporary stage.

The current status of the Corstone-1000 SoC requires that there is
a preloaded firmware for the external core. Preloading is done externally
either through the FPGA bootloader or the emulator (FVP) before powering
on the SoC.

Corstone-1000 will be upgraded in a way that the A core running Linux is able
to share memory with the remote core and also being able to access the remote
core memory so Linux can copy the firmware to. This HW changes are still
under development.

This is why this patchset is relying on a preloaded firmware. And it's the step 1
of adding remoteproc support for Corstone.

When the HW is ready, we will be able to avoid preloading the firmware
and the user can do the following:

1) Use a default firmware filename stated in the DT (firmware-name property),
that's the one remoteproc subsystem will use initially, load the firmware file
and start the remote core.

2) Then, the user can choose to use another firmware file:

    echo stop >/sys/class/remoteproc/remoteproc0/state
    echo -n new_firmware.elf > /sys/class/remoteproc/remoteproc0/firmware
    echo start >/sys/class/remoteproc/remoteproc0/state

> > The plan for the driver is as follows:
> >
> > Step 1: provide a foundation driver capable of turning the core on/off
> >
> > Step 2: provide mailbox support for comms
> >
> > Step 3: provide FW reload capability
> >
> What happens when a user wants to boot the remote processor with the
> firmware provided on the file system rather than the one preloaded
> into memory?

We will support this scenario when the HW is upgraded and copying the firmware
to the remote core memory becomes possible.

> Furthermore, how do we account for scenarios where the
> remote processor goes from running a firmware image on the file system
> to the firmware image loaded by an external entity?  Is this a valid
> scenario?

No, this scenario won't apply when we get the HW upgrade. No need for an
external entity anymore. The firmware(s) will all be files in the linux filesystem.

> > Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can share memory with
> > the remote core.
> >
> > I'm happy to provide more explanation in the commit log to reflect this status.
> >
> > Is it OK that we go with step 1 as a foundation please ?
> >
> 
> First let's clarify all the scenarios that need to be supported.  From
> there I will advise on how to proceed and what modifications to the
> subsystem's core should be made, if need be.

Thanks, I hope the answers above provide the information needed.

Cheers
Abdellatif

Mathieu Poirier March 13, 2024, 4:25 p.m. UTC | #8

On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> Hi Mathieu,
> 
> On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > (emulator).
> > >
> > >From the above I take it that booting with a preloaded firmware is a
> > scenario that needs to be supported and not just a temporary stage.
> 
> The current status of the Corstone-1000 SoC requires that there is
> a preloaded firmware for the external core. Preloading is done externally
> either through the FPGA bootloader or the emulator (FVP) before powering
> on the SoC.
> 

Ok

> Corstone-1000 will be upgraded in a way that the A core running Linux is able
> to share memory with the remote core and also being able to access the remote
> core memory so Linux can copy the firmware to. This HW changes are still
> This is why this patchset is relying on a preloaded firmware. And it's the step 1
> of adding remoteproc support for Corstone.
>

Ok, so there is a HW problem where A core and M core can't see each other's
memory, preventing the A core from copying the firmware image to the proper
location.

When the HW is fixed, will there be a need to support scenarios where the
firmware image has been preloaded into memory?

> When the HW is ready, we will be able to avoid preloading the firmware
> and the user can do the following:
> 
> 1) Use a default firmware filename stated in the DT (firmware-name property),
> that's the one remoteproc subsystem will use initially, load the firmware file
> and start the remote core.
> 
> 2) Then, the user can choose to use another firmware file:
> 
>     echo stop >/sys/class/remoteproc/remoteproc0/state
>     echo -n new_firmware.elf > /sys/class/remoteproc/remoteproc0/firmware
>     echo start >/sys/class/remoteproc/remoteproc0/state
> 
> > > The plan for the driver is as follows:
> > >
> > > Step 1: provide a foundation driver capable of turning the core on/off
> > >
> > > Step 2: provide mailbox support for comms
> > >
> > > Step 3: provide FW reload capability
> > >
> > What happens when a user wants to boot the remote processor with the
> > firmware provided on the file system rather than the one preloaded
> > into memory?
> 
> We will support this scenario when the HW is upgraded and copying the firmware
> to the remote core memory becomes possible.
> 
> > Furthermore, how do we account for scenarios where the
> > remote processor goes from running a firmware image on the file system
> > to the firmware image loaded by an external entity?  Is this a valid
> > scenario?
> 
> No, this scenario won't apply when we get the HW upgrade. No need for an
> external entity anymore. The firmware(s) will all be files in the linux filesystem.
> 
> > > Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can share memory with
> > > the remote core.
> > >
> > > I'm happy to provide more explanation in the commit log to reflect this status.
> > >
> > > Is it OK that we go with step 1 as a foundation please ?
> > >
> > 
> > First let's clarify all the scenarios that need to be supported.  From
> > there I will advise on how to proceed and what modifications to the
> > subsystem's core should be made, if need be.
> 
> Thanks, I hope the answers above provide the information needed.
> 
> Cheers
> Abdellatif

Abdellatif El Khlifi March 13, 2024, 5:17 p.m. UTC | #9

Hi Mathieu,

On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > Hi Mathieu,
> > 
> > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > (emulator).
> > > >
> > > >From the above I take it that booting with a preloaded firmware is a
> > > scenario that needs to be supported and not just a temporary stage.
> > 
> > The current status of the Corstone-1000 SoC requires that there is
> > a preloaded firmware for the external core. Preloading is done externally
> > either through the FPGA bootloader or the emulator (FVP) before powering
> > on the SoC.
> > 
> 
> Ok
> 
> > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > to share memory with the remote core and also being able to access the remote
> > core memory so Linux can copy the firmware to. This HW changes are still
> > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > of adding remoteproc support for Corstone.
> >
> 
> Ok, so there is a HW problem where A core and M core can't see each other's
> memory, preventing the A core from copying the firmware image to the proper
> location.
> 
> When the HW is fixed, will there be a need to support scenarios where the
> firmware image has been preloaded into memory?

No, this scenario won't apply when we get the HW upgrade. No need for an
external entity anymore. The firmware(s) will all be files in the linux filesystem.

Cheers
Abdellatif

Mathieu Poirier March 14, 2024, 2:52 p.m. UTC | #10

On Wed, Mar 13, 2024 at 05:17:56PM +0000, Abdellatif El Khlifi wrote:
> Hi Mathieu,
> 
> On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> > On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > > Hi Mathieu,
> > > 
> > > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > (emulator).
> > > > >
> > > > >From the above I take it that booting with a preloaded firmware is a
> > > > scenario that needs to be supported and not just a temporary stage.
> > > 
> > > The current status of the Corstone-1000 SoC requires that there is
> > > a preloaded firmware for the external core. Preloading is done externally
> > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > on the SoC.
> > > 
> > 
> > Ok
> > 
> > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > to share memory with the remote core and also being able to access the remote
> > > core memory so Linux can copy the firmware to. This HW changes are still
> > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > of adding remoteproc support for Corstone.
> > >
> > 
> > Ok, so there is a HW problem where A core and M core can't see each other's
> > memory, preventing the A core from copying the firmware image to the proper
> > location.
> > 
> > When the HW is fixed, will there be a need to support scenarios where the
> > firmware image has been preloaded into memory?
> 
> No, this scenario won't apply when we get the HW upgrade. No need for an
> external entity anymore. The firmware(s) will all be files in the linux filesystem.
>

Very well.  I am willing to continue with this driver but it does so little that
I wonder if it wouldn't simply be better to move forward with upstreaming when
the HW is fixed.  The choice is yours. 

Thanks,
Mathieu

Sudeep Holla March 14, 2024, 2:59 p.m. UTC | #11

On Thu, Mar 14, 2024 at 08:52:59AM -0600, Mathieu Poirier wrote:
> On Wed, Mar 13, 2024 at 05:17:56PM +0000, Abdellatif El Khlifi wrote:
> > Hi Mathieu,
> >
> > On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> > > On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > > > Hi Mathieu,
> > > >
> > > > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > (emulator).
> > > > > >
> > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > scenario that needs to be supported and not just a temporary stage.
> > > >
> > > > The current status of the Corstone-1000 SoC requires that there is
> > > > a preloaded firmware for the external core. Preloading is done externally
> > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > on the SoC.
> > > >
> > >
> > > Ok
> > >
> > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > to share memory with the remote core and also being able to access the remote
> > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > of adding remoteproc support for Corstone.
> > > >
> > >
> > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > memory, preventing the A core from copying the firmware image to the proper
> > > location.
> > >
> > > When the HW is fixed, will there be a need to support scenarios where the
> > > firmware image has been preloaded into memory?
> >
> > No, this scenario won't apply when we get the HW upgrade. No need for an
> > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> >
>
> Very well.  I am willing to continue with this driver but it does so little that
> I wonder if it wouldn't simply be better to move forward with upstreaming when
> the HW is fixed.  The choice is yours.
>

I think Robin has raised few points that need clarification. I think it was
done as part of DT binding patch. I share those concerns and I wanted to
reaching to the same concerns by starting the questions I asked on corstone
device tree changes.

--
Regards,
Sudeep

Abdellatif El Khlifi March 14, 2024, 3:16 p.m. UTC | #12

Hi Sudeep,

On Thu, Mar 14, 2024 at 02:59:20PM +0000, Sudeep Holla wrote:
> On Thu, Mar 14, 2024 at 08:52:59AM -0600, Mathieu Poirier wrote:
> > On Wed, Mar 13, 2024 at 05:17:56PM +0000, Abdellatif El Khlifi wrote:
> > > Hi Mathieu,
> > >
> > > On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> > > > On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > > > > Hi Mathieu,
> > > > >
> > > > > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > (emulator).
> > > > > > >
> > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > >
> > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > on the SoC.
> > > > >
> > > >
> > > > Ok
> > > >
> > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > to share memory with the remote core and also being able to access the remote
> > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > of adding remoteproc support for Corstone.
> > > > >
> > > >
> > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > memory, preventing the A core from copying the firmware image to the proper
> > > > location.
> > > >
> > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > firmware image has been preloaded into memory?
> > >
> > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > >
> >
> > Very well.  I am willing to continue with this driver but it does so little that
> > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > the HW is fixed.  The choice is yours.
> >
> 
> I think Robin has raised few points that need clarification. I think it was
> done as part of DT binding patch. I share those concerns and I wanted to
> reaching to the same concerns by starting the questions I asked on corstone
> device tree changes.

Please have a look at my answer to Robin with clarifications [1].

Apart from mapping the register area rather than using the reg property
I'll also add the mboxes property as Krzysztof confirmed.

[1]: https://lore.kernel.org/all/[e-mail]/

Cheers,
Abdellatif

Sudeep Holla March 14, 2024, 3:24 p.m. UTC | #13

On Thu, Mar 14, 2024 at 03:16:53PM +0000, Abdellatif El Khlifi wrote:
> Hi Sudeep,
>
> On Thu, Mar 14, 2024 at 02:59:20PM +0000, Sudeep Holla wrote:
> >
> > I think Robin has raised few points that need clarification. I think it was
> > done as part of DT binding patch. I share those concerns and I wanted to
> > reaching to the same concerns by starting the questions I asked on corstone
> > device tree changes.
>
> Please have a look at my answer to Robin with clarifications [1].
>

Yes I did take a look at the response but am not convinced yet. I have
responded to you email with other details which I want to be explored.
I don't think we need to rush to add the *foundation driver* as you call
before the bindings are defined well.

--
Regards,
Sudeep

Mathieu Poirier March 14, 2024, 4:29 p.m. UTC | #14

On Thu, 14 Mar 2024 at 08:59, Sudeep Holla <[e-mail]> wrote:
>
> On Thu, Mar 14, 2024 at 08:52:59AM -0600, Mathieu Poirier wrote:
> > On Wed, Mar 13, 2024 at 05:17:56PM +0000, Abdellatif El Khlifi wrote:
> > > Hi Mathieu,
> > >
> > > On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> > > > On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > > > > Hi Mathieu,
> > > > >
> > > > > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > (emulator).
> > > > > > >
> > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > >
> > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > on the SoC.
> > > > >
> > > >
> > > > Ok
> > > >
> > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > to share memory with the remote core and also being able to access the remote
> > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > of adding remoteproc support for Corstone.
> > > > >
> > > >
> > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > memory, preventing the A core from copying the firmware image to the proper
> > > > location.
> > > >
> > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > firmware image has been preloaded into memory?
> > >
> > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > >
> >
> > Very well.  I am willing to continue with this driver but it does so little that
> > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > the HW is fixed.  The choice is yours.
> >
>
> I think Robin has raised few points that need clarification. I think it was
> done as part of DT binding patch. I share those concerns and I wanted to
> reaching to the same concerns by starting the questions I asked on corstone
> device tree changes.
>

I also agree with Robin's point of view.  Proceeding with an initial
driver with minimal functionality doesn't preclude having complete
bindings.  But that said and as I pointed out, it might be better to
wait for the HW to be fixed before moving forward.

> --
> Regards,
> Sudeep

Abdellatif El Khlifi March 25, 2024, 5:13 p.m. UTC | #15

Hi Mathieu,

> > > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > > (emulator).
> > > > > > > >
> > > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > > >
> > > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > > on the SoC.
> > > > > >
> > > > >
> > > > > Ok
> > > > >
> > > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > > to share memory with the remote core and also being able to access the remote
> > > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > > of adding remoteproc support for Corstone.
> > > > > >
> > > > >
> > > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > > memory, preventing the A core from copying the firmware image to the proper
> > > > > location.
> > > > >
> > > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > > firmware image has been preloaded into memory?
> > > >
> > > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > > >
> > >
> > > Very well.  I am willing to continue with this driver but it does so little that
> > > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > > the HW is fixed.  The choice is yours.
> > >
> >
> > I think Robin has raised few points that need clarification. I think it was
> > done as part of DT binding patch. I share those concerns and I wanted to
> > reaching to the same concerns by starting the questions I asked on corstone
> > device tree changes.
> >
> 
> I also agree with Robin's point of view.  Proceeding with an initial
> driver with minimal functionality doesn't preclude having complete
> bindings.  But that said and as I pointed out, it might be better to
> wait for the HW to be fixed before moving forward.

We checked with the HW teams. The missing features will be implemented but
this will take time.

The foundation driver as it is right now is still valuable for people wanting to
know how to power control Corstone external systems in a future proof manner
(even in the incomplete state). We prefer to address all the review comments
made so it can be merged. This includes making the DT binding as complete as
possible as you advised. Then, once the HW is ready, I'll implement the comms
and the FW reload part. Is that OK please ?

Cheers,
Abdellatif

Mathieu Poirier March 26, 2024, 2:20 p.m. UTC | #16

On Mon, 25 Mar 2024 at 11:13, Abdellatif El Khlifi
<[e-mail]> wrote:
>
> Hi Mathieu,
>
> > > > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > > > (emulator).
> > > > > > > > >
> > > > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > > > >
> > > > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > > > on the SoC.
> > > > > > >
> > > > > >
> > > > > > Ok
> > > > > >
> > > > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > > > to share memory with the remote core and also being able to access the remote
> > > > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > > > of adding remoteproc support for Corstone.
> > > > > > >
> > > > > >
> > > > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > > > memory, preventing the A core from copying the firmware image to the proper
> > > > > > location.
> > > > > >
> > > > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > > > firmware image has been preloaded into memory?
> > > > >
> > > > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > > > >
> > > >
> > > > Very well.  I am willing to continue with this driver but it does so little that
> > > > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > > > the HW is fixed.  The choice is yours.
> > > >
> > >
> > > I think Robin has raised few points that need clarification. I think it was
> > > done as part of DT binding patch. I share those concerns and I wanted to
> > > reaching to the same concerns by starting the questions I asked on corstone
> > > device tree changes.
> > >
> >
> > I also agree with Robin's point of view.  Proceeding with an initial
> > driver with minimal functionality doesn't preclude having complete
> > bindings.  But that said and as I pointed out, it might be better to
> > wait for the HW to be fixed before moving forward.
>
> We checked with the HW teams. The missing features will be implemented but
> this will take time.
>
> The foundation driver as it is right now is still valuable for people wanting to
> know how to power control Corstone external systems in a future proof manner
> (even in the incomplete state). We prefer to address all the review comments
> made so it can be merged. This includes making the DT binding as complete as
> possible as you advised. Then, once the HW is ready, I'll implement the comms
> and the FW reload part. Is that OK please ?
>

I'm in agreement with that plan as long as we agree the current
preloaded heuristic is temporary and is not a valid long term
scenario.

> Cheers,
> Abdellatif

Abdellatif El Khlifi March 26, 2024, 5:14 p.m. UTC | #17

Hi Mathieu,

> > > > > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > > > > (emulator).
> > > > > > > > > >
> > > > > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > > > > >
> > > > > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > > > > on the SoC.
> > > > > > > >
> > > > > > >
> > > > > > > Ok
> > > > > > >
> > > > > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > > > > to share memory with the remote core and also being able to access the remote
> > > > > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > > > > of adding remoteproc support for Corstone.
> > > > > > > >
> > > > > > >
> > > > > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > > > > memory, preventing the A core from copying the firmware image to the proper
> > > > > > > location.
> > > > > > >
> > > > > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > > > > firmware image has been preloaded into memory?
> > > > > >
> > > > > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > > > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > > > > >
> > > > >
> > > > > Very well.  I am willing to continue with this driver but it does so little that
> > > > > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > > > > the HW is fixed.  The choice is yours.
> > > > >
> > > >
> > > > I think Robin has raised few points that need clarification. I think it was
> > > > done as part of DT binding patch. I share those concerns and I wanted to
> > > > reaching to the same concerns by starting the questions I asked on corstone
> > > > device tree changes.
> > > >
> > >
> > > I also agree with Robin's point of view.  Proceeding with an initial
> > > driver with minimal functionality doesn't preclude having complete
> > > bindings.  But that said and as I pointed out, it might be better to
> > > wait for the HW to be fixed before moving forward.
> >
> > We checked with the HW teams. The missing features will be implemented but
> > this will take time.
> >
> > The foundation driver as it is right now is still valuable for people wanting to
> > know how to power control Corstone external systems in a future proof manner
> > (even in the incomplete state). We prefer to address all the review comments
> > made so it can be merged. This includes making the DT binding as complete as
> > possible as you advised. Then, once the HW is ready, I'll implement the comms
> > and the FW reload part. Is that OK please ?
> >
> 
> I'm in agreement with that plan as long as we agree the current
> preloaded heuristic is temporary and is not a valid long term
> scenario.

Yes, that's the plan, no problem.

Cheers,
Abdellatif

Patch

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 <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	drivers/remoteproc/arm_rproc.c
+
 ARM SMC WATCHDOG DRIVER
 M:	Julius Werner <[e-mail]>
 R:	Evan Benn <[e-mail]>
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)
+
+	  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 <[e-mail]>
+ *
+ * Authors:
+ *   Abdellatif El Khlifi <[e-mail]>
+ */
+
+#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;
+
+/**
+ * struct arm_rproc_dcfg - Arm remote processor configuration
+ * @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)
+
+/**
+ * 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;
+
+	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))
+			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;
+
+	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");
+		return 0;
+	}
+
+	dev_err(dev, "the requested reset has been denied\n");
+	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)
+{
+	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;
+	}
+
+	dev_dbg(dev, "firmware-name: %s\n", fw_name);
+
+	rproc = rproc_alloc(dev, np->name, &arm_rproc_ops, fw_name,
+			    sizeof(*priv));
+	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));
+		goto err_free_rproc;
+	} 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;
+	} else {
+		dev_dbg(dev, "reset-status: %p\n",
+			priv->reset_cfg.state_reg);
+	}
+
+	platform_set_drvdata(pdev, rproc);
+
+	ret = rproc_add(rproc);
+	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");
+	}
+
+	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);
+}
+
+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);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Arm Remote Processor Control Driver");
+MODULE_AUTHOR("Abdellatif El Khlifi <[e-mail]>");