[PATCH 0/2] Build fixes for CONFIG_XPL_BUILD in dts files

[PATCH 0/2] Build fixes for CONFIG_XPL_BUILD in dts files

[Hendrik Donner]

Just 2 fixes related to CONFIG_XPL_BUILD usage in dts files with binman.
The second patch works for me, not sure if it is exactly the correct
approach.

Hendrik Donner (2):
  imx8qm: Fix build when using SPL
  Makefile.lib: Set xPL build related defines for DTB build

 arch/arm/dts/imx8qm-u-boot.dtsi |  2 ++
 scripts/Makefile.lib            | 10 ++++++++++
 2 files changed, 12 insertions(+)

-- 
2.43.0

[PATCH 1/2] imx8qm: Fix build when using SPL

[Hendrik Donner]

Analog to 645a42d7ee88dc8c3848c15d178b0f111c636933, imx8qm based boards
which use SPL currently fail during make all:

Image 'imx-boot' is missing external blobs and is non-functional: spl
/binman/imx-boot/spl (spl.bin):
   Missing blob
Image 'imx-boot' has faked external blobs and is non-functional: spl.bin
Some images are invalid

Guard creation of flash.bin with CONFIG_XPL_BUILD option.

Fixes: c9713c155127 ("imx8-u-boot: Fix SPL guard option")

Tested-by: Oliver Graute <oliver.graute@kococonnector.com>
Signed-off-by: Hendrik Donner <hd@os-cillation.de>
---
 arch/arm/dts/imx8qm-u-boot.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/dts/imx8qm-u-boot.dtsi b/arch/arm/dts/imx8qm-u-boot.dtsi
index af229501a4d..6679c9de462 100644
--- a/arch/arm/dts/imx8qm-u-boot.dtsi
+++ b/arch/arm/dts/imx8qm-u-boot.dtsi
@@ -122,6 +122,7 @@
 		};
 	};
 
+#ifdef CONFIG_XPL_BUILD
 	imx-boot {
 		filename = "flash.bin";
 		pad-byte = <0x00>;
@@ -132,4 +133,5 @@
 			type = "blob-ext";
 		};
 	};
+#endif
 };
-- 
2.43.0

[PATCH 2/2] Makefile.lib: Set xPL build related defines for DTB build

[Hendrik Donner]

The CONFIG_*PL_BUILD defines are currently not defined when
preprocessing the dts files, leading to build problems with binman. Set
the defines based on the related CONFIG_*PL values.

Tested-by: Oliver Graute <oliver.graute@kococonnector.com>
Signed-off-by: Hendrik Donner <hd@os-cillation.de>
---
 scripts/Makefile.lib | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 54403040f00..dd2c6363224 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -217,6 +217,16 @@ dtc_cpp_flags  = -Wp,-MD,$(depfile).pre.tmp -nostdinc                    \
 		 -D__ASSEMBLY__                                          \
 		 -undef -D__DTS__
 
+ifeq ($(CONFIG_SPL),y)
+dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_SPL_BUILD
+endif
+ifeq ($(CONFIG_TPL),y)
+dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_TPL_BUILD
+endif
+ifeq ($(CONFIG_VPL),y)
+dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_VPL_BUILD
+endif
+
 # Finds the multi-part object the current object will be linked into
 modname-multi = $(sort $(foreach m,$(multi-used),\
 		$(if $(filter $(subst $(obj)/,,$*.o), $($(m:.o=-objs)) $($(m:.o=-y))),$(m:.o=))))
-- 
2.43.0

Re: [PATCH 2/2] Makefile.lib: Set xPL build related defines for DTB build

[Quentin Schulz]

Hi Hendrik,

