[PATCH 03/24] spl: x86: Correct the binman symbols for SPL

[Simon Glass]

Hi Alper,

On Thu, 3 Mar 2022 at 14:14, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> On 24/02/2022 01:58, Simon Glass wrote:
> > Hi Alper,
> >
> > On Tue, 15 Feb 2022 at 04:52, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
> >>
> >> On 08/02/2022 21:49, Simon Glass wrote:
> >>> These symbols are incorrect, meaning that binman cannot find the
> >>> associated entry. This leads to errors like:
> >>>
> >>> binman: Section '/binman/simple-bin': Symbol '_binman_spl_prop_size'
> >>>    in entry '/binman/simple-bin/u-boot-spl/u-boot-spl-nodtb':
> >>>    Entry 'spl' not found in list (mkimage,u-boot-spl-nodtb,
> >>>    u-boot-spl-bss-pad,u-boot-spl-dtb,u-boot-spl,u-boot-img,main-section)
> >>
> >> I can't help but feel like this is a bug with entry expansion where the
> >> name of the expanded node is ignored (and replaced by its type?) when it
> >> comes to the symbols.
>
> I think I misunderstood that error message. I thought this error was for
> a board with the x86 dtsi where it wasn't finding an "spl" entry that
> actually existed. Not seeing "spl" in the list I assumed its name was
> being lost for some reason.
>
> Now I realize it must be for a board where "u-boot-spl" exists instead
> of "spl" (which the symbol was looking for), and you were correcting the
> symbol to match that and furthermore fixing the x86 dtsi to match the
> corrected symbol.
>
> (Perhaps it was a Rockchip board, related to the rest of the series :P )
>
> >
> > The problem is that there is only really one value for a symbol. E.g.
> > U-Boot has an image-pos and it doesn't matter what you call it; it is
> > the same value.
> >
> > So does it make sense to disallow different names for the same thing?
> > See testSymbol() which actually creates two SPLs and checks that both
> > are updated. That is the opposite to what you are talking about, of
> > course, since it is the properties of the 'u-boot' entry which are
> > used to write into the SPL entries.
> >
> > If we move to using the name instead, we could have two different
> > copies of U-Boot in the image and each SPL could refer to a different
> > one. At present this is done by puting the pairs into their own
> > section.
> >
> > I think this needs more discussion....what do you think?
>
> I think it's better to use the names, since there are reasonable cases
> where an image would have multiple entries of the same type: A/B updates
> and read-only recovery copies (like in Chrome OS firmware, or I guess
> eventually with your VPL series?).

Well that is already supported to some extent, they just need to go
into their own section. In fact I think we want things in their own
section anyway. For the Elf symbols, it picks the the value the entry
in the same section, so it is possible to have an A section and a B
section, each with its own SPL and U-Boot.

>
> >From what I can tell, the symbols are indeed set based on the entry
> names (not entry types), so multiple entries of the same type but with
> different names should already be working fine -- except no symbols are
> declared on the C side for arbitrary names. I guess putting multiple
> copies in different sections with different "name-prefix" values works
> fine the same way.

I think this could use a few tests to make this clear.

>
> However I lightly suspect this might be breaking down a bit with entry
> expansion, since the nodes generated in differently-named sections could
> have the same name (the desired entry type), but didn't test it or
> anything. I guess it works since the symbol would be declared for the
> node-to-expand anyway, with the correctly unique name?

Yes and often the node-to-expand is the thing you want anyway, since
you want to load the whole section.

>
> (Maybe the symbols might be based on the path instead, but that could be
> very verbose. I see an idea in binman.rst for a C library that can read
> binman things from device tree, which sounds nice for this as well.)

Yes we can do that but best done in the C library as you say. We have
a binman.c but it has very little functionality so far.

Regards,
Simon