[PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Tom Rini]

Introduce an option to control if we expect that a prior stage has
created a bloblist already and thus it is safe to scan the address. We
need to have this be configurable because we do not (and cannot) know if
the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
on-chip memory and so it may or may not be safe to read from this
address this early.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
Cc: Simon Glass <sjg@chromium.org>
---
 common/Kconfig | 32 ++++++++++++++++++++++++++++++++
 lib/fdtdec.c   | 11 +++--------
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/common/Kconfig b/common/Kconfig
index e8d89bf6eb9d..e8febad0f212 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
 	  is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
 	  proper), and this sane bloblist is used for subsequent phases.
 
+config BLOBLIST_PRIOR_STAGE
+	bool "Bloblist was created in a stage prior to U-Boot"
+	default y
+	depends on BLOBLIST_FIXED
+	help
+	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
+	  before U-Boot is started.
+
 config BLOBLIST_SIZE_RELOC
 	hex "Size of bloblist after relocation"
 	default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
@@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
 
 endchoice
 
+config SPL_BLOBLIST_PRIOR_STAGE
+	bool "Bloblist was created in a stage prior to SPL"
+	default y
+	depends on SPL_BLOBLIST_FIXED
+	help
+	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
+	  U-Boot SPL is started.
+
 endif # SPL_BLOBLIST
 
 if TPL_BLOBLIST
@@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
 
 endchoice
 
+config TPL_BLOBLIST_PRIOR_STAGE
+	bool "Bloblist was created in a stage prior to TPL"
+	default y
+	depends on TPL_BLOBLIST_FIXED
+	help
+	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
+	  U-Boot TPL is started.
+
 endif # TPL_BLOBLIST
 
 if VPL_BLOBLIST
@@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
 
 endchoice
 
+config VPL_BLOBLIST_PRIOR_STAGE
+	bool "Bloblist was created in a stage prior to VPL"
+	default y
+	depends on VPL_BLOBLIST_FIXED
+	help
+	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
+	  U-Boot VPL is started.
+
 endif # VPL_BLOBLIST
 
 endmenu
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index b06559880296..29bddab4150c 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
 	int ret = -ENOENT;
 
 	/*
-	 * If allowing a bloblist, check that first. There was discussion about
-	 * adding an OF_BLOBLIST Kconfig, but this was rejected.
-	 *
-	 * The necessary test is whether the previous phase passed a bloblist,
-	 * not whether this phase creates one.
+	 * Only scan for a bloblist and then if that bloblist contains a device
+	 * tree if we have been configured to expect one.
 	 */
