Hi Peng, On 24.03.23 11:20, Peng Fan wrote: > 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@xxxxxxx> >>>>>>>>> >>>>>>>>> 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. Ok, thanks for the explanation. Good to know. I thought there were other changes included in this patchset that are required. Thanks Frieder
