Re: [PATCH v3 2/2] remoteproc: add AMD BRAM-based remote processor driver

Re: [PATCH v3 2/2] remoteproc: add AMD BRAM-based remote processor driver

[Mathieu Poirier]

Good morning,

On Tue, Apr 28, 2026 at 07:26:33AM -0700, Ben Levinsky wrote:
> Add a remoteproc driver for AMD soft-core processor subsystems
> instantiated in programmable logic and using dual-port BRAM for
> firmware storage and execution.
> 
> The driver parses the firmware memory window from the remoteproc device
> node's reg property, interprets that address and size in the
> processor-local address space, and then uses standard devicetree
> address translation through the parent bus ranges property to obtain
> the corresponding Linux-visible system physical address.
> 
> The resulting translated region is registered as the executable
> remoteproc carveout and coredump segment.
> 
> The processor is controlled through an active-low reset GPIO and a
> subsystem clock. The clock is enabled before reset is released, and the
> processor is kept in reset until firmware loading completes.
> 
> The firmware-name property is optional, allowing firmware to be
> assigned later through the remoteproc framework. Firmware images
> without a resource table are also accepted.
> 
> Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
> ---
>  MAINTAINERS                         |   7 +
>  drivers/remoteproc/Kconfig          |  14 ++
>  drivers/remoteproc/Makefile         |   1 +
>  drivers/remoteproc/amd_bram_rproc.c | 243 ++++++++++++++++++++++++++++
>  4 files changed, 265 insertions(+)
>  create mode 100644 drivers/remoteproc/amd_bram_rproc.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c871acf2179c..172539971950 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1037,6 +1037,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
>  F:	drivers/w1/masters/amd_axi_w1.c
>  
> +AMD BRAM REMOTEPROC DRIVER
> +M:	Ben Levinsky <ben.levinsky@amd.com>
> +L:	linux-remoteproc@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml
> +F:	drivers/remoteproc/amd_bram_rproc.c
> +

There is no real advantage in adding this entry, checkpatch.pl should be
sufficient.

>  AMD CDX BUS DRIVER
>  M:	Nipun Gupta <nipun.gupta@amd.com>
>  M:	Nikhil Agarwal <nikhil.agarwal@amd.com>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index ee54436fea5a..9a2a887ede8a 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -23,6 +23,20 @@ config REMOTEPROC_CDEV
>  
>  	  It's safe to say N if you don't want to use this interface.
>  
> +config AMD_BRAM_REMOTEPROC
> +	tristate "AMD BRAM-based remoteproc support"
> +	depends on OF && COMMON_CLK && (GPIOLIB || COMPILE_TEST)
> +	help
> +	  Say y or m here to support a BRAM-based remote processor managed
> +	  through the remoteproc framework.
> +
> +	  This driver matches designs where executable firmware memory is
> +	  described in the BRAM-local address space and translated to
> +	  the system physical address space with standard devicetree address
> +	  translation.

Not sure how this paragraph helps decide whether the driver should be enabled or
not.  Please remove.

> +
> +	  If unsure, say N.
> +
>  config IMX_REMOTEPROC
>  	tristate "i.MX remoteproc support"
>  	depends on ARCH_MXC
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 1c7598b8475d..5c39664b50c3 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -11,6 +11,7 @@ remoteproc-y				+= remoteproc_sysfs.o
>  remoteproc-y				+= remoteproc_virtio.o
>  remoteproc-y				+= remoteproc_elf_loader.o
>  obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
> +obj-$(CONFIG_AMD_BRAM_REMOTEPROC)	+= amd_bram_rproc.o
>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>  obj-$(CONFIG_IMX_DSP_REMOTEPROC)	+= imx_dsp_rproc.o
>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
> diff --git a/drivers/remoteproc/amd_bram_rproc.c b/drivers/remoteproc/amd_bram_rproc.c
> new file mode 100644
> index 000000000000..9383964b6046
> --- /dev/null
> +++ b/drivers/remoteproc/amd_bram_rproc.c
> @@ -0,0 +1,243 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD BRAM-based Remote Processor driver
> + *
> + * Copyright (C) 2026 Advanced Micro Devices, Inc.
> + *
> + * This driver supports soft-core processors (MicroBlaze, MicroBlaze-V, or
> + * similar) instantiated in AMD programmable logic, using dual-port BRAM
> + * for firmware storage and execution.
> + *
> + * The firmware memory (BRAM) is described in the processor-local address
> + * space and translated to the Linux-visible system physical address with
> + * standard devicetree address translation.
> + *
> + * Reset is controlled via GPIO connected to Processor System Reset IP.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +
> +#include "remoteproc_internal.h"
> +
> +/**
> + * struct amd_bram_rproc - AMD BRAM-based remoteproc private data
> + * @dev: device pointer
> + * @reset: GPIO descriptor for reset control (active-low)
> + * @clk: processor clock
> + */
> +struct amd_bram_rproc {
> +	struct device *dev;
> +	struct gpio_desc *reset;
> +	struct clk *clk;
> +};
> +
> +static int amd_bram_rproc_mem_map(struct rproc *rproc,
> +				  struct rproc_mem_entry *mem)
> +{
> +	void __iomem *va;
> +
> +	va = ioremap_wc(mem->dma, mem->len);
> +	if (!va)
> +		return -ENOMEM;
> +
> +	mem->va = (__force void *)va;
> +	mem->is_iomem = true;
> +
> +	return 0;
> +}
> +
> +static int amd_bram_rproc_mem_unmap(struct rproc *rproc,
> +				    struct rproc_mem_entry *mem)
> +{
> +	iounmap((void __iomem *)mem->va);
> +
> +	return 0;
> +}