-	if (CONFIG_IS_ENABLED(BLOBLIST) &&
-	    (xpl_prev_phase() != PHASE_TPL ||
-	     !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
+	if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
 		ret = bloblist_maybe_init();
 		if (!ret) {
 			gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
-- 
2.43.0

[PATCH 2/2] bloblist: Disable CONFIG_SPL_BLOBLIST_PRIOR_STAGE on some platforms

[Tom Rini]

A few platforms, notably chromebooks bob, coral and kevin, create the
bloblist that will be used in SPL and are not provided one by a prior
stage. They also define CONFIG_BLOBLIST_ADDR to be in DRAM and so we
cannot scan the area until after DRAM init happens in SPL.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
Cc: Simon Glass <sjg@chromium.org>
---
 configs/chromebook_bob_defconfig   | 1 +
 configs/chromebook_coral_defconfig | 1 +
 configs/chromebook_kevin_defconfig | 1 +
 3 files changed, 3 insertions(+)

diff --git a/configs/chromebook_bob_defconfig b/configs/chromebook_bob_defconfig
index decac2e19352..e44775506682 100644
--- a/configs/chromebook_bob_defconfig
+++ b/configs/chromebook_bob_defconfig
@@ -38,6 +38,7 @@ CONFIG_BOARD_EARLY_INIT_R=y
 CONFIG_BLOBLIST=y
 CONFIG_BLOBLIST_ADDR=0x100000
 CONFIG_BLOBLIST_SIZE=0x1000
+# CONFIG_SPL_BLOBLIST_PRIOR_STAGE is not set
 CONFIG_SPL_MAX_SIZE=0x1e000
 CONFIG_SPL_PAD_TO=0x7f8000
 CONFIG_HANDOFF=y
diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig
index 0fb730497386..a5ef4fd2675e 100644
--- a/configs/chromebook_coral_defconfig
+++ b/configs/chromebook_coral_defconfig
@@ -46,6 +46,7 @@ CONFIG_BLOBLIST=y
 # CONFIG_TPL_BLOBLIST is not set
 CONFIG_BLOBLIST_ADDR=0x100000
 CONFIG_BLOBLIST_SIZE=0x30000
+# CONFIG_SPL_BLOBLIST_PRIOR_STAGE is not set
 CONFIG_SPL_NO_BSS_LIMIT=y
 CONFIG_HANDOFF=y
 CONFIG_SPL_SEPARATE_BSS=y
diff --git a/configs/chromebook_kevin_defconfig b/configs/chromebook_kevin_defconfig
index 5bbea6c42a8b..27fcfe866ee1 100644
--- a/configs/chromebook_kevin_defconfig
+++ b/configs/chromebook_kevin_defconfig
@@ -39,6 +39,7 @@ CONFIG_BOARD_EARLY_INIT_R=y
 CONFIG_BLOBLIST=y
 CONFIG_BLOBLIST_ADDR=0x100000
 CONFIG_BLOBLIST_SIZE=0x1000
+# CONFIG_SPL_BLOBLIST_PRIOR_STAGE is not set
 CONFIG_SPL_MAX_SIZE=0x1e000
 CONFIG_SPL_PAD_TO=0x7f8000
 CONFIG_HANDOFF=y
-- 
2.43.0

Re: [PATCH 2/2] bloblist: Disable CONFIG_SPL_BLOBLIST_PRIOR_STAGE on some platforms

[Ilias Apalodimas]

On Thu, Dec 12, 2024 at 09:11:42AM -0600, Tom Rini wrote:
> A few platforms, notably chromebooks bob, coral and kevin, create the
> bloblist that will be used in SPL and are not provided one by a prior
> stage. They also define CONFIG_BLOBLIST_ADDR to be in DRAM and so we
> cannot scan the area until after DRAM init happens in SPL.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  configs/chromebook_bob_defconfig   | 1 +
>  configs/chromebook_coral_defconfig | 1 +
>  configs/chromebook_kevin_defconfig | 1 +
>  3 files changed, 3 insertions(+)
>
> diff --git a/configs/chromebook_bob_defconfig b/configs/chromebook_bob_defconfig
> index decac2e19352..e44775506682 100644
> --- a/configs/chromebook_bob_defconfig
> +++ b/configs/chromebook_bob_defconfig
> @@ -38,6 +38,7 @@ CONFIG_BOARD_EARLY_INIT_R=y
>  CONFIG_BLOBLIST=y
>  CONFIG_BLOBLIST_ADDR=0x100000
>  CONFIG_BLOBLIST_SIZE=0x1000
> +# CONFIG_SPL_BLOBLIST_PRIOR_STAGE is not set
>  CONFIG_SPL_MAX_SIZE=0x1e000
>  CONFIG_SPL_PAD_TO=0x7f8000
>  CONFIG_HANDOFF=y
> diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig
> index 0fb730497386..a5ef4fd2675e 100644
> --- a/configs/chromebook_coral_defconfig
> +++ b/configs/chromebook_coral_defconfig
> @@ -46,6 +46,7 @@ CONFIG_BLOBLIST=y
>  # CONFIG_TPL_BLOBLIST is not set
>  CONFIG_BLOBLIST_ADDR=0x100000
>  CONFIG_BLOBLIST_SIZE=0x30000
> +# CONFIG_SPL_BLOBLIST_PRIOR_STAGE is not set
>  CONFIG_SPL_NO_BSS_LIMIT=y
>  CONFIG_HANDOFF=y
>  CONFIG_SPL_SEPARATE_BSS=y
> diff --git a/configs/chromebook_kevin_defconfig b/configs/chromebook_kevin_defconfig
> index 5bbea6c42a8b..27fcfe866ee1 100644
> --- a/configs/chromebook_kevin_defconfig
> +++ b/configs/chromebook_kevin_defconfig
> @@ -39,6 +39,7 @@ CONFIG_BOARD_EARLY_INIT_R=y
>  CONFIG_BLOBLIST=y
>  CONFIG_BLOBLIST_ADDR=0x100000
>  CONFIG_BLOBLIST_SIZE=0x1000
> +# CONFIG_SPL_BLOBLIST_PRIOR_STAGE is not set
>  CONFIG_SPL_MAX_SIZE=0x1e000
>  CONFIG_SPL_PAD_TO=0x7f8000
>  CONFIG_HANDOFF=y
> --
> 2.43.0
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Ilias Apalodimas]

Hi Tom,

On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> Introduce an option to control if we expect that a prior stage has
> created a bloblist already and thus it is safe to scan the address. We
> need to have this be configurable because we do not (and cannot) know if
> the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> on-chip memory and so it may or may not be safe to read from this
> address this early.

Yes this patch makes instead of making the OF_SEPARATE implicitly take
priority

>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
>  lib/fdtdec.c   | 11 +++--------
>  2 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/common/Kconfig b/common/Kconfig
> index e8d89bf6eb9d..e8febad0f212 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
>  	  is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
>  	  proper), and this sane bloblist is used for subsequent phases.
>
> +config BLOBLIST_PRIOR_STAGE
> +	bool "Bloblist was created in a stage prior to U-Boot"
> +	default y
> +	depends on BLOBLIST_FIXED
> +	help
> +	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> +	  before U-Boot is started.
> +
>  config BLOBLIST_SIZE_RELOC
>  	hex "Size of bloblist after relocation"
>  	default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
>
>  endchoice
>
> +config SPL_BLOBLIST_PRIOR_STAGE
> +	bool "Bloblist was created in a stage prior to SPL"
> +	default y
> +	depends on SPL_BLOBLIST_FIXED
> +	help
> +	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> +	  U-Boot SPL is started.
> +
>  endif # SPL_BLOBLIST
>
>  if TPL_BLOBLIST
> @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
>
>  endchoice
>
> +config TPL_BLOBLIST_PRIOR_STAGE
> +	bool "Bloblist was created in a stage prior to TPL"
> +	default y
> +	depends on TPL_BLOBLIST_FIXED
> +	help
> +	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> +	  U-Boot TPL is started.
> +
>  endif # TPL_BLOBLIST
>
>  if VPL_BLOBLIST
> @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
>
>  endchoice
>
> +config VPL_BLOBLIST_PRIOR_STAGE
> +	bool "Bloblist was created in a stage prior to VPL"
> +	default y
> +	depends on VPL_BLOBLIST_FIXED
> +	help
> +	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> +	  U-Boot VPL is started.
> +
>  endif # VPL_BLOBLIST
>
>  endmenu
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index b06559880296..29bddab4150c 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
>  	int ret = -ENOENT;
>
>  	/*
> -	 * If allowing a bloblist, check that first. There was discussion about
> -	 * adding an OF_BLOBLIST Kconfig, but this was rejected.
> -	 *
> -	 * The necessary test is whether the previous phase passed a bloblist,
> -	 * not whether this phase creates one.
> +	 * Only scan for a bloblist and then if that bloblist contains a device
> +	 * tree if we have been configured to expect one.
>  	 */
> -	if (CONFIG_IS_ENABLED(BLOBLIST) &&
> -	    (xpl_prev_phase() != PHASE_TPL ||
> -	     !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> +	if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {

Without wider context I am not sure but if that check can fold in
bloblist_maybe_init() and return -X if the config isn't enabled, it's going
to be easier to read

Thanks
/Ilias
>  		ret = bloblist_maybe_init();
>  		if (!ret) {
>  			gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
> --
> 2.43.0
>

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4207 bytes --]

On Thu, Dec 12, 2024 at 06:34:44PM +0200, Ilias Apalodimas wrote:
> Hi Tom,
> 
> On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> > Introduce an option to control if we expect that a prior stage has
> > created a bloblist already and thus it is safe to scan the address. We
> > need to have this be configurable because we do not (and cannot) know if
> > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > on-chip memory and so it may or may not be safe to read from this
> > address this early.
> 
> Yes this patch makes instead of making the OF_SEPARATE implicitly take
> priority
> 
> >
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> > Cc: Simon Glass <sjg@chromium.org>
> > ---
> >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> >  lib/fdtdec.c   | 11 +++--------
> >  2 files changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/common/Kconfig b/common/Kconfig
> > index e8d89bf6eb9d..e8febad0f212 100644
> > --- a/common/Kconfig
> > +++ b/common/Kconfig
> > @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
> >  	  is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
> >  	  proper), and this sane bloblist is used for subsequent phases.
> >
> > +config BLOBLIST_PRIOR_STAGE
> > +	bool "Bloblist was created in a stage prior to U-Boot"
> > +	default y
> > +	depends on BLOBLIST_FIXED
> > +	help
> > +	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> > +	  before U-Boot is started.
> > +
> >  config BLOBLIST_SIZE_RELOC
> >  	hex "Size of bloblist after relocation"
> >  	default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> > @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
> >
> >  endchoice
> >
> > +config SPL_BLOBLIST_PRIOR_STAGE
> > +	bool "Bloblist was created in a stage prior to SPL"
> > +	default y
> > +	depends on SPL_BLOBLIST_FIXED
> > +	help
> > +	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > +	  U-Boot SPL is started.
> > +
> >  endif # SPL_BLOBLIST
> >
> >  if TPL_BLOBLIST
> > @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
> >
> >  endchoice
> >
> > +config TPL_BLOBLIST_PRIOR_STAGE
> > +	bool "Bloblist was created in a stage prior to TPL"
> > +	default y
> > +	depends on TPL_BLOBLIST_FIXED
> > +	help
> > +	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > +	  U-Boot TPL is started.
> > +
> >  endif # TPL_BLOBLIST
> >
> >  if VPL_BLOBLIST
> > @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
> >
> >  endchoice
> >
> > +config VPL_BLOBLIST_PRIOR_STAGE
> > +	bool "Bloblist was created in a stage prior to VPL"
> > +	default y
> > +	depends on VPL_BLOBLIST_FIXED
> > +	help
> > +	  Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > +	  U-Boot VPL is started.
> > +
> >  endif # VPL_BLOBLIST
> >
> >  endmenu
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > index b06559880296..29bddab4150c 100644
> > --- a/lib/fdtdec.c
> > +++ b/lib/fdtdec.c
> > @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
> >  	int ret = -ENOENT;
> >
> >  	/*
> > -	 * If allowing a bloblist, check that first. There was discussion about
> > -	 * adding an OF_BLOBLIST Kconfig, but this was rejected.
> > -	 *
> > -	 * The necessary test is whether the previous phase passed a bloblist,
> > -	 * not whether this phase creates one.
> > +	 * Only scan for a bloblist and then if that bloblist contains a device
> > +	 * tree if we have been configured to expect one.
> >  	 */
> > -	if (CONFIG_IS_ENABLED(BLOBLIST) &&
> > -	    (xpl_prev_phase() != PHASE_TPL ||
> > -	     !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> > +	if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
> 
> Without wider context I am not sure but if that check can fold in
> bloblist_maybe_init() and return -X if the config isn't enabled, it's going
> to be easier to read

Hmm. We'd have:
	if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) && 
	    !(ret = bloblist_maybe_init()) {
		/* We have a bloblist, see if it has FDT */
		gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
		....

And I don't think that reads better.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Ilias Apalodimas]

On Thu, 12 Dec 2024 at 19:02, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Dec 12, 2024 at 06:34:44PM +0200, Ilias Apalodimas wrote:
> > Hi Tom,
> >
> > On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> > > Introduce an option to control if we expect that a prior stage has
> > > created a bloblist already and thus it is safe to scan the address. We
> > > need to have this be configurable because we do not (and cannot) know if
> > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > on-chip memory and so it may or may not be safe to read from this
> > > address this early.
> >
> > Yes this patch makes instead of making the OF_SEPARATE implicitly take
> > priority
> >
> > >
> > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > ---
> > > Cc: Simon Glass <sjg@chromium.org>
> > > ---
> > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > >  lib/fdtdec.c   | 11 +++--------
> > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/common/Kconfig b/common/Kconfig
> > > index e8d89bf6eb9d..e8febad0f212 100644
> > > --- a/common/Kconfig
> > > +++ b/common/Kconfig
> > > @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
> > >       is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
> > >       proper), and this sane bloblist is used for subsequent phases.
> > >
> > > +config BLOBLIST_PRIOR_STAGE
> > > +   bool "Bloblist was created in a stage prior to U-Boot"
> > > +   default y
> > > +   depends on BLOBLIST_FIXED
> > > +   help
> > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> > > +     before U-Boot is started.
> > > +
> > >  config BLOBLIST_SIZE_RELOC
> > >     hex "Size of bloblist after relocation"
> > >     default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> > > @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
> > >
> > >  endchoice
> > >
> > > +config SPL_BLOBLIST_PRIOR_STAGE
> > > +   bool "Bloblist was created in a stage prior to SPL"
> > > +   default y
> > > +   depends on SPL_BLOBLIST_FIXED
> > > +   help
> > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > +     U-Boot SPL is started.
> > > +
> > >  endif # SPL_BLOBLIST
> > >
> > >  if TPL_BLOBLIST
> > > @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
> > >
> > >  endchoice
> > >
> > > +config TPL_BLOBLIST_PRIOR_STAGE
> > > +   bool "Bloblist was created in a stage prior to TPL"
> > > +   default y
> > > +   depends on TPL_BLOBLIST_FIXED
> > > +   help
> > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > +     U-Boot TPL is started.
> > > +
> > >  endif # TPL_BLOBLIST
> > >
> > >  if VPL_BLOBLIST
> > > @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
> > >
> > >  endchoice
> > >
> > > +config VPL_BLOBLIST_PRIOR_STAGE
> > > +   bool "Bloblist was created in a stage prior to VPL"
> > > +   default y
> > > +   depends on VPL_BLOBLIST_FIXED
> > > +   help
> > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > +     U-Boot VPL is started.
> > > +
> > >  endif # VPL_BLOBLIST
> > >
> > >  endmenu
> > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > index b06559880296..29bddab4150c 100644
> > > --- a/lib/fdtdec.c
> > > +++ b/lib/fdtdec.c
> > > @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
> > >     int ret = -ENOENT;
> > >
> > >     /*
> > > -    * If allowing a bloblist, check that first. There was discussion about
> > > -    * adding an OF_BLOBLIST Kconfig, but this was rejected.
> > > -    *
> > > -    * The necessary test is whether the previous phase passed a bloblist,
> > > -    * not whether this phase creates one.
> > > +    * Only scan for a bloblist and then if that bloblist contains a device
> > > +    * tree if we have been configured to expect one.
> > >      */
> > > -   if (CONFIG_IS_ENABLED(BLOBLIST) &&
> > > -       (xpl_prev_phase() != PHASE_TPL ||
> > > -        !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> > > +   if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
> >
> > Without wider context I am not sure but if that check can fold in
> > bloblist_maybe_init() and return -X if the config isn't enabled, it's going
> > to be easier to read
>
> Hmm. We'd have:
>         if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) &&
>             !(ret = bloblist_maybe_init()) {
>                 /* We have a bloblist, see if it has FDT */

I mean move the check inside bloblist_maybe_init()

Cheers
/Ilias
>                 gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
>                 ....
>
> And I don't think that reads better.
>
> --
> Tom

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5146 bytes --]

On Thu, Dec 12, 2024 at 07:08:34PM +0200, Ilias Apalodimas wrote:
> On Thu, 12 Dec 2024 at 19:02, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Dec 12, 2024 at 06:34:44PM +0200, Ilias Apalodimas wrote:
> > > Hi Tom,
> > >
> > > On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> > > > Introduce an option to control if we expect that a prior stage has
> > > > created a bloblist already and thus it is safe to scan the address. We
> > > > need to have this be configurable because we do not (and cannot) know if
> > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > on-chip memory and so it may or may not be safe to read from this
> > > > address this early.
> > >
> > > Yes this patch makes instead of making the OF_SEPARATE implicitly take
> > > priority
> > >
> > > >
> > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > ---
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > >  lib/fdtdec.c   | 11 +++--------
> > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/common/Kconfig b/common/Kconfig
> > > > index e8d89bf6eb9d..e8febad0f212 100644
> > > > --- a/common/Kconfig
> > > > +++ b/common/Kconfig
> > > > @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
> > > >       is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
> > > >       proper), and this sane bloblist is used for subsequent phases.
> > > >
> > > > +config BLOBLIST_PRIOR_STAGE
> > > > +   bool "Bloblist was created in a stage prior to U-Boot"
> > > > +   default y
> > > > +   depends on BLOBLIST_FIXED
> > > > +   help
> > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> > > > +     before U-Boot is started.
> > > > +
> > > >  config BLOBLIST_SIZE_RELOC
> > > >     hex "Size of bloblist after relocation"
> > > >     default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> > > > @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
> > > >
> > > >  endchoice
> > > >
> > > > +config SPL_BLOBLIST_PRIOR_STAGE
> > > > +   bool "Bloblist was created in a stage prior to SPL"
> > > > +   default y
> > > > +   depends on SPL_BLOBLIST_FIXED
> > > > +   help
> > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > +     U-Boot SPL is started.
> > > > +
> > > >  endif # SPL_BLOBLIST
> > > >
> > > >  if TPL_BLOBLIST
> > > > @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
> > > >
> > > >  endchoice
> > > >
> > > > +config TPL_BLOBLIST_PRIOR_STAGE
> > > > +   bool "Bloblist was created in a stage prior to TPL"
> > > > +   default y
> > > > +   depends on TPL_BLOBLIST_FIXED
> > > > +   help
> > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > +     U-Boot TPL is started.
> > > > +
> > > >  endif # TPL_BLOBLIST
> > > >
> > > >  if VPL_BLOBLIST
> > > > @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
> > > >
> > > >  endchoice
> > > >
> > > > +config VPL_BLOBLIST_PRIOR_STAGE
> > > > +   bool "Bloblist was created in a stage prior to VPL"
> > > > +   default y
> > > > +   depends on VPL_BLOBLIST_FIXED
> > > > +   help
> > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > +     U-Boot VPL is started.
> > > > +
> > > >  endif # VPL_BLOBLIST
> > > >
> > > >  endmenu
> > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > > index b06559880296..29bddab4150c 100644
> > > > --- a/lib/fdtdec.c
> > > > +++ b/lib/fdtdec.c
> > > > @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
> > > >     int ret = -ENOENT;
> > > >
> > > >     /*
> > > > -    * If allowing a bloblist, check that first. There was discussion about
> > > > -    * adding an OF_BLOBLIST Kconfig, but this was rejected.
> > > > -    *
> > > > -    * The necessary test is whether the previous phase passed a bloblist,
> > > > -    * not whether this phase creates one.
> > > > +    * Only scan for a bloblist and then if that bloblist contains a device
> > > > +    * tree if we have been configured to expect one.
> > > >      */
> > > > -   if (CONFIG_IS_ENABLED(BLOBLIST) &&
> > > > -       (xpl_prev_phase() != PHASE_TPL ||
> > > > -        !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> > > > +   if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
> > >
> > > Without wider context I am not sure but if that check can fold in
> > > bloblist_maybe_init() and return -X if the config isn't enabled, it's going
> > > to be easier to read
> >
> > Hmm. We'd have:
> >         if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) &&
> >             !(ret = bloblist_maybe_init()) {
> >                 /* We have a bloblist, see if it has FDT */
> 
> I mean move the check inside bloblist_maybe_init()

As we talked about on IRC, no, because the "maybe" in that function
means "find or create". So the expected usage here is that we have
platforms that create the bloblist in SPL (post DRAM init) and use that
with the HANDOFF mechanism to pass information to full U-Boot.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Ilias Apalodimas]

On Thu, 12 Dec 2024 at 19:27, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Dec 12, 2024 at 07:08:34PM +0200, Ilias Apalodimas wrote:
> > On Thu, 12 Dec 2024 at 19:02, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Dec 12, 2024 at 06:34:44PM +0200, Ilias Apalodimas wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> > > > > Introduce an option to control if we expect that a prior stage has
> > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > need to have this be configurable because we do not (and cannot) know if
> > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > address this early.
> > > >
> > > > Yes this patch makes instead of making the OF_SEPARATE implicitly take
> > > > priority
> > > >
> > > > >
> > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > ---
> > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > >  lib/fdtdec.c   | 11 +++--------
> > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/common/Kconfig b/common/Kconfig
> > > > > index e8d89bf6eb9d..e8febad0f212 100644
> > > > > --- a/common/Kconfig
> > > > > +++ b/common/Kconfig
> > > > > @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
> > > > >       is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
> > > > >       proper), and this sane bloblist is used for subsequent phases.
> > > > >
> > > > > +config BLOBLIST_PRIOR_STAGE
> > > > > +   bool "Bloblist was created in a stage prior to U-Boot"
> > > > > +   default y
> > > > > +   depends on BLOBLIST_FIXED
> > > > > +   help
> > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> > > > > +     before U-Boot is started.
> > > > > +
> > > > >  config BLOBLIST_SIZE_RELOC
> > > > >     hex "Size of bloblist after relocation"
> > > > >     default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> > > > > @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
> > > > >
> > > > >  endchoice
> > > > >
> > > > > +config SPL_BLOBLIST_PRIOR_STAGE
> > > > > +   bool "Bloblist was created in a stage prior to SPL"
> > > > > +   default y
> > > > > +   depends on SPL_BLOBLIST_FIXED
> > > > > +   help
> > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > +     U-Boot SPL is started.
> > > > > +
> > > > >  endif # SPL_BLOBLIST
> > > > >
> > > > >  if TPL_BLOBLIST
> > > > > @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
> > > > >
> > > > >  endchoice
> > > > >
> > > > > +config TPL_BLOBLIST_PRIOR_STAGE
> > > > > +   bool "Bloblist was created in a stage prior to TPL"
> > > > > +   default y
> > > > > +   depends on TPL_BLOBLIST_FIXED
> > > > > +   help
> > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > +     U-Boot TPL is started.
> > > > > +
> > > > >  endif # TPL_BLOBLIST
> > > > >
> > > > >  if VPL_BLOBLIST
> > > > > @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
> > > > >
> > > > >  endchoice
> > > > >
> > > > > +config VPL_BLOBLIST_PRIOR_STAGE
> > > > > +   bool "Bloblist was created in a stage prior to VPL"
> > > > > +   default y
> > > > > +   depends on VPL_BLOBLIST_FIXED
> > > > > +   help
> > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > +     U-Boot VPL is started.
> > > > > +
> > > > >  endif # VPL_BLOBLIST
> > > > >
> > > > >  endmenu
> > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > > > index b06559880296..29bddab4150c 100644
> > > > > --- a/lib/fdtdec.c
> > > > > +++ b/lib/fdtdec.c
> > > > > @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
> > > > >     int ret = -ENOENT;
> > > > >
> > > > >     /*
> > > > > -    * If allowing a bloblist, check that first. There was discussion about
> > > > > -    * adding an OF_BLOBLIST Kconfig, but this was rejected.
> > > > > -    *
> > > > > -    * The necessary test is whether the previous phase passed a bloblist,
> > > > > -    * not whether this phase creates one.
> > > > > +    * Only scan for a bloblist and then if that bloblist contains a device
> > > > > +    * tree if we have been configured to expect one.
> > > > >      */
> > > > > -   if (CONFIG_IS_ENABLED(BLOBLIST) &&
> > > > > -       (xpl_prev_phase() != PHASE_TPL ||
> > > > > -        !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> > > > > +   if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
> > > >
> > > > Without wider context I am not sure but if that check can fold in
> > > > bloblist_maybe_init() and return -X if the config isn't enabled, it's going
> > > > to be easier to read
> > >
> > > Hmm. We'd have:
> > >         if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) &&
> > >             !(ret = bloblist_maybe_init()) {
> > >                 /* We have a bloblist, see if it has FDT */
> >
> > I mean move the check inside bloblist_maybe_init()
>
> As we talked about on IRC, no, because the "maybe" in that function
> means "find or create". So the expected usage here is that we have
> platforms that create the bloblist in SPL (post DRAM init) and use that
> with the HANDOFF mechanism to pass information to full U-Boot.
>

Yes thanks
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> --
> Tom

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Simon Glass]

Hi Tom,

On Thu, 12 Dec 2024 at 10:29, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, 12 Dec 2024 at 19:27, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Dec 12, 2024 at 07:08:34PM +0200, Ilias Apalodimas wrote:
> > > On Thu, 12 Dec 2024 at 19:02, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Dec 12, 2024 at 06:34:44PM +0200, Ilias Apalodimas wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > need to have this be configurable because we do not (and cannot) know if
> > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > address this early.
> > > > >
> > > > > Yes this patch makes instead of making the OF_SEPARATE implicitly take
> > > > > priority
> > > > >
> > > > > >
> > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > ---
> > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/common/Kconfig b/common/Kconfig
> > > > > > index e8d89bf6eb9d..e8febad0f212 100644
> > > > > > --- a/common/Kconfig
> > > > > > +++ b/common/Kconfig
> > > > > > @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
> > > > > >       is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
> > > > > >       proper), and this sane bloblist is used for subsequent phases.
> > > > > >
> > > > > > +config BLOBLIST_PRIOR_STAGE
> > > > > > +   bool "Bloblist was created in a stage prior to U-Boot"
> > > > > > +   default y
> > > > > > +   depends on BLOBLIST_FIXED
> > > > > > +   help
> > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> > > > > > +     before U-Boot is started.
> > > > > > +
> > > > > >  config BLOBLIST_SIZE_RELOC
> > > > > >     hex "Size of bloblist after relocation"
> > > > > >     default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> > > > > > @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
> > > > > >
> > > > > >  endchoice
> > > > > >
> > > > > > +config SPL_BLOBLIST_PRIOR_STAGE
> > > > > > +   bool "Bloblist was created in a stage prior to SPL"
> > > > > > +   default y
> > > > > > +   depends on SPL_BLOBLIST_FIXED
> > > > > > +   help
> > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > +     U-Boot SPL is started.
> > > > > > +
> > > > > >  endif # SPL_BLOBLIST
> > > > > >
> > > > > >  if TPL_BLOBLIST
> > > > > > @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
> > > > > >
> > > > > >  endchoice
> > > > > >
> > > > > > +config TPL_BLOBLIST_PRIOR_STAGE
> > > > > > +   bool "Bloblist was created in a stage prior to TPL"
> > > > > > +   default y
> > > > > > +   depends on TPL_BLOBLIST_FIXED
> > > > > > +   help
> > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > +     U-Boot TPL is started.
> > > > > > +
> > > > > >  endif # TPL_BLOBLIST
> > > > > >
> > > > > >  if VPL_BLOBLIST
> > > > > > @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
> > > > > >
> > > > > >  endchoice
> > > > > >
> > > > > > +config VPL_BLOBLIST_PRIOR_STAGE
> > > > > > +   bool "Bloblist was created in a stage prior to VPL"
> > > > > > +   default y
> > > > > > +   depends on VPL_BLOBLIST_FIXED
> > > > > > +   help
> > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > +     U-Boot VPL is started.
> > > > > > +
> > > > > >  endif # VPL_BLOBLIST
> > > > > >
> > > > > >  endmenu
> > > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > > > > index b06559880296..29bddab4150c 100644
> > > > > > --- a/lib/fdtdec.c
> > > > > > +++ b/lib/fdtdec.c
> > > > > > @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
> > > > > >     int ret = -ENOENT;
> > > > > >
> > > > > >     /*
> > > > > > -    * If allowing a bloblist, check that first. There was discussion about
> > > > > > -    * adding an OF_BLOBLIST Kconfig, but this was rejected.
> > > > > > -    *
> > > > > > -    * The necessary test is whether the previous phase passed a bloblist,
> > > > > > -    * not whether this phase creates one.
> > > > > > +    * Only scan for a bloblist and then if that bloblist contains a device
> > > > > > +    * tree if we have been configured to expect one.
> > > > > >      */
> > > > > > -   if (CONFIG_IS_ENABLED(BLOBLIST) &&
> > > > > > -       (xpl_prev_phase() != PHASE_TPL ||
> > > > > > -        !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> > > > > > +   if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
> > > > >
> > > > > Without wider context I am not sure but if that check can fold in
> > > > > bloblist_maybe_init() and return -X if the config isn't enabled, it's going
> > > > > to be easier to read
> > > >
> > > > Hmm. We'd have:
> > > >         if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) &&
> > > >             !(ret = bloblist_maybe_init()) {
> > > >                 /* We have a bloblist, see if it has FDT */
> > >
> > > I mean move the check inside bloblist_maybe_init()
> >
> > As we talked about on IRC, no, because the "maybe" in that function
> > means "find or create". So the expected usage here is that we have
> > platforms that create the bloblist in SPL (post DRAM init) and use that
> > with the HANDOFF mechanism to pass information to full U-Boot.
> >
>
> Yes thanks
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

I am struggling to see how this is different from the patch I sent a year ago

https://patchwork.ozlabs.org/project/uboot/patch/20231226094625.221671-1-sjg@chromium.org/

Is it just a rebase and a rename?

Regards,
Simon

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7085 bytes --]

On Thu, Dec 12, 2024 at 08:38:32PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 12 Dec 2024 at 10:29, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Thu, 12 Dec 2024 at 19:27, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Dec 12, 2024 at 07:08:34PM +0200, Ilias Apalodimas wrote:
> > > > On Thu, 12 Dec 2024 at 19:02, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Dec 12, 2024 at 06:34:44PM +0200, Ilias Apalodimas wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> > > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > > need to have this be configurable because we do not (and cannot) know if
> > > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > > address this early.
> > > > > >
> > > > > > Yes this patch makes instead of making the OF_SEPARATE implicitly take
> > > > > > priority
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > > ---
> > > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/common/Kconfig b/common/Kconfig
> > > > > > > index e8d89bf6eb9d..e8febad0f212 100644
> > > > > > > --- a/common/Kconfig
> > > > > > > +++ b/common/Kconfig
> > > > > > > @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
> > > > > > >       is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
> > > > > > >       proper), and this sane bloblist is used for subsequent phases.
> > > > > > >
> > > > > > > +config BLOBLIST_PRIOR_STAGE
> > > > > > > +   bool "Bloblist was created in a stage prior to U-Boot"
> > > > > > > +   default y
> > > > > > > +   depends on BLOBLIST_FIXED
> > > > > > > +   help
> > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> > > > > > > +     before U-Boot is started.
> > > > > > > +
> > > > > > >  config BLOBLIST_SIZE_RELOC
> > > > > > >     hex "Size of bloblist after relocation"
> > > > > > >     default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> > > > > > > @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
> > > > > > >
> > > > > > >  endchoice
> > > > > > >
> > > > > > > +config SPL_BLOBLIST_PRIOR_STAGE
> > > > > > > +   bool "Bloblist was created in a stage prior to SPL"
> > > > > > > +   default y
> > > > > > > +   depends on SPL_BLOBLIST_FIXED
> > > > > > > +   help
> > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > +     U-Boot SPL is started.
> > > > > > > +
> > > > > > >  endif # SPL_BLOBLIST
> > > > > > >
> > > > > > >  if TPL_BLOBLIST
> > > > > > > @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
> > > > > > >
> > > > > > >  endchoice
> > > > > > >
> > > > > > > +config TPL_BLOBLIST_PRIOR_STAGE
> > > > > > > +   bool "Bloblist was created in a stage prior to TPL"
> > > > > > > +   default y
> > > > > > > +   depends on TPL_BLOBLIST_FIXED
> > > > > > > +   help
> > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > +     U-Boot TPL is started.
> > > > > > > +
> > > > > > >  endif # TPL_BLOBLIST
> > > > > > >
> > > > > > >  if VPL_BLOBLIST
> > > > > > > @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
> > > > > > >
> > > > > > >  endchoice
> > > > > > >
> > > > > > > +config VPL_BLOBLIST_PRIOR_STAGE
> > > > > > > +   bool "Bloblist was created in a stage prior to VPL"
> > > > > > > +   default y
> > > > > > > +   depends on VPL_BLOBLIST_FIXED
> > > > > > > +   help
> > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > +     U-Boot VPL is started.
> > > > > > > +
> > > > > > >  endif # VPL_BLOBLIST
> > > > > > >
> > > > > > >  endmenu
> > > > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > > > > > index b06559880296..29bddab4150c 100644
> > > > > > > --- a/lib/fdtdec.c
> > > > > > > +++ b/lib/fdtdec.c
> > > > > > > @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
> > > > > > >     int ret = -ENOENT;
> > > > > > >
> > > > > > >     /*
> > > > > > > -    * If allowing a bloblist, check that first. There was discussion about
> > > > > > > -    * adding an OF_BLOBLIST Kconfig, but this was rejected.
> > > > > > > -    *
> > > > > > > -    * The necessary test is whether the previous phase passed a bloblist,
> > > > > > > -    * not whether this phase creates one.
> > > > > > > +    * Only scan for a bloblist and then if that bloblist contains a device
> > > > > > > +    * tree if we have been configured to expect one.
> > > > > > >      */
> > > > > > > -   if (CONFIG_IS_ENABLED(BLOBLIST) &&
> > > > > > > -       (xpl_prev_phase() != PHASE_TPL ||
> > > > > > > -        !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> > > > > > > +   if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
> > > > > >
> > > > > > Without wider context I am not sure but if that check can fold in
> > > > > > bloblist_maybe_init() and return -X if the config isn't enabled, it's going
> > > > > > to be easier to read
> > > > >
> > > > > Hmm. We'd have:
> > > > >         if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) &&
> > > > >             !(ret = bloblist_maybe_init()) {
> > > > >                 /* We have a bloblist, see if it has FDT */
> > > >
> > > > I mean move the check inside bloblist_maybe_init()
> > >
> > > As we talked about on IRC, no, because the "maybe" in that function
> > > means "find or create". So the expected usage here is that we have
> > > platforms that create the bloblist in SPL (post DRAM init) and use that
> > > with the HANDOFF mechanism to pass information to full U-Boot.
> > >
> >
> > Yes thanks
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> I am struggling to see how this is different from the patch I sent a year ago
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20231226094625.221671-1-sjg@chromium.org/
> 
> Is it just a rebase and a rename?

Well, no. I came up with this after debugging the failure on Bob / Kevin
via Gitlab. This in fact allows for the device tree to NOT come from the
bloblist. The biggest problem with the "OF_BLOBLIST" thread from last
year is that your explanation of the problem doesn't match the problem
you have.

What's really tragically funny now is that Jonas' add TPL to Bob/Kevin
and init DRAM there means that we don't need this series for that
platform and only Coral is still doing the likely ill-advised thing of
saying the bloblist exists in DRAM without ensuring that DRAM will have
been initialized already.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Simon Glass]

Hi Tom,

On Fri, 13 Dec 2024 at 07:03, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Dec 12, 2024 at 08:38:32PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 12 Dec 2024 at 10:29, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > On Thu, 12 Dec 2024 at 19:27, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Dec 12, 2024 at 07:08:34PM +0200, Ilias Apalodimas wrote:
> > > > > On Thu, 12 Dec 2024 at 19:02, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Dec 12, 2024 at 06:34:44PM +0200, Ilias Apalodimas wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> > > > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > > > need to have this be configurable because we do not (and cannot) know if
> > > > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > > > address this early.
> > > > > > >
> > > > > > > Yes this patch makes instead of making the OF_SEPARATE implicitly take
> > > > > > > priority
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > > > ---
> > > > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > > > ---
> > > > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/common/Kconfig b/common/Kconfig
> > > > > > > > index e8d89bf6eb9d..e8febad0f212 100644
> > > > > > > > --- a/common/Kconfig
> > > > > > > > +++ b/common/Kconfig
> > > > > > > > @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
> > > > > > > >       is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
> > > > > > > >       proper), and this sane bloblist is used for subsequent phases.
> > > > > > > >
> > > > > > > > +config BLOBLIST_PRIOR_STAGE
> > > > > > > > +   bool "Bloblist was created in a stage prior to U-Boot"
> > > > > > > > +   default y
> > > > > > > > +   depends on BLOBLIST_FIXED
> > > > > > > > +   help
> > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> > > > > > > > +     before U-Boot is started.
> > > > > > > > +
> > > > > > > >  config BLOBLIST_SIZE_RELOC
> > > > > > > >     hex "Size of bloblist after relocation"
> > > > > > > >     default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> > > > > > > > @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
> > > > > > > >
> > > > > > > >  endchoice
> > > > > > > >
> > > > > > > > +config SPL_BLOBLIST_PRIOR_STAGE
> > > > > > > > +   bool "Bloblist was created in a stage prior to SPL"
> > > > > > > > +   default y
> > > > > > > > +   depends on SPL_BLOBLIST_FIXED
> > > > > > > > +   help
> > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > > +     U-Boot SPL is started.
> > > > > > > > +
> > > > > > > >  endif # SPL_BLOBLIST
> > > > > > > >
> > > > > > > >  if TPL_BLOBLIST
> > > > > > > > @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
> > > > > > > >
> > > > > > > >  endchoice
> > > > > > > >
> > > > > > > > +config TPL_BLOBLIST_PRIOR_STAGE
> > > > > > > > +   bool "Bloblist was created in a stage prior to TPL"
> > > > > > > > +   default y
> > > > > > > > +   depends on TPL_BLOBLIST_FIXED
> > > > > > > > +   help
> > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > > +     U-Boot TPL is started.
> > > > > > > > +
> > > > > > > >  endif # TPL_BLOBLIST
> > > > > > > >
> > > > > > > >  if VPL_BLOBLIST
> > > > > > > > @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
> > > > > > > >
> > > > > > > >  endchoice
> > > > > > > >
> > > > > > > > +config VPL_BLOBLIST_PRIOR_STAGE
> > > > > > > > +   bool "Bloblist was created in a stage prior to VPL"
> > > > > > > > +   default y
> > > > > > > > +   depends on VPL_BLOBLIST_FIXED
> > > > > > > > +   help
> > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > > +     U-Boot VPL is started.
> > > > > > > > +
> > > > > > > >  endif # VPL_BLOBLIST
> > > > > > > >
> > > > > > > >  endmenu
> > > > > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > > > > > > index b06559880296..29bddab4150c 100644
> > > > > > > > --- a/lib/fdtdec.c
> > > > > > > > +++ b/lib/fdtdec.c
> > > > > > > > @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
> > > > > > > >     int ret = -ENOENT;
> > > > > > > >
> > > > > > > >     /*
> > > > > > > > -    * If allowing a bloblist, check that first. There was discussion about
> > > > > > > > -    * adding an OF_BLOBLIST Kconfig, but this was rejected.
> > > > > > > > -    *
> > > > > > > > -    * The necessary test is whether the previous phase passed a bloblist,
> > > > > > > > -    * not whether this phase creates one.
> > > > > > > > +    * Only scan for a bloblist and then if that bloblist contains a device
> > > > > > > > +    * tree if we have been configured to expect one.
> > > > > > > >      */
> > > > > > > > -   if (CONFIG_IS_ENABLED(BLOBLIST) &&
> > > > > > > > -       (xpl_prev_phase() != PHASE_TPL ||
> > > > > > > > -        !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> > > > > > > > +   if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
> > > > > > >
> > > > > > > Without wider context I am not sure but if that check can fold in
> > > > > > > bloblist_maybe_init() and return -X if the config isn't enabled, it's going
> > > > > > > to be easier to read
> > > > > >
> > > > > > Hmm. We'd have:
> > > > > >         if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) &&
> > > > > >             !(ret = bloblist_maybe_init()) {
> > > > > >                 /* We have a bloblist, see if it has FDT */
> > > > >
> > > > > I mean move the check inside bloblist_maybe_init()
> > > >
> > > > As we talked about on IRC, no, because the "maybe" in that function
> > > > means "find or create". So the expected usage here is that we have
> > > > platforms that create the bloblist in SPL (post DRAM init) and use that
> > > > with the HANDOFF mechanism to pass information to full U-Boot.
> > > >
> > >
> > > Yes thanks
> > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
> > I am struggling to see how this is different from the patch I sent a year ago
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20231226094625.221671-1-sjg@chromium.org/
> >
> > Is it just a rebase and a rename?
>
> Well, no. I came up with this after debugging the failure on Bob / Kevin
> via Gitlab. This in fact allows for the device tree to NOT come from the
> bloblist.

Yes that was the point of my patch, to allow me to disable OF_BLOBLIST

> The biggest problem with the "OF_BLOBLIST" thread from last
> year is that your explanation of the problem doesn't match the problem
> you have.

At this point I am worried you might be one of those people who is never wrong.

>
> What's really tragically funny now is that Jonas' add TPL to Bob/Kevin
> and init DRAM there means that we don't need this series for that
> platform and only Coral is still doing the likely ill-advised thing of
> saying the bloblist exists in DRAM without ensuring that DRAM will have
> been initialized already.

At least someone is enjoying all this. The whole thing is a bad memory
and makes me sick to think of it.

Regards,
Simon

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 8463 bytes --]

On Fri, Dec 13, 2024 at 07:15:09AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 13 Dec 2024 at 07:03, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Dec 12, 2024 at 08:38:32PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 12 Dec 2024 at 10:29, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > On Thu, 12 Dec 2024 at 19:27, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Dec 12, 2024 at 07:08:34PM +0200, Ilias Apalodimas wrote:
> > > > > > On Thu, 12 Dec 2024 at 19:02, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Thu, Dec 12, 2024 at 06:34:44PM +0200, Ilias Apalodimas wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> > > > > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > > > > need to have this be configurable because we do not (and cannot) know if
> > > > > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > > > > address this early.
> > > > > > > >
> > > > > > > > Yes this patch makes instead of making the OF_SEPARATE implicitly take
> > > > > > > > priority
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > > > > ---
> > > > > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > > > > ---
> > > > > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/common/Kconfig b/common/Kconfig
> > > > > > > > > index e8d89bf6eb9d..e8febad0f212 100644
> > > > > > > > > --- a/common/Kconfig
> > > > > > > > > +++ b/common/Kconfig
> > > > > > > > > @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
> > > > > > > > >       is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
> > > > > > > > >       proper), and this sane bloblist is used for subsequent phases.
> > > > > > > > >
> > > > > > > > > +config BLOBLIST_PRIOR_STAGE
> > > > > > > > > +   bool "Bloblist was created in a stage prior to U-Boot"
> > > > > > > > > +   default y
> > > > > > > > > +   depends on BLOBLIST_FIXED
> > > > > > > > > +   help
> > > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> > > > > > > > > +     before U-Boot is started.
> > > > > > > > > +
> > > > > > > > >  config BLOBLIST_SIZE_RELOC
> > > > > > > > >     hex "Size of bloblist after relocation"
> > > > > > > > >     default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> > > > > > > > > @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
> > > > > > > > >
> > > > > > > > >  endchoice
> > > > > > > > >
> > > > > > > > > +config SPL_BLOBLIST_PRIOR_STAGE
> > > > > > > > > +   bool "Bloblist was created in a stage prior to SPL"
> > > > > > > > > +   default y
> > > > > > > > > +   depends on SPL_BLOBLIST_FIXED
> > > > > > > > > +   help
> > > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > > > +     U-Boot SPL is started.
> > > > > > > > > +
> > > > > > > > >  endif # SPL_BLOBLIST
> > > > > > > > >
> > > > > > > > >  if TPL_BLOBLIST
> > > > > > > > > @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
> > > > > > > > >
> > > > > > > > >  endchoice
> > > > > > > > >
> > > > > > > > > +config TPL_BLOBLIST_PRIOR_STAGE
> > > > > > > > > +   bool "Bloblist was created in a stage prior to TPL"
> > > > > > > > > +   default y
> > > > > > > > > +   depends on TPL_BLOBLIST_FIXED
> > > > > > > > > +   help
> > > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > > > +     U-Boot TPL is started.
> > > > > > > > > +
> > > > > > > > >  endif # TPL_BLOBLIST
> > > > > > > > >
> > > > > > > > >  if VPL_BLOBLIST
> > > > > > > > > @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
> > > > > > > > >
> > > > > > > > >  endchoice
> > > > > > > > >
> > > > > > > > > +config VPL_BLOBLIST_PRIOR_STAGE
> > > > > > > > > +   bool "Bloblist was created in a stage prior to VPL"
> > > > > > > > > +   default y
> > > > > > > > > +   depends on VPL_BLOBLIST_FIXED
> > > > > > > > > +   help
> > > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > > > +     U-Boot VPL is started.
> > > > > > > > > +
> > > > > > > > >  endif # VPL_BLOBLIST
> > > > > > > > >
> > > > > > > > >  endmenu
> > > > > > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > > > > > > > index b06559880296..29bddab4150c 100644
> > > > > > > > > --- a/lib/fdtdec.c
> > > > > > > > > +++ b/lib/fdtdec.c
> > > > > > > > > @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
> > > > > > > > >     int ret = -ENOENT;
> > > > > > > > >
> > > > > > > > >     /*
> > > > > > > > > -    * If allowing a bloblist, check that first. There was discussion about
> > > > > > > > > -    * adding an OF_BLOBLIST Kconfig, but this was rejected.
> > > > > > > > > -    *
> > > > > > > > > -    * The necessary test is whether the previous phase passed a bloblist,
> > > > > > > > > -    * not whether this phase creates one.
> > > > > > > > > +    * Only scan for a bloblist and then if that bloblist contains a device
> > > > > > > > > +    * tree if we have been configured to expect one.
> > > > > > > > >      */
> > > > > > > > > -   if (CONFIG_IS_ENABLED(BLOBLIST) &&
> > > > > > > > > -       (xpl_prev_phase() != PHASE_TPL ||
> > > > > > > > > -        !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> > > > > > > > > +   if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
> > > > > > > >
> > > > > > > > Without wider context I am not sure but if that check can fold in
> > > > > > > > bloblist_maybe_init() and return -X if the config isn't enabled, it's going
> > > > > > > > to be easier to read
> > > > > > >
> > > > > > > Hmm. We'd have:
> > > > > > >         if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) &&
> > > > > > >             !(ret = bloblist_maybe_init()) {
> > > > > > >                 /* We have a bloblist, see if it has FDT */
> > > > > >
> > > > > > I mean move the check inside bloblist_maybe_init()
> > > > >
> > > > > As we talked about on IRC, no, because the "maybe" in that function
> > > > > means "find or create". So the expected usage here is that we have
> > > > > platforms that create the bloblist in SPL (post DRAM init) and use that
> > > > > with the HANDOFF mechanism to pass information to full U-Boot.
> > > > >
> > > >
> > > > Yes thanks
> > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > >
> > > I am struggling to see how this is different from the patch I sent a year ago
> > >
> > > https://patchwork.ozlabs.org/project/uboot/patch/20231226094625.221671-1-sjg@chromium.org/
> > >
> > > Is it just a rebase and a rename?
> >
> > Well, no. I came up with this after debugging the failure on Bob / Kevin
> > via Gitlab. This in fact allows for the device tree to NOT come from the
> > bloblist.
> 
> Yes that was the point of my patch, to allow me to disable OF_BLOBLIST

The entire reason that you needed that was never ever explained or
someone would have told you to move the bloblist to IRAM instead of DDR.
Instead we went round and round about something else.

> > The biggest problem with the "OF_BLOBLIST" thread from last
> > year is that your explanation of the problem doesn't match the problem
> > you have.
> 
> At this point I am worried you might be one of those people who is never wrong.

If so, I'll be sure to continue to enjoy your company.

> > What's really tragically funny now is that Jonas' add TPL to Bob/Kevin
> > and init DRAM there means that we don't need this series for that
> > platform and only Coral is still doing the likely ill-advised thing of
> > saying the bloblist exists in DRAM without ensuring that DRAM will have
> > been initialized already.
> 
> At least someone is enjoying all this. The whole thing is a bad memory
> and makes me sick to think of it.

I mean in the Shakespeareian sense. None of us are enjoying it.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Simon Glass]

Hi Tom,

On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini@konsulko.com> wrote:
>
> Introduce an option to control if we expect that a prior stage has
> created a bloblist already and thus it is safe to scan the address. We
> need to have this be configurable because we do not (and cannot) know if
> the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> on-chip memory and so it may or may not be safe to read from this
> address this early.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
>  lib/fdtdec.c   | 11 +++--------
>  2 files changed, 35 insertions(+), 8 deletions(-)
>

Since this is the essentially the same as my OF_BLOBLIST patch, which
was rejected:

NAK

The issue is not whether some 'previous stage' set up a bloblist. For
example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
that TPL set it up. But it doesn't mean that TPL put the FDT in there.

You are basically adding an option to tell U-Boot not to look at the
bloblist when setting up the FDT, since it might not be there yet and
the machine will hang. So confusing. Only the board author knows the
intended flow of boot, so just let the board author decide! Within
U-Boot you can infer from the bloblist options which phase actually
creates the bloblist, and there is some attempt to do this in
bloblist_init(), although it is pretty messy ATM. Anyway, As soon as
you say that some secret-source blob might create a bloblist, you need
a way to describe that. But again, only the board author knows.

Regards,
Simon

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1663 bytes --]

On Fri, Dec 13, 2024 at 07:32:24AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini@konsulko.com> wrote:
> >
> > Introduce an option to control if we expect that a prior stage has
> > created a bloblist already and thus it is safe to scan the address. We
> > need to have this be configurable because we do not (and cannot) know if
> > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > on-chip memory and so it may or may not be safe to read from this
> > address this early.
> >
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> > Cc: Simon Glass <sjg@chromium.org>
> > ---
> >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> >  lib/fdtdec.c   | 11 +++--------
> >  2 files changed, 35 insertions(+), 8 deletions(-)
> >
> 
> Since this is the essentially the same as my OF_BLOBLIST patch, which
> was rejected:
> 
> NAK
> 
> The issue is not whether some 'previous stage' set up a bloblist. For
> example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
> that TPL set it up. But it doesn't mean that TPL put the FDT in there.

This is wrong. The root problem is saying that the bloblist is in
possibly uninitialized memory. The code is quite happy to NOT find the
device tree in the bloblist and continue searching other paths.

An alternative here would be to better document the requirements of
BLOBLIST_ADDR, and then just disable it on chromebook_coral until
someone is inclined to move DDR init in to TPL, or someone finds a
non-main-memory address to use for BLOBLIST_ADDR or switches to using
BLOBLIST_ALLOC.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Simon Glass]

Hi Tom,

On Fri, 13 Dec 2024 at 10:07, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Dec 13, 2024 at 07:32:24AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > Introduce an option to control if we expect that a prior stage has
> > > created a bloblist already and thus it is safe to scan the address. We
> > > need to have this be configurable because we do not (and cannot) know
if
> > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > on-chip memory and so it may or may not be safe to read from this
> > > address this early.
> > >
> > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > ---
> > > Cc: Simon Glass <sjg@chromium.org>
> > > ---
> > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > >  lib/fdtdec.c   | 11 +++--------
> > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > >
> >
> > Since this is the essentially the same as my OF_BLOBLIST patch, which
> > was rejected:
> >
> > NAK
> >
> > The issue is not whether some 'previous stage' set up a bloblist. For
> > example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
> > that TPL set it up. But it doesn't mean that TPL put the FDT in there.
>
> This is wrong. The root problem is saying that the bloblist is in
> possibly uninitialized memory. The code is quite happy to NOT find the
> device tree in the bloblist and continue searching other paths.

But until the bloblist is set up, it doesn't exist. We should not be
looking for a bloblist that we know is not there yet. The bloblist is set
up by bloblist_init(). You seem to be imputing your own semantics for
bloblist.

In SPL, init happens in board_init_r(), i.e. after RAM is set up. So don't
look in the bloblist until it is set up! It doesn't exist.

In U-Boot proper, we look for it before relocation, so we can relocate and
expand it for use with ACPI tables, etc.

Take a look at this patch, too:

3d6531803e1 bloblist: Support initing from multiple places

>
> An alternative here would be to better document the requirements of
> BLOBLIST_ADDR, and then just disable it on chromebook_coral until
> someone is inclined to move DDR init in to TPL, or someone finds a
> non-main-memory address to use for BLOBLIST_ADDR or switches to using
> BLOBLIST_ALLOC.

Coral's TPL runs in cache-as-ram, with a 30KB limit on code size. It cannot
do DDR init in TPL because the DDR blob is large (160KB?) and there is a
ton of setup to do first, in any case. When CAR goes away, its memory
vanishes, so you need to have copied the bloblist somewhere else (the
bloblist holds the MRC data which needs to be written to SPI flash later
on, when possible). This all works perfectly and there is really no need to
change anything.

Again, you seem to be imputing semantics to bloblist which don't exist.

Regards,
Simon

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4293 bytes --]

On Sun, Dec 15, 2024 at 05:25:24PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 13 Dec 2024 at 10:07, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Dec 13, 2024 at 07:32:24AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > Introduce an option to control if we expect that a prior stage has
> > > > created a bloblist already and thus it is safe to scan the address. We
> > > > need to have this be configurable because we do not (and cannot) know
> if
> > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > on-chip memory and so it may or may not be safe to read from this
> > > > address this early.
> > > >
> > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > ---
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > >  lib/fdtdec.c   | 11 +++--------
> > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > >
> > >
> > > Since this is the essentially the same as my OF_BLOBLIST patch, which
> > > was rejected:
> > >
> > > NAK
> > >
> > > The issue is not whether some 'previous stage' set up a bloblist. For
> > > example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
> > > that TPL set it up. But it doesn't mean that TPL put the FDT in there.
> >
> > This is wrong. The root problem is saying that the bloblist is in
> > possibly uninitialized memory. The code is quite happy to NOT find the
> > device tree in the bloblist and continue searching other paths.
> 
> But until the bloblist is set up, it doesn't exist.

An almost philosophical statement, yes. How should the generic code know
if it exists, or does not exist? That is the question.

> We should not be
> looking for a bloblist that we know is not there yet. The bloblist is set
> up by bloblist_init(). You seem to be imputing your own semantics for
> bloblist.

We don't know if the bloblist exists or not. That's the problem. It
could already exist. That's the whole reason we're in this particular
argument.

> In SPL, init happens in board_init_r(), i.e. after RAM is set up. So don't
> look in the bloblist until it is set up! It doesn't exist.

Unless of course it was setup before. We don't know if it was setup
before or not until we look. A big part of the problem is that for prior
to U-Boot bloblist we don't need to use main DRAM, the bloblist is tiny
and we have some other persistent memory available. Unfortunately, x86
does not.

> In U-Boot proper, we look for it before relocation, so we can relocate and
> expand it for use with ACPI tables, etc.

I'm not sure that's relevant. This means we've taken what we were passed
at a fixed address and allocated more space for it and are using it
somewhere else.

> Take a look at this patch, too:
> 
> 3d6531803e1 bloblist: Support initing from multiple places

I'm not sure it's relevant. It does show we have a problem of not
knowing if the bloblist exists, or not. And I don't think we're solving
that right today (nor does v1 of my patch).

> > An alternative here would be to better document the requirements of
> > BLOBLIST_ADDR, and then just disable it on chromebook_coral until
> > someone is inclined to move DDR init in to TPL, or someone finds a
> > non-main-memory address to use for BLOBLIST_ADDR or switches to using
> > BLOBLIST_ALLOC.
> 
> Coral's TPL runs in cache-as-ram, with a 30KB limit on code size. It cannot
> do DDR init in TPL because the DDR blob is large (160KB?) and there is a
> ton of setup to do first, in any case. When CAR goes away, its memory
> vanishes, so you need to have copied the bloblist somewhere else (the
> bloblist holds the MRC data which needs to be written to SPI flash later
> on, when possible). This all works perfectly and there is really no need to
> change anything.

Ah, so there's some of the details finally. And oh goodness, we're
writing to the SPI flash on each boot? That's not good for longevity...

> Again, you seem to be imputing semantics to bloblist which don't exist.

Unfortunately some things were missed along the way, and are still
missing.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Simon Glass]

Hi Tom,

On Mon, 16 Dec 2024 at 21:00, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Dec 15, 2024 at 05:25:24PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 13 Dec 2024 at 10:07, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Dec 13, 2024 at 07:32:24AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > Introduce an option to control if we expect that a prior stage has
> > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > need to have this be configurable because we do not (and cannot) know
> > if
> > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > address this early.
> > > > >
> > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > ---
> > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > >  lib/fdtdec.c   | 11 +++--------
> > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > >
> > > >
> > > > Since this is the essentially the same as my OF_BLOBLIST patch, which
> > > > was rejected:
> > > >
> > > > NAK
> > > >
> > > > The issue is not whether some 'previous stage' set up a bloblist. For
> > > > example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
> > > > that TPL set it up. But it doesn't mean that TPL put the FDT in there.
> > >
> > > This is wrong. The root problem is saying that the bloblist is in
> > > possibly uninitialized memory. The code is quite happy to NOT find the
> > > device tree in the bloblist and continue searching other paths.
> >
> > But until the bloblist is set up, it doesn't exist.
>
> An almost philosophical statement, yes. How should the generic code know
> if it exists, or does not exist? That is the question.

U-Boot code 'knows', since if this is the first phase which enables
CONFIG_IS_ENABLED(BLOBLOST), then the bloblist is available
before/after relocation as well as after RAM setup in SPL.

>
> > We should not be
> > looking for a bloblist that we know is not there yet. The bloblist is set
> > up by bloblist_init(). You seem to be imputing your own semantics for
> > bloblist.
>
> We don't know if the bloblist exists or not. That's the problem. It
> could already exist. That's the whole reason we're in this particular
> argument.

See above.

>
> > In SPL, init happens in board_init_r(), i.e. after RAM is set up. So don't
> > look in the bloblist until it is set up! It doesn't exist.
>
> Unless of course it was setup before. We don't know if it was setup
> before or not until we look. A big part of the problem is that for prior
> to U-Boot bloblist we don't need to use main DRAM, the bloblist is tiny
> and we have some other persistent memory available. Unfortunately, x86
> does not.

The only situation where this matters is for the devicetree, which I
why i created OF_BLOBLIST, a way to indicate that the bloblist should
be checked for a devicetree.

>
> > In U-Boot proper, we look for it before relocation, so we can relocate and
> > expand it for use with ACPI tables, etc.
>
> I'm not sure that's relevant. This means we've taken what we were passed
> at a fixed address and allocated more space for it and are using it
> somewhere else.

Yes

>
> > Take a look at this patch, too:
> >
> > 3d6531803e1 bloblist: Support initing from multiple places
>
> I'm not sure it's relevant. It does show we have a problem of not
> knowing if the bloblist exists, or not. And I don't think we're solving
> that right today (nor does v1 of my patch).

OK

>
> > > An alternative here would be to better document the requirements of
> > > BLOBLIST_ADDR, and then just disable it on chromebook_coral until
> > > someone is inclined to move DDR init in to TPL, or someone finds a
> > > non-main-memory address to use for BLOBLIST_ADDR or switches to using
> > > BLOBLIST_ALLOC.
> >
> > Coral's TPL runs in cache-as-ram, with a 30KB limit on code size. It cannot
> > do DDR init in TPL because the DDR blob is large (160KB?) and there is a
> > ton of setup to do first, in any case. When CAR goes away, its memory
> > vanishes, so you need to have copied the bloblist somewhere else (the
> > bloblist holds the MRC data which needs to be written to SPI flash later
> > on, when possible). This all works perfectly and there is really no need to
> > change anything.
>
> Ah, so there's some of the details finally. And oh goodness, we're
> writing to the SPI flash on each boot? That's not good for longevity...

It writes a sector each time, across an area which can hold quite a
few writes. It only writes when the details change, so it is fine.
This algorithm has worked in the field for years across tens of
millions of devices and is common on x86.

>
> > Again, you seem to be imputing semantics to bloblist which don't exist.
>
> Unfortunately some things were missed along the way, and are still
> missing.

Well we should have started from OF_BLOBLIST and then dealt with
problems as they came up. In fact, so far there are no problems which
it can't solve.

As it is, we have Raymond sending a patch to push the prior-stage mess
into TPM, for reasons which make no sense to me. It is just adding
confusion.

One other thing we would have likely done by now if my patch[1] had
gone in a year ago, is moving away from BLOBLIST_FIXED to using a
register protocol. That is more deterministic.

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7028 bytes --]

On Tue, Dec 17, 2024 at 12:45:46PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 16 Dec 2024 at 21:00, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Dec 15, 2024 at 05:25:24PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 13 Dec 2024 at 10:07, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Dec 13, 2024 at 07:32:24AM -0700, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > need to have this be configurable because we do not (and cannot) know
> > > if
> > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > address this early.
> > > > > >
> > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > ---
> > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > >
> > > > >
> > > > > Since this is the essentially the same as my OF_BLOBLIST patch, which
> > > > > was rejected:
> > > > >
> > > > > NAK
> > > > >
> > > > > The issue is not whether some 'previous stage' set up a bloblist. For
> > > > > example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
> > > > > that TPL set it up. But it doesn't mean that TPL put the FDT in there.
> > > >
> > > > This is wrong. The root problem is saying that the bloblist is in
> > > > possibly uninitialized memory. The code is quite happy to NOT find the
> > > > device tree in the bloblist and continue searching other paths.
> > >
> > > But until the bloblist is set up, it doesn't exist.
> >
> > An almost philosophical statement, yes. How should the generic code know
> > if it exists, or does not exist? That is the question.
> 
> U-Boot code 'knows', since if this is the first phase which enables
> CONFIG_IS_ENABLED(BLOBLOST), then the bloblist is available
> before/after relocation as well as after RAM setup in SPL.

And when U-Boot isn't the first stage?

> > > We should not be
> > > looking for a bloblist that we know is not there yet. The bloblist is set
> > > up by bloblist_init(). You seem to be imputing your own semantics for
> > > bloblist.
> >
> > We don't know if the bloblist exists or not. That's the problem. It
> > could already exist. That's the whole reason we're in this particular
> > argument.
> 
> See above.

Yes, you seem to be missing the case where U-Boot isn't the first stage.

> > > In SPL, init happens in board_init_r(), i.e. after RAM is set up. So don't
> > > look in the bloblist until it is set up! It doesn't exist.
> >
> > Unless of course it was setup before. We don't know if it was setup
> > before or not until we look. A big part of the problem is that for prior
> > to U-Boot bloblist we don't need to use main DRAM, the bloblist is tiny
> > and we have some other persistent memory available. Unfortunately, x86
> > does not.
> 
> The only situation where this matters is for the devicetree, which I
> why i created OF_BLOBLIST, a way to indicate that the bloblist should
> be checked for a devicetree.

Yes, you keep looking at this from the wrong direction and seem to have
backed yourself in to a corner. Why are you insisting that for the
normal use case of memory infos and maybe display parameters we need to
carve out a hunk of main memory? A bloblist is perfectly capable of NOT
containing things. We can avoid having to care about what phase of
things we're in too.

> > > In U-Boot proper, we look for it before relocation, so we can relocate and
> > > expand it for use with ACPI tables, etc.
> >
> > I'm not sure that's relevant. This means we've taken what we were passed
> > at a fixed address and allocated more space for it and are using it
> > somewhere else.
> 
> Yes
> 
> >
> > > Take a look at this patch, too:
> > >
> > > 3d6531803e1 bloblist: Support initing from multiple places
> >
> > I'm not sure it's relevant. It does show we have a problem of not
> > knowing if the bloblist exists, or not. And I don't think we're solving
> > that right today (nor does v1 of my patch).
> 
> OK
> 
> >
> > > > An alternative here would be to better document the requirements of
> > > > BLOBLIST_ADDR, and then just disable it on chromebook_coral until
> > > > someone is inclined to move DDR init in to TPL, or someone finds a
> > > > non-main-memory address to use for BLOBLIST_ADDR or switches to using
> > > > BLOBLIST_ALLOC.
> > >
> > > Coral's TPL runs in cache-as-ram, with a 30KB limit on code size. It cannot
> > > do DDR init in TPL because the DDR blob is large (160KB?) and there is a
> > > ton of setup to do first, in any case. When CAR goes away, its memory
> > > vanishes, so you need to have copied the bloblist somewhere else (the
> > > bloblist holds the MRC data which needs to be written to SPI flash later
> > > on, when possible). This all works perfectly and there is really no need to
> > > change anything.
> >
> > Ah, so there's some of the details finally. And oh goodness, we're
> > writing to the SPI flash on each boot? That's not good for longevity...
> 
> It writes a sector each time, across an area which can hold quite a
> few writes. It only writes when the details change, so it is fine.
> This algorithm has worked in the field for years across tens of
> millions of devices and is common on x86.

To be clear, I wasn't saying that its your design.

> > > Again, you seem to be imputing semantics to bloblist which don't exist.
> >
> > Unfortunately some things were missed along the way, and are still
> > missing.
> 
> Well we should have started from OF_BLOBLIST and then dealt with
> problems as they came up. In fact, so far there are no problems which
> it can't solve.
> 
> As it is, we have Raymond sending a patch to push the prior-stage mess
> into TPM, for reasons which make no sense to me. It is just adding
> confusion.
> 
> One other thing we would have likely done by now if my patch[1] had
> gone in a year ago, is moving away from BLOBLIST_FIXED to using a
> register protocol. That is more deterministic.
> 
> Regards,
> Simon
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/

I think once again your obsession with OF_BLOBLIST and device tree is
causing you to miss everything else. Today we can take a *bloblist* via
register. Linaro wrote that, even. The next problem really is that we
can't pass that bloblist along to the next stage because all of the
U-Boot side of things assumes fixed address.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Simon Glass]

Hi Tom,

On Tue, 17 Dec 2024 at 13:15, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 17, 2024 at 12:45:46PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 16 Dec 2024 at 21:00, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Dec 15, 2024 at 05:25:24PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Fri, 13 Dec 2024 at 10:07, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Fri, Dec 13, 2024 at 07:32:24AM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > > need to have this be configurable because we do not (and cannot) know
> > > > if
> > > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > > address this early.
> > > > > > >
> > > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > > ---
> > > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > >
> > > > > > Since this is the essentially the same as my OF_BLOBLIST patch, which
> > > > > > was rejected:
> > > > > >
> > > > > > NAK
> > > > > >
> > > > > > The issue is not whether some 'previous stage' set up a bloblist. For
> > > > > > example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
> > > > > > that TPL set it up. But it doesn't mean that TPL put the FDT in there.
> > > > >
> > > > > This is wrong. The root problem is saying that the bloblist is in
> > > > > possibly uninitialized memory. The code is quite happy to NOT find the
> > > > > device tree in the bloblist and continue searching other paths.
> > > >
> > > > But until the bloblist is set up, it doesn't exist.
> > >
> > > An almost philosophical statement, yes. How should the generic code know
> > > if it exists, or does not exist? That is the question.
> >
> > U-Boot code 'knows', since if this is the first phase which enables
> > CONFIG_IS_ENABLED(BLOBLOST), then the bloblist is available
> > before/after relocation as well as after RAM setup in SPL.
>
> And when U-Boot isn't the first stage?

The only case where that matters is for the devicetree, so we have
OF_BLOBLIST to control whether U-Boot looks for it. Eventually we can
drop BLOBLIST_FIXED and use registers, perhaps even on x86, which will
make things easier.

>
> > > > We should not be
> > > > looking for a bloblist that we know is not there yet. The bloblist is set
> > > > up by bloblist_init(). You seem to be imputing your own semantics for
> > > > bloblist.
> > >
> > > We don't know if the bloblist exists or not. That's the problem. It
> > > could already exist. That's the whole reason we're in this particular
> > > argument.
> >
> > See above.
>
> Yes, you seem to be missing the case where U-Boot isn't the first stage.

I really can't see how you came to that conclusion. Just enable
OF_BLOBLIST - in fact we can enable it for almost all boards today and
it won't harm anything.

>
> > > > In SPL, init happens in board_init_r(), i.e. after RAM is set up. So don't
> > > > look in the bloblist until it is set up! It doesn't exist.
> > >
> > > Unless of course it was setup before. We don't know if it was setup
> > > before or not until we look. A big part of the problem is that for prior
> > > to U-Boot bloblist we don't need to use main DRAM, the bloblist is tiny
> > > and we have some other persistent memory available. Unfortunately, x86
> > > does not.
> >
> > The only situation where this matters is for the devicetree, which I
> > why i created OF_BLOBLIST, a way to indicate that the bloblist should
> > be checked for a devicetree.
>
> Yes, you keep looking at this from the wrong direction and seem to have
> backed yourself in to a corner. Why are you insisting that for the
> normal use case of memory infos and maybe display parameters we need to
> carve out a hunk of main memory? A bloblist is perfectly capable of NOT
> containing things. We can avoid having to care about what phase of
> things we're in too.

Devicetree is the only 'strange' thing here because we need to look at
it early.  For Raymond's patch, it shouldn't need any conditions.

>
> > > > In U-Boot proper, we look for it before relocation, so we can relocate and
> > > > expand it for use with ACPI tables, etc.
> > >
> > > I'm not sure that's relevant. This means we've taken what we were passed
> > > at a fixed address and allocated more space for it and are using it
> > > somewhere else.
> >
> > Yes
> >
> > >
> > > > Take a look at this patch, too:
> > > >
> > > > 3d6531803e1 bloblist: Support initing from multiple places
> > >
> > > I'm not sure it's relevant. It does show we have a problem of not
> > > knowing if the bloblist exists, or not. And I don't think we're solving
> > > that right today (nor does v1 of my patch).
> >
> > OK
> >
> > >
> > > > > An alternative here would be to better document the requirements of
> > > > > BLOBLIST_ADDR, and then just disable it on chromebook_coral until
> > > > > someone is inclined to move DDR init in to TPL, or someone finds a
> > > > > non-main-memory address to use for BLOBLIST_ADDR or switches to using
> > > > > BLOBLIST_ALLOC.
> > > >
> > > > Coral's TPL runs in cache-as-ram, with a 30KB limit on code size. It cannot
> > > > do DDR init in TPL because the DDR blob is large (160KB?) and there is a
> > > > ton of setup to do first, in any case. When CAR goes away, its memory
> > > > vanishes, so you need to have copied the bloblist somewhere else (the
> > > > bloblist holds the MRC data which needs to be written to SPI flash later
> > > > on, when possible). This all works perfectly and there is really no need to
> > > > change anything.
> > >
> > > Ah, so there's some of the details finally. And oh goodness, we're
> > > writing to the SPI flash on each boot? That's not good for longevity...
> >
> > It writes a sector each time, across an area which can hold quite a
> > few writes. It only writes when the details change, so it is fine.
> > This algorithm has worked in the field for years across tens of
> > millions of devices and is common on x86.
>
> To be clear, I wasn't saying that its your design.

No, it isn't my design, but I would be happy if it were. You were
suggesting it had a longevity problem, so I wanted to explain it.

>
> > > > Again, you seem to be imputing semantics to bloblist which don't exist.
> > >
> > > Unfortunately some things were missed along the way, and are still
> > > missing.
> >
> > Well we should have started from OF_BLOBLIST and then dealt with
> > problems as they came up. In fact, so far there are no problems which
> > it can't solve.
> >
> > As it is, we have Raymond sending a patch to push the prior-stage mess
> > into TPM, for reasons which make no sense to me. It is just adding
> > confusion.
> >
> > One other thing we would have likely done by now if my patch[1] had
> > gone in a year ago, is moving away from BLOBLIST_FIXED to using a
> > register protocol. That is more deterministic.
> >
> > Regards,
> > Simon
> >
> > [1] https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/
>
> I think once again your obsession with OF_BLOBLIST and device tree is
> causing you to miss everything else. Today we can take a *bloblist* via
> register. Linaro wrote that, even. The next problem really is that we
> can't pass that bloblist along to the next stage because all of the
> U-Boot side of things assumes fixed address.

Perhaps if we had applied OF_BLOBLIST a year ago, so my special 3
ancient chromebooks that nobody cares about or uses apart from me,
worked, we could have got bloblist to where it should be today. You
are missing the big picture, but stopping me from pursuing it.

I am disappointed that it is only at this stage that you have shown
any interest in getting my special 3 ancient chromebooks running.

Regards,
Simon

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 10330 bytes --]

On Tue, Dec 17, 2024 at 04:42:26PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 17 Dec 2024 at 13:15, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Dec 17, 2024 at 12:45:46PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 16 Dec 2024 at 21:00, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Dec 15, 2024 at 05:25:24PM -0700, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Fri, 13 Dec 2024 at 10:07, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Fri, Dec 13, 2024 at 07:32:24AM -0700, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > > > need to have this be configurable because we do not (and cannot) know
> > > > > if
> > > > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > > > address this early.
> > > > > > > >
> > > > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > > > ---
> > > > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > > > ---
> > > > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > >
> > > > > > > Since this is the essentially the same as my OF_BLOBLIST patch, which
> > > > > > > was rejected:
> > > > > > >
> > > > > > > NAK
> > > > > > >
> > > > > > > The issue is not whether some 'previous stage' set up a bloblist. For
> > > > > > > example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
> > > > > > > that TPL set it up. But it doesn't mean that TPL put the FDT in there.
> > > > > >
> > > > > > This is wrong. The root problem is saying that the bloblist is in
> > > > > > possibly uninitialized memory. The code is quite happy to NOT find the
> > > > > > device tree in the bloblist and continue searching other paths.
> > > > >
> > > > > But until the bloblist is set up, it doesn't exist.
> > > >
> > > > An almost philosophical statement, yes. How should the generic code know
> > > > if it exists, or does not exist? That is the question.
> > >
> > > U-Boot code 'knows', since if this is the first phase which enables
> > > CONFIG_IS_ENABLED(BLOBLOST), then the bloblist is available
> > > before/after relocation as well as after RAM setup in SPL.
> >
> > And when U-Boot isn't the first stage?
> 
> The only case where that matters is for the devicetree, so we have
> OF_BLOBLIST to control whether U-Boot looks for it. Eventually we can
> drop BLOBLIST_FIXED and use registers, perhaps even on x86, which will
> make things easier.

The only reason it ever matters is if you're picking memory that's not
always accessible. Which is a bad choice and should be avoided whenever
possible and practical. Which it is here.

> > > > > We should not be
> > > > > looking for a bloblist that we know is not there yet. The bloblist is set
> > > > > up by bloblist_init(). You seem to be imputing your own semantics for
> > > > > bloblist.
> > > >
> > > > We don't know if the bloblist exists or not. That's the problem. It
> > > > could already exist. That's the whole reason we're in this particular
> > > > argument.
> > >
> > > See above.
> >
> > Yes, you seem to be missing the case where U-Boot isn't the first stage.
> 
> I really can't see how you came to that conclusion. Just enable
> OF_BLOBLIST - in fact we can enable it for almost all boards today and
> it won't harm anything.

I mean, there's very few places using BLOBLIST today. But we can't
enable OF_BLOBLIST for any of them unless we say that the BLOBLIST
exists in always accessible memory. And that wouldn't be most of them.

> > > > > In SPL, init happens in board_init_r(), i.e. after RAM is set up. So don't
> > > > > look in the bloblist until it is set up! It doesn't exist.
> > > >
> > > > Unless of course it was setup before. We don't know if it was setup
> > > > before or not until we look. A big part of the problem is that for prior
> > > > to U-Boot bloblist we don't need to use main DRAM, the bloblist is tiny
> > > > and we have some other persistent memory available. Unfortunately, x86
> > > > does not.
> > >
> > > The only situation where this matters is for the devicetree, which I
> > > why i created OF_BLOBLIST, a way to indicate that the bloblist should
> > > be checked for a devicetree.
> >
> > Yes, you keep looking at this from the wrong direction and seem to have
> > backed yourself in to a corner. Why are you insisting that for the
> > normal use case of memory infos and maybe display parameters we need to
> > carve out a hunk of main memory? A bloblist is perfectly capable of NOT
> > containing things. We can avoid having to care about what phase of
> > things we're in too.
> 
> Devicetree is the only 'strange' thing here because we need to look at
> it early.

Device tree is also "strange" in that it's the only thing we read (not
write) today prior to U-Boot too from bloblist.

> For Raymond's patch, it shouldn't need any conditions.

Yes, and I replied to him that way too.

> > > > > In U-Boot proper, we look for it before relocation, so we can relocate and
> > > > > expand it for use with ACPI tables, etc.
> > > >
> > > > I'm not sure that's relevant. This means we've taken what we were passed
> > > > at a fixed address and allocated more space for it and are using it
> > > > somewhere else.
> > >
> > > Yes
> > >
> > > >
> > > > > Take a look at this patch, too:
> > > > >
> > > > > 3d6531803e1 bloblist: Support initing from multiple places
> > > >
> > > > I'm not sure it's relevant. It does show we have a problem of not
> > > > knowing if the bloblist exists, or not. And I don't think we're solving
> > > > that right today (nor does v1 of my patch).
> > >
> > > OK
> > >
> > > >
> > > > > > An alternative here would be to better document the requirements of
> > > > > > BLOBLIST_ADDR, and then just disable it on chromebook_coral until
> > > > > > someone is inclined to move DDR init in to TPL, or someone finds a
> > > > > > non-main-memory address to use for BLOBLIST_ADDR or switches to using
> > > > > > BLOBLIST_ALLOC.
> > > > >
> > > > > Coral's TPL runs in cache-as-ram, with a 30KB limit on code size. It cannot
> > > > > do DDR init in TPL because the DDR blob is large (160KB?) and there is a
> > > > > ton of setup to do first, in any case. When CAR goes away, its memory
> > > > > vanishes, so you need to have copied the bloblist somewhere else (the
> > > > > bloblist holds the MRC data which needs to be written to SPI flash later
> > > > > on, when possible). This all works perfectly and there is really no need to
> > > > > change anything.
> > > >
> > > > Ah, so there's some of the details finally. And oh goodness, we're
> > > > writing to the SPI flash on each boot? That's not good for longevity...
> > >
> > > It writes a sector each time, across an area which can hold quite a
> > > few writes. It only writes when the details change, so it is fine.
> > > This algorithm has worked in the field for years across tens of
> > > millions of devices and is common on x86.
> >
> > To be clear, I wasn't saying that its your design.
> 
> No, it isn't my design, but I would be happy if it were. You were
> suggesting it had a longevity problem, so I wanted to explain it.
> 
> >
> > > > > Again, you seem to be imputing semantics to bloblist which don't exist.
> > > >
> > > > Unfortunately some things were missed along the way, and are still
> > > > missing.
> > >
> > > Well we should have started from OF_BLOBLIST and then dealt with
> > > problems as they came up. In fact, so far there are no problems which
> > > it can't solve.
> > >
> > > As it is, we have Raymond sending a patch to push the prior-stage mess
> > > into TPM, for reasons which make no sense to me. It is just adding
> > > confusion.
> > >
> > > One other thing we would have likely done by now if my patch[1] had
> > > gone in a year ago, is moving away from BLOBLIST_FIXED to using a
> > > register protocol. That is more deterministic.
> > >
> > > Regards,
> > > Simon
> > >
> > > [1] https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/
> >
> > I think once again your obsession with OF_BLOBLIST and device tree is
> > causing you to miss everything else. Today we can take a *bloblist* via
> > register. Linaro wrote that, even. The next problem really is that we
> > can't pass that bloblist along to the next stage because all of the
> > U-Boot side of things assumes fixed address.
> 
> Perhaps if we had applied OF_BLOBLIST a year ago, so my special 3
> ancient chromebooks that nobody cares about or uses apart from me,
> worked, we could have got bloblist to where it should be today. You
> are missing the big picture, but stopping me from pursuing it.
> 
> I am disappointed that it is only at this stage that you have shown
> any interest in getting my special 3 ancient chromebooks running.

And I am disappointed that it's only at this stage there's an
understanding of what the actual problem is with your special 3 ancient
chromebooks. Everything last year was read by me as you arguing that you
need a way to control passing the device tree because that's where the
device tree is:
https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/#3174895
And if not every at least most of your other replies in that thread are
about how device tree is passed in. This is why I assumed that your
boards were working after having a way to enforce that the device tree
comes from bloblist.

So if there's some "big picture" about bloblist being delayed, it is
because once again your vision isn't communicated to everyone else
you're trying to work with and that is the big problem.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

[Simon Glass]

Hi Tom,

On Tue, 17 Dec 2024 at 17:18, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 17, 2024 at 04:42:26PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 17 Dec 2024 at 13:15, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Dec 17, 2024 at 12:45:46PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 16 Dec 2024 at 21:00, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Dec 15, 2024 at 05:25:24PM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Fri, 13 Dec 2024 at 10:07, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Fri, Dec 13, 2024 at 07:32:24AM -0700, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > > > > need to have this be configurable because we do not (and cannot) know
> > > > > > if
> > > > > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > > > > address this early.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > > > > ---
> > > > > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > > > > ---
> > > > > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > > > > >
> > > > > > > >
> > > > > > > > Since this is the essentially the same as my OF_BLOBLIST patch, which
> > > > > > > > was rejected:
> > > > > > > >
> > > > > > > > NAK
> > > > > > > >
> > > > > > > > The issue is not whether some 'previous stage' set up a bloblist. For
> > > > > > > > example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
> > > > > > > > that TPL set it up. But it doesn't mean that TPL put the FDT in there.
> > > > > > >
> > > > > > > This is wrong. The root problem is saying that the bloblist is in
> > > > > > > possibly uninitialized memory. The code is quite happy to NOT find the
> > > > > > > device tree in the bloblist and continue searching other paths.
> > > > > >
> > > > > > But until the bloblist is set up, it doesn't exist.
> > > > >
> > > > > An almost philosophical statement, yes. How should the generic code know
> > > > > if it exists, or does not exist? That is the question.
> > > >
> > > > U-Boot code 'knows', since if this is the first phase which enables
> > > > CONFIG_IS_ENABLED(BLOBLOST), then the bloblist is available
> > > > before/after relocation as well as after RAM setup in SPL.
> > >
> > > And when U-Boot isn't the first stage?
> >
> > The only case where that matters is for the devicetree, so we have
> > OF_BLOBLIST to control whether U-Boot looks for it. Eventually we can
> > drop BLOBLIST_FIXED and use registers, perhaps even on x86, which will
> > make things easier.
>
> The only reason it ever matters is if you're picking memory that's not
> always accessible. Which is a bad choice and should be avoided whenever
> possible and practical. Which it is here.
>
> > > > > > We should not be
> > > > > > looking for a bloblist that we know is not there yet. The bloblist is set
> > > > > > up by bloblist_init(). You seem to be imputing your own semantics for
> > > > > > bloblist.
> > > > >
> > > > > We don't know if the bloblist exists or not. That's the problem. It
> > > > > could already exist. That's the whole reason we're in this particular
> > > > > argument.
> > > >
> > > > See above.
> > >
> > > Yes, you seem to be missing the case where U-Boot isn't the first stage.
> >
> > I really can't see how you came to that conclusion. Just enable
> > OF_BLOBLIST - in fact we can enable it for almost all boards today and
> > it won't harm anything.
>
> I mean, there's very few places using BLOBLIST today. But we can't
> enable OF_BLOBLIST for any of them unless we say that the BLOBLIST
> exists in always accessible memory. And that wouldn't be most of them.
>
> > > > > > In SPL, init happens in board_init_r(), i.e. after RAM is set up. So don't
> > > > > > look in the bloblist until it is set up! It doesn't exist.
> > > > >
> > > > > Unless of course it was setup before. We don't know if it was setup
> > > > > before or not until we look. A big part of the problem is that for prior
> > > > > to U-Boot bloblist we don't need to use main DRAM, the bloblist is tiny
> > > > > and we have some other persistent memory available. Unfortunately, x86
> > > > > does not.
> > > >
> > > > The only situation where this matters is for the devicetree, which I
> > > > why i created OF_BLOBLIST, a way to indicate that the bloblist should
> > > > be checked for a devicetree.
> > >
> > > Yes, you keep looking at this from the wrong direction and seem to have
> > > backed yourself in to a corner. Why are you insisting that for the
> > > normal use case of memory infos and maybe display parameters we need to
> > > carve out a hunk of main memory? A bloblist is perfectly capable of NOT
> > > containing things. We can avoid having to care about what phase of
> > > things we're in too.
> >
> > Devicetree is the only 'strange' thing here because we need to look at
> > it early.
>
> Device tree is also "strange" in that it's the only thing we read (not
> write) today prior to U-Boot too from bloblist.
>
> > For Raymond's patch, it shouldn't need any conditions.
>
> Yes, and I replied to him that way too.
>
> > > > > > In U-Boot proper, we look for it before relocation, so we can relocate and
> > > > > > expand it for use with ACPI tables, etc.
> > > > >
> > > > > I'm not sure that's relevant. This means we've taken what we were passed
> > > > > at a fixed address and allocated more space for it and are using it
> > > > > somewhere else.
> > > >
> > > > Yes
> > > >
> > > > >
> > > > > > Take a look at this patch, too:
> > > > > >
> > > > > > 3d6531803e1 bloblist: Support initing from multiple places
> > > > >
> > > > > I'm not sure it's relevant. It does show we have a problem of not
> > > > > knowing if the bloblist exists, or not. And I don't think we're solving
> > > > > that right today (nor does v1 of my patch).
> > > >
> > > > OK
> > > >
> > > > >
> > > > > > > An alternative here would be to better document the requirements of
> > > > > > > BLOBLIST_ADDR, and then just disable it on chromebook_coral until
> > > > > > > someone is inclined to move DDR init in to TPL, or someone finds a
> > > > > > > non-main-memory address to use for BLOBLIST_ADDR or switches to using
> > > > > > > BLOBLIST_ALLOC.
> > > > > >
> > > > > > Coral's TPL runs in cache-as-ram, with a 30KB limit on code size. It cannot
> > > > > > do DDR init in TPL because the DDR blob is large (160KB?) and there is a
> > > > > > ton of setup to do first, in any case. When CAR goes away, its memory
> > > > > > vanishes, so you need to have copied the bloblist somewhere else (the
> > > > > > bloblist holds the MRC data which needs to be written to SPI flash later
> > > > > > on, when possible). This all works perfectly and there is really no need to
> > > > > > change anything.
> > > > >
> > > > > Ah, so there's some of the details finally. And oh goodness, we're
> > > > > writing to the SPI flash on each boot? That's not good for longevity...
> > > >
> > > > It writes a sector each time, across an area which can hold quite a
> > > > few writes. It only writes when the details change, so it is fine.
> > > > This algorithm has worked in the field for years across tens of
> > > > millions of devices and is common on x86.
> > >
> > > To be clear, I wasn't saying that its your design.
> >
> > No, it isn't my design, but I would be happy if it were. You were
> > suggesting it had a longevity problem, so I wanted to explain it.
> >
> > >
> > > > > > Again, you seem to be imputing semantics to bloblist which don't exist.
> > > > >
> > > > > Unfortunately some things were missed along the way, and are still
> > > > > missing.
> > > >
> > > > Well we should have started from OF_BLOBLIST and then dealt with
> > > > problems as they came up. In fact, so far there are no problems which
> > > > it can't solve.
> > > >
> > > > As it is, we have Raymond sending a patch to push the prior-stage mess
> > > > into TPM, for reasons which make no sense to me. It is just adding
> > > > confusion.
> > > >
> > > > One other thing we would have likely done by now if my patch[1] had
> > > > gone in a year ago, is moving away from BLOBLIST_FIXED to using a
> > > > register protocol. That is more deterministic.
> > > >
> > > > Regards,
> > > > Simon
> > > >
> > > > [1] https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/
> > >
> > > I think once again your obsession with OF_BLOBLIST and device tree is
> > > causing you to miss everything else. Today we can take a *bloblist* via
> > > register. Linaro wrote that, even. The next problem really is that we
> > > can't pass that bloblist along to the next stage because all of the
> > > U-Boot side of things assumes fixed address.
> >
> > Perhaps if we had applied OF_BLOBLIST a year ago, so my special 3
> > ancient chromebooks that nobody cares about or uses apart from me,
> > worked, we could have got bloblist to where it should be today. You
> > are missing the big picture, but stopping me from pursuing it.
> >
> > I am disappointed that it is only at this stage that you have shown
> > any interest in getting my special 3 ancient chromebooks running.
>
> And I am disappointed that it's only at this stage there's an
> understanding of what the actual problem is with your special 3 ancient
> chromebooks. Everything last year was read by me as you arguing that you
> need a way to control passing the device tree because that's where the
> device tree is:
> https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/#3174895
> And if not every at least most of your other replies in that thread are
> about how device tree is passed in. This is why I assumed that your
> boards were working after having a way to enforce that the device tree
> comes from bloblist.

That thread is crazy. I should have given up at v1. I wish you hadn't
assumed that and I am still a bit shocked that you did, particularly
after 4-5 revisions.

>
> So if there's some "big picture" about bloblist being delayed, it is
> because once again your vision isn't communicated to everyone else
> you're trying to work with and that is the big problem.

Yes, agreed, that is exactly the problem. I hope my tree will solve
that problem and everyone will be happier.

Regards,
Simon