Messages in this thread | | | Date | Tue, 14 Feb 2023 11:56:30 +0200 | Subject | Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in DDR | From | Iuliana Prodan <> |
| |
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. >>
| |