| Message ID | 20230317102639.464263-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | scripts/Makefile.lib: also consider $(CONFIG_SYS_BOARD)-u-boot.dtsi | expand |
Hi Rasmus, On Fri, 17 Mar 2023 at 04:26, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > I have a couple of boards, e.g. foo21, bar33, each with a few > different revisions, so I have > > foo21-revA.dts > foo21-revB.dts > bar33-revA.dts > bar33-revB.dts > > Now the necessary U-Boot specific additions for the foo21 boards > doesn't depend on the revision (likely for bar33), so I just want to > have and maintain one > > foo21-u-boot.dtsi > > But currently I need to add dummy files foo21-revA-u-boot.dtsi and > foo21-revB-u-boot.dtsi each just containing '#include > foo21-u-boot.dtsi'. And similarly for bar33, and all those files > become quite unwieldy as more revisions need to be supported. > > It's quite natural to look for a file named after CONFIG_SYS_BOARD, > with lower precedence of course than a -u-boot.dtsi file with the same > basename as the .dts, but higher than CONFIG_SYS_SOC. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > > Of course, this can cause unwanted changes for existing boards. But a > bit of ad hoc scripting shows that the risk is low: I first grabbed > all 'default "foo"' stanzas of all 'config SYS_BOARD' instances, as > well as all values of CONFIG_SYS_BOARD set in *_defconfig files. Then > I made a list of all existing *-u-boot.dtsi files, and from these > removed any where there is a corresponding .dts file. That leaves just > > imx6ul > imx8mm > mt7620 > mt7621 > mt7622 > mt7623 > mt7628 > mt8516 > socfpga_arria10 > sunxi > > and inspecting a few of those suggests that they set SYS_SOC to the > same as SYS_BOARD, i.e. they were already picking up the .dtsi file > due to the SYS_SOC rule. > > 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. But I do notice that > > > doc/develop/devicetree/control.rst | 1 + > scripts/Makefile.lib | 2 ++ > 2 files changed, 3 insertions(+) Reviewed-by: Simon Glass <sjg@chromium.org> There is something missing at the end of your note. What U-Boot-specific things are you using? Regards, Simon
On 18/03/2023 21.20, Simon Glass 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. But I do notice that >> >> >> doc/develop/devicetree/control.rst | 1 + >> scripts/Makefile.lib | 2 ++ >> 2 files changed, 3 insertions(+) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > There is something missing at the end of your note. Ah, sorry. I meant to say something about that it seems some existing in-tree cases could make use of this and thus be simplified, e.g. all the t8103-*-u-boot.dtsi just include t8103-u-boot.dtsi, but then I wasn't really sure if that was a proper example, so decided to edit that part out (but left that small fragment in). > What U-Boot-specific things are you using? Just the u-boot,dm-spl kind of stuff. Rasmus
Hi Rasmus, On Sun, 19 Mar 2023 at 13:34, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 18/03/2023 21.20, Simon Glass 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. But I do notice that > >> > >> > >> doc/develop/devicetree/control.rst | 1 + > >> scripts/Makefile.lib | 2 ++ > >> 2 files changed, 3 insertions(+) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > There is something missing at the end of your note. > > Ah, sorry. I meant to say something about that it seems some existing > in-tree cases could make use of this and thus be simplified, e.g. all > the t8103-*-u-boot.dtsi just include t8103-u-boot.dtsi, but then I > wasn't really sure if that was a proper example, so decided to edit that > part out (but left that small fragment in). > > > What U-Boot-specific things are you using? > > Just the u-boot,dm-spl kind of stuff. With the new bindings, we can put those tags (bootph-...) in the kernel DTs. Regards, Simon
On 19/03/2023 20.29, Simon Glass wrote: > Hi Rasmus, > > On Sun, 19 Mar 2023 at 13:34, Rasmus Villemoes > <rasmus.villemoes@prevas.dk> wrote: >> >> On 18/03/2023 21.20, Simon Glass 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. But I do notice that >>>> >>>> >>>> doc/develop/devicetree/control.rst | 1 + >>>> scripts/Makefile.lib | 2 ++ >>>> 2 files changed, 3 insertions(+) >>> >>> Reviewed-by: Simon Glass <sjg@chromium.org> >>> >>> There is something missing at the end of your note. >> >> Ah, sorry. I meant to say something about that it seems some existing >> in-tree cases could make use of this and thus be simplified, e.g. all >> the t8103-*-u-boot.dtsi just include t8103-u-boot.dtsi, but then I >> wasn't really sure if that was a proper example, so decided to edit that >> part out (but left that small fragment in). >> >>> What U-Boot-specific things are you using? >> >> Just the u-boot,dm-spl kind of stuff. > > With the new bindings, we can put those tags (bootph-...) in the kernel DTs. Hm, ok, but actually I was a bit too fast, there's also some binman stuff, and a few things that I want/need to do for U-Boot but not the kernel and vice versa (e.g. some gpio-hogs), so all in all, having this patch in would still be very useful to me. Rasmus
Hi Rasmus, On Wed, 12 Apr 2023 at 05:05, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 19/03/2023 20.29, Simon Glass wrote: > > Hi Rasmus, > > > > On Sun, 19 Mar 2023 at 13:34, Rasmus Villemoes > > <rasmus.villemoes@prevas.dk> wrote: > >> > >> On 18/03/2023 21.20, Simon Glass 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. But I do notice that > >>>> > >>>> > >>>> doc/develop/devicetree/control.rst | 1 + > >>>> scripts/Makefile.lib | 2 ++ > >>>> 2 files changed, 3 insertions(+) > >>> > >>> Reviewed-by: Simon Glass <sjg@chromium.org> > >>> > >>> There is something missing at the end of your note. > >> > >> Ah, sorry. I meant to say something about that it seems some existing > >> in-tree cases could make use of this and thus be simplified, e.g. all > >> the t8103-*-u-boot.dtsi just include t8103-u-boot.dtsi, but then I > >> wasn't really sure if that was a proper example, so decided to edit that > >> part out (but left that small fragment in). > >> > >>> What U-Boot-specific things are you using? > >> > >> Just the u-boot,dm-spl kind of stuff. > > > > With the new bindings, we can put those tags (bootph-...) in the kernel DTs. > > Hm, ok, but actually I was a bit too fast, there's also some binman > stuff, and a few things that I want/need to do for U-Boot but not the > kernel and vice versa (e.g. some gpio-hogs), so all in all, having this > patch in would still be very useful to me. Reviewed-by: Simon Glass <sjg@chromium.org> I have too many things open at the moment but at some point I will look at upstreaming the binman bindings. For gpio-hogs, can you send a patch to Linux? Regards, SImon
On Fri, Mar 17, 2023 at 11:26:39AM +0100, Rasmus Villemoes wrote: > I have a couple of boards, e.g. foo21, bar33, each with a few > different revisions, so I have > > foo21-revA.dts > foo21-revB.dts > bar33-revA.dts > bar33-revB.dts > > Now the necessary U-Boot specific additions for the foo21 boards > doesn't depend on the revision (likely for bar33), so I just want to > have and maintain one > > foo21-u-boot.dtsi > > But currently I need to add dummy files foo21-revA-u-boot.dtsi and > foo21-revB-u-boot.dtsi each just containing '#include > foo21-u-boot.dtsi'. And similarly for bar33, and all those files > become quite unwieldy as more revisions need to be supported. > > It's quite natural to look for a file named after CONFIG_SYS_BOARD, > with lower precedence of course than a -u-boot.dtsi file with the same > basename as the .dts, but higher than CONFIG_SYS_SOC. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > Reviewed-by: Simon Glass <sjg@chromium.org> > Reviewed-by: Simon Glass <sjg@chromium.org> > --- > > Of course, this can cause unwanted changes for existing boards. But a > bit of ad hoc scripting shows that the risk is low: I first grabbed > all 'default "foo"' stanzas of all 'config SYS_BOARD' instances, as > well as all values of CONFIG_SYS_BOARD set in *_defconfig files. Then > I made a list of all existing *-u-boot.dtsi files, and from these > removed any where there is a corresponding .dts file. That leaves just > > imx6ul > imx8mm > mt7620 > mt7621 > mt7622 > mt7623 > mt7628 > mt8516 > socfpga_arria10 > sunxi > > and inspecting a few of those suggests that they set SYS_SOC to the > same as SYS_BOARD, i.e. they were already picking up the .dtsi file > due to the SYS_SOC rule. > > 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. But I do notice 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.
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? Rasmus
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. CI didn't get to the point of failing on that particular problem (as it's in the last stage and qemu randomly failed as it does earlier).
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? 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? Rasmus
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.
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
On Thu, Apr 27, 2023 at 10:24:58AM -0600, Simon Glass wrote: > 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. Right, but Linux is disinclined to build targets that support a single board and the common case is you add a new platform with "just" the DTS files. That's not how we build tho. > 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. Right but note that the options we're talking about are the ones to set the device tree (or trees) that are valid to the resulting binary.
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.
On 27/04/2023 19.31, Tom Rini wrote: >> >> 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. > Hm, the former sounds like a bug in the defconfig, the second sounds like a legit use case (or why would we have SPL_OF_LIST). Anyway, both should be fixable by just changing the logic of scripts/Makefile.dts a little; say add the union of DEFAULT_DEVICE_TREE, OF_LIST and SPL_OF_LIST to dtb-y. Something like diff --git a/scripts/Makefile.dts b/scripts/Makefile.dts index 2561025da8..5e2429c617 100644 --- a/scripts/Makefile.dts +++ b/scripts/Makefile.dts @@ -1,3 +1,3 @@ # SPDX-License-Identifier: GPL-2.0+ -dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_$(SPL_)OF_LIST))) +dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_DEFAULT_DEVICE_TREE) $(CONFIG_OF_LIST) $(CONFIG_SPL_OF_LIST)))
Hi Rasmus, On Mon, 1 May 2023 at 02:49, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 27/04/2023 19.31, Tom Rini wrote: > >> > >> 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. > > > > Hm, the former sounds like a bug in the defconfig, the second sounds > like a legit use case (or why would we have SPL_OF_LIST). Anyway, both > should be fixable by just changing the logic of scripts/Makefile.dts a > little; say add the union of DEFAULT_DEVICE_TREE, OF_LIST and > SPL_OF_LIST to dtb-y. Something like > > diff --git a/scripts/Makefile.dts b/scripts/Makefile.dts > index 2561025da8..5e2429c617 100644 > --- a/scripts/Makefile.dts > +++ b/scripts/Makefile.dts > @@ -1,3 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0+ > > -dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_$(SPL_)OF_LIST))) > +dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_DEFAULT_DEVICE_TREE) > $(CONFIG_OF_LIST) $(CONFIG_SPL_OF_LIST))) I think we should require that all the DTs in the list are already built using the standard rule. i.e. Makefile.dts should just have a check that OF_LIST doesn't bring in anything new. If it does, then the build should fail. The list is really about which ones should be put into the FIT. We shouldn't be putting in things that don't exist normally for that SoC. Meanwhile I think we should move towards prohibiting CONFIG_TARGET_... in Makefile rules, since this is creating some of this problem. i.e. we should do what Linux does. I was pushing back on this problem in reviews, but not for a while, and I see that it has grown. That could be a checkpatch rule. It should be possible to use 'moveconfig -i' to find which SoC things belong to. Regards, Simon
On 01/05/2023 18.32, Simon Glass wrote: > Hi Rasmus, > > On Mon, 1 May 2023 at 02:49, Rasmus Villemoes > <rasmus.villemoes@prevas.dk> wrote: >> >> On 27/04/2023 19.31, Tom Rini wrote: >>>> >>>> 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. >>> >> >> Hm, the former sounds like a bug in the defconfig, the second sounds >> like a legit use case (or why would we have SPL_OF_LIST). Anyway, both >> should be fixable by just changing the logic of scripts/Makefile.dts a >> little; say add the union of DEFAULT_DEVICE_TREE, OF_LIST and >> SPL_OF_LIST to dtb-y. Something like >> >> diff --git a/scripts/Makefile.dts b/scripts/Makefile.dts >> index 2561025da8..5e2429c617 100644 >> --- a/scripts/Makefile.dts >> +++ b/scripts/Makefile.dts >> @@ -1,3 +1,3 @@ >> # SPDX-License-Identifier: GPL-2.0+ >> >> -dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_$(SPL_)OF_LIST))) >> +dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_DEFAULT_DEVICE_TREE) >> $(CONFIG_OF_LIST) $(CONFIG_SPL_OF_LIST))) > > I think we should require that all the DTs in the list are already > built using the standard rule. > > i.e. Makefile.dts should just have a check that OF_LIST doesn't bring > in anything new. If it does, then the build should fail. I disagree. IMO, having those enourmous lists is unmaintainable, and as I've pointed out, actively misleading because (like it or not), the result of building foo.dtb depends (or to be pedantically correct, _may depend_) on whether we're building foo_defconfig or bar_defconfig, despite both foo and bar being members of the same SOC family or vendor or however foo.dtb and bar.dtb have been categorized. > The list is really about which ones should be put into the FIT. We > shouldn't be putting in things that don't exist normally for that SoC. Yes, and I'm not talking about changing any of _that_. I'm just saying: The board, in the form of the defconfig, already provides information on which dtbs can be relevant for any bootphase, so let's just build the union of those, _but nothing else_. > Meanwhile I think we should move towards prohibiting CONFIG_TARGET_... > in Makefile rules, I think we can get rid of all of it, or perhaps with some exceptions for sandbox and the like. I mean, Tom's quick test didn't show that many problems from just nuking almost all of arch/*/dts/Makefile. since this is creating some of this problem. i.e. > we should do what Linux does. I don't think so. Linux (and in general an OS kernel) is in a completely different situation than a bootloader. It's possible to build one kernel that will boot on many different boards with just an appropriate dtb being passed. A bootloader will always need to be built for the specific target (or for a very close family of targets); you can't really imagine building an imx_defconfig u-boot binary and expect that to run on all imx-boards. That said, there's another thing "Linux does", at least right now for arm64 and soonish also for arm32, namely putting dts files in individual vendor directories. _That_ I'd really love to see happen in U-Boot as well, because it's not just the 1400 line Makefile that's unmaintanable, it's also the 2600+ files in one directory. Rasmus
On Mon, May 01, 2023 at 10:32:44AM -0600, Simon Glass wrote: > Hi Rasmus, > > On Mon, 1 May 2023 at 02:49, Rasmus Villemoes > <rasmus.villemoes@prevas.dk> wrote: > > > > On 27/04/2023 19.31, Tom Rini wrote: > > >> > > >> 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. > > > > > > > Hm, the former sounds like a bug in the defconfig, the second sounds > > like a legit use case (or why would we have SPL_OF_LIST). Anyway, both > > should be fixable by just changing the logic of scripts/Makefile.dts a > > little; say add the union of DEFAULT_DEVICE_TREE, OF_LIST and > > SPL_OF_LIST to dtb-y. Something like > > > > diff --git a/scripts/Makefile.dts b/scripts/Makefile.dts > > index 2561025da8..5e2429c617 100644 > > --- a/scripts/Makefile.dts > > +++ b/scripts/Makefile.dts > > @@ -1,3 +1,3 @@ > > # SPDX-License-Identifier: GPL-2.0+ > > > > -dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_$(SPL_)OF_LIST))) > > +dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_DEFAULT_DEVICE_TREE) > > $(CONFIG_OF_LIST) $(CONFIG_SPL_OF_LIST))) > > I think we should require that all the DTs in the list are already > built using the standard rule. No, that's the opposite of what we're finally able to do. We don't need a huge list of build-this-maybe-use-it, we can: - Build only and all of the dtb files that will get used - make to build a given device tree while developing it. > i.e. Makefile.dts should just have a check that OF_LIST doesn't bring > in anything new. If it does, then the build should fail. > > The list is really about which ones should be put into the FIT. We > shouldn't be putting in things that don't exist normally for that SoC. But that's not how it's been used. And especially given how the SPL logic (oddly but maybe not intentionally) works, we list many more dtbs there than a given config will use, so that all configs will work.
On Wed, May 03, 2023 at 10:25:21AM +0200, Rasmus Villemoes wrote: [snip] > That said, there's another thing "Linux does", at least right now for > arm64 and soonish also for arm32, namely putting dts files in individual > vendor directories. _That_ I'd really love to see happen in U-Boot as > well, because it's not just the 1400 line Makefile that's unmaintanable, > it's also the 2600+ files in one directory. I would like to see this, but it also means we need to make more progress on getting our device trees in sync with upstream.
On Mon, May 01, 2023 at 10:49:22AM +0200, Rasmus Villemoes wrote: > On 27/04/2023 19.31, Tom Rini wrote: > >> > >> 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. > > > > Hm, the former sounds like a bug in the defconfig, the second sounds > like a legit use case (or why would we have SPL_OF_LIST). Anyway, both > should be fixable by just changing the logic of scripts/Makefile.dts a > little; say add the union of DEFAULT_DEVICE_TREE, OF_LIST and > SPL_OF_LIST to dtb-y. Something like > > diff --git a/scripts/Makefile.dts b/scripts/Makefile.dts > index 2561025da8..5e2429c617 100644 > --- a/scripts/Makefile.dts > +++ b/scripts/Makefile.dts > @@ -1,3 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0+ > > -dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_$(SPL_)OF_LIST))) > +dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_DEFAULT_DEVICE_TREE) > $(CONFIG_OF_LIST) $(CONFIG_SPL_OF_LIST))) OK, lemme see what happens now. Assuming this is enough, please post as a proper patch, thanks!
On Wed, May 03, 2023 at 09:51:58AM -0400, Tom Rini wrote: > On Mon, May 01, 2023 at 10:49:22AM +0200, Rasmus Villemoes wrote: > > On 27/04/2023 19.31, Tom Rini wrote: > > >> > > >> 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. > > > > > > > Hm, the former sounds like a bug in the defconfig, the second sounds > > like a legit use case (or why would we have SPL_OF_LIST). Anyway, both > > should be fixable by just changing the logic of scripts/Makefile.dts a > > little; say add the union of DEFAULT_DEVICE_TREE, OF_LIST and > > SPL_OF_LIST to dtb-y. Something like > > > > diff --git a/scripts/Makefile.dts b/scripts/Makefile.dts > > index 2561025da8..5e2429c617 100644 > > --- a/scripts/Makefile.dts > > +++ b/scripts/Makefile.dts > > @@ -1,3 +1,3 @@ > > # SPDX-License-Identifier: GPL-2.0+ > > > > -dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_$(SPL_)OF_LIST))) > > +dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_DEFAULT_DEVICE_TREE) > > $(CONFIG_OF_LIST) $(CONFIG_SPL_OF_LIST))) > > OK, lemme see what happens now. Assuming this is enough, please post as > a proper patch, thanks! The one last problem now is on stm32mp15_dhcor_basic which is a defconfig missing one from OF_LIST but including it in the its file, so the above is the patch we need.
Hi Rasmus, On Wed, 3 May 2023 at 02:25, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 01/05/2023 18.32, Simon Glass wrote: > > Hi Rasmus, > > > > On Mon, 1 May 2023 at 02:49, Rasmus Villemoes > > <rasmus.villemoes@prevas.dk> wrote: > >> > >> On 27/04/2023 19.31, Tom Rini wrote: > >>>> > >>>> 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. > >>> > >> > >> Hm, the former sounds like a bug in the defconfig, the second sounds > >> like a legit use case (or why would we have SPL_OF_LIST). Anyway, both > >> should be fixable by just changing the logic of scripts/Makefile.dts a > >> little; say add the union of DEFAULT_DEVICE_TREE, OF_LIST and > >> SPL_OF_LIST to dtb-y. Something like > >> > >> diff --git a/scripts/Makefile.dts b/scripts/Makefile.dts > >> index 2561025da8..5e2429c617 100644 > >> --- a/scripts/Makefile.dts > >> +++ b/scripts/Makefile.dts > >> @@ -1,3 +1,3 @@ > >> # SPDX-License-Identifier: GPL-2.0+ > >> > >> -dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_$(SPL_)OF_LIST))) > >> +dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_DEFAULT_DEVICE_TREE) > >> $(CONFIG_OF_LIST) $(CONFIG_SPL_OF_LIST))) > > > > I think we should require that all the DTs in the list are already > > built using the standard rule. > > > > i.e. Makefile.dts should just have a check that OF_LIST doesn't bring > > in anything new. If it does, then the build should fail. > > I disagree. > > IMO, having those enourmous lists is unmaintainable, and > as I've pointed out, actively misleading because (like it or not), the > result of building foo.dtb depends (or to be pedantically correct, _may > depend_) on whether we're building foo_defconfig or bar_defconfig, > despite both foo and bar being members of the same SOC family or vendor > or however foo.dtb and bar.dtb have been categorized. That's actually not how it is supposed to work, though. > > > The list is really about which ones should be put into the FIT. We > > shouldn't be putting in things that don't exist normally for that SoC. > > Yes, and I'm not talking about changing any of _that_. I'm just saying: > The board, in the form of the defconfig, already provides information on > which dtbs can be relevant for any bootphase, so let's just build the > union of those, _but nothing else_. But then we end up with lots of TARGET rules. Plus the Makefile no-longer describes which DTs are built?? Perhaps I just misunderstand where you are heading. > > > Meanwhile I think we should move towards prohibiting CONFIG_TARGET_... > > in Makefile rules, > > I think we can get rid of all of it, or perhaps with some exceptions for > sandbox and the like. I mean, Tom's quick test didn't show that many > problems from just nuking almost all of arch/*/dts/Makefile. > since this is creating some of this problem. i.e. > > we should do what Linux does. > > I don't think so. Linux (and in general an OS kernel) is in a completely > different situation than a bootloader. It's possible to build one kernel > that will boot on many different boards with just an appropriate dtb > being passed. A bootloader will always need to be built for the specific > target (or for a very close family of targets); you can't really imagine > building an imx_defconfig u-boot binary and expect that to run on all > imx-boards. Actually that's really not true, at least not as broadly as you have said it. The original purpose of bringing DT into U-Boot was to allow an exynos5 build to run on 3-4 different boards, just with the DT. As you have seen we have boards that provide an SPL_OF_LIST to deal with differences relevant to U-Boot. The DEFAULT_DEVICE_TREE for a board is really just that. It should be possible to use a different DT with the same U-Boot binary and have it work on a different board (with the same SoC). If you think about the implications of that, it means that we need to use compatible strings for board features, not #ifdefs and special C files. What sort of things are ending up in the DT that make it build differently from other boards? Is it Kconfig options? I am aware of this on x86, since there is a common u-boot.dtsi for all boards. We definitely take a risk by including Kconfig options in the DT, but I would hope that DT differences are in the board's .dts, not in shared .dtsi files. > > That said, there's another thing "Linux does", at least right now for > arm64 and soonish also for arm32, namely putting dts files in individual > vendor directories. _That_ I'd really love to see happen in U-Boot as > well, because it's not just the 1400 line Makefile that's unmaintanable, > it's also the 2600+ files in one directory. I never list the directory, but yes it would be great to move to how Linux does this too. Regards, Simon
On Wed, May 03, 2023 at 04:56:18PM -0600, Simon Glass wrote: > Hi Rasmus, > > On Wed, 3 May 2023 at 02:25, Rasmus Villemoes > <rasmus.villemoes@prevas.dk> wrote: > > > > On 01/05/2023 18.32, Simon Glass wrote: > > > Hi Rasmus, > > > > > > On Mon, 1 May 2023 at 02:49, Rasmus Villemoes > > > <rasmus.villemoes@prevas.dk> wrote: > > >> > > >> On 27/04/2023 19.31, Tom Rini wrote: > > >>>> > > >>>> 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. > > >>> > > >> > > >> Hm, the former sounds like a bug in the defconfig, the second sounds > > >> like a legit use case (or why would we have SPL_OF_LIST). Anyway, both > > >> should be fixable by just changing the logic of scripts/Makefile.dts a > > >> little; say add the union of DEFAULT_DEVICE_TREE, OF_LIST and > > >> SPL_OF_LIST to dtb-y. Something like > > >> > > >> diff --git a/scripts/Makefile.dts b/scripts/Makefile.dts > > >> index 2561025da8..5e2429c617 100644 > > >> --- a/scripts/Makefile.dts > > >> +++ b/scripts/Makefile.dts > > >> @@ -1,3 +1,3 @@ > > >> # SPDX-License-Identifier: GPL-2.0+ > > >> > > >> -dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_$(SPL_)OF_LIST))) > > >> +dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_DEFAULT_DEVICE_TREE) > > >> $(CONFIG_OF_LIST) $(CONFIG_SPL_OF_LIST))) > > > > > > I think we should require that all the DTs in the list are already > > > built using the standard rule. > > > > > > i.e. Makefile.dts should just have a check that OF_LIST doesn't bring > > > in anything new. If it does, then the build should fail. > > > > I disagree. > > > > IMO, having those enourmous lists is unmaintainable, and > > as I've pointed out, actively misleading because (like it or not), the > > result of building foo.dtb depends (or to be pedantically correct, _may > > depend_) on whether we're building foo_defconfig or bar_defconfig, > > despite both foo and bar being members of the same SOC family or vendor > > or however foo.dtb and bar.dtb have been categorized. > > That's actually not how it is supposed to work, though. Er, what do you mean? The Makefiles are supposed to produce functional binaries. Which they mostly only do not due to the logic in arch/*/dts/Makefile but the logic in scripts/Makefile.dts that ensure the dtbs the config file says it needs are built. > > > The list is really about which ones should be put into the FIT. We > > > shouldn't be putting in things that don't exist normally for that SoC. > > > > Yes, and I'm not talking about changing any of _that_. I'm just saying: > > The board, in the form of the defconfig, already provides information on > > which dtbs can be relevant for any bootphase, so let's just build the > > union of those, _but nothing else_. > > But then we end up with lots of TARGET rules. Plus the Makefile > no-longer describes which DTs are built?? Perhaps I just misunderstand > where you are heading. Yes, what we're doing is getting rid of all of the TARGET rules, and all of the SOC rules and so forth, as the config file needs (and almost always does) say what dtbs are going to be used. > > > Meanwhile I think we should move towards prohibiting CONFIG_TARGET_... > > > in Makefile rules, > > > > I think we can get rid of all of it, or perhaps with some exceptions for > > sandbox and the like. I mean, Tom's quick test didn't show that many > > problems from just nuking almost all of arch/*/dts/Makefile. > > since this is creating some of this problem. i.e. > > > we should do what Linux does. > > > > I don't think so. Linux (and in general an OS kernel) is in a completely > > different situation than a bootloader. It's possible to build one kernel > > that will boot on many different boards with just an appropriate dtb > > being passed. A bootloader will always need to be built for the specific > > target (or for a very close family of targets); you can't really imagine > > building an imx_defconfig u-boot binary and expect that to run on all > > imx-boards. > > Actually that's really not true, at least not as broadly as you have > said it. The original purpose of bringing DT into U-Boot was to allow > an exynos5 build to run on 3-4 different boards, just with the DT. As > you have seen we have boards that provide an SPL_OF_LIST to deal with > differences relevant to U-Boot. The DEFAULT_DEVICE_TREE for a board is > really just that. It should be possible to use a different DT with the > same U-Boot binary and have it work on a different board (with the > same SoC). In theory, yes, we can support multiple boards with a single binary, and even do that when they're non-trivial variants. Many of the TI defconfigs do this (with some "fun" code at run time to read the EEPROM and see what we're really on). But to do that we don't rely on the Makefile to have some explicit rule for the dtbs to generate but instead OF_LIST and DEFAULT_DEVICE_TREE. Take a look at https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 to see what arch/*/dts/Makefile will look like, once this is done.
On 03/05/2023 16.54, Tom Rini wrote: > On Wed, May 03, 2023 at 09:51:58AM -0400, Tom Rini wrote: >> On Mon, May 01, 2023 at 10:49:22AM +0200, Rasmus Villemoes wrote: >>> On 27/04/2023 19.31, Tom Rini wrote: >>>>> >>>>> 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. >>>> >>> >>> Hm, the former sounds like a bug in the defconfig, the second sounds >>> like a legit use case (or why would we have SPL_OF_LIST). Anyway, both >>> should be fixable by just changing the logic of scripts/Makefile.dts a >>> little; say add the union of DEFAULT_DEVICE_TREE, OF_LIST and >>> SPL_OF_LIST to dtb-y. Something like >>> >>> diff --git a/scripts/Makefile.dts b/scripts/Makefile.dts >>> index 2561025da8..5e2429c617 100644 >>> --- a/scripts/Makefile.dts >>> +++ b/scripts/Makefile.dts >>> @@ -1,3 +1,3 @@ >>> # SPDX-License-Identifier: GPL-2.0+ >>> >>> -dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_$(SPL_)OF_LIST))) >>> +dtb-y += $(patsubst %,%.dtb,$(subst ",,$(CONFIG_DEFAULT_DEVICE_TREE) >>> $(CONFIG_OF_LIST) $(CONFIG_SPL_OF_LIST))) >> >> OK, lemme see what happens now. Assuming this is enough, please post as >> a proper patch, thanks! > > The one last problem now is on stm32mp15_dhcor_basic which is a > defconfig missing one from OF_LIST but including it in the its file, so > the above is the patch we need. > Hm, well, for now I think at least all .dtbo targets need to be left in the Makefiles, since nothing in the defconfig tells us to build those. Maybe the CI build will complete because missing those dtbos will just be seen as random missing binary blobs, but it's something that needs to be thought of. There's no CONFIG knob to mention the required dtbos, and adding them to OF_LIST doesn't work because those are without the .dtb suffix (we add that in Makefile.dts). But maybe we can just leave the dtbos in the Makefile (perhaps per SOC or whatever) - they are explicitly _not_ built with the special U-Boot .dtb rule, so doesn't depend on config options and don't have magic .dtsi files included. There's anyway so few of them so far that it'd still be a massive win to trim the Makefiles of the .dtb rules. I just sent two Makefile.lib patches related to the dtb/dtbo logic, but they are not strictly required for sorting this out. But I do think we should do the .dts->.dtso renames sooner rather than later. Rasmus
On 04/05/2023 14.35, Rasmus Villemoes wrote: > On 03/05/2023 16.54, Tom Rini wrote: >> The one last problem now is on stm32mp15_dhcor_basic which is a >> defconfig missing one from OF_LIST but including it in the its file, so >> the above is the patch we need. >> > > Hm, well, for now I think at least all .dtbo targets need to be left in > the Makefiles, since nothing in the defconfig tells us to build those. Hi Tom Can I persuade you to try something like https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 again, but leaving the .dtbo targets in there? I could send a patch, but it's entirely mechanical, and not really meant for being applied until we know if there's more to be cleaned up. Rasmus
On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > On 04/05/2023 14.35, Rasmus Villemoes wrote: > > On 03/05/2023 16.54, Tom Rini wrote: > > >> The one last problem now is on stm32mp15_dhcor_basic which is a > >> defconfig missing one from OF_LIST but including it in the its file, so > >> the above is the patch we need. > >> > > > > Hm, well, for now I think at least all .dtbo targets need to be left in > > the Makefiles, since nothing in the defconfig tells us to build those. > > Hi Tom > > Can I persuade you to try something like > https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > again, but leaving the .dtbo targets in there? > > I could send a patch, but it's entirely mechanical, and not really meant > for being applied until we know if there's more to be cleaned up. So what ended up being the problem I think is the case Simon pointed out where we do take the output from "make all" and concatenate one of the dtbs that was generated with u-boot.img or so, and it works. But maybe that should just list all of the valid DTBs that it needs in the defconfig to start with? I don't quite know, it was a case I hadn't considered at the time.
On 25/09/2023 20.19, Tom Rini wrote: > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: >> On 04/05/2023 14.35, Rasmus Villemoes wrote: >>> On 03/05/2023 16.54, Tom Rini wrote: >> >>>> The one last problem now is on stm32mp15_dhcor_basic which is a >>>> defconfig missing one from OF_LIST but including it in the its file, so >>>> the above is the patch we need. >>>> >> >> Hi Tom >> >> Can I persuade you to try something like >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 >> again, but leaving the .dtbo targets in there? >> >> I could send a patch, but it's entirely mechanical, and not really meant >> for being applied until we know if there's more to be cleaned up. > > So what ended up being the problem I think is the case Simon pointed out > where we do take the output from "make all" and concatenate one of the > dtbs that was generated with u-boot.img or so, and it works. But maybe > that should just list all of the valid DTBs that it needs in the > defconfig to start with? I don't quite know, it was a case I hadn't > considered at the time. > Re-reading the thread, I can't see where that was mentioned. But yes, if some boards (still) need that, and have more than one possible .dtb, the board can't set an OF_LIST different from the default consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires SPL_LOAD_FIT || MULTI_DTB_FIT. How do we figure out if such boards even exist? Rasmus
Hi Rasmus, On Mon, 25 Sept 2023 at 13:05, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 25/09/2023 20.19, Tom Rini wrote: > > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > >> On 04/05/2023 14.35, Rasmus Villemoes wrote: > >>> On 03/05/2023 16.54, Tom Rini wrote: > >> > >>>> The one last problem now is on stm32mp15_dhcor_basic which is a > >>>> defconfig missing one from OF_LIST but including it in the its file, so > >>>> the above is the patch we need. > >>>> > >> > >> Hi Tom > >> > >> Can I persuade you to try something like > >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > >> again, but leaving the .dtbo targets in there? > >> > >> I could send a patch, but it's entirely mechanical, and not really meant > >> for being applied until we know if there's more to be cleaned up. > > > > So what ended up being the problem I think is the case Simon pointed out > > where we do take the output from "make all" and concatenate one of the > > dtbs that was generated with u-boot.img or so, and it works. But maybe > > that should just list all of the valid DTBs that it needs in the > > defconfig to start with? I don't quite know, it was a case I hadn't > > considered at the time. > > > > Re-reading the thread, I can't see where that was mentioned. > > But yes, if some boards (still) need that, and have more than one > possible .dtb, the board can't set an OF_LIST different from the default > consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires > SPL_LOAD_FIT || MULTI_DTB_FIT. > > How do we figure out if such boards even exist? Honestly at this point I've forgotten what this is all about. Perhaps the easiest approach is to create a new Kconfig to control whether a board-level .dtsi is included in the list of wildcard searches. Then you can enable it for your board without affecting others. Regards, Simon
On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: > Hi Rasmus, > > On Mon, 25 Sept 2023 at 13:05, Rasmus Villemoes > <rasmus.villemoes@prevas.dk> wrote: > > > > On 25/09/2023 20.19, Tom Rini wrote: > > > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > > >> On 04/05/2023 14.35, Rasmus Villemoes wrote: > > >>> On 03/05/2023 16.54, Tom Rini wrote: > > >> > > >>>> The one last problem now is on stm32mp15_dhcor_basic which is a > > >>>> defconfig missing one from OF_LIST but including it in the its file, so > > >>>> the above is the patch we need. > > >>>> > > >> > > >> Hi Tom > > >> > > >> Can I persuade you to try something like > > >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > > >> again, but leaving the .dtbo targets in there? > > >> > > >> I could send a patch, but it's entirely mechanical, and not really meant > > >> for being applied until we know if there's more to be cleaned up. > > > > > > So what ended up being the problem I think is the case Simon pointed out > > > where we do take the output from "make all" and concatenate one of the > > > dtbs that was generated with u-boot.img or so, and it works. But maybe > > > that should just list all of the valid DTBs that it needs in the > > > defconfig to start with? I don't quite know, it was a case I hadn't > > > considered at the time. > > > > > > > Re-reading the thread, I can't see where that was mentioned. > > > > But yes, if some boards (still) need that, and have more than one > > possible .dtb, the board can't set an OF_LIST different from the default > > consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires > > SPL_LOAD_FIT || MULTI_DTB_FIT. > > > > How do we figure out if such boards even exist? > > Honestly at this point I've forgotten what this is all about. > > Perhaps the easiest approach is to create a new Kconfig to control > whether a board-level .dtsi is included in the list of wildcard > searches. Then you can enable it for your board without affecting > others. That's getting things backwards, from what this cleanup does. Today we have messy lists of "build these device trees" and then don't use most of them, and some of the list is just Wrong (listing dts files as an output). With the series to handle dtbo files, we could remove virtually all of that, and the only use cases that don't Just Work still are the ones I forget which board you mentioned (I think it was Samsung tho?) where the defconfig doesn't list all of the device trees, just one of them, and the other 5 that we build can also be easily used. Does that ring a bell?
Hi Tom, On Fri, 29 Sept 2023 at 10:02, Tom Rini <trini@konsulko.com> wrote: > > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: > > Hi Rasmus, > > > > On Mon, 25 Sept 2023 at 13:05, Rasmus Villemoes > > <rasmus.villemoes@prevas.dk> wrote: > > > > > > On 25/09/2023 20.19, Tom Rini wrote: > > > > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > > > >> On 04/05/2023 14.35, Rasmus Villemoes wrote: > > > >>> On 03/05/2023 16.54, Tom Rini wrote: > > > >> > > > >>>> The one last problem now is on stm32mp15_dhcor_basic which is a > > > >>>> defconfig missing one from OF_LIST but including it in the its file, so > > > >>>> the above is the patch we need. > > > >>>> > > > >> > > > >> Hi Tom > > > >> > > > >> Can I persuade you to try something like > > > >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > > > >> again, but leaving the .dtbo targets in there? > > > >> > > > >> I could send a patch, but it's entirely mechanical, and not really meant > > > >> for being applied until we know if there's more to be cleaned up. > > > > > > > > So what ended up being the problem I think is the case Simon pointed out > > > > where we do take the output from "make all" and concatenate one of the > > > > dtbs that was generated with u-boot.img or so, and it works. But maybe > > > > that should just list all of the valid DTBs that it needs in the > > > > defconfig to start with? I don't quite know, it was a case I hadn't > > > > considered at the time. > > > > > > > > > > Re-reading the thread, I can't see where that was mentioned. > > > > > > But yes, if some boards (still) need that, and have more than one > > > possible .dtb, the board can't set an OF_LIST different from the default > > > consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires > > > SPL_LOAD_FIT || MULTI_DTB_FIT. > > > > > > How do we figure out if such boards even exist? > > > > Honestly at this point I've forgotten what this is all about. > > > > Perhaps the easiest approach is to create a new Kconfig to control > > whether a board-level .dtsi is included in the list of wildcard > > searches. Then you can enable it for your board without affecting > > others. > > That's getting things backwards, from what this cleanup does. Today we > have messy lists of "build these device trees" and then don't use most > of them, and some of the list is just Wrong (listing dts files as an > output). With the series to handle dtbo files, we could remove > virtually all of that, and the only use cases that don't Just Work still > are the ones I forget which board you mentioned (I think it was Samsung > tho?) where the defconfig doesn't list all of the device trees, just one > of them, and the other 5 that we build can also be easily used. Does > that ring a bell? Yes it does...but what is the problem here? The DT files for an SoC are supposed to be buildable without needing to have the context of a particular board. I don't know what the point of the dtbo files is...is that for using overlays somehow at runtime? The defconfig does not need to mention all the DTs...we just need to make sure they get built. Is this another instance of something that needs cleaning up, like the config fragments? I am find with making the boards list the DTs that they can run with, if there is an easy way of doing that. CONFIG_SPL_OF_LIST is just for SPL, I think. Regards, Simon
On Sun, Oct 01, 2023 at 07:17:27PM -0600, Simon Glass wrote: > Hi Tom, > > On Fri, 29 Sept 2023 at 10:02, Tom Rini <trini@konsulko.com> wrote: > > > > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: > > > Hi Rasmus, > > > > > > On Mon, 25 Sept 2023 at 13:05, Rasmus Villemoes > > > <rasmus.villemoes@prevas.dk> wrote: > > > > > > > > On 25/09/2023 20.19, Tom Rini wrote: > > > > > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > > > > >> On 04/05/2023 14.35, Rasmus Villemoes wrote: > > > > >>> On 03/05/2023 16.54, Tom Rini wrote: > > > > >> > > > > >>>> The one last problem now is on stm32mp15_dhcor_basic which is a > > > > >>>> defconfig missing one from OF_LIST but including it in the its file, so > > > > >>>> the above is the patch we need. > > > > >>>> > > > > >> > > > > >> Hi Tom > > > > >> > > > > >> Can I persuade you to try something like > > > > >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > > > > >> again, but leaving the .dtbo targets in there? > > > > >> > > > > >> I could send a patch, but it's entirely mechanical, and not really meant > > > > >> for being applied until we know if there's more to be cleaned up. > > > > > > > > > > So what ended up being the problem I think is the case Simon pointed out > > > > > where we do take the output from "make all" and concatenate one of the > > > > > dtbs that was generated with u-boot.img or so, and it works. But maybe > > > > > that should just list all of the valid DTBs that it needs in the > > > > > defconfig to start with? I don't quite know, it was a case I hadn't > > > > > considered at the time. > > > > > > > > > > > > > Re-reading the thread, I can't see where that was mentioned. > > > > > > > > But yes, if some boards (still) need that, and have more than one > > > > possible .dtb, the board can't set an OF_LIST different from the default > > > > consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires > > > > SPL_LOAD_FIT || MULTI_DTB_FIT. > > > > > > > > How do we figure out if such boards even exist? > > > > > > Honestly at this point I've forgotten what this is all about. > > > > > > Perhaps the easiest approach is to create a new Kconfig to control > > > whether a board-level .dtsi is included in the list of wildcard > > > searches. Then you can enable it for your board without affecting > > > others. > > > > That's getting things backwards, from what this cleanup does. Today we > > have messy lists of "build these device trees" and then don't use most > > of them, and some of the list is just Wrong (listing dts files as an > > output). With the series to handle dtbo files, we could remove > > virtually all of that, and the only use cases that don't Just Work still > > are the ones I forget which board you mentioned (I think it was Samsung > > tho?) where the defconfig doesn't list all of the device trees, just one > > of them, and the other 5 that we build can also be easily used. Does > > that ring a bell? > > Yes it does...but what is the problem here? Messy and unused and incorrect Makefile content. > The DT files for an SoC are supposed to be buildable without needing > to have the context of a particular board. They're still buildable, without an explicit rule, they just need to (like they can now) be built explicitly. > I don't know what the point of the dtbo files is...is that for using > overlays somehow at runtime? I think that side-tracked you from my question. > The defconfig does not need to mention all the DTs...we just need to > make sure they get built. Why? This is the use case that I don't understand and isn't obvious. > Is this another instance of something that needs cleaning up, like the > config fragments? No, this isn't like the config fragments. > I am find with making the boards list the DTs that they can run with, > if there is an easy way of doing that. CONFIG_SPL_OF_LIST is just for > SPL, I think. Yes, every board except for some use case you've described before as far as I know lists the device trees that they use in the defconfig. Which is why there's an impetus to clean up arch/*/dts/Makefile as 95% of those lines can just be removed.
Hi Tom, On Mon, 2 Oct 2023 at 08:09, Tom Rini <trini@konsulko.com> wrote: > > On Sun, Oct 01, 2023 at 07:17:27PM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Fri, 29 Sept 2023 at 10:02, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: > > > > Hi Rasmus, > > > > > > > > On Mon, 25 Sept 2023 at 13:05, Rasmus Villemoes > > > > <rasmus.villemoes@prevas.dk> wrote: > > > > > > > > > > On 25/09/2023 20.19, Tom Rini wrote: > > > > > > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > > > > > >> On 04/05/2023 14.35, Rasmus Villemoes wrote: > > > > > >>> On 03/05/2023 16.54, Tom Rini wrote: > > > > > >> > > > > > >>>> The one last problem now is on stm32mp15_dhcor_basic which is a > > > > > >>>> defconfig missing one from OF_LIST but including it in the its file, so > > > > > >>>> the above is the patch we need. > > > > > >>>> > > > > > >> > > > > > >> Hi Tom > > > > > >> > > > > > >> Can I persuade you to try something like > > > > > >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > > > > > >> again, but leaving the .dtbo targets in there? > > > > > >> > > > > > >> I could send a patch, but it's entirely mechanical, and not really meant > > > > > >> for being applied until we know if there's more to be cleaned up. > > > > > > > > > > > > So what ended up being the problem I think is the case Simon pointed out > > > > > > where we do take the output from "make all" and concatenate one of the > > > > > > dtbs that was generated with u-boot.img or so, and it works. But maybe > > > > > > that should just list all of the valid DTBs that it needs in the > > > > > > defconfig to start with? I don't quite know, it was a case I hadn't > > > > > > considered at the time. > > > > > > > > > > > > > > > > Re-reading the thread, I can't see where that was mentioned. > > > > > > > > > > But yes, if some boards (still) need that, and have more than one > > > > > possible .dtb, the board can't set an OF_LIST different from the default > > > > > consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires > > > > > SPL_LOAD_FIT || MULTI_DTB_FIT. > > > > > > > > > > How do we figure out if such boards even exist? > > > > > > > > Honestly at this point I've forgotten what this is all about. > > > > > > > > Perhaps the easiest approach is to create a new Kconfig to control > > > > whether a board-level .dtsi is included in the list of wildcard > > > > searches. Then you can enable it for your board without affecting > > > > others. > > > > > > That's getting things backwards, from what this cleanup does. Today we > > > have messy lists of "build these device trees" and then don't use most > > > of them, and some of the list is just Wrong (listing dts files as an > > > output). With the series to handle dtbo files, we could remove > > > virtually all of that, and the only use cases that don't Just Work still > > > are the ones I forget which board you mentioned (I think it was Samsung > > > tho?) where the defconfig doesn't list all of the device trees, just one > > > of them, and the other 5 that we build can also be easily used. Does > > > that ring a bell? > > > > Yes it does...but what is the problem here? > > Messy and unused and incorrect Makefile content. The problem I see there is people using TARGET in arch/arm/dts/Makefile for example. There are 80 instances of that. The rules should depend on SoC (e.g. use ARCH_EXYNOS5), as Linux does it. > > > The DT files for an SoC are supposed to be buildable without needing > > to have the context of a particular board. > > They're still buildable, without an explicit rule, they just need to > (like they can now) be built explicitly. But isn't that creating dead code? It will rot. > > > I don't know what the point of the dtbo files is...is that for using > > overlays somehow at runtime? > > I think that side-tracked you from my question. > > > The defconfig does not need to mention all the DTs...we just need to > > make sure they get built. > > Why? This is the use case that I don't understand and isn't obvious. > > > Is this another instance of something that needs cleaning up, like the > > config fragments? > > No, this isn't like the config fragments. > > > I am find with making the boards list the DTs that they can run with, > > if there is an easy way of doing that. CONFIG_SPL_OF_LIST is just for > > SPL, I think. > > Yes, every board except for some use case you've described before as far > as I know lists the device trees that they use in the defconfig. Which > is why there's an impetus to clean up arch/*/dts/Makefile as 95% of > those lines can just be removed. It seems like you are wanting a board-level CONFIG which lists the DTs which need to be built for that board. Is that right? You are suggesting that this already exists, but I am not aware of it. Do you mean SPL_OF_LIST, perhaps? Regards, Simon
On Mon, Oct 02, 2023 at 08:43:41AM -0600, Simon Glass wrote: > Hi Tom, > > On Mon, 2 Oct 2023 at 08:09, Tom Rini <trini@konsulko.com> wrote: > > > > On Sun, Oct 01, 2023 at 07:17:27PM -0600, Simon Glass wrote: > > > Hi Tom, > > > > > > On Fri, 29 Sept 2023 at 10:02, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: > > > > > Hi Rasmus, > > > > > > > > > > On Mon, 25 Sept 2023 at 13:05, Rasmus Villemoes > > > > > <rasmus.villemoes@prevas.dk> wrote: > > > > > > > > > > > > On 25/09/2023 20.19, Tom Rini wrote: > > > > > > > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > > > > > > >> On 04/05/2023 14.35, Rasmus Villemoes wrote: > > > > > > >>> On 03/05/2023 16.54, Tom Rini wrote: > > > > > > >> > > > > > > >>>> The one last problem now is on stm32mp15_dhcor_basic which is a > > > > > > >>>> defconfig missing one from OF_LIST but including it in the its file, so > > > > > > >>>> the above is the patch we need. > > > > > > >>>> > > > > > > >> > > > > > > >> Hi Tom > > > > > > >> > > > > > > >> Can I persuade you to try something like > > > > > > >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > > > > > > >> again, but leaving the .dtbo targets in there? > > > > > > >> > > > > > > >> I could send a patch, but it's entirely mechanical, and not really meant > > > > > > >> for being applied until we know if there's more to be cleaned up. > > > > > > > > > > > > > > So what ended up being the problem I think is the case Simon pointed out > > > > > > > where we do take the output from "make all" and concatenate one of the > > > > > > > dtbs that was generated with u-boot.img or so, and it works. But maybe > > > > > > > that should just list all of the valid DTBs that it needs in the > > > > > > > defconfig to start with? I don't quite know, it was a case I hadn't > > > > > > > considered at the time. > > > > > > > > > > > > > > > > > > > Re-reading the thread, I can't see where that was mentioned. > > > > > > > > > > > > But yes, if some boards (still) need that, and have more than one > > > > > > possible .dtb, the board can't set an OF_LIST different from the default > > > > > > consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires > > > > > > SPL_LOAD_FIT || MULTI_DTB_FIT. > > > > > > > > > > > > How do we figure out if such boards even exist? > > > > > > > > > > Honestly at this point I've forgotten what this is all about. > > > > > > > > > > Perhaps the easiest approach is to create a new Kconfig to control > > > > > whether a board-level .dtsi is included in the list of wildcard > > > > > searches. Then you can enable it for your board without affecting > > > > > others. > > > > > > > > That's getting things backwards, from what this cleanup does. Today we > > > > have messy lists of "build these device trees" and then don't use most > > > > of them, and some of the list is just Wrong (listing dts files as an > > > > output). With the series to handle dtbo files, we could remove > > > > virtually all of that, and the only use cases that don't Just Work still > > > > are the ones I forget which board you mentioned (I think it was Samsung > > > > tho?) where the defconfig doesn't list all of the device trees, just one > > > > of them, and the other 5 that we build can also be easily used. Does > > > > that ring a bell? > > > > > > Yes it does...but what is the problem here? > > > > Messy and unused and incorrect Makefile content. > > The problem I see there is people using TARGET in > arch/arm/dts/Makefile for example. There are 80 instances of that. The > rules should depend on SoC (e.g. use ARCH_EXYNOS5), as Linux does it. It shouldn't be there at all since there's almost no cases where we "just" take an arbitrary dtb file and u-boot.img and then the system boot. That's what this series is about fixing. > > > The DT files for an SoC are supposed to be buildable without needing > > > to have the context of a particular board. > > > > They're still buildable, without an explicit rule, they just need to > > (like they can now) be built explicitly. > > But isn't that creating dead code? It will rot. No, that's the problem we have today, people list something in the Makefile, since they think they need to list something, and then put the device trees they use in the defconfig. [snip] > > > I am find with making the boards list the DTs that they can run with, > > > if there is an easy way of doing that. CONFIG_SPL_OF_LIST is just for > > > SPL, I think. > > > > Yes, every board except for some use case you've described before as far > > as I know lists the device trees that they use in the defconfig. Which > > is why there's an impetus to clean up arch/*/dts/Makefile as 95% of > > those lines can just be removed. > > It seems like you are wanting a board-level CONFIG which lists the DTs > which need to be built for that board. Is that right? You are > suggesting that this already exists, but I am not aware of it. Do you > mean SPL_OF_LIST, perhaps? I mean today CONFIG_DEFAULT_DEVICE_TREE + CONFIG_OF_LIST + CONFIG_SPL_OF_LIST is set and correct for everyone board except some use case you have, which I think is something about exynos? And so we only need scripts/Makefile.dts in arch/*/dts/Makefile
Hi Tom, On Mon, 2 Oct 2023 at 09:12, Tom Rini <trini@konsulko.com> wrote: > > On Mon, Oct 02, 2023 at 08:43:41AM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Mon, 2 Oct 2023 at 08:09, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Sun, Oct 01, 2023 at 07:17:27PM -0600, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Fri, 29 Sept 2023 at 10:02, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: > > > > > > Hi Rasmus, > > > > > > > > > > > > On Mon, 25 Sept 2023 at 13:05, Rasmus Villemoes > > > > > > <rasmus.villemoes@prevas.dk> wrote: > > > > > > > > > > > > > > On 25/09/2023 20.19, Tom Rini wrote: > > > > > > > > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > > > > > > > >> On 04/05/2023 14.35, Rasmus Villemoes wrote: > > > > > > > >>> On 03/05/2023 16.54, Tom Rini wrote: > > > > > > > >> > > > > > > > >>>> The one last problem now is on stm32mp15_dhcor_basic which is a > > > > > > > >>>> defconfig missing one from OF_LIST but including it in the its file, so > > > > > > > >>>> the above is the patch we need. > > > > > > > >>>> > > > > > > > >> > > > > > > > >> Hi Tom > > > > > > > >> > > > > > > > >> Can I persuade you to try something like > > > > > > > >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > > > > > > > >> again, but leaving the .dtbo targets in there? > > > > > > > >> > > > > > > > >> I could send a patch, but it's entirely mechanical, and not really meant > > > > > > > >> for being applied until we know if there's more to be cleaned up. > > > > > > > > > > > > > > > > So what ended up being the problem I think is the case Simon pointed out > > > > > > > > where we do take the output from "make all" and concatenate one of the > > > > > > > > dtbs that was generated with u-boot.img or so, and it works. But maybe > > > > > > > > that should just list all of the valid DTBs that it needs in the > > > > > > > > defconfig to start with? I don't quite know, it was a case I hadn't > > > > > > > > considered at the time. > > > > > > > > > > > > > > > > > > > > > > Re-reading the thread, I can't see where that was mentioned. > > > > > > > > > > > > > > But yes, if some boards (still) need that, and have more than one > > > > > > > possible .dtb, the board can't set an OF_LIST different from the default > > > > > > > consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires > > > > > > > SPL_LOAD_FIT || MULTI_DTB_FIT. > > > > > > > > > > > > > > How do we figure out if such boards even exist? > > > > > > > > > > > > Honestly at this point I've forgotten what this is all about. > > > > > > > > > > > > Perhaps the easiest approach is to create a new Kconfig to control > > > > > > whether a board-level .dtsi is included in the list of wildcard > > > > > > searches. Then you can enable it for your board without affecting > > > > > > others. > > > > > > > > > > That's getting things backwards, from what this cleanup does. Today we > > > > > have messy lists of "build these device trees" and then don't use most > > > > > of them, and some of the list is just Wrong (listing dts files as an > > > > > output). With the series to handle dtbo files, we could remove > > > > > virtually all of that, and the only use cases that don't Just Work still > > > > > are the ones I forget which board you mentioned (I think it was Samsung > > > > > tho?) where the defconfig doesn't list all of the device trees, just one > > > > > of them, and the other 5 that we build can also be easily used. Does > > > > > that ring a bell? > > > > > > > > Yes it does...but what is the problem here? > > > > > > Messy and unused and incorrect Makefile content. > > > > The problem I see there is people using TARGET in > > arch/arm/dts/Makefile for example. There are 80 instances of that. The > > rules should depend on SoC (e.g. use ARCH_EXYNOS5), as Linux does it. > > It shouldn't be there at all since there's almost no cases where we > "just" take an arbitrary dtb file and u-boot.img and then the system > boot. That's what this series is about fixing. I'm really not sure that replacing build rules with a board CONFIG is a good idea. I suppose part of my confusion is why the Makefile is considered a problem? > > > > > The DT files for an SoC are supposed to be buildable without needing > > > > to have the context of a particular board. > > > > > > They're still buildable, without an explicit rule, they just need to > > > (like they can now) be built explicitly. > > > > But isn't that creating dead code? It will rot. > > No, that's the problem we have today, people list something in the > Makefile, since they think they need to list something, and then put the > device trees they use in the defconfig. If they don't list something, it won't build, right? > > [snip] > > > > I am find with making the boards list the DTs that they can run with, > > > > if there is an easy way of doing that. CONFIG_SPL_OF_LIST is just for > > > > SPL, I think. > > > > > > Yes, every board except for some use case you've described before as far > > > as I know lists the device trees that they use in the defconfig. Which > > > is why there's an impetus to clean up arch/*/dts/Makefile as 95% of > > > those lines can just be removed. > > > > It seems like you are wanting a board-level CONFIG which lists the DTs > > which need to be built for that board. Is that right? You are > > suggesting that this already exists, but I am not aware of it. Do you > > mean SPL_OF_LIST, perhaps? > > I mean today CONFIG_DEFAULT_DEVICE_TREE + CONFIG_OF_LIST + > CONFIG_SPL_OF_LIST is set and correct for everyone board except some use > case you have, which I think is something about exynos? And so we only > need scripts/Makefile.dts in arch/*/dts/Makefile Yes exynos5 boards (the original reason for DT) have / had the same u-boot-nodtb.bin and you can add the DT you want to boot it on particular hardware. That is one of the goals of DT. The OF_LIST option is a little vague but I think it means that the DTs are packaged into a FIT in u-boot.img - is that right? But they presumably have to be built first. I think this would be better discussed f2f. Regards, Simon
On Mon, Oct 02, 2023 at 09:39:18AM -0600, Simon Glass wrote: > Hi Tom, > > On Mon, 2 Oct 2023 at 09:12, Tom Rini <trini@konsulko.com> wrote: > > > > On Mon, Oct 02, 2023 at 08:43:41AM -0600, Simon Glass wrote: > > > Hi Tom, > > > > > > On Mon, 2 Oct 2023 at 08:09, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Sun, Oct 01, 2023 at 07:17:27PM -0600, Simon Glass wrote: > > > > > Hi Tom, > > > > > > > > > > On Fri, 29 Sept 2023 at 10:02, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > > > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: > > > > > > > Hi Rasmus, > > > > > > > > > > > > > > On Mon, 25 Sept 2023 at 13:05, Rasmus Villemoes > > > > > > > <rasmus.villemoes@prevas.dk> wrote: > > > > > > > > > > > > > > > > On 25/09/2023 20.19, Tom Rini wrote: > > > > > > > > > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > > > > > > > > >> On 04/05/2023 14.35, Rasmus Villemoes wrote: > > > > > > > > >>> On 03/05/2023 16.54, Tom Rini wrote: > > > > > > > > >> > > > > > > > > >>>> The one last problem now is on stm32mp15_dhcor_basic which is a > > > > > > > > >>>> defconfig missing one from OF_LIST but including it in the its file, so > > > > > > > > >>>> the above is the patch we need. > > > > > > > > >>>> > > > > > > > > >> > > > > > > > > >> Hi Tom > > > > > > > > >> > > > > > > > > >> Can I persuade you to try something like > > > > > > > > >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > > > > > > > > >> again, but leaving the .dtbo targets in there? > > > > > > > > >> > > > > > > > > >> I could send a patch, but it's entirely mechanical, and not really meant > > > > > > > > >> for being applied until we know if there's more to be cleaned up. > > > > > > > > > > > > > > > > > > So what ended up being the problem I think is the case Simon pointed out > > > > > > > > > where we do take the output from "make all" and concatenate one of the > > > > > > > > > dtbs that was generated with u-boot.img or so, and it works. But maybe > > > > > > > > > that should just list all of the valid DTBs that it needs in the > > > > > > > > > defconfig to start with? I don't quite know, it was a case I hadn't > > > > > > > > > considered at the time. > > > > > > > > > > > > > > > > > > > > > > > > > Re-reading the thread, I can't see where that was mentioned. > > > > > > > > > > > > > > > > But yes, if some boards (still) need that, and have more than one > > > > > > > > possible .dtb, the board can't set an OF_LIST different from the default > > > > > > > > consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires > > > > > > > > SPL_LOAD_FIT || MULTI_DTB_FIT. > > > > > > > > > > > > > > > > How do we figure out if such boards even exist? > > > > > > > > > > > > > > Honestly at this point I've forgotten what this is all about. > > > > > > > > > > > > > > Perhaps the easiest approach is to create a new Kconfig to control > > > > > > > whether a board-level .dtsi is included in the list of wildcard > > > > > > > searches. Then you can enable it for your board without affecting > > > > > > > others. > > > > > > > > > > > > That's getting things backwards, from what this cleanup does. Today we > > > > > > have messy lists of "build these device trees" and then don't use most > > > > > > of them, and some of the list is just Wrong (listing dts files as an > > > > > > output). With the series to handle dtbo files, we could remove > > > > > > virtually all of that, and the only use cases that don't Just Work still > > > > > > are the ones I forget which board you mentioned (I think it was Samsung > > > > > > tho?) where the defconfig doesn't list all of the device trees, just one > > > > > > of them, and the other 5 that we build can also be easily used. Does > > > > > > that ring a bell? > > > > > > > > > > Yes it does...but what is the problem here? > > > > > > > > Messy and unused and incorrect Makefile content. > > > > > > The problem I see there is people using TARGET in > > > arch/arm/dts/Makefile for example. There are 80 instances of that. The > > > rules should depend on SoC (e.g. use ARCH_EXYNOS5), as Linux does it. > > > > It shouldn't be there at all since there's almost no cases where we > > "just" take an arbitrary dtb file and u-boot.img and then the system > > boot. That's what this series is about fixing. > > I'm really not sure that replacing build rules with a board CONFIG is > a good idea. I suppose part of my confusion is why the Makefile is > considered a problem? Because it's duplicative and as Rasmus points out, often wrong. > > > > > The DT files for an SoC are supposed to be buildable without needing > > > > > to have the context of a particular board. > > > > > > > > They're still buildable, without an explicit rule, they just need to > > > > (like they can now) be built explicitly. > > > > > > But isn't that creating dead code? It will rot. > > > > No, that's the problem we have today, people list something in the > > Makefile, since they think they need to list something, and then put the > > device trees they use in the defconfig. > > If they don't list something, it won't build, right? No, everyone builds fine since CONFIG_DEFAULT_DEVICE_TREE is pretty much always set. And for run time, if we need more, *OF_LIST gets set. > > [snip] > > > > > I am find with making the boards list the DTs that they can run with, > > > > > if there is an easy way of doing that. CONFIG_SPL_OF_LIST is just for > > > > > SPL, I think. > > > > > > > > Yes, every board except for some use case you've described before as far > > > > as I know lists the device trees that they use in the defconfig. Which > > > > is why there's an impetus to clean up arch/*/dts/Makefile as 95% of > > > > those lines can just be removed. > > > > > > It seems like you are wanting a board-level CONFIG which lists the DTs > > > which need to be built for that board. Is that right? You are > > > suggesting that this already exists, but I am not aware of it. Do you > > > mean SPL_OF_LIST, perhaps? > > > > I mean today CONFIG_DEFAULT_DEVICE_TREE + CONFIG_OF_LIST + > > CONFIG_SPL_OF_LIST is set and correct for everyone board except some use > > case you have, which I think is something about exynos? And so we only > > need scripts/Makefile.dts in arch/*/dts/Makefile > > Yes exynos5 boards (the original reason for DT) have / had the same > u-boot-nodtb.bin and you can add the DT you want to boot it on > particular hardware. That is one of the goals of DT. Yes, but "and you concat the two files" isn't common. We _can_ keep the EXYNOS list, if that's actually being used. But most of that list is not, as far as I can tell, because the flow is "U-Boot binarie(s) + DTB files + stuff" to get the correctly formatted blob for the firmware. > The OF_LIST option is a little vague but I think it means that the DTs > are packaged into a FIT in u-boot.img - is that right? But they > presumably have to be built first. No, they don't have to be built first because scripts/Makefile.dts ensures that we build everything in *OF_LIST.
Hi Tom, On Mon, 2 Oct 2023 at 10:22, Tom Rini <trini@konsulko.com> wrote: > > On Mon, Oct 02, 2023 at 09:39:18AM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Mon, 2 Oct 2023 at 09:12, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Mon, Oct 02, 2023 at 08:43:41AM -0600, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Mon, 2 Oct 2023 at 08:09, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > On Sun, Oct 01, 2023 at 07:17:27PM -0600, Simon Glass wrote: > > > > > > Hi Tom, > > > > > > > > > > > > On Fri, 29 Sept 2023 at 10:02, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > > > > > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: > > > > > > > > Hi Rasmus, > > > > > > > > > > > > > > > > On Mon, 25 Sept 2023 at 13:05, Rasmus Villemoes > > > > > > > > <rasmus.villemoes@prevas.dk> wrote: > > > > > > > > > > > > > > > > > > On 25/09/2023 20.19, Tom Rini wrote: > > > > > > > > > > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: > > > > > > > > > >> On 04/05/2023 14.35, Rasmus Villemoes wrote: > > > > > > > > > >>> On 03/05/2023 16.54, Tom Rini wrote: > > > > > > > > > >> > > > > > > > > > >>>> The one last problem now is on stm32mp15_dhcor_basic which is a > > > > > > > > > >>>> defconfig missing one from OF_LIST but including it in the its file, so > > > > > > > > > >>>> the above is the patch we need. > > > > > > > > > >>>> > > > > > > > > > >> > > > > > > > > > >> Hi Tom > > > > > > > > > >> > > > > > > > > > >> Can I persuade you to try something like > > > > > > > > > >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 > > > > > > > > > >> again, but leaving the .dtbo targets in there? > > > > > > > > > >> > > > > > > > > > >> I could send a patch, but it's entirely mechanical, and not really meant > > > > > > > > > >> for being applied until we know if there's more to be cleaned up. > > > > > > > > > > > > > > > > > > > > So what ended up being the problem I think is the case Simon pointed out > > > > > > > > > > where we do take the output from "make all" and concatenate one of the > > > > > > > > > > dtbs that was generated with u-boot.img or so, and it works. But maybe > > > > > > > > > > that should just list all of the valid DTBs that it needs in the > > > > > > > > > > defconfig to start with? I don't quite know, it was a case I hadn't > > > > > > > > > > considered at the time. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Re-reading the thread, I can't see where that was mentioned. > > > > > > > > > > > > > > > > > > But yes, if some boards (still) need that, and have more than one > > > > > > > > > possible .dtb, the board can't set an OF_LIST different from the default > > > > > > > > > consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires > > > > > > > > > SPL_LOAD_FIT || MULTI_DTB_FIT. > > > > > > > > > > > > > > > > > > How do we figure out if such boards even exist? > > > > > > > > > > > > > > > > Honestly at this point I've forgotten what this is all about. > > > > > > > > > > > > > > > > Perhaps the easiest approach is to create a new Kconfig to control > > > > > > > > whether a board-level .dtsi is included in the list of wildcard > > > > > > > > searches. Then you can enable it for your board without affecting > > > > > > > > others. > > > > > > > > > > > > > > That's getting things backwards, from what this cleanup does. Today we > > > > > > > have messy lists of "build these device trees" and then don't use most > > > > > > > of them, and some of the list is just Wrong (listing dts files as an > > > > > > > output). With the series to handle dtbo files, we could remove > > > > > > > virtually all of that, and the only use cases that don't Just Work still > > > > > > > are the ones I forget which board you mentioned (I think it was Samsung > > > > > > > tho?) where the defconfig doesn't list all of the device trees, just one > > > > > > > of them, and the other 5 that we build can also be easily used. Does > > > > > > > that ring a bell? > > > > > > > > > > > > Yes it does...but what is the problem here? > > > > > > > > > > Messy and unused and incorrect Makefile content. > > > > > > > > The problem I see there is people using TARGET in > > > > arch/arm/dts/Makefile for example. There are 80 instances of that. The > > > > rules should depend on SoC (e.g. use ARCH_EXYNOS5), as Linux does it. > > > > > > It shouldn't be there at all since there's almost no cases where we > > > "just" take an arbitrary dtb file and u-boot.img and then the system > > > boot. That's what this series is about fixing. > > > > I'm really not sure that replacing build rules with a board CONFIG is > > a good idea. I suppose part of my confusion is why the Makefile is > > considered a problem? > > Because it's duplicative and as Rasmus points out, often wrong. To me, that is under the control of the board maintainers. They should not put CONFIG_SYS_BOARD in the DT...it obviously goes against what DT is for and I'm not sure what binding would allow it anyway. If it helps, I could do a little series to remove those? I think it is just arch/arm/dts/rockchip-optee.dtsi ? This seems like a case of throwing the baby out with the bathwater. > > > > > > > The DT files for an SoC are supposed to be buildable without needing > > > > > > to have the context of a particular board. > > > > > > > > > > They're still buildable, without an explicit rule, they just need to > > > > > (like they can now) be built explicitly. > > > > > > > > But isn't that creating dead code? It will rot. > > > > > > No, that's the problem we have today, people list something in the > > > Makefile, since they think they need to list something, and then put the > > > device trees they use in the defconfig. > > > > If they don't list something, it won't build, right? > > No, everyone builds fine since CONFIG_DEFAULT_DEVICE_TREE is pretty much > always set. And for run time, if we need more, *OF_LIST gets set. OK, makes sense. So what DT does u-boot.bin contain in that case? Just the default? And u-boot.img contains a FIT with all of them? > > > > [snip] > > > > > > I am find with making the boards list the DTs that they can run with, > > > > > > if there is an easy way of doing that. CONFIG_SPL_OF_LIST is just for > > > > > > SPL, I think. > > > > > > > > > > Yes, every board except for some use case you've described before as far > > > > > as I know lists the device trees that they use in the defconfig. Which > > > > > is why there's an impetus to clean up arch/*/dts/Makefile as 95% of > > > > > those lines can just be removed. > > > > > > > > It seems like you are wanting a board-level CONFIG which lists the DTs > > > > which need to be built for that board. Is that right? You are > > > > suggesting that this already exists, but I am not aware of it. Do you > > > > mean SPL_OF_LIST, perhaps? > > > > > > I mean today CONFIG_DEFAULT_DEVICE_TREE + CONFIG_OF_LIST + > > > CONFIG_SPL_OF_LIST is set and correct for everyone board except some use > > > case you have, which I think is something about exynos? And so we only > > > need scripts/Makefile.dts in arch/*/dts/Makefile > > > > Yes exynos5 boards (the original reason for DT) have / had the same > > u-boot-nodtb.bin and you can add the DT you want to boot it on > > particular hardware. That is one of the goals of DT. > > Yes, but "and you concat the two files" isn't common. We _can_ keep the > EXYNOS list, if that's actually being used. But most of that list is > not, as far as I can tell, because the flow is "U-Boot binarie(s) + DTB > files + stuff" to get the correctly formatted blob for the firmware. I believe rockchip allows just the DT to be changed, to work on a board with the same SoC. In principle it would be possible to build a common u-boot-nodtb.bin too, e.g. for 32-bit. We are trying to use DT (and not custom C code) to deal with the differences between boards. So long as that is preserved, I am happy enough. > > > The OF_LIST option is a little vague but I think it means that the DTs > > are packaged into a FIT in u-boot.img - is that right? But they > > presumably have to be built first. > > No, they don't have to be built first because scripts/Makefile.dts > ensures that we build everything in *OF_LIST. That is from this commit: 3609e1dc5f4 dts: automatically build necessary .dtb files which was for use by custom boards using a private branch. Did it have a wider purpose that I didn't understand? Regards, Simon
On 29/09/2023 18.02, Tom Rini wrote: > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: >> Honestly at this point I've forgotten what this is all about. Fair enough, let me try to recap, though even a summary is a bit long. (1) I wanted to do what $subject says, and you seemed to be ok with that https://lore.kernel.org/u-boot/CAPnjgZ1B+3v_5rCSBnUgZ6cVO4odjjDfeS0rx3iMw4vjXUo0VA@mail.gmail.com/ (2) Tom pointed out that, unfortunately, as-is, the patch caused a bunch of build failures. With hindsight, that was inevitable. https://lore.kernel.org/u-boot/c3c94614-9916-7316-e009-04ddbdc205bc@prevas.dk/ (3) I still very much would like the original patch to go in, and so I pointed out that most of the current arch/*/dts/Makefile are actually completely redundant, with the logic in scripts/Makefile.dts being in place. This is when the thread turned away from talking about the original patch, but rather the cleanup of the makefile logic that turns out to be a prerequisite for said patch to go anywhere. (4) Tom tested a patch that nuked most of the arch/arm/dts/Makefile, which revealed a few defects, partly in Makefile.dts (got fixed in 6923f49d3ac2) partly in a board defconfig (got fixed in 2d158d3c387d). >> Perhaps the easiest approach is to create a new Kconfig to control >> whether a board-level .dtsi is included in the list of wildcard >> searches. Then you can enable it for your board without affecting >> others. > > That's getting things backwards, from what this cleanup does. Today we > have messy lists of "build these device trees" and then don't use most > of them, and some of the list is just Wrong (listing dts files as an > output). (5) Then a few months passed. I'm still interested in the original patch, and also the cleanup. So I pinged Tom to redo that build test with most of the Makefile gone. While revisiting this and doing that mechanical strip-down of the Makefile, I noticed that the Makefile has grown not just one but two .dts files listed in dtb-y, which is of course bogus, and nobody noticed or cared because the scripts/Makefile.dts logic JustWorks. So, given that today, each board, in the form of the defconfig used to build for it, in 99.9% of cases already includes all the information the build system needs in order to ensure that all relevant .dtb files gets built [because there's only one and that's DEFAULT_DEVICETREE, or there's more and those are in OF_LIST or SPL_OF_LIST], and the Makefile is error-prone to maintain and when adding a new board one chooses some semi-random-list to add one's .dtbs (or .dts.....) to, we want to simply stop having all those lists in arch/arm/dts/Makefile (and the other arch/*/dts/Makefile, but those are much smaller and can be handled later). No TARGET lists, no SOC lists, no nothing. Yes, there may still be some 0.1%. We're trying to figure out if they exist, which they are, and how best to handle them. An easy fix is to drop the condition on when OF_LIST is settable to something different from its default (which is DEFAULT_DEVICETREE). But without knowing just exactly which boards and which .dtbs we're talking about, it's hard to know what the best solution is or if there is actually anything that needs to be done. Rasmus
On 02/10/2023 20.56, Simon Glass wrote: > Hi Tom, > > On Mon, 2 Oct 2023 at 10:22, Tom Rini <trini@konsulko.com> wrote: >> >>> I'm really not sure that replacing build rules with a board CONFIG is >>> a good idea. I suppose part of my confusion is why the Makefile is >>> considered a problem? >> >> Because it's duplicative and as Rasmus points out, often wrong. > > To me, that is under the control of the board maintainers. They should > not put CONFIG_SYS_BOARD in the DT...it obviously goes against what DT > is for and I'm not sure what binding would allow it anyway. > > If it helps, I could do a little series to remove those? I think it is > just arch/arm/dts/rockchip-optee.dtsi ? > > This seems like a case of throwing the baby out with the bathwater. > Huh? I don't think you understood what Tom was referring to, and I certainly don't understand what you're talking about. (1) duplicative = the _defconfig already has the necessary information in *OF_LIST / DEFAULT_DEVICETREE (2) often wrong = people add .dts targets to dtb-y, not .dtb targets. They are harmless no-ops because people do get their .dtb files built due to (1). "often" is perhaps a bit of an overstatement. > We are trying to use DT (and not custom C code) to deal with the > differences between boards. So long as that is preserved, I am happy > enough. Of course. This has nothing at all to do with C code versus device tree(s). Just how we ensure that all relevant .dtbs that get built today with the massive [mostly redundant] lists will still be built if we remove all those dtb-$(FOO) += lists. >>> The OF_LIST option is a little vague but I think it means that the DTs >>> are packaged into a FIT in u-boot.img - is that right? But they >>> presumably have to be built first. >> >> No, they don't have to be built first because scripts/Makefile.dts >> ensures that we build everything in *OF_LIST. > > That is from this commit: > > 3609e1dc5f4 dts: automatically build necessary .dtb files > > which was for use by custom boards using a private branch. Did it have > a wider purpose that I didn't understand? Not at the time, but it turns out that since we now _do_ have it, new in-tree boards also don't have to modify an arch/*/dts/Makefile, and do it wrong. Rasmus
Hi Rasmus, On Mon, 2 Oct 2023 at 13:02, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 29/09/2023 18.02, Tom Rini wrote: > > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: > > >> Honestly at this point I've forgotten what this is all about. > > Fair enough, let me try to recap, though even a summary is a bit long. > > (1) I wanted to do what $subject says, and you seemed to be ok with that > > https://lore.kernel.org/u-boot/CAPnjgZ1B+3v_5rCSBnUgZ6cVO4odjjDfeS0rx3iMw4vjXUo0VA@mail.gmail.com/ > > (2) Tom pointed out that, unfortunately, as-is, the patch caused a bunch > of build failures. With hindsight, that was inevitable. > > https://lore.kernel.org/u-boot/c3c94614-9916-7316-e009-04ddbdc205bc@prevas.dk/ > > (3) I still very much would like the original patch to go in, and so I > pointed out that most of the current arch/*/dts/Makefile are actually > completely redundant, with the logic in scripts/Makefile.dts being in place. > > This is when the thread turned away from talking about the original > patch, but rather the cleanup of the makefile logic that turns out to be > a prerequisite for said patch to go anywhere. > > (4) Tom tested a patch that nuked most of the arch/arm/dts/Makefile, > which revealed a few defects, partly in Makefile.dts (got fixed in > 6923f49d3ac2) partly in a board defconfig (got fixed in 2d158d3c387d). > > >> Perhaps the easiest approach is to create a new Kconfig to control > >> whether a board-level .dtsi is included in the list of wildcard > >> searches. Then you can enable it for your board without affecting > >> others. > > > > That's getting things backwards, from what this cleanup does. Today we > > have messy lists of "build these device trees" and then don't use most > > of them, and some of the list is just Wrong (listing dts files as an > > output). > > (5) Then a few months passed. I'm still interested in the original > patch, and also the cleanup. So I pinged Tom to redo that build test > with most of the Makefile gone. While revisiting this and doing that > mechanical strip-down of the Makefile, I noticed that the Makefile has > grown not just one but two .dts files listed in dtb-y, which is of > course bogus, and nobody noticed or cared because the > scripts/Makefile.dts logic JustWorks. > > So, given that today, each board, in the form of the defconfig used to > build for it, in 99.9% of cases already includes all the information the > build system needs in order to ensure that all relevant .dtb files gets > built [because there's only one and that's DEFAULT_DEVICETREE, or > there's more and those are in OF_LIST or SPL_OF_LIST], and the Makefile > is error-prone to maintain and when adding a new board one chooses some > semi-random-list to add one's .dtbs (or .dts.....) to, we want to simply > stop having all those lists in arch/arm/dts/Makefile (and the other > arch/*/dts/Makefile, but those are much smaller and can be handled > later). No TARGET lists, no SOC lists, no nothing. > > Yes, there may still be some 0.1%. We're trying to figure out if they > exist, which they are, and how best to handle them. An easy fix is to > drop the condition on when OF_LIST is settable to something different > from its default (which is DEFAULT_DEVICETREE). But without knowing just > exactly which boards and which .dtbs we're talking about, it's hard to > know what the best solution is or if there is actually anything that > needs to be done. Thanks for all the details. My memory for things that last across months is not good. It is clearer now. I don't really see any specific problem with this. It does seem duplicative though, in that each board (in a group that differ only in their DT) has to have the same list. Why not have it once, in the Makefile, based on ARCH_SOC...? Perhaps I am unrealistic in hoping that much of the C code for a board will go away, but we do see boards which don't have much / any code, but do have hardware differences. I would love to put a stake in the ground and say that boards that use the same SoC should be able to use the same C code and work correctly, just with a different DT. For example I think most/all rockchip boards are like that. Then we might head closer to the kernel which can boot on various boards with the same build. It seems to me that heading the way you describe, we are making it harder for people to do this, or even suggesting that it is not desirable. Anyway I will leave you and Tom to figure this out, since you seem very keen on this idea. Thank you again for explaining it all. Regards, Simon
diff --git a/doc/develop/devicetree/control.rst b/doc/develop/devicetree/control.rst index c71570d64b..831d225fb1 100644 --- a/doc/develop/devicetree/control.rst +++ b/doc/develop/devicetree/control.rst @@ -174,6 +174,7 @@ devicetree for your board, searching for them in the same directory as the main file, in this order:: <orig_filename>-u-boot.dtsi + <CONFIG_SYS_BOARD>-u-boot.dtsi <CONFIG_SYS_SOC>-u-boot.dtsi <CONFIG_SYS_CPU>-u-boot.dtsi <CONFIG_SYS_VENDOR>-u-boot.dtsi diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index ecc15041ee..69f4d560d1 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -160,6 +160,7 @@ ld_flags = $(KBUILD_LDFLAGS) $(ldflags-y) $(LDFLAGS_$(@F)) # Try these files in order to find the U-Boot-specific .dtsi include file u_boot_dtsi_options = $(strip $(wildcard $(dir $<)$(basename $(notdir $<))-u-boot.dtsi) \ + $(wildcard $(dir $<)$(subst $\",,$(CONFIG_SYS_BOARD))-u-boot.dtsi) \ $(wildcard $(dir $<)$(subst $\",,$(CONFIG_SYS_SOC))-u-boot.dtsi) \ $(wildcard $(dir $<)$(subst $\",,$(CONFIG_SYS_CPU))-u-boot.dtsi) \ $(wildcard $(dir $<)$(subst $\",,$(CONFIG_SYS_VENDOR))-u-boot.dtsi) \ @@ -167,6 +168,7 @@ u_boot_dtsi_options = $(strip $(wildcard $(dir $<)$(basename $(notdir $<))-u-boo u_boot_dtsi_options_raw = $(warning Automatic .dtsi inclusion: options: \ $(dir $<)$(basename $(notdir $<))-u-boot.dtsi \ + $(dir $<)$(subst $\",,$(CONFIG_SYS_BOARD))-u-boot.dtsi \ $(dir $<)$(subst $\",,$(CONFIG_SYS_SOC))-u-boot.dtsi \ $(dir $<)$(subst $\",,$(CONFIG_SYS_CPU))-u-boot.dtsi \ $(dir $<)$(subst $\",,$(CONFIG_SYS_VENDOR))-u-boot.dtsi \
I have a couple of boards, e.g. foo21, bar33, each with a few different revisions, so I have foo21-revA.dts foo21-revB.dts bar33-revA.dts bar33-revB.dts Now the necessary U-Boot specific additions for the foo21 boards doesn't depend on the revision (likely for bar33), so I just want to have and maintain one foo21-u-boot.dtsi But currently I need to add dummy files foo21-revA-u-boot.dtsi and foo21-revB-u-boot.dtsi each just containing '#include foo21-u-boot.dtsi'. And similarly for bar33, and all those files become quite unwieldy as more revisions need to be supported. It's quite natural to look for a file named after CONFIG_SYS_BOARD, with lower precedence of course than a -u-boot.dtsi file with the same basename as the .dts, but higher than CONFIG_SYS_SOC. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- Of course, this can cause unwanted changes for existing boards. But a bit of ad hoc scripting shows that the risk is low: I first grabbed all 'default "foo"' stanzas of all 'config SYS_BOARD' instances, as well as all values of CONFIG_SYS_BOARD set in *_defconfig files. Then I made a list of all existing *-u-boot.dtsi files, and from these removed any where there is a corresponding .dts file. That leaves just imx6ul imx8mm mt7620 mt7621 mt7622 mt7623 mt7628 mt8516 socfpga_arria10 sunxi and inspecting a few of those suggests that they set SYS_SOC to the same as SYS_BOARD, i.e. they were already picking up the .dtsi file due to the SYS_SOC rule. 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. But I do notice that doc/develop/devicetree/control.rst | 1 + scripts/Makefile.lib | 2 ++ 2 files changed, 3 insertions(+)