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

[Hendrik Donner <hd@os-cillation.de>]

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