On Thu, Apr 27, 2023 at 12:21:00PM -0400, Tom Rini wrote: > On Wed, Apr 26, 2023 at 11:04:24AM +0200, Rasmus Villemoes wrote: > > On 25/04/2023 22.09, Tom Rini wrote: > > > On Tue, Apr 25, 2023 at 10:02:01PM +0200, Rasmus Villemoes wrote: > > >> On 25/04/2023 21.31, Tom Rini wrote: > > >>> On Fri, Mar 17, 2023 at 11:26:39AM +0100, Rasmus Villemoes wrote: > > >>> > > >> > > >>>> Now, the only way to be really sure is to build the world > > >>>> with/without this patch and check if any .dtb file changes, but I > > >>>> don't have the means to do that. > > >>> > > >>> So, yes, this causes a bunch of fail to builds, as you noted above. The > > >>> easiest way I think to confirm things before / after would be to make a > > >>> quick change to tools/buildman/builderthread.py and self.CopyFiles line > > >>> for keep_outputs to also keep the dtb or some dts files so you can diff > > >>> before / after to make sure the end result is the same. > > >> > > >> Do the builds outright fail, or do they fail in the sense that some > > >> machinery detects a change in the binary artifacts? Can you point me at > > >> one or two CI builds that show this? > > > > > > They outright fail to build, mt8516_pumpkin is the one I was testing > > > with. > > > > Gah, of course. > > > > dtb-$(CONFIG_ARCH_MEDIATEK) += \ > > mt7622-rfb.dtb \ > > mt7623a-unielec-u7623-02-emmc.dtb \ > > mt7622-bananapi-bpi-r64.dtb \ > > mt7623n-bananapi-bpi-r2.dtb \ > > mt7629-rfb.dtb \ > > mt7981-rfb.dtb \ > > mt7981-emmc-rfb.dtb \ > > mt7981-sd-rfb.dtb \ > > mt7986a-rfb.dtb \ > > mt7986b-rfb.dtb \ > > mt7986a-sd-rfb.dtb \ > > mt7986b-sd-rfb.dtb \ > > mt7986a-emmc-rfb.dtb \ > > mt7986b-emmc-rfb.dtb \ > > mt8183-pumpkin.dtb \ > > mt8512-bm1-emmc.dtb \ > > mt8516-pumpkin.dtb \ > > mt8518-ap1-emmc.dtb > > > > means that we end up building a million .dtbs that are not actually > > relevant to the board we're building for, and the value of > > CONFIG_SYS_BOARD==mt8516 is of course completely inappropriate for > > mt7622-rfb.dtb, and the nodes mentioned in mt8516-u-boot.dtsi don't > > exist in mt7622... > > > > [To add insult to injury, it seems that currently mt8516-u-boot.dtsi is > > not actually being included when building mt8516-pumpkin.dtb, but it > > seems that the intention very much is that it should be - except that > > mt8516-u-boot.dtsi has a typo (it refers to a label topckgen_ , but the > > trailing underscore shouldn't be there) - confirming that it does in > > fact not get used.] > > ... ugh. > > > I wonder why this isn't already a problem, but I guess that in practice > > we never hit the CONFIG_SYS_CPU or CONFIG_SYS_VENDOR cases. > > > > Not sure what to do. I think it's a little counterproductive to build > > all these .dtbs when they are not needed, and silly to have to add one's > > .dtb to some semi-random list - which is why I pushed for 3609e1dc5f4d > > to get in. > > > > And since we very much allow the .dtbs to depend on various CONFIG_ > > settings - both because different .configs can cause different > > -u-boot-dtsi files to get included, and also because we allow direct use > > of CONFIG_* values (or in #if, #ifdef), there's no guarantee that the > > mt7629-rfb.dtb built with mt8516_pumpkin_defconfig is identical to the > > one built with mt7629_rfb_defconfig. So what exactly is the point of > > building all those irrelevant .dtbs? > > > > So obviously my patch cannot go in as-is. But I do think there are some > > things that need to be rethought in our build system. > > > > Now that all of CONFIG_OF_LIST gets built automatically, couldn't we > > delete most if not all of the dtb-$(CONFIG_SOMETHING) += stanzas? > > So yes, I think with 3609e1dc5f4d we should be able to massively delete > arch/arm/dts/Makefile (and all of the rest, too). > > > Or to make the build of those extra dtbs a little more deterministic and > > fix the issue with a random/wrong/inapplicable -u-boot.dtsi being picked > > up, perhaps change the %.dtb rule so that the the magic *-u-boot.dtsi > > file (whichever one applies) is only included when $@ is in > > CONFIG_$(SPL_)OF_LIST? > > Well, I'm not sure there's a use case for building all of the extra > device trees. I think what I'll do right now is fire off a CI run (or a > few, in the event of problems) where we just use the logic of > 3609e1dc5f4d and see what falls down.
So this gets us a few failures. You can see them on https://source.denx.de/u-boot/u-boot/-/jobs/618127 but one type of failure seems to be the case where CONFIG_DEFAULT_DEVICE_TREE isn't contained in CONFIG_OF_LIST (ls1088aqds_tfa for example) and the other case is where CONFIG_OF_LIST != CONFIG_SPL_OF_LIST and this fails because fdtgrep runs NOT on spl/arch/.../foo.dtb but rather arch/.../foo.dtb and so we don't have the dtb file around. -- Tom
signature.asc
Description: PGP signature