On Wed, Jun 24, 2026 at 03:01:58PM -0700, Ben Levinsky wrote:
> Hi Mathieu
>
> Picking this back up now. Please see my replies inline below.
>
> On 5/11/26 10:41 AM, Mathieu Poirier wrote:
> > 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.
> >>>
Looking at the code again, I'm not sure why I asked to used @da.
>
> For the coredump segment, I agree with using resource_size(&res) for
> the size, but I think the address should remain @da rather than
> @res.start.
>
> The binding describes the reg property in the processor-local address
> space and uses the parent bus ranges property only to translate that
> window to the Linux-visible system physical address. That means @da and
> @res.start are not necessarily in the same address space. For example,
> the BRAM can appear at 0x0 to the soft-core processor while Linux sees
> the same memory at a translated system physical address such as
> 0xa0000000.
>
> rproc_coredump_add_segment() stores the address as a device address, and
> the coredump path later resolves it through rproc_da_to_va() against the
> registered carveout's device address. Since this driver registers the
> carveout with @da as the device address and @res.start as the host-side
> physical address used for ioremap_wc(), passing @res.start to
> rproc_coredump_add_segment() could fail to match the carveout when those
> addresses differ.
>
> So in the respin I plan to use:
>
> rproc_coredump_add_segment(rproc, da, resource_size(&res));
>
> Does that match your expectation for this address model?
Yes, I agree with you proposal.
>
> Thank you
> Ben
>
> >>>> +
> >>>> + 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
> >>>>
> >>>
> >>
>
