[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 at 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 at 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 at 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 at 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