Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in DDR

[Iuliana Prodan <iuliana.prodan@nxp.com>]

On 2/13/2023 7:50 PM, Mathieu Poirier wrote:
> On Mon, Feb 13, 2023 at 12:15:59PM +0200, Iuliana Prodan wrote:
>> On 2/12/2023 9:43 AM, Peng Fan wrote:
>>> Hi Iuliana,
>>>
>>>> Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
>>>> DDR
>>>>
>>>>
>>>> On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>
>>>>> V3:
>>>>>
>>>>>     Daniel, Iuliana
>>>>>
>>>>>       Please help review this patchset per Mathieu's comments.
>>>>>
>>>>>     Thanks,
>>>>>     Peng.
>>>>>
>>>>>     Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
>>>>>     IMX_RPROC_ANY in patch 3 Per Mathieu
>>>>>     Update comment and commit log in patch 5, 6.
>>>>>
>>>>>     NXP SDK provides ".interrupts" section, but I am not sure how others
>>>>>     build the firmware. So I still keep patch 6 as v2, return bootaddr
>>>>>     if there is no ".interrupts" section.
>>>>>
>>>>> V2:
>>>>>     patch 4 is introduced for sparse check warning fix
>>>>>
>>>>> This pachset is to support i.MX8M and i.MX93 Cortex-M core firmware
>>>>> could be in DDR, not just the default TCM.
>>>>>
>>>>> i.MX8M needs stack/pc value be stored in TCML entry address[0,4], the
>>>>> initial value could be got from firmware first section ".interrupts".
>>>>> i.MX93 is a bit different, it just needs the address of .interrupts
>>>>> section. NXP SDK always has .interrupts section.
>>>>>
>>>>> So first we need find the .interrupts section from firmware, so patch
>>>>> 1 is to reuse the code of find_table to introduce a new API
>>>>> rproc_elf_find_shdr to find shdr, the it could reused by i.MX driver.
>>>>>
>>>>> Patch 2 is introduce devtype for i.MX8M/93
>>>>>
>>>>> Although patch 3 is correct the mapping, but this area was never used
>>>>> by NXP SW team, we directly use the DDR region, not the alias region.
>>>>> Since this patchset is first to support firmware in DDR, mark this
>>>>> patch as a fix does not make much sense.
>>>>>
>>>>> patch 4 and 5 is support i.MX8M/93 firmware in DDR with parsing
>>>>> .interrupts section. Detailed information in each patch commit message.
>>>>>
>>>>> Patches were tested on i.MX8MQ-EVK i.MX8MP-EVK i.MX93-11x11-EVK
>>>> If one can build their firmware as they want, then the .interrupt section can
>>>> also be called differently.
>>>> I don't think is a good idea to base all your implementation on this
>>>> assumption.
>>>>
>>>> It's clear there's a limitation when linking firmware in DDR, so this should be
>>>> well documented so one can compile their firmware and put the needed
>>>> section (interrupt as we call it in NXP SDK) always in TCML - independently
>>>> where the other section go.
>>> Ok, so .interrupt section should be a must in elf file if I understand correctly.
>>>
>>> I could add a check in V4 that if .interrupt section is not there, driver will report
>>> failure.
>>>
>>> How do you think?
>> Peng, I stand by my opinion that the limitation of linking firmware in DDR
>> should be documented in an Application Note, or maybe there are other
>> documents where how to use imx_rproc is explained.
>>
>> The implementation based on the .interrupt section is not robust.
>> Maybe a user linked his firmware correctly in TCML, but the section is not
>> called .interrupt so the firmware loading will work.
>>
>> So, instead of using the section name, you should use the address.
> Can you be more specific on the above?
Yes, I was thinking of the same thing you proposed below, to have a 
section in TCML.
>> First, check whether there is a section linked to TCML.
>> If there is none, check for section name - as you did.
>> If there is no section called .interrupt, give an error message.
> We have two ways of booting, one that puts the firmware image in the TCML and
> another in RAM.  Based on the processor type, the first 8 bytes of the TCML need
> to include the address for the stack and PC value.
>
> I think the first thing to do is have two different firmware images, one for
> i.MX8M and another one for i.MX93.  That should greatly simplify things.
Yes, definitely, there should be firmware images for each platform.
>
> Second, there should always be a segment that adds the right information to the
> TMCL.  That segment doesn't need a name, it simply have to be part of the
> segments that are copied to memory (any kind of memory) so that function
> rproc_elf_load_segments() can do its job.
I totally agree!

> That pushes the complexity to the tool that generates the firmware image,
> exactly where it should be.
>
> This is how I think we should solve this problem based on the very limited
> information provided with this patchset.  Please let me know if I missed
> something and we'll go from there.
>
>> For all the above options please add comments in code, explaining each step.
>>