[PATCH] Revert "spl: Drop bd_info in the data section"

[Adam Ford]

On Fri, Apr 9, 2021 at 2:20 PM Alex G. <mr.nuke.me at 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.me at 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
>