| Message ID | 20220924205327.1489102-1-thomas.petazzoni@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/4] arch/Config.in.xtensa: adjust endianness logic to avoid bogus configurations | expand |
Thomas, All, On 2022-09-24 22:53 +0200, Thomas Petazzoni via buildroot spake thusly: > The Xtensa architecture supports both LE and BE configurations. When > BR2_XTENSA_CUSTOM is chosen, a choice in menuconfig to choose between > LE and BE. > > However, if using the internal toolchain backend, when the > BR2_XTENSA_OVERLAY_FILE is empty, in practice, the configuration will > always be big endian. But the choice being present, random > configuration testing ends up testing configurations where > BR2_XTENSA_OVERLAY_FILE is empty, but the endianness selected is LE, > which is incorrect. > > This commit fixes this by: > > (1) Showing the overlay file option only when building an internal > toolchain. For external toolchain configurations, it does not > make sense to show it as it will have no effect. This should be a separate patch. > (2) Showing the endianness choice either for internal toolchain > configurations with non-empty BR2_XTENSA_OVERLAY_FILE, or for > external toolchain configurations. As an empty BR2_XTENSA_OVERLAY_FILE is not accepted anymore (see below), this can be simplified to "sharing the endianness chice for internal and external toolchains". > (2) Defaulting to BE when BR2_XTENSA_OVERLAY_FILE is empty. An empty BR2_XTENSA_OVERLAY_FILE is now no longer accepted, after commit 4cbf7336914f (arch/xtensa: custom configuration requires an overlay). > Fixes: > http://autobuild.buildroot.net/results/702e2886156f291466375dfcf412c20f1aa5857d/ This commit if from August the 15th; I could not find any such failure since 4cbf7336914f was applied; the last occured 2022-09-08, on commit d2141f65e449, which is the parent of 4cbf7336914f (there are build failures on older branches, of course, but not on master). Regards, Yann E. MORIN. > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > --- > arch/Config.in.xtensa | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/arch/Config.in.xtensa b/arch/Config.in.xtensa > index f9d5adb4c9..e9263f888e 100644 > --- a/arch/Config.in.xtensa > +++ b/arch/Config.in.xtensa > @@ -14,7 +14,7 @@ endchoice > > config BR2_XTENSA_OVERLAY_FILE > string "Overlay file for custom configuration" > - depends on BR2_XTENSA_CUSTOM > + depends on BR2_XTENSA_CUSTOM && BR2_TOOLCHAIN_BUILDROOT > help > Enter the path to the overlay tarball for a custom processor > configuration. > @@ -31,6 +31,7 @@ choice > prompt "Target Architecture Endianness" > default BR2_XTENSA_LITTLE_ENDIAN > depends on BR2_XTENSA_CUSTOM > + depends on BR2_XTENSA_OVERLAY_FILE != "" || BR2_TOOLCHAIN_EXTERNAL > > config BR2_XTENSA_LITTLE_ENDIAN > bool "Little endian" > @@ -51,7 +52,9 @@ config BR2_XTENSA_USE_MMU > > config BR2_ENDIAN > default "LITTLE" if BR2_XTENSA_LITTLE_ENDIAN > - default "BIG" if BR2_xtensa_fsf || BR2_XTENSA_BIG_ENDIAN > + default "BIG" if BR2_XTENSA_BIG_ENDIAN > + default "BIG" if BR2_xtensa_fsf > + default "BIG" if BR2_XTENSA_CUSTOM && BR2_XTENSA_OVERLAY_FILE = "" > > config BR2_ARCH > default "xtensa" if BR2_xtensa > -- > 2.37.3 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
Hello, On Sun, 25 Sep 2022 09:25:02 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > (1) Showing the overlay file option only when building an internal > > toolchain. For external toolchain configurations, it does not > > make sense to show it as it will have no effect. > > This should be a separate patch. ACK. > > (2) Showing the endianness choice either for internal toolchain > > configurations with non-empty BR2_XTENSA_OVERLAY_FILE, or for > > external toolchain configurations. > > As an empty BR2_XTENSA_OVERLAY_FILE is not accepted anymore (see below), See my patch 2/4. Not accepting an empty BR2_XTENSA_OVERLAY_FILE is totally wrong, as it breaks the build with external toolchains. External toolchains can target a custom Xtensa core, but specifying a overlay file with an external toolchain does not make any sense. My patch 2/4 therefore reverts 4cbf7336914f, which is wrong. > this can be simplified to "sharing the endianness chice for internal and > external toolchains". > > > (2) Defaulting to BE when BR2_XTENSA_OVERLAY_FILE is empty. > > An empty BR2_XTENSA_OVERLAY_FILE is now no longer accepted, after commit > 4cbf7336914f (arch/xtensa: custom configuration requires an overlay). Right, but this commit is incorrect, and needs to be reverted. > > Fixes: > > http://autobuild.buildroot.net/results/702e2886156f291466375dfcf412c20f1aa5857d/ > > This commit if from August the 15th; I could not find any such failure > since 4cbf7336914f was applied; the last occured 2022-09-08, on commit > d2141f65e449, which is the parent of 4cbf7336914f (there are build > failures on older branches, of course, but not on master). See above: 4cbf7336914f is incorrect. It breaks for example the test case for the Bootlin Xtensa LX60 external toolchain. An empty overlay file is perfectly valid for a custom Xtensa core, when using an external toolchain. And I am precisely doing the revert *after* this patch 1/4 so that the series is bisectable. I.e introduce the correct fix, and then remove the incorrect fix. Best regards, Thomas
On Sun, 25 Sep 2022 10:30:19 +0200 Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > On Sun, 25 Sep 2022 09:25:02 +0200 > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > > > (1) Showing the overlay file option only when building an internal > > > toolchain. For external toolchain configurations, it does not > > > make sense to show it as it will have no effect. > > > > This should be a separate patch. > > ACK. Except in fact the overlay thing needs to be allowed even with an external toolchain: it can patch Linux and U-Boot as well as the toolchain components. I'll rework the patch series with this in mind. Thomas
diff --git a/arch/Config.in.xtensa b/arch/Config.in.xtensa index f9d5adb4c9..e9263f888e 100644 --- a/arch/Config.in.xtensa +++ b/arch/Config.in.xtensa @@ -14,7 +14,7 @@ endchoice config BR2_XTENSA_OVERLAY_FILE string "Overlay file for custom configuration" - depends on BR2_XTENSA_CUSTOM + depends on BR2_XTENSA_CUSTOM && BR2_TOOLCHAIN_BUILDROOT help Enter the path to the overlay tarball for a custom processor configuration. @@ -31,6 +31,7 @@ choice prompt "Target Architecture Endianness" default BR2_XTENSA_LITTLE_ENDIAN depends on BR2_XTENSA_CUSTOM + depends on BR2_XTENSA_OVERLAY_FILE != "" || BR2_TOOLCHAIN_EXTERNAL config BR2_XTENSA_LITTLE_ENDIAN bool "Little endian" @@ -51,7 +52,9 @@ config BR2_XTENSA_USE_MMU config BR2_ENDIAN default "LITTLE" if BR2_XTENSA_LITTLE_ENDIAN - default "BIG" if BR2_xtensa_fsf || BR2_XTENSA_BIG_ENDIAN + default "BIG" if BR2_XTENSA_BIG_ENDIAN + default "BIG" if BR2_xtensa_fsf + default "BIG" if BR2_XTENSA_CUSTOM && BR2_XTENSA_OVERLAY_FILE = "" config BR2_ARCH default "xtensa" if BR2_xtensa
The Xtensa architecture supports both LE and BE configurations. When BR2_XTENSA_CUSTOM is chosen, a choice in menuconfig to choose between LE and BE. However, if using the internal toolchain backend, when the BR2_XTENSA_OVERLAY_FILE is empty, in practice, the configuration will always be big endian. But the choice being present, random configuration testing ends up testing configurations where BR2_XTENSA_OVERLAY_FILE is empty, but the endianness selected is LE, which is incorrect. This commit fixes this by: (1) Showing the overlay file option only when building an internal toolchain. For external toolchain configurations, it does not make sense to show it as it will have no effect. (2) Showing the endianness choice either for internal toolchain configurations with non-empty BR2_XTENSA_OVERLAY_FILE, or for external toolchain configurations. (2) Defaulting to BE when BR2_XTENSA_OVERLAY_FILE is empty. Fixes: http://autobuild.buildroot.net/results/702e2886156f291466375dfcf412c20f1aa5857d/ Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> --- arch/Config.in.xtensa | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)