[Buildroot] [PATCH v2 0/5] Fixes/improvements in Xtensa handling

[Buildroot] [PATCH v2 0/5] Fixes/improvements in Xtensa handling

[Thomas Petazzoni]

Hello,

This is a v2 of my patches to fix/improve xtensa handling.

The starting point is that commit
4cbf7336914f25478aea943456ba7dc3c892c21a ("arch/xtensa: custom
configuration requires an overlay") broke external Xtensa toolchain
configurations, which select a custom core. Indeed, this commit made
the assumption that it is not possible to have an empty overlay file
with a custom core, but that is not a correct assumption.

So instead, commit 1/5 in this series fixes the original issue
(autobuilders reporting problems of endianness) by making it not
possible to select "big endian" with internal toolchain when the
overlay file is empty. Indeed, with an empty overlay file, the CPU
will always be little endian.

We then revert commit 4cbf7336914f25478aea943456ba7dc3c892c21a in
PATCH 2/5, which allows two of the runtime tests to build again
properly.

In patch 3 to 5, we add explicit support for the DC233C Xtensa core.

Thomas

Thomas Petazzoni (5):
  arch/Config.in.xtensa: adjust endianness logic to avoid bogus
    configurations
  Revert "arch/xtensa: custom configuration requires an overlay"
  arch/Config.in.xtensa: use one line per case in BR2_ENDIAN definition
  arch/{Config.in,arch.mk}.xtensa: add explicit support for DC233C core
  configs/qemu_xtensa_lx60*: use new BR2_xtensa_dc233c option

 arch/Config.in.xtensa                    | 13 +++++++++++--
 arch/arch.mk.xtensa                      | 13 +++++++------
 configs/qemu_xtensa_lx60_defconfig       |  3 +--
 configs/qemu_xtensa_lx60_nommu_defconfig |  3 +--
 4 files changed, 20 insertions(+), 12 deletions(-)

-- 
2.37.3

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 1/5] arch/Config.in.xtensa: adjust endianness logic to avoid bogus configurations

[Thomas Petazzoni]

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 preventing the selection of
BR2_XTENSA_BIG_ENDIAN when an internal toolchain is used with an empty
overlay file.

This commit will allow reverting
4cbf7336914f25478aea943456ba7dc3c892c21a ("arch/xtensa: custom
configuration requires an overlay") which broke the build for external
toolchains that use a custom Xtensa core.

Fixes (even though currently hidden by commit
4cbf7336914f25478aea943456ba7dc3c892c21a):

  http://autobuild.buildroot.net/results/702e2886156f291466375dfcf412c20f1aa5857d/

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 arch/Config.in.xtensa | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/Config.in.xtensa b/arch/Config.in.xtensa
index f9d5adb4c9..4bb58e1964 100644
--- a/arch/Config.in.xtensa
+++ b/arch/Config.in.xtensa
@@ -37,6 +37,10 @@ config BR2_XTENSA_LITTLE_ENDIAN
 
 config BR2_XTENSA_BIG_ENDIAN
 	bool "Big endian"
+	# Internal toolchains without an overlay file are always
+	# little-endian, so we prevent from selecting big endian in
+	# this case.
+	depends on BR2_XTENSA_OVERLAY_FILE != "" || BR2_TOOLCHAIN_EXTERNAL
 
 endchoice
 
-- 
2.37.3

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 2/5] Revert "arch/xtensa: custom configuration requires an overlay"

[Thomas Petazzoni]

This reverts commit 4cbf7336914f25478aea943456ba7dc3c892c21a.

This commit breaks the build of BR2_XTENSA_CUSTOM=y configurations
that use an external toolchain. In such configurations, having an
empty overlay is perfectly fine. For example, this commit broke the
following two runtime tests:

  tests.toolchain.test_external_bootlin.TestExternalToolchainBootlinXtensalx60UclibcBleedingEdge
  tests.toolchain.test_external_bootlin.TestExternalToolchainBootlinXtensalx60UclibcStable

And in fact, having an empty overlay is even correct when building an
internal toolchain. The endianness issue that
4cbf7336914f25478aea943456ba7dc3c892c21a was attempting to fix has
been fixed in a better way by the previous commit.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 arch/arch.mk.xtensa | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa
index 7b6c59cecd..fd410f6bfa 100644
--- a/arch/arch.mk.xtensa
+++ b/arch/arch.mk.xtensa
@@ -1,9 +1,3 @@
-BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE))
-
-ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:)
-$(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting)
-endif
-
 ################################################################################
 # This variable can be used by packages that need to extract the overlay.
 #
