Hi Hendrik, On Thu, 6 Mar 2025 at 09:28, Hendrik Donner <h...@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 h...@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.gra...@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 <h...@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
