Hi Rasmus,
On Wed, 26 Apr 2023 at 03:04, Rasmus Villemoes rasmus.villemoes@prevas.dk 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.]
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?
Well we try to have one makefile rule for the SoC family, rather than one for each board, as it is a pain to deal with lots of rules. We sort-of follow what Linux does here.
DT files are supposed to be stand-alone for the most part, although we do add binding includes. Since we include the autoconf you can use CONFIG options also, but using board-specific CONFIG options is not good practice, I think. After all, our goal is to upstream all these files, and CONFIG_SYS_BOARD won't be defined in Linux.
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?
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?
Eek...
Regards, Simon