@@ -15,6 +9,7 @@ endif
 #       tar xf $(ARCH_XTENSA_OVERLAY_FILE) -C $(@D) --strip-components=1 gcc
 #   endif
 ################################################################################
+BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE))
 ifneq ($(filter http://% https://% ftp://% scp://%,$(BR_ARCH_XTENSA_OVERLAY_FILE)),)
 ARCH_XTENSA_OVERLAY_URL = $(BR_ARCH_XTENSA_OVERLAY_FILE)
 ARCH_XTENSA_OVERLAY_FILE = $($(PKG)_DL_DIR)/$(notdir $(BR_ARCH_XTENSA_OVERLAY_FILE))
-- 
2.37.3

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 3/5] arch/Config.in.xtensa: use one line per case in BR2_ENDIAN definition

[Thomas Petazzoni]

As we're about to introduce another Xtensa core, let's tidy up the
BR2_ENDIAN definition to have one line per case.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 arch/Config.in.xtensa | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/Config.in.xtensa b/arch/Config.in.xtensa
index 4bb58e1964..3745f84537 100644
--- a/arch/Config.in.xtensa
+++ b/arch/Config.in.xtensa
@@ -55,7 +55,8 @@ 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
 
 config BR2_ARCH
 	default "xtensa"	if BR2_xtensa
-- 
2.37.3

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 4/5] arch/{Config.in, arch.mk}.xtensa: add explicit support for DC233C core

[Thomas Petazzoni]

The DC233C Xtensa core is the one used by our Qemu Xtensa
defconfigs, and it is a little endian core. It makes sense to promote
it as a supported Xtensa core in arch/Config.in.xtensa, so that our
autobuilder testing covers build testing of little endian Xtensa
configurations. Indeed currently, both the FSF core and the custom
core are big endian (the custom core cannot be little endian without
an overlay).

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 arch/Config.in.xtensa | 6 +++++-
 arch/arch.mk.xtensa   | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/Config.in.xtensa b/arch/Config.in.xtensa
index 3745f84537..9aaf11203b 100644
--- a/arch/Config.in.xtensa
+++ b/arch/Config.in.xtensa
@@ -10,6 +10,9 @@ config BR2_xtensa_fsf
 	bool "fsf - Default configuration"
 	select BR2_USE_MMU
 
+config BR2_xtensa_dc233c
+	bool "dc233c"
+
 endchoice
 
 config BR2_XTENSA_OVERLAY_FILE
@@ -47,7 +50,7 @@ endchoice
 config BR2_XTENSA_USE_MMU
 	bool "MMU support"
 	default y
-	depends on BR2_XTENSA_CUSTOM
+	depends on BR2_XTENSA_CUSTOM || BR2_xtensa_dc233c
 	select BR2_USE_MMU
 	help
 	  Enable this option if your Xtensa core has a MMU (Memory
@@ -55,6 +58,7 @@ config BR2_XTENSA_USE_MMU
 
 config BR2_ENDIAN
 	default "LITTLE"	if BR2_XTENSA_LITTLE_ENDIAN
+	default "LITTLE"	if BR2_xtensa_dc233c
 	default "BIG"		if BR2_XTENSA_BIG_ENDIAN
 	default "BIG"		if BR2_xtensa_fsf
 
diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa
index fd410f6bfa..92499f396f 100644
--- a/arch/arch.mk.xtensa
+++ b/arch/arch.mk.xtensa
@@ -9,7 +9,13 @@
 #       tar xf $(ARCH_XTENSA_OVERLAY_FILE) -C $(@D) --strip-components=1 gcc
 #   endif
 ################################################################################
+
+ifeq ($(BR2_xtensa_dc233c),y)
+BR_ARCH_XTENSA_OVERLAY_FILE = https://github.com/jcmvbkbc/xtensa-toolchain-build/raw/95291b7c39e6f790d0b2f062c945a630290f2c81/overlays/xtensa_dc233c.tar.gz
+else ifeq ($(BR2_XTENSA_CUSTOM),y)
 BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE))
