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

Hi Mathieu,

On 7/8/26 9:23 AM, Mathieu Poirier wrote:
> On Tue, Jul 07, 2026 at 10:59:55AM +0200, Michal Simek wrote:
>>
>>
>> On 7/3/26 18:39, Mathieu Poirier wrote:
>>> Hi Ben,
>>>
>>> On Mon, Jun 29, 2026 at 09:40:03AM -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]>
>>>> ---
>>>>   drivers/remoteproc/Kconfig          |  11 ++
>>>>   drivers/remoteproc/Makefile         |   1 +
>>>>   drivers/remoteproc/amd_bram_rproc.c | 213 ++++++++++++++++++++++++++++
>>>>   3 files changed, 225 insertions(+)
>>>>   create mode 100644 drivers/remoteproc/amd_bram_rproc.c
>>>>
>>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>>> index c521c744e7db..58fa566b609f 100644
>>>> --- a/drivers/remoteproc/Kconfig
>>>> +++ b/drivers/remoteproc/Kconfig
>>>> @@ -23,6 +23,17 @@ 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.
>>>> +
>>>> +	  The processor is controlled through a reset GPIO and clock.
>>>> +
>>>
>>> This last sentence doesn't belong here - please remove.
Sure will do.
>>>
>>> Looking at the bindings and this Kconfig I wonder why the emphasis of the naming
>>> convention, i.e "BRAM-based remoteproc" is placed on the kind of memory the
>>> remote processor is connected to rather than the remote processor itself.
>>>
>>> Wouldn't it be better to have something like "AMD MicroBlaze/V remote
>>> processor"?  What happens when we get another AMD softcore that is completely
>>> different than MicroBlaze/V that is also connected to the same type of memory?
>>>
>>> I'm good with the implemenation, I just wonder about the name...
>>
>> We started with very generic amd,bram-proc compatible string and then based
>> on discussion
>> https://lore.kernel.org/all/[email protected]/
> 
> Thanks for the link, it provides useful context.
> 
>> we got recommendation to treat is as soc specific instead of generic.
>> Which give us back to origin point if driver should be more soc specific or not.
>>
>> I think no issue with AMD prefix.
> 
> Agreed.
> 
>>
>> BRAM - that's technology used for access. And I see value in symbol to state
>> it because access to different memory can be done differently too.
>>
> 
> Agreed.
> 
>> Shouldn't be there any soc string? This is for zynqmp, versal* SOCs that's
>> why hard to choose which one.
>>
> 
> Why not just adding a Zynqmp somewhere in there?  But admittedly, the relation
> between Zynqmp and MicroBlaze/V is unknown to me.
> 
>> The driver is written in very generic way that if there is arm, mips, ppc,
>> different risc-v or others access will be the same. Our primary target is
>> obviously MicroBlaze/V and I don't think it is a problem to change
>> description to just reference them even it is technically possible to
>> connect different cpus too.
>>
>> Is this enough?
>>
>> -	tristate "AMD BRAM-based remoteproc support"
>> +	tristate "AMD MicroBlaze/V BRAM-based remoteproc support"
> 
> Yes, that would be just fine. "AMD Zynqmp BRAM-based remoteproc support" would
> also work.
> 
>>
>> Are you fine with AMD_BRAM_REMOTEPROC symbol name?
> 
> AMD_MBV_BRAM_REMOTEPROC or AMD_ZQ_BRAM_REMOTEPROC
Ok will use AMD_MBV_BRAM_REMOTEPROC 

Thank you
Ben
> 
> I'm good with anything that is slightly more descriptive, just pick the one that
> makes the most sense.  
> 
>>
>> Thanks,
>> Michal