On 3/5/25 7:35 PM, Hendrik Donner wrote:
> [You don't often get email from hd@os-cillation.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> The CONFIG_*PL_BUILD defines are currently not defined when
> preprocessing the dts files, leading to build problems with binman. Set
> the defines based on the related CONFIG_*PL values.
> 
> Tested-by: Oliver Graute <oliver.graute@kococonnector.com>

Please have this person publicly answer they tested it instead of having 
it already in the v1 of the patch, too easy to impersonate someone and 
give a false sense of trust into a patch that may not actually have been 
tested.

> Signed-off-by: Hendrik Donner <hd@os-cillation.de>
> ---
>   scripts/Makefile.lib | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 54403040f00..dd2c6363224 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -217,6 +217,16 @@ dtc_cpp_flags  = -Wp,-MD,$(depfile).pre.tmp -nostdinc                    \
>                   -D__ASSEMBLY__                                          \
>                   -undef -D__DTS__
> 
> +ifeq ($(CONFIG_SPL),y)
> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_SPL_BUILD
> +endif
> +ifeq ($(CONFIG_TPL),y)
> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_TPL_BUILD
> +endif
> +ifeq ($(CONFIG_VPL),y)
> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_VPL_BUILD
> +endif
> +

You don't actually explain what you're trying to fix here and why this 
fixes it?

Why would you need those symbols for the DT?

For removing nodes in xPL, you can use one of the bootph- properties.

The DT is supposed to represent the HW, so having a different DT between 
stages is very likely wrong. Note that having a subset of the full DT in 
xPL stages is a bit of an exception here, for size purposes (usually 
because of limited SRAM) or boot time purposes (you don't need to enable 
everything in the DT in the first stage after BootROM simply to init 
UART and DRAM :) ).

As for binman, why would it even need to be run during the xPL build 
stages? Can you provide more context on this?