+endif
+
 ifneq ($(filter http://% https://% ftp://% scp://%,$(BR_ARCH_XTENSA_OVERLAY_FILE)),)
 ARCH_XTENSA_OVERLAY_URL = $(BR_ARCH_XTENSA_OVERLAY_FILE)
 ARCH_XTENSA_OVERLAY_FILE = $($(PKG)_DL_DIR)/$(notdir $(BR_ARCH_XTENSA_OVERLAY_FILE))
-- 
2.37.3

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 5/5] configs/qemu_xtensa_lx60*: use new BR2_xtensa_dc233c option

[Thomas Petazzoni]

Now that Xtensa DC233C is directly supported as an Xtensa core in
menuconfig, let's use the new option.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 configs/qemu_xtensa_lx60_defconfig       | 3 +--
 configs/qemu_xtensa_lx60_nommu_defconfig | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/configs/qemu_xtensa_lx60_defconfig b/configs/qemu_xtensa_lx60_defconfig
index cb78106091..845d466caf 100644
--- a/configs/qemu_xtensa_lx60_defconfig
+++ b/configs/qemu_xtensa_lx60_defconfig
@@ -1,7 +1,6 @@
 # Architecture
 BR2_xtensa=y
-BR2_XTENSA_CUSTOM=y
-BR2_XTENSA_OVERLAY_FILE="https://github.com/jcmvbkbc/xtensa-toolchain-build/raw/95291b7c39e6f790d0b2f062c945a630290f2c81/overlays/xtensa_dc233c.tar.gz"
+BR2_xtensa_dc233c=y
 
 # System
 BR2_SYSTEM_DHCP="eth0"
diff --git a/configs/qemu_xtensa_lx60_nommu_defconfig b/configs/qemu_xtensa_lx60_nommu_defconfig
index 4be1b66edc..f94019540f 100644
--- a/configs/qemu_xtensa_lx60_nommu_defconfig
+++ b/configs/qemu_xtensa_lx60_nommu_defconfig
@@ -1,7 +1,6 @@
 # Architecture
 BR2_xtensa=y
-BR2_XTENSA_CUSTOM=y
-BR2_XTENSA_OVERLAY_FILE="https://github.com/jcmvbkbc/xtensa-toolchain-build/raw/95291b7c39e6f790d0b2f062c945a630290f2c81/overlays/xtensa_dc233c.tar.gz"
+BR2_xtensa_dc233c=y
 # BR2_XTENSA_USE_MMU is not set
 
 # Use minimal busybox with hush and networking tools
-- 
2.37.3

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 2/5] Revert "arch/xtensa: custom configuration requires an overlay"

[Yann E. MORIN]

Thomas, All,

On 2022-09-25 11:09 +0200, Thomas Petazzoni spake thusly:
> This reverts commit 4cbf7336914f25478aea943456ba7dc3c892c21a.
> 
> This commit breaks the build of BR2_XTENSA_CUSTOM=y configurations
> that use an external toolchain. In such configurations, having an
> empty overlay is perfectly fine.

As 4cbf7336914f explained, an empty overlay is equivalent to using the
fsf variant, so in that case, there would be no point in asking for a
custom core to begin with, and Max Filipov seemed to agree:
    https://lore.kernel.org/buildroot/CAMo8BfKHKuVC86uOwP0z8CzE425q7u0B_q6jdeU0Rw9upaiY=Q@mail.gmail.com/

So, I think we are maybe taking the issue from the wrong side here. We
should forbid an empty overlay only when using our internal toolchain
for a custom xtensa core. I.e.:

    diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa
    index 7b6c59cecd..1799528a93 100644
    --- a/arch/arch.mk.xtensa
    +++ b/arch/arch.mk.xtensa
    @@ -1,8 +1,10 @@
     BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE))
     
    -ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:)
    +ifeq ($(BR_BUILDING)$(BR2_TOOLCHAIN_BUILDROOT),yy)
    +ifeq ($(BR_ARCH_XTENSA_OVERLAY_FILE),)
     $(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting)
     endif
    +endif
     
     ################################################################################
     # This variable can be used by packages that need to extract the
     # overlay.

Regards,
Yann E. MORIN.

