Messages in this thread | | | Date | Fri, 24 Mar 2023 18:20:08 +0800 | Subject | Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in DDR | From | Peng Fan <> |
| |
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@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? >>>> >>>>> >>>>> 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
| |