From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Tue, 4 Apr 2017 22:17:41 +0200 Subject: [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support In-Reply-To: <98fa8e01dbf42824f1ac8239405e78e5@agner.ch> References: <20170329195827.6217-1-stefan@agner.ch> <20170403132024.514307fd@jawa> <0d0cd2362362fb2029e46821678f2f8b@agner.ch> <15002942-54e4-2271-f88e-fe67e9892f7d@denx.de> <42b4c04248e2c10ed12a794913e0d428@agner.ch> <5304be93916a877348055d59375cd14c@agner.ch> <859596dd-4b0b-9dff-f50d-38e85a7b49bf@denx.de> <44bf06ec115c3246a4f2a93e38bb3324@agner.ch> <1d98ecaf-bcef-07de-fa28-a6adeb3d766d@denx.de> <98fa8e01dbf42824f1ac8239405e78e5@agner.ch> Message-ID: <51864dfd-a792-9619-88dd-2a7a43076117@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 04/04/2017 09:45 PM, Stefan Agner wrote: > On 2017-04-04 11:38, Marek Vasut wrote: >> On 04/04/2017 07:57 PM, Stefan Agner wrote: >>> On 2017-04-04 02:22, Marek Vasut wrote: >>>> On 04/04/2017 02:02 AM, Stefan Agner wrote: >>>> [...] >>>>>>>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc >>>>>>>> cores, you can very well also boot secondary cores on the current CPU >>>>>>>> complex with the same command. Why not ? >>>>>>> >>>>>>> Sure, it could be done. I just feel it is not the right design. >>>>>>> >>>>>>> Auxiliary cores have usually a different view to memory, this is why I >>>>>>> had to add the get_host_mapping callback in the elf loader code to let >>>>>>> architecture dependent code translate to host addresses. SMP systems >>>>>>> don't need that. >>>>>>> >>>>>>> Also flush caches is not necessary on some cache coherent CPU's >>>>>>> (depending on how your cache coherence between I and D cache looks >>>>>>> like). >>>>>> >>>>>> So SMP is just a reduced special-case of this , yes ? >>>>> >>>>> Yeah, I guess you can get away with dummy callback implementation and a >>>>> performance hit due to cash flushes. >>>> >>>> Or you can abstract things out ? >>>> >>> >>> There is one callback to arch for translation and one for cache flush, >>> what more can I abstract out? >> >> Well then I don't think I understand your concerns about cache flushing >> in SMP . >> > > It makes things unnecessary slower. You always have to flush cache if you expect the peripheral to read cached memory , so I don't think I understand your remark ... >>>>>>> Creating a new command like bootaux comes with very few overhead. >>>>>> >>>>>> The overhead is the new command, we already have many ad-hoc commands. >>>>>> >>>>> >>>>> Agreed, and I really wished that this got discussed when that command >>>>> initially got added. I brought it up back then... >>>>> https://lists.denx.de/pipermail/u-boot/2016-January/240323.html >>>>> >>>>> It seemed to be acceptable to just add this ad hoc command, with some >>>>> "random binary format" support back then... >>>> >>>> Based on that discussion, I only see that noone opposed, but I don't see >>>> any agreement. >>>> >>> >>> Maybe I need to word my emails a bit stronger, but with that email I >>> actually tried to oppose: >>> https://lists.denx.de/pipermail/u-boot/2016-January/240989.html >> >> Well, I do not like adding ad-hoc commands as it's not managable in the >> long run. >> > > I argue that a remote core loading command is _much_ more manageable > than making the bootm command even more complex by support > SMP/remoteproc and whatnot usecases... I would even argue that a bunch > of those commands are more manageable than a single ifdef/if hell.... I guess a boot* option to start remote core would do ? > That said, I still would push for keeping the image processing code > generic, whenever it makes sense. Agreed >>>>> Ok, it is not entirely >>>>> random, since it is the format of a binary how it ends up in >>>>> microcontrollers execute in place flash (stack pointer and reset vector >>>>> are the two first words).... >>>> >>>> I thought this command was starting the core by loading data to RAM , >>>> not flash ? >>>> >>> >>> Ok, maybe I am a bit unclear here: >>> >>> bootaux currently supports a Cortex-M specific binary format only. The >>> binary format starts with a Cortex-M class vector table. The vector >>> tables first vector is the "reset vector". >>> >>> In a regular microcontroller, that binary gets flashed on a NOR flash >>> which is mapped at 0x0 for the CPU. The CPU has no "boot ROM", the CPU >>> starts by calling the reset vector. So when NXP defined how the bootaux >>> command should look like, they just took that binary format which was >>> laying around and implemented a U-Boot command around it. >>> >>> So this is the history of the binary format. And my point here is, that >>> the binary format supported by bootaux is _very_ Cortex-M class >>> specific. >> >> Aha, so I now totally don't understand why this command cannot be >> fixed/extended to support other/generic cores or SMP systems etc. >> But looking at the initial proposal, I think maybe the intention of this >> patchset was not to add that support, but to fix the command to >> support loading ELF files ? We already have bootelf for that though ... >> > > Yes, that is pretty much it. I would like to teach that command a more > generic format, which would be at least a step towards something more > generic/standardized. > > bootelf is really meant for the primary CPU. That would be an entirely > different direction: Make all common boot commands "aux core" capable. > > But I would strongly vote against that. First, those commands have > already complex arguments and argument handling (e.g. bootm), and their > implementation supports use cases which we hardly would ever use on aux > cores (initramfs..). That might need some further thinking/consolidation . IMO out of scope of this discussion ... >>>>> However, making this ad hoc command now >>>>> generic really feels weird to me, since we would end up supporting that >>>>> format for A class CPUs etc... bootaux is really suited for auxiliary >>>>> M-class cores on ARM, as it is right now. Maybe we should have named it >>>>> bootm ;-) >>>> >>>> We always try to avoid one-off hacks, so it's not weird. I still don't >>>> quite understand how it is a problem to abstract things out . I am not >>>> asking you to support CA, but to avoid CM specific implementation which >>>> cannot be extended to support CA or even MIPS/Nios2/etc . >>>> >>> >>> Again, why would you support a Cortex-M class specific file format for >>> MIPS/Nios2 etc...? >>> >>> bootaux is M4 specific. We can through it away, and start over, but then >>> we should call the command differently. >> >> Unless you look very carefully, it is not clear that it is cortex M4 >> specific at all based on this discussion. I was under the impression >> that the goal here was to support all sorts of secondary cores ... >> > > Yeah it's also only that binary format handling which is M4 specific. > And frankly, that is just 2 lines of code, basically dereferencing the > first two words: > > + if (valid_elf_image(addr)) { > + stack = 0x0; > + pc = load_elf_image_phdr(addr); > + if (!pc) > + return CMD_RET_FAILURE; > + > + } else { > + /* > + * Assume binary file with vector table at the beginning. > + * Cortex-M4 vector tables start with the stack pointer (SP) > + * and reset vector (initial PC). > + */ > + stack = *(u32 *)addr; > + pc = *(u32 *)(addr + 4); > + } > >>>>>>> This are the reasons why I feel creating a new command for a SMP boot >>>>>>> case makes more sense. We can still reuse functions which are very >>>>>>> similar by moving them into some common location, where it makes sense. >>>>>>> >>>>>>>> >>>>>>>> Also, I think this might come useful when booting stuff like "Altera >>>>>>>> Sparrow" ... >>>>>>> >>>>>>> I am not familiar with that architecture, what kind of core does it >>>>>>> provide which needs to be booted by U-Boot? >>>>>> >>>>>> The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the >>>>>> FPGA. >>>>> >>>>> In my thinking, the Nios2 core seems like such a remote processor well >>>>> suited for the bootaux command. For the secondary A9, I would create a >>>>> new command. >>>> >>>> Uh, why, that does not make any sense to me. Both the random core in >>>> FPGA and the secondary CA9 core have almost the same bringup sequence. >>>> What is so different about this stuff ? >>>> >>> >>> A cache coherent core and a non-cache coherent core remote core >>> somewhere in the SoC is very much different IMHO. >>> >>> Lets compare how you bring up a A class CPU, or cache coherent SMP style >>> CPU in general: >>> >>> 1. Load code into RAM >>> 2. Some cache flushing might be necessary due to I/D-cache incoherency >>> 3. Write entry point into some architecture specific register >>> 4. kick off the CPU by writing another architecture specific register >>> >>> M class/remote CPU >>> >>> 1. Load code in RAM, depending on image format translate core specific >>> memory map to host memory map >>> 2. Flush D-cache >>> 3. (Potentially set up IOMMU, not in the NXP case though) >>> 4. Write entry point into some architecture specific register >>> 5. kick off the CPU by writing another architecture specific register >>> >>> From what I can see, everything is different other than "Load code in >>> RAM" which is image specific. If the image format is complex, we >>> certainly should factor that out to avoid code duplication. >> >> Image format is usually binary blob or elf file, we support both. >> The rest is "load file to RAM, do magic setup (can be CPU specific), >> unreset CPU". >> > > Loading files to RAM is already generic since it is handled by separate > commands. > > It is really the image handling which has the potential of reusability. OK >>>>> If we want to support the two with the same command, we already have a >>>>> first problem: How do we address them? Of course, we could add just a >>>>> index or something, but this would already break backward compatibility >>>>> of the bootaux command. >>>> >>>> So we already accepted a command which has shit commandline semantics >>>> and we're stuck with it , hrm. You can always specify a default core >>> >>> Add shitty binary file format to the list. >> >> Why do we care about this ? The CM just needs a vectoring table, so set >> it up (in the elf file or whatever) and point the core at the reset vector. >> > > Not sure what you want to say here, but I guess we need to keep support > for this file format for backward compatibility? I guess so, I didn't look into the requirements for the CM4 core on MX6. >>>> index for those two iMX boards and it's done. Or just fix the semantics >>>> in their default environment, which is not awesome, but I doubt this is >>>> wildly used yet. >>> >>> I guess my general point is, bootaux is already broken in two ways: >>> Command line interface does not allow for extensibility, the only format >>> supported right now is a binary format which is not generic. >>> >>> My patchset was an attempt to improve the situation to give it at least >>> a decent elf format support. >> >> Maybe this information was lost ... Lukasz ?! >> >>> Note that there is also bootelf, where the elf headers have been >>> introduced. Reusing that command seems impractical since it is meant to >>> boot on the primary CPU. My first attempt was reusing at least the load >>> function, but it seemed impractical due to memory map translation and >>> since the load function is rather short. If it helps for the acceptance >>> of this patchset, I still could try to make that happen. >> >> Extending bootelf might indeed make more sense IMO. But that needs >> discussion. >> > > There are two variants of reuse here: > 1. Reuse the elf parsing and loading function (function > load_elf_image_phdr in this case). > 2. Reuse the bootelf command. This would need extending the command with > some parameter to tell it to not boot the primary CPU but boot some > other auxiliary processor. > > > Let's get a bit more concrete here, take bootelf as a case study: > > 1 would introduce the architecture specific translation callback > load_elf_image_phdr (see my load_elf_image_phdr implementation in this > patchset). For regular/primary boot we can implement a dummy 1:1 > translation. > > > > 2 would need 1, and changes to do_bootelf. Since the command has already > optional arguments adding core selection as an optional argument is not > possible. But we could add a parameter, e.g. -c (core): > > bootelf -c aux > > or > > bootelf -c 1 Works for me ... > The exact enumeration would have to be discussed. I guess this would > also be architecture specific. > > Adding the new parameter handling plus callbacks into architecture > dependent code probably makes do_bootelf double in length. Also, half of > the existing code cannot be reused: The call do_bootelf_exec is primary > CPU specific, passing the arguments (this is likely not supported by > remote cores) and the return code handling (since a remote core would > not return), autostart handling, not sure? > > 2 would also need to either disable the section header functionality for > the remote core, or we add another if in do_bootelf and avoid parsing > section header even if requested... > > > Again, I strongly disfavor solution 2. That makes boot commands with > image types we want to support even more complex. bootelf already starts > to look ugly when I start implementing that, and bootelf is really one > of the cleanest and simplest boot commands I have seen... I probably > would give it a shot if this is the only way to bring elf format support > for aux cores upstream... Good luck for the guy implementing FIT image > support (bootm) for auxiliary cores :-D So what is the problem with this ? Load the payload somewhere and ungate the secondary core , why is that so hard ? > 1 we could do, and I would be willing to do it. I personally feel that > the small code duplication is cleaner than reusing that function from > cmd/elf.c and implementing the dummy 1:1 translation, but both works for > me.... > > IMHO, commands are really lightweight and nice in U-Boot, I think we > should make use of them, even when it means some namespace pollution. > > -- > Stefan > > -- Best regards, Marek Vasut