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

[Simon Glass <sjg@chromium.org>]

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.

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?

Regards,
Simon


>
> >
> > Fix it.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  arch/x86/dts/u-boot.dtsi | 2 +-
> >  common/spl/spl.c         | 8 ++++----
> >  include/spl.h            | 4 ++--
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/dts/u-boot.dtsi b/arch/x86/dts/u-boot.dtsi
> > index ca84d18ad9..24e692f988 100644
> > --- a/arch/x86/dts/u-boot.dtsi
> > +++ b/arch/x86/dts/u-boot.dtsi
> > @@ -37,7 +37,7 @@
> >       u-boot-tpl-dtb {
> >       };
> >  #endif
> > -     spl {
> > +     u-boot-spl {
> >               type = "u-boot-spl";
>
> I guess the type can be removed now that it's the same as the node name.
>
> >               offset = <CONFIG_X86_OFFSET_SPL>;
> >       };
> > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > index 884102bdea..444907432c 100644
> > --- a/common/spl/spl.c
> > +++ b/common/spl/spl.c
> > @@ -54,8 +54,8 @@ binman_sym_declare(ulong, u_boot_any, image_pos);
> >  binman_sym_declare(ulong, u_boot_any, size);
> >
> >  #ifdef CONFIG_TPL
> > -binman_sym_declare(ulong, spl, image_pos);
> > -binman_sym_declare(ulong, spl, size);
> > +binman_sym_declare(ulong, u_boot_spl, image_pos);
> > +binman_sym_declare(ulong, u_boot_spl, size);
> >  #endif
> >
> >  /* Define board data structure */
> > @@ -143,14 +143,14 @@ void spl_fixup_fdt(void *fdt_blob)
> >  ulong spl_get_image_pos(void)
> >  {
> >       return spl_phase() == PHASE_TPL ?
> > -             binman_sym(ulong, spl, image_pos) :
> > +             binman_sym(ulong, u_boot_spl, image_pos) :
> >               binman_sym(ulong, u_boot_any, image_pos);
> >  }
> >
> >  ulong spl_get_image_size(void)
> >  {
> >       return spl_phase() == PHASE_TPL ?
> > -             binman_sym(ulong, spl, size) :
> > +             binman_sym(ulong, u_boot_spl, size) :
> >               binman_sym(ulong, u_boot_any, size);
> >  }
> >
> > diff --git a/include/spl.h b/include/spl.h
> > index bb92bc6ec6..8ceb3c0f09 100644
> > --- a/include/spl.h
> > +++ b/include/spl.h
> > @@ -269,8 +269,8 @@ struct spl_load_info {
> >   */
> >  binman_sym_extern(ulong, u_boot_any, image_pos);
> >  binman_sym_extern(ulong, u_boot_any, size);
> > -binman_sym_extern(ulong, spl, image_pos);
> > -binman_sym_extern(ulong, spl, size);
> > +binman_sym_extern(ulong, u_boot_spl, image_pos);
> > +binman_sym_extern(ulong, u_boot_spl, size);
> >
> >  /**
> >   * spl_get_image_pos() - get the image position of the next phase