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

[Mathieu Poirier <mathieu.poirier@linaro.org>]

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
>