> For example, this commit broke the
> following two runtime tests:
> 
>   tests.toolchain.test_external_bootlin.TestExternalToolchainBootlinXtensalx60UclibcBleedingEdge
>   tests.toolchain.test_external_bootlin.TestExternalToolchainBootlinXtensalx60UclibcStable
> 
> And in fact, having an empty overlay is even correct when building an
> internal toolchain. The endianness issue that
> 4cbf7336914f25478aea943456ba7dc3c892c21a was attempting to fix has
> been fixed in a better way by the previous commit.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  arch/arch.mk.xtensa | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa
> index 7b6c59cecd..fd410f6bfa 100644
> --- a/arch/arch.mk.xtensa
> +++ b/arch/arch.mk.xtensa
> @@ -1,9 +1,3 @@
> -BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE))
> -
> -ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:)
> -$(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting)
> -endif
> -
>  ################################################################################
>  # This variable can be used by packages that need to extract the overlay.
>  #
> @@ -15,6 +9,7 @@ endif
>  #       tar xf $(ARCH_XTENSA_OVERLAY_FILE) -C $(@D) --strip-components=1 gcc
>  #   endif
>  ################################################################################
> +BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE))
>  ifneq ($(filter http://% https://% ftp://% scp://%,$(BR_ARCH_XTENSA_OVERLAY_FILE)),)
>  ARCH_XTENSA_OVERLAY_URL = $(BR_ARCH_XTENSA_OVERLAY_FILE)
>  ARCH_XTENSA_OVERLAY_FILE = $($(PKG)_DL_DIR)/$(notdir $(BR_ARCH_XTENSA_OVERLAY_FILE))
> -- 
> 2.37.3
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/5] arch/Config.in.xtensa: adjust endianness logic to avoid bogus configurations

[Yann E. MORIN]

Thomas, All,

On 2022-09-25 11:09 +0200, Thomas Petazzoni 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 preventing the selection of
> BR2_XTENSA_BIG_ENDIAN when an internal toolchain is used with an empty
> overlay file.
> 
> This commit will allow reverting
> 4cbf7336914f25478aea943456ba7dc3c892c21a ("arch/xtensa: custom
> configuration requires an overlay") which broke the build for external
> toolchains that use a custom Xtensa core.
> 
> Fixes (even though currently hidden by commit
> 4cbf7336914f25478aea943456ba7dc3c892c21a):
> 
>   http://autobuild.buildroot.net/results/702e2886156f291466375dfcf412c20f1aa5857d/
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  arch/Config.in.xtensa | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/Config.in.xtensa b/arch/Config.in.xtensa
> index f9d5adb4c9..4bb58e1964 100644
> --- a/arch/Config.in.xtensa
> +++ b/arch/Config.in.xtensa
> @@ -37,6 +37,10 @@ config BR2_XTENSA_LITTLE_ENDIAN
>  
>  config BR2_XTENSA_BIG_ENDIAN
>  	bool "Big endian"
> +	# Internal toolchains without an overlay file are always
> +	# little-endian, so we prevent from selecting big endian in
> +	# this case.
> +	depends on BR2_XTENSA_OVERLAY_FILE != "" || BR2_TOOLCHAIN_EXTERNAL

I think that dependency should go to the whole choice instead:

    choice
        prompt "Target Architecture Endianness"
        default BR2_XTENSA_LITTLE_ENDIAN
        depends on BR2_XTENSA_CUSTOM || BR2_TOOLCHAIN_EXTERNAL

This should cover the external toolchain case, and the internal
toolchain for a custom core (assuming we keep the no-empty overlay for
custom cores).

I.e. semantically, choosing between big or little is not about having or
not having an overlay, but about being a custom core or not (or using a
custom toolchain).

That way, even for gdb and the kernel, with an external toolchain, we
still allow building the; with an empty overlay.

Regards,
Yann E. MORIN.

>  endchoice
>  
> -- 
> 2.37.3
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 2/5] Revert "arch/xtensa: custom configuration requires an overlay"

[Thomas Petazzoni]

Hello,

On Sun, 25 Sep 2022 12:03:55 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> As 4cbf7336914f explained, an empty overlay is equivalent to using the
> fsf variant, so in that case, there would be no point in asking for a
> custom core to begin with, and Max Filipov seemed to agree:
>     https://lore.kernel.org/buildroot/CAMo8BfKHKuVC86uOwP0z8CzE425q7u0B_q6jdeU0Rw9upaiY=Q@mail.gmail.com/
> 
> So, I think we are maybe taking the issue from the wrong side here. We
> should forbid an empty overlay only when using our internal toolchain
> for a custom xtensa core. I.e.:
> 
>     diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa
>     index 7b6c59cecd..1799528a93 100644
>     --- a/arch/arch.mk.xtensa
>     +++ b/arch/arch.mk.xtensa
>     @@ -1,8 +1,10 @@
>      BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE))
>      
>     -ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:)
>     +ifeq ($(BR_BUILDING)$(BR2_TOOLCHAIN_BUILDROOT),yy)
>     +ifeq ($(BR_ARCH_XTENSA_OVERLAY_FILE),)
>      $(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting)
>      endif
>     +endif

