On Fri, 8 May 2026 at 10:59, Ben Levinsky <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
> >> +L: [email protected]
> >> +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 <[email protected]>
> >> M: Nikhil Agarwal <[email protected]>
> >> 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 <[email protected]>");
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.34.1
> >>
> >
>