The above 2 are identical to what is found in xlnx_r5_remoteproc.c.  Please
coordinate with Tanmay to split that into common code that can be reused by both
drivers.

> +
> +static int amd_bram_rproc_prepare(struct rproc *rproc)
> +{
> +	struct amd_bram_rproc *priv = rproc->priv;
> +	struct rproc_mem_entry *mem;
> +	struct resource res;
> +	u64 da, size;
> +	int ret;
> +
> +	ret = of_property_read_reg(priv->dev->of_node, 0, &da, &size);
> +	if (ret) {
> +		dev_err(priv->dev, "failed to parse executable memory reg\n");
> +		return ret;
> +	}
> +
> +	if (!size || size > U32_MAX) {
> +		dev_err(priv->dev, "invalid executable memory size\n");
> +		return -EINVAL;
> +	}
> +
> +	if (da > U32_MAX) {
> +		dev_err(priv->dev, "invalid executable memory address\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_address_to_resource(priv->dev->of_node, 0, &res);
> +	if (ret) {
> +		dev_err(priv->dev, "failed to translate executable memory reg\n");
> +		return ret;
> +	}
> +
> +	mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)res.start,
> +				   (size_t)size, da,
> +				   amd_bram_rproc_mem_map,
> +				   amd_bram_rproc_mem_unmap,
> +				   dev_name(priv->dev));
> +	if (!mem)
> +		return -ENOMEM;
> +
> +	rproc_add_carveout(rproc, mem);
> +	rproc_coredump_add_segment(rproc, da, (size_t)size);

I'm pretty sure you want @res.start instead of @da, and resource_size(&res)
instead of @size.

> +
> +	return 0;
> +}
> +
> +static int amd_bram_rproc_start(struct rproc *rproc)
> +{
> +	struct amd_bram_rproc *priv = rproc->priv;
> +	int ret;
> +
> +	/* Enable clock before releasing reset */
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		dev_err(priv->dev, "failed to enable clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Deassert reset and let the processor run. */
> +	ret = gpiod_set_value_cansleep(priv->reset, 0);
> +	if (ret) {
> +		dev_err(priv->dev, "failed to deassert reset: %d\n", ret);
> +		clk_disable_unprepare(priv->clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int amd_bram_rproc_stop(struct rproc *rproc)
> +{
> +	struct amd_bram_rproc *priv = rproc->priv;
> +	int ret;
> +
> +	/* Assert reset before disabling the processor clock. */
> +	ret = gpiod_set_value_cansleep(priv->reset, 1);
> +	if (ret) {
> +		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Disable clock after asserting reset */
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int amd_bram_rproc_parse_fw(struct rproc *rproc,
> +				   const struct firmware *fw)
> +{
> +	int ret;
> +
> +	ret = rproc_elf_load_rsc_table(rproc, fw);
> +	if (ret == -EINVAL) {
> +		dev_dbg(&rproc->dev, "no resource table found\n");
> +		return 0;
> +	}
> +
> +	return ret;
> +}

This too should go in common code or simply replaced by
rproc_elf_load_rsc_table() in @amd_bram_rproc_ops - the choice is yours.

Thanks,
Mathieu

> +
> +static const struct rproc_ops amd_bram_rproc_ops = {
> +	.prepare	= amd_bram_rproc_prepare,
> +	.start		= amd_bram_rproc_start,
> +	.stop		= amd_bram_rproc_stop,
> +	.load		= rproc_elf_load_segments,
> +	.sanity_check	= rproc_elf_sanity_check,
> +	.get_boot_addr	= rproc_elf_get_boot_addr,
> +	.parse_fw	= amd_bram_rproc_parse_fw,
> +};
> +
> +static int amd_bram_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct amd_bram_rproc *priv;
> +	const char *fw_name = NULL;
> +	struct rproc *rproc;
> +	int ret;
> +
> +	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> +	if (ret < 0 && ret != -EINVAL)
> +		return dev_err_probe(dev, ret,
> +				     "failed to parse firmware-name property\n");
> +
> +	rproc = devm_rproc_alloc(dev, dev_name(dev), &amd_bram_rproc_ops,
> +				 fw_name, sizeof(*priv));
> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	priv = rproc->priv;
> +	priv->dev = dev;
> +
> +	/* Get the processor clock */
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "failed to get clock\n");
> +
> +	/*
> +	 * Keep the processor in reset until remoteproc has finished loading
> +	 * firmware into the executable memory window described by reg and
> +	 * translated through the parent bus ranges property.
> +	 */
> +	priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset))
> +		return dev_err_probe(dev, PTR_ERR(priv->reset),
> +				     "failed to get reset gpio\n");
> +
> +	rproc->auto_boot = false;
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set DMA mask\n");
> +
> +	platform_set_drvdata(pdev, rproc);
> +
> +	ret = devm_rproc_add(dev, rproc);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register rproc\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id amd_bram_rproc_of_match[] = {
> +	{ .compatible = "xlnx,zynqmp-bram-rproc" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, amd_bram_rproc_of_match);
> +
> +static struct platform_driver amd_bram_rproc_driver = {
> +	.probe = amd_bram_rproc_probe,
> +	.driver = {
> +		.name = "amd-bram-rproc",
> +		.of_match_table = amd_bram_rproc_of_match,
> +	},
> +};
> +module_platform_driver(amd_bram_rproc_driver);
> +
> +MODULE_DESCRIPTION("AMD BRAM-based Remote Processor driver");
> +MODULE_AUTHOR("Ben Levinsky <ben.levinsky@amd.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

Re: [PATCH v3 2/2] remoteproc: add AMD BRAM-based remote processor driver

[Ben Levinsky]

Hi Mathieu,


On 5/8/26 8:47 AM, Mathieu Poirier wrote:
> Good morning,
> 
> On Tue, Apr 28, 2026 at 07:26:33AM -0700, Ben Levinsky wrote:
>> Add a remoteproc driver for AMD soft-core processor subsystems
>> instantiated in programmable logic and using dual-port BRAM for
>> firmware storage and execution.
>>
>> The driver parses the firmware memory window from the remoteproc device
>> node's reg property, interprets that address and size in the
>> processor-local address space, and then uses standard devicetree
>> address translation through the parent bus ranges property to obtain
>> the corresponding Linux-visible system physical address.
>>
>> The resulting translated region is registered as the executable
>> remoteproc carveout and coredump segment.
>>
>> The processor is controlled through an active-low reset GPIO and a
>> subsystem clock. The clock is enabled before reset is released, and the
>> processor is kept in reset until firmware loading completes.
>>
>> The firmware-name property is optional, allowing firmware to be
>> assigned later through the remoteproc framework. Firmware images
>> without a resource table are also accepted.
>>
>> Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
>> ---
>>  MAINTAINERS                         |   7 +
>>  drivers/remoteproc/Kconfig          |  14 ++
>>  drivers/remoteproc/Makefile         |   1 +
>>  drivers/remoteproc/amd_bram_rproc.c | 243 ++++++++++++++++++++++++++++
>>  4 files changed, 265 insertions(+)
>>  create mode 100644 drivers/remoteproc/amd_bram_rproc.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c871acf2179c..172539971950 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1037,6 +1037,13 @@ S:	Maintained
>>  F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
>>  F:	drivers/w1/masters/amd_axi_w1.c
>>  
>> +AMD BRAM REMOTEPROC DRIVER
>> +M:	Ben Levinsky <ben.levinsky@amd.com>
>> +L:	linux-remoteproc@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml
>> +F:	drivers/remoteproc/amd_bram_rproc.c
>> +
> 
> There is no real advantage in adding this entry, checkpatch.pl should be
> sufficient.
> 
>>  AMD CDX BUS DRIVER
>>  M:	Nipun Gupta <nipun.gupta@amd.com>
>>  M:	Nikhil Agarwal <nikhil.agarwal@amd.com>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index ee54436fea5a..9a2a887ede8a 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -23,6 +23,20 @@ config REMOTEPROC_CDEV
>>  
>>  	  It's safe to say N if you don't want to use this interface.
>>  
>> +config AMD_BRAM_REMOTEPROC
>> +	tristate "AMD BRAM-based remoteproc support"
>> +	depends on OF && COMMON_CLK && (GPIOLIB || COMPILE_TEST)
>> +	help
>> +	  Say y or m here to support a BRAM-based remote processor managed
>> +	  through the remoteproc framework.
>> +
>> +	  This driver matches designs where executable firmware memory is
>> +	  described in the BRAM-local address space and translated to
>> +	  the system physical address space with standard devicetree address
>> +	  translation.
> 
> Not sure how this paragraph helps decide whether the driver should be enabled or
> not.  Please remove.
> 
>> +
>> +	  If unsure, say N.
>> +
>>  config IMX_REMOTEPROC
>>  	tristate "i.MX remoteproc support"
>>  	depends on ARCH_MXC
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index 1c7598b8475d..5c39664b50c3 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -11,6 +11,7 @@ remoteproc-y				+= remoteproc_sysfs.o
>>  remoteproc-y				+= remoteproc_virtio.o
>>  remoteproc-y				+= remoteproc_elf_loader.o
>>  obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
>> +obj-$(CONFIG_AMD_BRAM_REMOTEPROC)	+= amd_bram_rproc.o
>>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>>  obj-$(CONFIG_IMX_DSP_REMOTEPROC)	+= imx_dsp_rproc.o
>>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
>> diff --git a/drivers/remoteproc/amd_bram_rproc.c b/drivers/remoteproc/amd_bram_rproc.c
>> new file mode 100644
>> index 000000000000..9383964b6046
>> --- /dev/null
>> +++ b/drivers/remoteproc/amd_bram_rproc.c
>> @@ -0,0 +1,243 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD BRAM-based Remote Processor driver
>> + *
>> + * Copyright (C) 2026 Advanced Micro Devices, Inc.
>> + *
>> + * This driver supports soft-core processors (MicroBlaze, MicroBlaze-V, or
>> + * similar) instantiated in AMD programmable logic, using dual-port BRAM
>> + * for firmware storage and execution.
>> + *
>> + * The firmware memory (BRAM) is described in the processor-local address
>> + * space and translated to the Linux-visible system physical address with
>> + * standard devicetree address translation.
>> + *
>> + * Reset is controlled via GPIO connected to Processor System Reset IP.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +
>> +#include "remoteproc_internal.h"
>> +
>> +/**
>> + * struct amd_bram_rproc - AMD BRAM-based remoteproc private data
>> + * @dev: device pointer
>> + * @reset: GPIO descriptor for reset control (active-low)
>> + * @clk: processor clock
>> + */
>> +struct amd_bram_rproc {
>> +	struct device *dev;
>> +	struct gpio_desc *reset;
>> +	struct clk *clk;
>> +};
>> +
>> +static int amd_bram_rproc_mem_map(struct rproc *rproc,
>> +				  struct rproc_mem_entry *mem)
>> +{
>> +	void __iomem *va;
>> +
>> +	va = ioremap_wc(mem->dma, mem->len);
>> +	if (!va)
>> +		return -ENOMEM;
>> +
>> +	mem->va = (__force void *)va;
>> +	mem->is_iomem = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int amd_bram_rproc_mem_unmap(struct rproc *rproc,
>> +				    struct rproc_mem_entry *mem)
>> +{
>> +	iounmap((void __iomem *)mem->va);
>> +
>> +	return 0;
>> +}
> 
> The above 2 are identical to what is found in xlnx_r5_remoteproc.c.  Please
> coordinate with Tanmay to split that into common code that can be reused by both
> drivers.
> 
>> +
>> +static int amd_bram_rproc_prepare(struct rproc *rproc)
>> +{
>> +	struct amd_bram_rproc *priv = rproc->priv;
>> +	struct rproc_mem_entry *mem;
>> +	struct resource res;
>> +	u64 da, size;
>> +	int ret;
>> +
>> +	ret = of_property_read_reg(priv->dev->of_node, 0, &da, &size);
>> +	if (ret) {
>> +		dev_err(priv->dev, "failed to parse executable memory reg\n");
>> +		return ret;
>> +	}
>> +
>> +	if (!size || size > U32_MAX) {
>> +		dev_err(priv->dev, "invalid executable memory size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (da > U32_MAX) {
>> +		dev_err(priv->dev, "invalid executable memory address\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = of_address_to_resource(priv->dev->of_node, 0, &res);
>> +	if (ret) {
>> +		dev_err(priv->dev, "failed to translate executable memory reg\n");
>> +		return ret;
>> +	}
>> +
>> +	mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)res.start,
>> +				   (size_t)size, da,
>> +				   amd_bram_rproc_mem_map,
>> +				   amd_bram_rproc_mem_unmap,
>> +				   dev_name(priv->dev));
>> +	if (!mem)
>> +		return -ENOMEM;
>> +
>> +	rproc_add_carveout(rproc, mem);
>> +	rproc_coredump_add_segment(rproc, da, (size_t)size);
> 
> I'm pretty sure you want @res.start instead of @da, and resource_size(&res)
> instead of @size.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int amd_bram_rproc_start(struct rproc *rproc)
>> +{
>> +	struct amd_bram_rproc *priv = rproc->priv;
>> +	int ret;
>> +
>> +	/* Enable clock before releasing reset */
>> +	ret = clk_prepare_enable(priv->clk);
>> +	if (ret) {
>> +		dev_err(priv->dev, "failed to enable clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Deassert reset and let the processor run. */
>> +	ret = gpiod_set_value_cansleep(priv->reset, 0);
>> +	if (ret) {
>> +		dev_err(priv->dev, "failed to deassert reset: %d\n", ret);
>> +		clk_disable_unprepare(priv->clk);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int amd_bram_rproc_stop(struct rproc *rproc)
>> +{
>> +	struct amd_bram_rproc *priv = rproc->priv;
>> +	int ret;
>> +
>> +	/* Assert reset before disabling the processor clock. */
>> +	ret = gpiod_set_value_cansleep(priv->reset, 1);
>> +	if (ret) {
>> +		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Disable clock after asserting reset */
>> +	clk_disable_unprepare(priv->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int amd_bram_rproc_parse_fw(struct rproc *rproc,
>> +				   const struct firmware *fw)
>> +{
>> +	int ret;
>> +
>> +	ret = rproc_elf_load_rsc_table(rproc, fw);
>> +	if (ret == -EINVAL) {
>> +		dev_dbg(&rproc->dev, "no resource table found\n");
>> +		return 0;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> This too should go in common code or simply replaced by
> rproc_elf_load_rsc_table() in @amd_bram_rproc_ops - the choice is yours.
> 
> Thanks,
> Mathieu

  Thanks for the review.

  I went through the remoteproc drivers to scope the cleanup points you
  called out.

  For the plain carveout map/unmap callbacks, the same ioremap_wc()/iounmap()
  pattern exists not only in amd_bram_rproc and xlnx_r5_remoteproc, but also
  in rcar_rproc, st_remoteproc, stm32_rproc, imx_rproc, and imx_dsp_rproc.

  The xlnx_r5 TCM path is close as well, but that one still needs a wrapper
  since it clears the memory after ioremap_wc().

  For the optional resource-table parsing, amd_bram_rproc and xlnx_r5_remoteproc
  share the same pattern of treating only -EINVAL from rproc_elf_load_rsc_table()
  as non-fatal. PRU is similar, but has additional firmware parsing after that.
  Other drivers such as rcar/imx/imx_dsp/stm32 also tolerate missing resource
  tables, but their current behavior is not identical since they flatten all
  errors to success and only log.

  For the next revision, would you prefer the following approach?

  1. Add a small common helper for the plain carveout ioremap_wc()/iounmap()
     case and use it in amd_bram_rproc and xlnx_r5_remoteproc.

  2. For the optional resource-table handling, either:
     - add a small common helper for the "missing table is OK" case
       (i.e. return 0 on -EINVAL and propagate other errors), and use that
       in amd_bram_rproc and xlnx_r5_remoteproc, or
     - drop the custom AMD parse_fw() path and use rproc_elf_load_rsc_table()
       directly, which would make the resource table mandatory there.

  Also, for the plain map/unmap helper, should I keep the cleanup scoped to
  the drivers directly involved here, or would you prefer that I fold the
  other exact-match users (rcar, st, stm32, imx, imx_dsp) into the same
  cleanup patch as well?

  I want to make sure I take the direction you prefer before respinning.

  Thanks,
  Ben
> 
>> +
>> +static const struct rproc_ops amd_bram_rproc_ops = {
>> +	.prepare	= amd_bram_rproc_prepare,
>> +	.start		= amd_bram_rproc_start,
>> +	.stop		= amd_bram_rproc_stop,
>> +	.load		= rproc_elf_load_segments,
>> +	.sanity_check	= rproc_elf_sanity_check,
>> +	.get_boot_addr	= rproc_elf_get_boot_addr,
>> +	.parse_fw	= amd_bram_rproc_parse_fw,
>> +};
>> +
>> +static int amd_bram_rproc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct amd_bram_rproc *priv;
>> +	const char *fw_name = NULL;
>> +	struct rproc *rproc;
>> +	int ret;
>> +
>> +	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
>> +	if (ret < 0 && ret != -EINVAL)
>> +		return dev_err_probe(dev, ret,
>> +				     "failed to parse firmware-name property\n");
>> +
>> +	rproc = devm_rproc_alloc(dev, dev_name(dev), &amd_bram_rproc_ops,
>> +				 fw_name, sizeof(*priv));
>> +	if (!rproc)
>> +		return -ENOMEM;
>> +
>> +	priv = rproc->priv;
>> +	priv->dev = dev;
>> +
>> +	/* Get the processor clock */
>> +	priv->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(priv->clk))
>> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
>> +				     "failed to get clock\n");
>> +
>> +	/*
>> +	 * Keep the processor in reset until remoteproc has finished loading
>> +	 * firmware into the executable memory window described by reg and
>> +	 * translated through the parent bus ranges property.
>> +	 */
>> +	priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(priv->reset))
>> +		return dev_err_probe(dev, PTR_ERR(priv->reset),
>> +				     "failed to get reset gpio\n");
>> +
>> +	rproc->auto_boot = false;
>> +
>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to set DMA mask\n");
>> +
>> +	platform_set_drvdata(pdev, rproc);
>> +
>> +	ret = devm_rproc_add(dev, rproc);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to register rproc\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id amd_bram_rproc_of_match[] = {
>> +	{ .compatible = "xlnx,zynqmp-bram-rproc" },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, amd_bram_rproc_of_match);
>> +
>> +static struct platform_driver amd_bram_rproc_driver = {
>> +	.probe = amd_bram_rproc_probe,
>> +	.driver = {
>> +		.name = "amd-bram-rproc",
>> +		.of_match_table = amd_bram_rproc_of_match,
>> +	},
>> +};
>> +module_platform_driver(amd_bram_rproc_driver);
>> +
>> +MODULE_DESCRIPTION("AMD BRAM-based Remote Processor driver");
>> +MODULE_AUTHOR("Ben Levinsky <ben.levinsky@amd.com>");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v3 2/2] remoteproc: add AMD BRAM-based remote processor driver

[Mathieu Poirier]

On Fri, 8 May 2026 at 10:59, Ben Levinsky <blevinsk@amd.com> wrote:
>
> Hi Mathieu,
>
>
> On 5/8/26 8:47 AM, Mathieu Poirier wrote:
> > Good morning,
> >
> > On Tue, Apr 28, 2026 at 07:26:33AM -0700, Ben Levinsky wrote:
> >> Add a remoteproc driver for AMD soft-core processor subsystems
> >> instantiated in programmable logic and using dual-port BRAM for
> >> firmware storage and execution.
> >>
> >> The driver parses the firmware memory window from the remoteproc device
> >> node's reg property, interprets that address and size in the
> >> processor-local address space, and then uses standard devicetree
> >> address translation through the parent bus ranges property to obtain
> >> the corresponding Linux-visible system physical address.
> >>
> >> The resulting translated region is registered as the executable
> >> remoteproc carveout and coredump segment.
> >>
> >> The processor is controlled through an active-low reset GPIO and a
> >> subsystem clock. The clock is enabled before reset is released, and the
> >> processor is kept in reset until firmware loading completes.
> >>
> >> The firmware-name property is optional, allowing firmware to be
> >> assigned later through the remoteproc framework. Firmware images
> >> without a resource table are also accepted.
> >>
> >> Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
> >> ---
> >>  MAINTAINERS                         |   7 +
> >>  drivers/remoteproc/Kconfig          |  14 ++
> >>  drivers/remoteproc/Makefile         |   1 +
> >>  drivers/remoteproc/amd_bram_rproc.c | 243 ++++++++++++++++++++++++++++
> >>  4 files changed, 265 insertions(+)
> >>  create mode 100644 drivers/remoteproc/amd_bram_rproc.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index c871acf2179c..172539971950 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -1037,6 +1037,13 @@ S:    Maintained
> >>  F:  Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
> >>  F:  drivers/w1/masters/amd_axi_w1.c
> >>
> >> +AMD BRAM REMOTEPROC DRIVER
> >> +M:  Ben Levinsky <ben.levinsky@amd.com>
> >> +L:  linux-remoteproc@vger.kernel.org
> >> +S:  Maintained
> >> +F:  Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml
> >> +F:  drivers/remoteproc/amd_bram_rproc.c
> >> +
> >
> > There is no real advantage in adding this entry, checkpatch.pl should be
> > sufficient.
> >
> >>  AMD CDX BUS DRIVER
> >>  M:  Nipun Gupta <nipun.gupta@amd.com>
> >>  M:  Nikhil Agarwal <nikhil.agarwal@amd.com>
> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >> index ee54436fea5a..9a2a887ede8a 100644
> >> --- a/drivers/remoteproc/Kconfig
> >> +++ b/drivers/remoteproc/Kconfig
> >> @@ -23,6 +23,20 @@ config REMOTEPROC_CDEV
> >>
> >>        It's safe to say N if you don't want to use this interface.
> >>
> >> +config AMD_BRAM_REMOTEPROC
> >> +    tristate "AMD BRAM-based remoteproc support"
> >> +    depends on OF && COMMON_CLK && (GPIOLIB || COMPILE_TEST)
> >> +    help
> >> +      Say y or m here to support a BRAM-based remote processor managed
> >> +      through the remoteproc framework.
> >> +
> >> +      This driver matches designs where executable firmware memory is
> >> +      described in the BRAM-local address space and translated to
> >> +      the system physical address space with standard devicetree address
> >> +      translation.
> >
> > Not sure how this paragraph helps decide whether the driver should be enabled or
> > not.  Please remove.
> >
> >> +
> >> +      If unsure, say N.
> >> +
> >>  config IMX_REMOTEPROC
> >>      tristate "i.MX remoteproc support"
> >>      depends on ARCH_MXC
> >> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> >> index 1c7598b8475d..5c39664b50c3 100644
> >> --- a/drivers/remoteproc/Makefile
> >> +++ b/drivers/remoteproc/Makefile
> >> @@ -11,6 +11,7 @@ remoteproc-y                               += remoteproc_sysfs.o
> >>  remoteproc-y                                += remoteproc_virtio.o
> >>  remoteproc-y                                += remoteproc_elf_loader.o
> >>  obj-$(CONFIG_REMOTEPROC_CDEV)               += remoteproc_cdev.o
> >> +obj-$(CONFIG_AMD_BRAM_REMOTEPROC)   += amd_bram_rproc.o
> >>  obj-$(CONFIG_IMX_REMOTEPROC)                += imx_rproc.o
> >>  obj-$(CONFIG_IMX_DSP_REMOTEPROC)    += imx_dsp_rproc.o
> >>  obj-$(CONFIG_INGENIC_VPU_RPROC)             += ingenic_rproc.o
> >> diff --git a/drivers/remoteproc/amd_bram_rproc.c b/drivers/remoteproc/amd_bram_rproc.c
> >> new file mode 100644
> >> index 000000000000..9383964b6046
> >> --- /dev/null
> >> +++ b/drivers/remoteproc/amd_bram_rproc.c
> >> @@ -0,0 +1,243 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * AMD BRAM-based Remote Processor driver
> >> + *
> >> + * Copyright (C) 2026 Advanced Micro Devices, Inc.
> >> + *
> >> + * This driver supports soft-core processors (MicroBlaze, MicroBlaze-V, or
> >> + * similar) instantiated in AMD programmable logic, using dual-port BRAM
> >> + * for firmware storage and execution.
> >> + *
> >> + * The firmware memory (BRAM) is described in the processor-local address
> >> + * space and translated to the Linux-visible system physical address with
> >> + * standard devicetree address translation.
> >> + *
> >> + * Reset is controlled via GPIO connected to Processor System Reset IP.
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/dma-mapping.h>
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/io.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/remoteproc.h>
> >> +
> >> +#include "remoteproc_internal.h"
> >> +
> >> +/**
> >> + * struct amd_bram_rproc - AMD BRAM-based remoteproc private data
> >> + * @dev: device pointer
> >> + * @reset: GPIO descriptor for reset control (active-low)
> >> + * @clk: processor clock
> >> + */
> >> +struct amd_bram_rproc {
> >> +    struct device *dev;
> >> +    struct gpio_desc *reset;
> >> +    struct clk *clk;
> >> +};
> >> +
> >> +static int amd_bram_rproc_mem_map(struct rproc *rproc,
> >> +                              struct rproc_mem_entry *mem)
> >> +{
> >> +    void __iomem *va;
> >> +
> >> +    va = ioremap_wc(mem->dma, mem->len);
> >> +    if (!va)
> >> +            return -ENOMEM;
> >> +
> >> +    mem->va = (__force void *)va;
> >> +    mem->is_iomem = true;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int amd_bram_rproc_mem_unmap(struct rproc *rproc,
> >> +                                struct rproc_mem_entry *mem)
> >> +{
> >> +    iounmap((void __iomem *)mem->va);
> >> +
> >> +    return 0;
> >> +}
> >
> > The above 2 are identical to what is found in xlnx_r5_remoteproc.c.  Please
> > coordinate with Tanmay to split that into common code that can be reused by both
> > drivers.
> >
> >> +
> >> +static int amd_bram_rproc_prepare(struct rproc *rproc)
> >> +{
> >> +    struct amd_bram_rproc *priv = rproc->priv;
> >> +    struct rproc_mem_entry *mem;
> >> +    struct resource res;
> >> +    u64 da, size;
> >> +    int ret;
> >> +
> >> +    ret = of_property_read_reg(priv->dev->of_node, 0, &da, &size);
> >> +    if (ret) {
> >> +            dev_err(priv->dev, "failed to parse executable memory reg\n");
> >> +            return ret;
> >> +    }
> >> +
> >> +    if (!size || size > U32_MAX) {
> >> +            dev_err(priv->dev, "invalid executable memory size\n");
> >> +            return -EINVAL;
> >> +    }
> >> +
> >> +    if (da > U32_MAX) {
> >> +            dev_err(priv->dev, "invalid executable memory address\n");
> >> +            return -EINVAL;
> >> +    }
> >> +
> >> +    ret = of_address_to_resource(priv->dev->of_node, 0, &res);
> >> +    if (ret) {
> >> +            dev_err(priv->dev, "failed to translate executable memory reg\n");
> >> +            return ret;
> >> +    }
> >> +
> >> +    mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)res.start,
> >> +                               (size_t)size, da,
> >> +                               amd_bram_rproc_mem_map,
> >> +                               amd_bram_rproc_mem_unmap,
> >> +                               dev_name(priv->dev));
> >> +    if (!mem)
> >> +            return -ENOMEM;
> >> +
> >> +    rproc_add_carveout(rproc, mem);
> >> +    rproc_coredump_add_segment(rproc, da, (size_t)size);
> >
> > I'm pretty sure you want @res.start instead of @da, and resource_size(&res)
> > instead of @size.
> >
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int amd_bram_rproc_start(struct rproc *rproc)
> >> +{
> >> +    struct amd_bram_rproc *priv = rproc->priv;
> >> +    int ret;
> >> +
> >> +    /* Enable clock before releasing reset */
> >> +    ret = clk_prepare_enable(priv->clk);
> >> +    if (ret) {
> >> +            dev_err(priv->dev, "failed to enable clock: %d\n", ret);
> >> +            return ret;
> >> +    }
> >> +
> >> +    /* Deassert reset and let the processor run. */
> >> +    ret = gpiod_set_value_cansleep(priv->reset, 0);
> >> +    if (ret) {
> >> +            dev_err(priv->dev, "failed to deassert reset: %d\n", ret);
> >> +            clk_disable_unprepare(priv->clk);
> >> +            return ret;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int amd_bram_rproc_stop(struct rproc *rproc)
> >> +{
> >> +    struct amd_bram_rproc *priv = rproc->priv;
> >> +    int ret;
> >> +
> >> +    /* Assert reset before disabling the processor clock. */
> >> +    ret = gpiod_set_value_cansleep(priv->reset, 1);
> >> +    if (ret) {
> >> +            dev_err(priv->dev, "failed to assert reset: %d\n", ret);
> >> +            return ret;
> >> +    }
> >> +
> >> +    /* Disable clock after asserting reset */
> >> +    clk_disable_unprepare(priv->clk);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int amd_bram_rproc_parse_fw(struct rproc *rproc,
> >> +                               const struct firmware *fw)
> >> +{
> >> +    int ret;
> >> +
> >> +    ret = rproc_elf_load_rsc_table(rproc, fw);
> >> +    if (ret == -EINVAL) {
> >> +            dev_dbg(&rproc->dev, "no resource table found\n");
> >> +            return 0;
> >> +    }
> >> +
> >> +    return ret;
> >> +}
> >
> > This too should go in common code or simply replaced by
> > rproc_elf_load_rsc_table() in @amd_bram_rproc_ops - the choice is yours.
> >
> > Thanks,
> > Mathieu
>
>   Thanks for the review.
>
>   I went through the remoteproc drivers to scope the cleanup points you
>   called out.
>
>   For the plain carveout map/unmap callbacks, the same ioremap_wc()/iounmap()
>   pattern exists not only in amd_bram_rproc and xlnx_r5_remoteproc, but also
>   in rcar_rproc, st_remoteproc, stm32_rproc, imx_rproc, and imx_dsp_rproc.
>
>   The xlnx_r5 TCM path is close as well, but that one still needs a wrapper
>   since it clears the memory after ioremap_wc().
>
>   For the optional resource-table parsing, amd_bram_rproc and xlnx_r5_remoteproc
>   share the same pattern of treating only -EINVAL from rproc_elf_load_rsc_table()
>   as non-fatal. PRU is similar, but has additional firmware parsing after that.
>   Other drivers such as rcar/imx/imx_dsp/stm32 also tolerate missing resource
>   tables, but their current behavior is not identical since they flatten all
>   errors to success and only log.
>
>   For the next revision, would you prefer the following approach?
>
>   1. Add a small common helper for the plain carveout ioremap_wc()/iounmap()
>      case and use it in amd_bram_rproc and xlnx_r5_remoteproc.
>
>   2. For the optional resource-table handling, either:
>      - add a small common helper for the "missing table is OK" case
>        (i.e. return 0 on -EINVAL and propagate other errors), and use that
>        in amd_bram_rproc and xlnx_r5_remoteproc, or

I would prefer to go with the common helper that returns 0 on -EINVAL
and propagates other errors, and apply it to other architectures such
as stm32, rcar, imx and imx_dsp.

>      - drop the custom AMD parse_fw() path and use rproc_elf_load_rsc_table()
>        directly, which would make the resource table mandatory there.
>
>   Also, for the plain map/unmap helper, should I keep the cleanup scoped to
>   the drivers directly involved here, or would you prefer that I fold the
>   other exact-match users (rcar, st, stm32, imx, imx_dsp) into the same
>   cleanup patch as well?
>

Proceed with the other exact-match as well.

>   I want to make sure I take the direction you prefer before respinning.

I think the best approach is to send out a cleanup patchset with the
above changes, followed by another respin of this set once the cleanup
is merged.

Thanks for being proactive.

>
>   Thanks,
>   Ben
> >
> >> +
> >> +static const struct rproc_ops amd_bram_rproc_ops = {
> >> +    .prepare        = amd_bram_rproc_prepare,
> >> +    .start          = amd_bram_rproc_start,
> >> +    .stop           = amd_bram_rproc_stop,
> >> +    .load           = rproc_elf_load_segments,
> >> +    .sanity_check   = rproc_elf_sanity_check,
> >> +    .get_boot_addr  = rproc_elf_get_boot_addr,
> >> +    .parse_fw       = amd_bram_rproc_parse_fw,
> >> +};
> >> +
> >> +static int amd_bram_rproc_probe(struct platform_device *pdev)
> >> +{
> >> +    struct device *dev = &pdev->dev;
> >> +    struct amd_bram_rproc *priv;
> >> +    const char *fw_name = NULL;
> >> +    struct rproc *rproc;
> >> +    int ret;
> >> +
> >> +    ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> >> +    if (ret < 0 && ret != -EINVAL)
> >> +            return dev_err_probe(dev, ret,
> >> +                                 "failed to parse firmware-name property\n");
> >> +
> >> +    rproc = devm_rproc_alloc(dev, dev_name(dev), &amd_bram_rproc_ops,
> >> +                             fw_name, sizeof(*priv));
> >> +    if (!rproc)
> >> +            return -ENOMEM;
> >> +
> >> +    priv = rproc->priv;
> >> +    priv->dev = dev;
> >> +
> >> +    /* Get the processor clock */
> >> +    priv->clk = devm_clk_get(dev, NULL);
> >> +    if (IS_ERR(priv->clk))
> >> +            return dev_err_probe(dev, PTR_ERR(priv->clk),
> >> +                                 "failed to get clock\n");
> >> +
> >> +    /*
> >> +     * Keep the processor in reset until remoteproc has finished loading
> >> +     * firmware into the executable memory window described by reg and
> >> +     * translated through the parent bus ranges property.
> >> +     */
> >> +    priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> >> +    if (IS_ERR(priv->reset))
> >> +            return dev_err_probe(dev, PTR_ERR(priv->reset),
> >> +                                 "failed to get reset gpio\n");
> >> +
> >> +    rproc->auto_boot = false;
> >> +
> >> +    ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> >> +    if (ret)
> >> +            return dev_err_probe(dev, ret, "failed to set DMA mask\n");
> >> +
> >> +    platform_set_drvdata(pdev, rproc);
> >> +
> >> +    ret = devm_rproc_add(dev, rproc);
> >> +    if (ret)
> >> +            return dev_err_probe(dev, ret, "failed to register rproc\n");
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static const struct of_device_id amd_bram_rproc_of_match[] = {
> >> +    { .compatible = "xlnx,zynqmp-bram-rproc" },
> >> +    { /* sentinel */ },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, amd_bram_rproc_of_match);
> >> +
> >> +static struct platform_driver amd_bram_rproc_driver = {
> >> +    .probe = amd_bram_rproc_probe,
> >> +    .driver = {
> >> +            .name = "amd-bram-rproc",
> >> +            .of_match_table = amd_bram_rproc_of_match,
> >> +    },
> >> +};
> >> +module_platform_driver(amd_bram_rproc_driver);
> >> +
> >> +MODULE_DESCRIPTION("AMD BRAM-based Remote Processor driver");
> >> +MODULE_AUTHOR("Ben Levinsky <ben.levinsky@amd.com>");
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.34.1
> >>
> >
>