Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL
Hi Alper, On Thu, 3 Mar 2022 at 14:14, Alper Nebi Yasak wrote: > > On 24/02/2022 01:58, Simon Glass wrote: > > Hi Alper, > > > > On Tue, 15 Feb 2022 at 04:52, Alper Nebi Yasak > > 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 discussionwhat 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
Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL
On 24/02/2022 01:58, Simon Glass wrote: > Hi Alper, > > On Tue, 15 Feb 2022 at 04:52, Alper Nebi Yasak > 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 discussionwhat 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.)
Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL
Hi Alper,
On Tue, 15 Feb 2022 at 04:52, Alper Nebi Yasak 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 discussionwhat do you think?
Regards,
Simon
>
> >
> > Fix it.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > 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 = ;
> > };
> > 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
Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL
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. > > Fix it. > > Signed-off-by: Simon Glass > --- > > arch/x86/dts/u-boot.dtsi | 2 +- > common/spl/spl.c | 8 > include/spl.h| 4 ++-- > 3 files changed, 7 insertions(+), 7 deletions(-) > Applied to u-boot-dm, thanks!
Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL
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.
>
> Fix it.
>
> Signed-off-by: Simon Glass
> ---
>
> 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 = ;
> };
> 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
[PATCH 03/24] spl: x86: Correct the binman symbols for SPL
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)
Fix it.
Signed-off-by: Simon Glass
---
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";
offset = ;
};
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
--
2.35.0.263.gb82422642f-goog
