Message-ID: <8736e543-3e8c-25b3-0107-ce59296227c9@oss.nxp.com>
Date:   Fri, 24 Mar 2023 18:20:08 +0800
From:   Peng Fan <peng.fan@....nxp.com>
To:     Frieder Schrempf <frieder.schrempf@...tron.de>,
        Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Iuliana Prodan <iuliana.prodan@....com>,
        Peng Fan <peng.fan@....com>,
        "andersson@...nel.org" <andersson@...nel.org>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "arnaud.pouliquen@...s.st.com" <arnaud.pouliquen@...s.st.com>,
        Daniel Baluta <daniel.baluta@....com>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        "linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in DDR

Hi Frieder,

On 3/22/2023 6:59 PM, Frieder Schrempf wrote:
> Hi,
> 
> On 07.03.23 21:26, Mathieu Poirier wrote:
>> On Sat, Mar 04, 2023 at 03:59:38PM +0800, Peng Fan wrote:
>>>
>>>
>>> On 2/14/2023 1:50 AM, 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@....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?
>>>>
>>>>>
>>>>> 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.
>>>
>>> sorry, I not got your points. i.MX8M and i.MX93 are not sharing firmware
>>
>> Perfect.
>>
>>> images. i.MX93 M33 has ROM, kicking M33 firmware just requires the
>>> address of the .interrupt address which holds stack/pc value.
>>> i.MX8M not has ROM, kick M33 firmware requires driver to copy
>>> stack/pc into the TCML beginning address.
>>
>> It's been more than a month since I have looked at this patchset so the details are
>> vague in my memory.  That said, there should be one image for the TCML and
>> another one for the RAM.  And the image that runs in RAM should have a program
>> segment that write the correct information in the first 8 bytes.
>>
>>>
>>> Whether i.MX8M/i.MX93, the NXP released MCU SDK use the section
>>> ".interrupt" to hold stack/pc initialization value in the beginning
>>> 8 bytes of the section.
>>>
>>
>> And that is fine.  Simply release another version of the SDK that does the right
>> thing.
>>
>> I suggest to work with Daniel and Iuliana if some details are still unclear.
>> Unlike me, they have access to the reference manual and the boot requirements.
>>
>>
>>>>
>>>> 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.
>>>>
>>>> That pushes the complexity to the tool that generates the firmware image,
>>>> exactly where it should be.
>>>
>>> For i.MX8M, yes. For i.MX93, the M33 ROM needs address of storing stack/pc.
>>>>
>>>> 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.
>>>
>>> I am not sure how to proceed on supporting the current firmware. what should
>>> I continue with current patchset?
> 
> I've successfully tested this on i.MX8MM with an elf file generated by
> the NXP SDK.
> 
> I would really like to see this upstreamed. If this requires changes
> that are not compatible with binaries compiled with the current SDK as
> discussed above, that would be fine for me as long as the kernel is able
> to detect the malformed binary and warns the user about it.
> 
> The user can then manually adjust the linker script, etc. in the SDK to
> match the requirements of the kernel.

If you have adjust linker script, you will not need this patch to load
m4 image to DDR for i.MX8MM. Just put the pc/stack in a seperate section
in your linker file, and the address is TCML start address, I think
it would be ok.

This patchset is just for images not has dedicated section saying
NXP ones has pc/stack in the beginning of .interrupts section.

Thanks,
Peng.

> 
> Thanks
> Frieder

Powered by blists - more mailing lists