You're right, this seems to be a better approach. In the case of an
internal toolchain with custom core, it means the user still sees the
little/big endian choice, but if he hasn't specified an overlay, the
build will be aborted early on.

I guess I'm on for a v3!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/5] arch/Config.in.xtensa: adjust endianness logic to avoid bogus configurations

[Thomas Petazzoni via buildroot]

Hello,

Sorry to bring back this topic only now, but there are still Xtensa
issues that we need to resolve, and this series was aiming at solving
them. But I have further questions.

On Sun, 25 Sep 2022 12:10:41 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> > diff --git a/arch/Config.in.xtensa b/arch/Config.in.xtensa
> > index f9d5adb4c9..4bb58e1964 100644
> > --- a/arch/Config.in.xtensa
> > +++ b/arch/Config.in.xtensa
> > @@ -37,6 +37,10 @@ config BR2_XTENSA_LITTLE_ENDIAN
> >  
> >  config BR2_XTENSA_BIG_ENDIAN
> >  	bool "Big endian"
> > +	# Internal toolchains without an overlay file are always
> > +	# little-endian, so we prevent from selecting big endian in
> > +	# this case.
> > +	depends on BR2_XTENSA_OVERLAY_FILE != "" || BR2_TOOLCHAIN_EXTERNAL  
> 
> I think that dependency should go to the whole choice instead:
> 
>     choice
>         prompt "Target Architecture Endianness"
>         default BR2_XTENSA_LITTLE_ENDIAN
>         depends on BR2_XTENSA_CUSTOM || BR2_TOOLCHAIN_EXTERNAL

My memory is probably vague, but I don't see how this solve the
problem. The problem was to prevent the user from choosing "big endian"
if he has selected a custom core but not specified an overlay.

> This should cover the external toolchain case, and the internal
> toolchain for a custom core (assuming we keep the no-empty overlay for
> custom cores).

Not sure what you mean by "assuming we keep the no-empty overlay for
custom cores".

> 
> I.e. semantically, choosing between big or little is not about having or
> not having an overlay, but about being a custom core or not (or using a
> custom toolchain).

Not sure to follow you here. The only issue is precisely that if you
don't specify an overlay, there is no way your custom Xtensa core can
be big endian: it will be little endian. The configuration we want to
prevent the user from creating is: custom core, no overlay, big endian.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 2/5] Revert "arch/xtensa: custom configuration requires an overlay"

[Thomas Petazzoni via buildroot]

On Sun, 25 Sep 2022 12:13:02 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> > As 4cbf7336914f explained, an empty overlay is equivalent to using the
> > fsf variant, so in that case, there would be no point in asking for a
> > custom core to begin with, and Max Filipov seemed to agree:
> >     https://lore.kernel.org/buildroot/CAMo8BfKHKuVC86uOwP0z8CzE425q7u0B_q6jdeU0Rw9upaiY=Q@mail.gmail.com/
> > 
> > So, I think we are maybe taking the issue from the wrong side here. We
> > should forbid an empty overlay only when using our internal toolchain
> > for a custom xtensa core. I.e.:
> > 
> >     diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa
> >     index 7b6c59cecd..1799528a93 100644
> >     --- a/arch/arch.mk.xtensa
> >     +++ b/arch/arch.mk.xtensa
> >     @@ -1,8 +1,10 @@
> >      BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE))
> >      
> >     -ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:)
> >     +ifeq ($(BR_BUILDING)$(BR2_TOOLCHAIN_BUILDROOT),yy)
> >     +ifeq ($(BR_ARCH_XTENSA_OVERLAY_FILE),)
> >      $(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting)
> >      endif
> >     +endif  
> 
> You're right, this seems to be a better approach. In the case of an
> internal toolchain with custom core, it means the user still sees the
> little/big endian choice, but if he hasn't specified an overlay, the
> build will be aborted early on.

Now that I re-read this, I'm no longer sure to understand. If you have
a configuration BR2_TOOLCHAIN_BUILDROOT=y, BR2_xtensa_fsf=y, then
BR2_XTENSA_OVERLAY_FILE will be empty, but it's perfectly normal. But
with your proposed change, such a configuration would no longer build,
as we would now always require an overlay to be specified.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot