On Fri, Apr 9, 2021 at 2:20 PM Alex G. <mr.nuke...@gmail.com> wrote: > > Hi Simon > > On 4/8/21 6:55 PM, Simon Glass wrote: > > Hi Alexandru, > > > > On Fri, 9 Apr 2021 at 04:56, Alexandru Gagniuc <mr.nuke...@gmail.com> wrote: > >> > >> This reverts commit 38d6b7ebdaee3e0e8426ef1b9df88bdce8ae2e75. > >> > >> struct global_data contains a pointer to the bd_info structure. This > >> pointer was populated spl_set_bd() to a pre-allocated bd_info in the > >> ".data" section. The referenced commit replaced this mechanism to one > >> that uses malloc(). That new mechanism is only used if SPL_ALLOC_BD=y. > >> which very few boards do. > >> > >> The result is that (struct global_data)->bd is NULL in SPL on most > >> platforms. This breaks falcon mode, since arch_fixup_fdt() tries to > >> access (struct global_data)->bd and set the "/memory" node in the > >> devicetree. The result is that the "/memory" node contains garbage > >> values, causing linux to panic() as it sets up the page table. > >> > >> Instead of trying to fix the mess, potentially causing other issues, > >> revert to the code that worked, while this change is reworked. > > > > The goal here is to drop a feature that few boards use and reduce > > memory usage in SPL. It has been in place for two releases now. > > > > If Falcon mode needs it, perhaps you could add an 'imply' in the > > Kconfig for that feature? Is there one? Or perhaps > > CONFIG_ARCH_FIXUP_FDT_MEMORY ? > > > > One option would be to return an error in arch_fixup_fdt(). In > > general, fixups make things tricky because there is no way to > > determine when they are used but at least this one has a CONFIG. > > > > The argument that this has been in place for two releases is incorrect. > Commit 38d6b7ebdaee3e0e8426ef1b9df88bdce8ae2e75 was only introduced with > the v2021.04 release. It definitely was not in 2021.01. It's only in the > last release, which is four days old t the time of this writing. > > Although I was able to find one example, the reality is that we don't > know the full extent of the breakage. The prudent thing at this point is > to revert. > > The knowledge of how to init the platform is in the devicetree and code. > Why should kconfig also be involved in storing this knowledge? By this > model, as the number of boards increases without bounds, every "if" > predicate tends to be Kconfig driven. That is not maintainable, and why > I think the original change --and the proposed fixes-- are broken by design. > > Furthermore, I'm happy to discuss what to do about Falcon mode, and if > we should kill it entirely (I have an alternative proposal). But we > shouldn't have that discussion in a manner holding my platform hostage. > To be fair to you, I don't think reverting a 64-byte savings in .data > will hold your platform hostage either.
That original patch broke a bunch of OMAP boards, but enabling SPL_ALLOC_BD was the solution to fix the issue. Can you try enabling SPL_ALLOC_BD and see if that fixes it? Maybe we can make falcon mode imply it. adam > > Alex >