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

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

On 24/02/2022 01:58, Simon Glass wrote:
> Hi Alper,
> 
> On Tue, 15 Feb 2022 at 04:52, Alper Nebi Yasak <alpernebiyasak@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?).

>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.

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?

(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.)