We have some big nasty ifdefery in arch/arm/dts/rockchip-u-boot.dtsi for 
example, which has some logic on whether some symbols are defined, but 
they are defined regardless of the stage and I assume binman gets run 
only once, and not in xPL build phase? Is this something not applicable 
for (I assume that's the user of that) imx8?

Cheers,
Quentin

Re: [PATCH 2/2] Makefile.lib: Set xPL build related defines for DTB build

[Oliver Graute]

On 06/03/25, Quentin Schulz wrote:
> Hi Hendrik,
> 
> On 3/5/25 7:35 PM, Hendrik Donner wrote:
> > [You don't often get email from hd@os-cillation.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > The CONFIG_*PL_BUILD defines are currently not defined when
> > preprocessing the dts files, leading to build problems with binman. Set
> > the defines based on the related CONFIG_*PL values.
> > 
> > Tested-by: Oliver Graute <oliver.graute@kococonnector.com>

This is fine for me

Tested-by: Oliver Graute <oliver.graute@kococonnector.com>
> 
> Please have this person publicly answer they tested it instead of having it
> already in the v1 of the patch, too easy to impersonate someone and give a
> false sense of trust into a patch that may not actually have been tested.

Re: [PATCH 2/2] Makefile.lib: Set xPL build related defines for DTB build

[Hendrik Donner]

Hello,

On 06.03.25 11:18, Quentin Schulz wrote:
> Hi Hendrik,
> 
> On 3/5/25 7:35 PM, Hendrik Donner wrote:
>> [You don't often get email from hd@os-cillation.de. Learn why this is 
>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> The CONFIG_*PL_BUILD defines are currently not defined when
>> preprocessing the dts files, leading to build problems with binman. Set
>> the defines based on the related CONFIG_*PL values.
>>
>> Tested-by: Oliver Graute <oliver.graute@kococonnector.com>
> 
> Please have this person publicly answer they tested it instead of having 
> it already in the v1 of the patch, too easy to impersonate someone and 
> give a false sense of trust into a patch that may not actually have been 
> tested.
> 
>> Signed-off-by: Hendrik Donner <hd@os-cillation.de>
>> ---
>>   scripts/Makefile.lib | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index 54403040f00..dd2c6363224 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -217,6 +217,16 @@ dtc_cpp_flags  = -Wp,-MD,$(depfile).pre.tmp - 
>> nostdinc                    \
>>                   - 
>> D__ASSEMBLY__                                          \
>>                   -undef -D__DTS__
>>
>> +ifeq ($(CONFIG_SPL),y)
>> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_SPL_BUILD
>> +endif
>> +ifeq ($(CONFIG_TPL),y)
>> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_TPL_BUILD
>> +endif
>> +ifeq ($(CONFIG_VPL),y)
>> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_VPL_BUILD
>> +endif
>> +
> 
> You don't actually explain what you're trying to fix here and why this 
> fixes it?
> 
> Why would you need those symbols for the DT?
> 

well they are already in use in tree ([1], [2]), which breaks the build.

The history of both files is that the guards were introduced as
CONFIG_SPL, which builds, to CONFIG_SPL_BUILD, which should break the
build, haven't tested, and somewhat recently to CONFIG_XPL_BUILD,
which is when i noticed that it breaks the build for me.

Now the guards are around SPL specific nodes, so i didn't question it.
My understanding was that those are needed not necessarily during the
SPL phase, but only if SPL is defined at all.

If i understand you correctly none of it should have been introduced
like that in the first place?

I noticed it's broken in the uboot CI run too, but since the boards
need firmware blobs the CI build is erroring out in the first place, so
i guess no one noticed.

> For removing nodes in xPL, you can use one of the bootph- properties.
> 
> The DT is supposed to represent the HW, so having a different DT between 
> stages is very likely wrong. Note that having a subset of the full DT in 
> xPL stages is a bit of an exception here, for size purposes (usually 
> because of limited SRAM) or boot time purposes (you don't need to enable 
> everything in the DT in the first stage after BootROM simply to init 
> UART and DRAM :) ).
> 
> As for binman, why would it even need to be run during the xPL build 
> stages? Can you provide more context on this?
> 
> We have some big nasty ifdefery in arch/arm/dts/rockchip-u-boot.dtsi for 
> example, which has some logic on whether some symbols are defined, but 
> they are defined regardless of the stage and I assume binman gets run 
> only once, and not in xPL build phase? Is this something not applicable 
> for (I assume that's the user of that) imx8?

The arch/arm/dts/rockchip-u-boot.dtsi ifdefery is based directly on
CONFIG_SPL, not CONFIG_SPL_BUILD nor CONFIG_XPL_BUILD, so maybe the 2
dtsi i mentioned should have kept using CONFIG_SPL in the first place.

So move those back to using CONFIG_SPL?

Regards,
Hendrik

[1] 
https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/imx8qm-u-boot.dtsi#L13
[2] 
https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/imx8qxp-u-boot.dtsi#L13

> 
> Cheers,
> Quentin

Re: [PATCH 2/2] Makefile.lib: Set xPL build related defines for DTB build

[Quentin Schulz]

Hi Hendrik,

On 3/6/25 5:28 PM, Hendrik Donner wrote:
> [You don't often get email from hd@os-cillation.de. Learn why this is 
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello,
> 
> On 06.03.25 11:18, Quentin Schulz wrote:
>> Hi Hendrik,
>>
>> On 3/5/25 7:35 PM, Hendrik Donner wrote:
>>> [You don't often get email from hd@os-cillation.de. Learn why this is
>>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> The CONFIG_*PL_BUILD defines are currently not defined when
>>> preprocessing the dts files, leading to build problems with binman. Set
>>> the defines based on the related CONFIG_*PL values.
>>>
>>> Tested-by: Oliver Graute <oliver.graute@kococonnector.com>
>>
>> Please have this person publicly answer they tested it instead of having
>> it already in the v1 of the patch, too easy to impersonate someone and
>> give a false sense of trust into a patch that may not actually have been
>> tested.
>>
>>> Signed-off-by: Hendrik Donner <hd@os-cillation.de>
>>> ---
>>>   scripts/Makefile.lib | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>>> index 54403040f00..dd2c6363224 100644
>>> --- a/scripts/Makefile.lib
>>> +++ b/scripts/Makefile.lib
>>> @@ -217,6 +217,16 @@ dtc_cpp_flags  = -Wp,-MD,$(depfile).pre.tmp -
>>> nostdinc                    \
>>>                   -
>>> D__ASSEMBLY__                                          \
>>>                   -undef -D__DTS__
>>>
>>> +ifeq ($(CONFIG_SPL),y)
>>> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_SPL_BUILD
>>> +endif
>>> +ifeq ($(CONFIG_TPL),y)
>>> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_TPL_BUILD
>>> +endif
>>> +ifeq ($(CONFIG_VPL),y)
>>> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_VPL_BUILD
>>> +endif
>>> +
>>
>> You don't actually explain what you're trying to fix here and why this
>> fixes it?
>>
>> Why would you need those symbols for the DT?
>>
> 
> well they are already in use in tree ([1], [2]), which breaks the build.
> 
> The history of both files is that the guards were introduced as
> CONFIG_SPL, which builds, to CONFIG_SPL_BUILD, which should break the
> build, haven't tested, and somewhat recently to CONFIG_XPL_BUILD,
> which is when i noticed that it breaks the build for me.
> 
> Now the guards are around SPL specific nodes, so i didn't question it.
> My understanding was that those are needed not necessarily during the
> SPL phase, but only if SPL is defined at all.
> 
> If i understand you correctly none of it should have been introduced
> like that in the first place?
> 

Yes. I believe we should keep CONFIG_SPL in there and not CONFIG_XPL_BUILD.

+Cc Fabio as author of the (assumed) breaking commit.

As far as I can tell, it's because the mkimage used in the 
/binman/spl/mkimage node expects the same external blob as 
/binman/itb/fit/images/seco, named mx8qm-ahab-container.img.

If you do

dd if=/dev/zero of=mx8qm-ahab-container.img count=1

and recompile, then it fails a bit later on with:

```
binman: Error 1 running 'mkimage -d ./mkimage.spl.mkimage -n 
spl/u-boot-spl.cfgout -T imx8image -e 0x100000 
./mkimage-out.spl.mkimage': parsing spl/u-boot-spl.cfgout
New Container: 	0
CONTAINER Sector size:	00000400
CONTAINER FUSE VERSION:	0x00
CONTAINER SW VERSION:	0x0000
Platform:	i.MX8QM B0
Failure Read header 0
```

which I assume is expected.

This file seems to be required because of its presence in some 
IMX_CONFIG script? Is it actually required? Maybe we should make it 
optional (at least for the seco node, that shouldn't be too difficult by 
adding "optional;" in it?).

I assume a probably more proper way to handle that would also be to 
migrate the script handling IMX_CONFIG into full binman with a new 
`entry` instead of relying on mkimage to find the dependencies and all?

> I noticed it's broken in the uboot CI run too, but since the boards
> need firmware blobs the CI build is erroring out in the first place, so
> i guess no one noticed.
> 

c9713c155127 ("imx8-u-boot: Fix SPL guard option") essentially entirely 
removed the code block as you discovered. So yes, the error isn't here 
anymore, because the code isn't run anymore.

>> For removing nodes in xPL, you can use one of the bootph- properties.
>>
>> The DT is supposed to represent the HW, so having a different DT between
>> stages is very likely wrong. Note that having a subset of the full DT in
>> xPL stages is a bit of an exception here, for size purposes (usually
>> because of limited SRAM) or boot time purposes (you don't need to enable
>> everything in the DT in the first stage after BootROM simply to init
>> UART and DRAM :) ).
>>
>> As for binman, why would it even need to be run during the xPL build
>> stages? Can you provide more context on this?
>>
>> We have some big nasty ifdefery in arch/arm/dts/rockchip-u-boot.dtsi for
>> example, which has some logic on whether some symbols are defined, but
>> they are defined regardless of the stage and I assume binman gets run
>> only once, and not in xPL build phase? Is this something not applicable
>> for (I assume that's the user of that) imx8?
> 
> The arch/arm/dts/rockchip-u-boot.dtsi ifdefery is based directly on
> CONFIG_SPL, not CONFIG_SPL_BUILD nor CONFIG_XPL_BUILD, so maybe the 2
> dtsi i mentioned should have kept using CONFIG_SPL in the first place.
> 
> So move those back to using CONFIG_SPL?
> 

I believe so, but maybe some more work is needed? At the very least for 
the CI, no clue what this external blob is, how you generate it or get it.

Cheers,
Quentin

[PATCH 0/2] Do not use CONFIG_XPL_BUILD in device tree

[Hendrik Donner]

As discussed on the mailing list, move instances of CONFIG_XPL_BUILD
back to the initially used CONFIG_SPL in device trees.

Hendrik Donner (2):
  imx8qxp: Do not use CONFIG_XPL_BUILD in device tree
  imx8qm: Do not use CONFIG_XPL_BUILD in device tree

 arch/arm/dts/imx8qm-u-boot.dtsi  | 4 +++-
 arch/arm/dts/imx8qxp-u-boot.dtsi | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH 1/2] imx8qxp: Do not use CONFIG_XPL_BUILD in device tree

[Hendrik Donner]

In c9713c155127 the device tree was moved
from CONFIG_SPL to CONFIG_SPL_BUILD and later to CONFIG_XPL_BUILD, but
the CONFIG_xPL_BUILD defines are never set for device trees, breaking
the build. Move the guards back to CONFIG_SPL.

Fixes: c9713c155127 ("imx8-u-boot: Fix SPL guard option")
Signed-off-by: Hendrik Donner <hd@os-cillation.de>
---
 arch/arm/dts/imx8qxp-u-boot.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/dts/imx8qxp-u-boot.dtsi b/arch/arm/dts/imx8qxp-u-boot.dtsi
index 8058caae9ba..db6a3dedb95 100644
--- a/arch/arm/dts/imx8qxp-u-boot.dtsi
+++ b/arch/arm/dts/imx8qxp-u-boot.dtsi
@@ -10,7 +10,7 @@
 };
 
 &binman {
-#ifdef CONFIG_XPL_BUILD
+#ifdef CONFIG_SPL
 	u-boot-spl-ddr {
 		align = <4>;
 		align-size = <4>;
@@ -120,7 +120,7 @@
 		};
 	};
 
-#ifdef CONFIG_XPL_BUILD
+#ifdef CONFIG_SPL
 	imx-boot {
 		filename = "flash.bin";
 		pad-byte = <0x00>;
-- 
2.43.0

Re: [PATCH 1/2] imx8qxp: Do not use CONFIG_XPL_BUILD in device tree

[Fabio Estevam]

Hi Hendrik,

On Fri, Mar 14, 2025 at 5:09 PM Hendrik Donner <hd@os-cillation.de> wrote:
>
> In c9713c155127 the device tree was moved
> from CONFIG_SPL to CONFIG_SPL_BUILD and later to CONFIG_XPL_BUILD, but
> the CONFIG_xPL_BUILD defines are never set for device trees, breaking
> the build. Move the guards back to CONFIG_SPL.

Please provide details about the "breaking the build". Which target
fails to build currently?

After applying this series, the previous build failures return:

aarch64: + imx8qm_mek
291
+WARNING 'mx8qm-ahab-container.img' not found, resulting binary is
not-functional
292
+binman: Error 1 running 'mkimage -d ./mkimage.spl.mkimage -n
spl/u-boot-spl.cfgout -T imx8image -e 0x100000
./mkimage-out.spl.mkimage': Fail open first container file
mx8qm-ahab-container.img
293
+
294
+make[1]: *** [Makefile:1133: .binman_stamp] Error 1
295
+make: *** [Makefile:177: sub-make] Error 2

Please check:
https://source.denx.de/u-boot/custodians/u-boot-imx/-/jobs/1062980/viewer

Re: [PATCH 1/2] imx8qxp: Do not use CONFIG_XPL_BUILD in device tree

[Quentin Schulz]

Hi Fabio,

On 3/15/25 12:58 PM, Fabio Estevam wrote:
> Hi Hendrik,
> 
> On Fri, Mar 14, 2025 at 5:09 PM Hendrik Donner <hd@os-cillation.de> wrote:
>>
>> In c9713c155127 the device tree was moved
>> from CONFIG_SPL to CONFIG_SPL_BUILD and later to CONFIG_XPL_BUILD, but
>> the CONFIG_xPL_BUILD defines are never set for device trees, breaking
>> the build. Move the guards back to CONFIG_SPL.
> 
> Please provide details about the "breaking the build". Which target

The nodes will simply never be added, because CONFIG_XPL_BUILD (or 
CONFIG_SPL_BUILD) simply aren't passed to dtc.

So essentially, this is equivalent to dead/noop code.

> fails to build currently?
> 
> After applying this series, the previous build failures return:
> 

Yes, it was never fixed, just silenced by never building it.

See 
https://lore.kernel.org/u-boot/2849ff05-ef67-4471-aece-422daa57040f@cherry.de/

Cheers,
Quentin

Re: [PATCH 1/2] imx8qxp: Do not use CONFIG_XPL_BUILD in device tree

[Hendrik Donner]

Hello,

On 17.03.25 12:10, Quentin Schulz wrote:
> Hi Fabio,
> 
> On 3/15/25 12:58 PM, Fabio Estevam wrote:
>> Hi Hendrik,
>>
>> On Fri, Mar 14, 2025 at 5:09 PM Hendrik Donner <hd@os-cillation.de> 
>> wrote:
>>>
>>> In c9713c155127 the device tree was moved
>>> from CONFIG_SPL to CONFIG_SPL_BUILD and later to CONFIG_XPL_BUILD, but
>>> the CONFIG_xPL_BUILD defines are never set for device trees, breaking
>>> the build. Move the guards back to CONFIG_SPL.
>>
>> Please provide details about the "breaking the build". Which target
> 
> The nodes will simply never be added, because CONFIG_XPL_BUILD (or 
> CONFIG_SPL_BUILD) simply aren't passed to dtc.
> 
> So essentially, this is equivalent to dead/noop code.
> 
>> fails to build currently?
>>
>> After applying this series, the previous build failures return:
>>
> 
> Yes, it was never fixed, just silenced by never building it.
> 
> See https://lore.kernel.org/u-boot/2849ff05-ef67-4471- 
> aece-422daa57040f@cherry.de/
> 

additional information: the documentation is already telling the user to
acquire the missing binary blobs:

https://docs.u-boot.org/en/latest/board/nxp/imx8qxp_mek.html#get-scfw-tcm-bin-and-ahab-container-img

So the error from make should not happen with the correct blobs being
present.

But most aspect were already discussed in the mail thread this patch
series was in response to.

Regards,
Hendrik

> Cheers,
> Quentin

Re: [PATCH 1/2] imx8qxp: Do not use CONFIG_XPL_BUILD in device tree

[Tom Rini]

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

On Mon, Mar 17, 2025 at 12:10:04PM +0100, Quentin Schulz wrote:
> Hi Fabio,
> 
> On 3/15/25 12:58 PM, Fabio Estevam wrote:
> > Hi Hendrik,
> > 
> > On Fri, Mar 14, 2025 at 5:09 PM Hendrik Donner <hd@os-cillation.de> wrote:
> > > 
> > > In c9713c155127 the device tree was moved
> > > from CONFIG_SPL to CONFIG_SPL_BUILD and later to CONFIG_XPL_BUILD, but
> > > the CONFIG_xPL_BUILD defines are never set for device trees, breaking
> > > the build. Move the guards back to CONFIG_SPL.
> > 
> > Please provide details about the "breaking the build". Which target
> 
> The nodes will simply never be added, because CONFIG_XPL_BUILD (or
> CONFIG_SPL_BUILD) simply aren't passed to dtc.
> 
> So essentially, this is equivalent to dead/noop code.
> 
> > fails to build currently?
> > 
> > After applying this series, the previous build failures return:
> > 
> 
> Yes, it was never fixed, just silenced by never building it.
> 
> See https://lore.kernel.org/u-boot/2849ff05-ef67-4471-aece-422daa57040f@cherry.de/

It sounds to me like imx8/9 and faking external blobs isn't being
handled correctly at this time. Along with needing to make use of
making use of CONFIG_BUILD_TARGET so that "make all" will build the
right binary (when the right external blobs exist).

-- 
Tom

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

[PATCH 2/2] imx8qm: Do not use CONFIG_XPL_BUILD in device tree

[Hendrik Donner]

In c9713c15512 the device tree was moved
from CONFIG_SPL to CONFIG_SPL_BUILD and later to CONFIG_XPL_BUILD, but
the CONFIG_xPL_BUILD defines are never set for device trees, breaking
the build. Move the guards back to CONFIG_SPL.

Also add a missing guard analog to 645a42d7ee8 to fix a similar build
issue.

Fixes: c9713c15512 ("imx8-u-boot: Fix SPL guard option")
Signed-off-by: Hendrik Donner <hd@os-cillation.de>
---
 arch/arm/dts/imx8qm-u-boot.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/imx8qm-u-boot.dtsi b/arch/arm/dts/imx8qm-u-boot.dtsi
index af229501a4d..8492b3f4c78 100644
--- a/arch/arm/dts/imx8qm-u-boot.dtsi
+++ b/arch/arm/dts/imx8qm-u-boot.dtsi
@@ -10,7 +10,7 @@
 };
 
 &binman {
-#ifdef CONFIG_XPL_BUILD
+#ifdef CONFIG_SPL
 	u-boot-spl-ddr {
 		align = <4>;
 		align-size = <4>;
@@ -122,6 +122,7 @@
 		};
 	};
 
+#ifdef CONFIG_SPL
 	imx-boot {
 		filename = "flash.bin";
 		pad-byte = <0x00>;
@@ -132,4 +133,5 @@
 			type = "blob-ext";
 		};
 	};
+#endif
 };
-- 
2.43.0

Re: [PATCH 2/2] Makefile.lib: Set xPL build related defines for DTB build

[Simon Glass]

Hi Hendrik,

On Thu, 6 Mar 2025 at 09:28, Hendrik Donner <hd@os-cillation.de> wrote:
>
>
> Hello,
>
> On 06.03.25 11:18, Quentin Schulz wrote:
> > Hi Hendrik,
> >
> > On 3/5/25 7:35 PM, Hendrik Donner wrote:
> >> [You don't often get email from hd@os-cillation.de. Learn why this is
> >> important at https://aka.ms/LearnAboutSenderIdentification ]
> >>
> >> The CONFIG_*PL_BUILD defines are currently not defined when
> >> preprocessing the dts files, leading to build problems with binman. Set
> >> the defines based on the related CONFIG_*PL values.
> >>
> >> Tested-by: Oliver Graute <oliver.graute@kococonnector.com>
> >
> > Please have this person publicly answer they tested it instead of having
> > it already in the v1 of the patch, too easy to impersonate someone and
> > give a false sense of trust into a patch that may not actually have been
> > tested.
> >
> >> Signed-off-by: Hendrik Donner <hd@os-cillation.de>
> >> ---
> >>   scripts/Makefile.lib | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> >> index 54403040f00..dd2c6363224 100644
> >> --- a/scripts/Makefile.lib
> >> +++ b/scripts/Makefile.lib
> >> @@ -217,6 +217,16 @@ dtc_cpp_flags  = -Wp,-MD,$(depfile).pre.tmp -
> >> nostdinc                    \
> >>                   -
> >> D__ASSEMBLY__                                          \
> >>                   -undef -D__DTS__
> >>
> >> +ifeq ($(CONFIG_SPL),y)
> >> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_SPL_BUILD
> >> +endif
> >> +ifeq ($(CONFIG_TPL),y)
> >> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_TPL_BUILD
> >> +endif
> >> +ifeq ($(CONFIG_VPL),y)
> >> +dtc_cpp_flags += -DCONFIG_XPL_BUILD  -DCONFIG_VPL_BUILD
> >> +endif
> >> +
> >
> > You don't actually explain what you're trying to fix here and why this
> > fixes it?
> >
> > Why would you need those symbols for the DT?
> >
>
> well they are already in use in tree ([1], [2]), which breaks the build.
>
> The history of both files is that the guards were introduced as
> CONFIG_SPL, which builds, to CONFIG_SPL_BUILD, which should break the
> build, haven't tested, and somewhat recently to CONFIG_XPL_BUILD,
> which is when i noticed that it breaks the build for me.
>
> Now the guards are around SPL specific nodes, so i didn't question it.
> My understanding was that those are needed not necessarily during the
> SPL phase, but only if SPL is defined at all.
>
> If i understand you correctly none of it should have been introduced
> like that in the first place?
>
> I noticed it's broken in the uboot CI run too, but since the boards
> need firmware blobs the CI build is erroring out in the first place, so
> i guess no one noticed.
>
> > For removing nodes in xPL, you can use one of the bootph- properties.
> >
> > The DT is supposed to represent the HW, so having a different DT between
> > stages is very likely wrong. Note that having a subset of the full DT in
> > xPL stages is a bit of an exception here, for size purposes (usually
> > because of limited SRAM) or boot time purposes (you don't need to enable
> > everything in the DT in the first stage after BootROM simply to init
> > UART and DRAM :) ).
> >
> > As for binman, why would it even need to be run during the xPL build
> > stages? Can you provide more context on this?
> >
> > We have some big nasty ifdefery in arch/arm/dts/rockchip-u-boot.dtsi for
> > example, which has some logic on whether some symbols are defined, but
> > they are defined regardless of the stage and I assume binman gets run
> > only once, and not in xPL build phase? Is this something not applicable
> > for (I assume that's the user of that) imx8?
>
> The arch/arm/dts/rockchip-u-boot.dtsi ifdefery is based directly on
> CONFIG_SPL, not CONFIG_SPL_BUILD nor CONFIG_XPL_BUILD, so maybe the 2
> dtsi i mentioned should have kept using CONFIG_SPL in the first place.
>
> So move those back to using CONFIG_SPL?

Yes, that is the best thing. The condition should be whether xPL is
enabled, not whether this is the xPL build.

We must not allow xPL_BUILD in .dts files since it will make it
impossible for binman to produce a functional image in many cases.

Can you perhaps look at a checkpatch test for that?

>
> Regards,
> Hendrik
>
> [1]
> https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/imx8qm-u-boot.dtsi#L13
> [2]
> https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/imx8qxp-u-boot.dtsi#L13
>
> >
> > Cheers,
> > Quentin
>

Regards,
Simon