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

[Quentin Schulz <quentin.schulz@cherry.de>]

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