All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
@ 2018-08-31  9:11 Mark Corbin
  2018-09-02 19:40 ` Thomas Petazzoni
  2018-09-03 23:00 ` Arnout Vandecappelle
  0 siblings, 2 replies; 15+ messages in thread
From: Mark Corbin @ 2018-08-31  9:11 UTC (permalink / raw)
  To: buildroot

This enables a riscv64 system to be built with a Buildroot generated
toolchain (gcc >= 7.x, binutils >= 2.30, glibc only).

This configuration has been used to successfully build a qemu-bootable
riscv-linux-4.15 kernel (https://github.com/riscv/riscv-linux.git).

Signed-off-by: Mark Corbin <mark.corbin@embecosm.com>
---
 Makefile                                |   1 +
 arch/Config.in                          |  15 ++++
 arch/Config.in.riscv                    | 104 ++++++++++++++++++++++++
 arch/arch.mk.riscv                      |  24 ++++++
 package/binutils/Config.in.host         |   3 +
 toolchain/toolchain-buildroot/Config.in |  12 ++-
 6 files changed, 156 insertions(+), 3 deletions(-)
 create mode 100644 arch/Config.in.riscv
 create mode 100644 arch/arch.mk.riscv

diff --git a/Makefile b/Makefile
index 413ec921cd..2d67d5bc94 100644
--- a/Makefile
+++ b/Makefile
@@ -433,6 +433,7 @@ KERNEL_ARCH := $(shell echo "$(ARCH)" | sed -e "s/-.*//" \
 	-e s/parisc64/parisc/ \
 	-e s/powerpc64.*/powerpc/ \
 	-e s/ppc.*/powerpc/ -e s/mips.*/mips/ \
+	-e s/riscv.*/riscv/ \
 	-e s/sh.*/sh/ \
 	-e s/microblazeel/microblaze/)
 
diff --git a/arch/Config.in b/arch/Config.in
index 7d1aeb2174..44e250e480 100644
--- a/arch/Config.in
+++ b/arch/Config.in
@@ -198,6 +198,17 @@ config BR2_powerpc64le
 	  http://www.power.org/
 	  http://en.wikipedia.org/wiki/Powerpc
 
+config BR2_riscv64
+	bool "RISCV64"
+	select BR2_ARCH_IS_64
+	select BR2_ARCH_HAS_MMU_OPTIONAL
+	help
+	  RISC-V is an open, free Instruction Set Architecture created
+	  by the UC Berkeley Architecture Research group and supported
+	  and promoted by RISC-V Foundation.
+	  https://riscv.org/
+	  https://en.wikipedia.org/wiki/RISC-V
+
 config BR2_sh
 	bool "SuperH"
 	select BR2_ARCH_HAS_MMU_OPTIONAL
@@ -423,6 +434,10 @@ if BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le
 source "arch/Config.in.powerpc"
 endif
 
+if BR2_riscv64
+source "arch/Config.in.riscv"
+endif
+
 if BR2_sh
 source "arch/Config.in.sh"
 endif
diff --git a/arch/Config.in.riscv b/arch/Config.in.riscv
new file mode 100644
index 0000000000..0bee422581
--- /dev/null
+++ b/arch/Config.in.riscv
@@ -0,0 +1,104 @@
+# RISC-V CPU ISA extensions.
+
+config BR2_RISCV_ISA_RVI
+	bool
+	select BR2_ARCH_NEEDS_GCC_AT_LEAST_7
+
+config BR2_RISCV_ISA_RVM
+	bool
+
+config BR2_RISCV_ISA_RVA
+	bool
+
+config BR2_RISCV_ISA_RVF
+	bool
+
+config BR2_RISCV_ISA_RVD
+	bool
+
+config BR2_RISCV_ISA_RVC
+	bool
+
+
+choice
+	prompt "Target Architecture Variant"
+	default BR2_riscv_g
+
+config BR2_riscv_g
+	bool "General purpose (G)"
+	select BR2_RISCV_ISA_RVI
+	select BR2_RISCV_ISA_RVM
+	select BR2_RISCV_ISA_RVA
+	select BR2_RISCV_ISA_RVF
+	select BR2_RISCV_ISA_RVD
+	help
+	  General purpose (G) is equivalent to IMAFD.
+
+config BR2_riscv_custom
+	bool "Custom architecture"
+	select BR2_RISCV_ISA_RVI
+
+endchoice
+
+if BR2_riscv_custom
+
+comment "Instruction Set Extensions"
+
+config BR2_RISCV_ISA_CUSTOM_RVM
+	bool "Integer Multiplication and Division (M)"
+	select BR2_RISCV_ISA_RVM
+
+config BR2_RISCV_ISA_CUSTOM_RVA
+	bool "Atomic Instructions (A)"
+	select BR2_RISCV_ISA_RVA
+
+config BR2_RISCV_ISA_CUSTOM_RVF
+	bool "Single-precision Floating-point (F)"
+	select BR2_RISCV_ISA_RVF
+
+config BR2_RISCV_ISA_CUSTOM_RVD
+	bool "Double-precision Floating-point (D)"
+	depends on BR2_RISCV_ISA_RVF
+	select BR2_RISCV_ISA_RVD
+
+config BR2_RISCV_ISA_CUSTOM_RVC
+	bool "Compressed Instructions (C)"
+	select BR2_RISCV_ISA_RVC
+endif
+
+
+choice
+	prompt "Target ABI"
+	default BR2_RISCV_ABI_LP64
+
+config BR2_RISCV_ABI_LP64
+	bool "lp64"
+	depends on BR2_ARCH_IS_64
+
+config BR2_RISCV_ABI_LP64F
+	bool "lp64f"
+	depends on BR2_ARCH_IS_64 && BR2_RISCV_ISA_RVF
+
+config BR2_RISCV_ABI_LP64D
+	bool "lp64d"
+	depends on BR2_ARCH_IS_64 && BR2_RISCV_ISA_RVD
+endchoice
+
+config BR2_ARCH
+	default "riscv64"
+
+config BR2_ENDIAN
+	default "LITTLE"
+
+config BR2_GCC_TARGET_ARCH
+#	Instruction set extension characters are appended by arch/arch.mk.riscv
+	default "rv64i"
+
+config BR2_GCC_TARGET_ABI
+	default "lp64" if BR2_RISCV_ABI_LP64
+	default "lp64f" if BR2_RISCV_ABI_LP64F
+	default "lp64d" if BR2_RISCV_ABI_LP64D
+
+config BR2_READELF_ARCH_NAME
+	default "RISC-V"
+
diff --git a/arch/arch.mk.riscv b/arch/arch.mk.riscv
new file mode 100644
index 0000000000..ad2e26903b
--- /dev/null
+++ b/arch/arch.mk.riscv
@@ -0,0 +1,24 @@
+#
+# Append the appropriate RISC-V ISA extensions to the
+# BR2_GCC_TARGET_ARCH variable.
+#
+
+BR2_RISCV_GCC_ARCH = $(call qstrip,$(BR2_GCC_TARGET_ARCH))
+
+ifeq ($(BR2_RISCV_ISA_RVM),y)
+BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)m
+endif
+ifeq ($(BR2_RISCV_ISA_RVA),y)
+BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)a
+endif
+ifeq ($(BR2_RISCV_ISA_RVF),y)
+BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)f
+endif
+ifeq ($(BR2_RISCV_ISA_RVD),y)
+BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)d
+endif
+ifeq ($(BR2_RISCV_ISA_RVC),y)
+BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)c
+endif
+
+BR2_GCC_TARGET_ARCH := $(patsubst %,"%",$(BR2_RISCV_GCC_ARCH))
diff --git a/package/binutils/Config.in.host b/package/binutils/Config.in.host
index 21dc84e498..747bac57ed 100644
--- a/package/binutils/Config.in.host
+++ b/package/binutils/Config.in.host
@@ -6,15 +6,18 @@ choice
 	default BR2_BINUTILS_VERSION_2_28_X if BR2_ARM_INSTRUCTIONS_THUMB2
 	default BR2_BINUTILS_VERSION_2_29_X if !BR2_arc
 	default BR2_BINUTILS_VERSION_ARC if BR2_arc
+	default BR2_BINUTILS_VERSION_2_30_X if BR2_riscv64
 	help
 	  Select the version of binutils you wish to use.
 
 config BR2_BINUTILS_VERSION_2_28_X
 	bool "binutils 2.28.1"
 	depends on !BR2_arc
+	depends on !BR2_riscv64
 
 config BR2_BINUTILS_VERSION_2_29_X
 	bool "binutils 2.29.1"
+	depends on !BR2_riscv64
 
 config BR2_BINUTILS_VERSION_2_30_X
 	bool "binutils 2.30"
diff --git a/toolchain/toolchain-buildroot/Config.in b/toolchain/toolchain-buildroot/Config.in
index 75e8191f46..6798448bda 100644
--- a/toolchain/toolchain-buildroot/Config.in
+++ b/toolchain/toolchain-buildroot/Config.in
@@ -23,7 +23,7 @@ config BR2_TOOLCHAIN_BUILDROOT_VENDOR
 choice
 	prompt "C library"
 	default BR2_TOOLCHAIN_BUILDROOT_UCLIBC
-	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64
+	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64 || BR2_riscv64
 
 config BR2_TOOLCHAIN_BUILDROOT_UCLIBC
 	bool "uClibc-ng"
@@ -46,14 +46,16 @@ config BR2_TOOLCHAIN_BUILDROOT_GLIBC
 		   BR2_aarch64_be  || BR2_i386       || BR2_mips    || \
 		   BR2_mipsel      || BR2_mips64     || BR2_mips64el|| \
 		   BR2_powerpc     || BR2_powerpc64  || BR2_powerpc64le || \
-		   BR2_sh          || BR2_sparc64    || BR2_x86_64 || \
-		   BR2_microblaze  || BR2_nios2      || BR2_archs38
+		   BR2_riscv64     || BR2_sh         || BR2_sparc64     || \
+		   BR2_x86_64      || BR2_microblaze || BR2_nios2       || \
+		   BR2_archs38
 	depends on BR2_USE_MMU
 	depends on !BR2_STATIC_LIBS
 	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_2
 	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_10 || !BR2_powerpc64le
 	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5 || !BR2_MIPS_NAN_2008
 	depends on !BR2_powerpc_SPE
+	depends on BR2_RISCV_ISA_RVA || !BR2_riscv64
 	select BR2_TOOLCHAIN_USES_GLIBC
 	# our glibc.mk enables RPC support
 	select BR2_TOOLCHAIN_HAS_NATIVE_RPC
@@ -77,6 +79,10 @@ comment "glibc on MIPS w/ NAN2008 needs a toolchain w/ headers >= 4.5"
 	depends on BR2_MIPS_NAN_2008
 	depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5
 
+comment "glibc needs ISA extension A on riscv64"
+	depends on BR2_riscv64
+	depends on !BR2_RISCV_ISA_RVA
+
 config BR2_TOOLCHAIN_BUILDROOT_MUSL
 	bool "musl"
 	depends on BR2_aarch64	   || BR2_arm   || BR2_armeb   || BR2_i386 || \
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-08-31  9:11 [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture Mark Corbin
@ 2018-09-02 19:40 ` Thomas Petazzoni
  2018-09-03 11:26   ` Mark Corbin
  2018-09-04 10:03   ` Mark Corbin
  2018-09-03 23:00 ` Arnout Vandecappelle
  1 sibling, 2 replies; 15+ messages in thread
From: Thomas Petazzoni @ 2018-09-02 19:40 UTC (permalink / raw)
  To: buildroot

Hello Mark,

On Fri, 31 Aug 2018 10:11:54 +0100, Mark Corbin wrote:
> This enables a riscv64 system to be built with a Buildroot generated
> toolchain (gcc >= 7.x, binutils >= 2.30, glibc only).
> 
> This configuration has been used to successfully build a qemu-bootable
> riscv-linux-4.15 kernel (https://github.com/riscv/riscv-linux.git).
> 
> Signed-off-by: Mark Corbin <mark.corbin@embecosm.com>

Thanks for this work on adding RISC-V support in Buildroot! As I
already said on another patch, it would really, really be helpful if
you sent a complete patch series instead of individual patches. Your
patch series should include:

 - The preliminary patches to support specifying a custom version of
   the kernel headers
 - This patch adding the RISC-V support
 - The patch adding the qemu defconfig

This way, the dependencies between the patches are clearly visible.

Could you add an entry in the DEVELOPERS file as well, listing all the
files you're adding under your name/e-mail?

> +config BR2_riscv64
> +	bool "RISCV64"
> +	select BR2_ARCH_IS_64
> +	select BR2_ARCH_HAS_MMU_OPTIONAL

Does your patch support the situation where the MMU is not used? I.e,
does the RISC-V toolchain support noMMU RISC-V ?

> diff --git a/arch/Config.in.riscv b/arch/Config.in.riscv
> new file mode 100644
> index 0000000000..0bee422581
> --- /dev/null
> +++ b/arch/Config.in.riscv
> @@ -0,0 +1,104 @@
> +# RISC-V CPU ISA extensions.
> +
> +config BR2_RISCV_ISA_RVI
> +	bool
> +	select BR2_ARCH_NEEDS_GCC_AT_LEAST_7

Why is this select here instead of on the BR2_riscv64 option, since
anyway there is not support at all for riscv64 before gcc 7.x ?


> +choice
> +	prompt "Target ABI"
> +	default BR2_RISCV_ABI_LP64
> +
> +config BR2_RISCV_ABI_LP64
> +	bool "lp64"
> +	depends on BR2_ARCH_IS_64
> +
> +config BR2_RISCV_ABI_LP64F
> +	bool "lp64f"
> +	depends on BR2_ARCH_IS_64 && BR2_RISCV_ISA_RVF
> +
> +config BR2_RISCV_ABI_LP64D
> +	bool "lp64d"
> +	depends on BR2_ARCH_IS_64 && BR2_RISCV_ISA_RVD

The BR2_ARCH_IS_64 conditions don't seem to be very useful, since only
riscv64 is supported for now... and it is always 64-bit.

> +endchoice
> +
> +config BR2_ARCH
> +	default "riscv64"
> +
> +config BR2_ENDIAN
> +	default "LITTLE"
> +
> +config BR2_GCC_TARGET_ARCH
> +#	Instruction set extension characters are appended by arch/arch.mk.riscv
> +	default "rv64i"
> +
> +config BR2_GCC_TARGET_ABI
> +	default "lp64" if BR2_RISCV_ABI_LP64
> +	default "lp64f" if BR2_RISCV_ABI_LP64F
> +	default "lp64d" if BR2_RISCV_ABI_LP64D
> +
> +config BR2_READELF_ARCH_NAME
> +	default "RISC-V"
> +
> diff --git a/arch/arch.mk.riscv b/arch/arch.mk.riscv
> new file mode 100644
> index 0000000000..ad2e26903b
> --- /dev/null
> +++ b/arch/arch.mk.riscv
> @@ -0,0 +1,24 @@
> +#
> +# Append the appropriate RISC-V ISA extensions to the
> +# BR2_GCC_TARGET_ARCH variable.
> +#
> +
> +BR2_RISCV_GCC_ARCH = $(call qstrip,$(BR2_GCC_TARGET_ARCH))
> +
> +ifeq ($(BR2_RISCV_ISA_RVM),y)
> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)m
> +endif
> +ifeq ($(BR2_RISCV_ISA_RVA),y)
> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)a
> +endif
> +ifeq ($(BR2_RISCV_ISA_RVF),y)
> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)f
> +endif
> +ifeq ($(BR2_RISCV_ISA_RVD),y)
> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)d
> +endif
> +ifeq ($(BR2_RISCV_ISA_RVC),y)
> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)c
> +endif
> +
> +BR2_GCC_TARGET_ARCH := $(patsubst %,"%",$(BR2_RISCV_GCC_ARCH))

So, this is probably the part I'm the least happy with. I think it is
the only occurrence where make logic would tweak BR2_* variables. I'm
not sure I want to set a precedent for this, as it could become
"strange" to see values of BR2_* options that don't match what is
visible in the .config file.

So, I think this will require a bit more rework. The solution I would
propose is to introduce arch/arch.mk, which does:

GCC_TARGET_ARCH = $(call qstrip,$(BR2_GCC_TARGET_ARCH))
GCC_TARGET_ABI  = $(call qstrip,$(BR2_GCC_TARGET_ABI))
GCC_TARGET_NAN  = $(call qstrip,$(BR2_GCC_TARGET_NAN))
# etc. for all BR2_GCC_TARGET_* options listed in arch/Config.in

-include $(sort $(wildcard arch/arch.mk.*))

Then, in the main Makefile, you include arch/arch.mk instead of
wildcard arch/arch.mk.*.

Then, in arch/arch.mk.riscv, you can override GCC_TARGET_ARCH and al.

Of course, every place where BR2_GCC_TARGET_ARCH was used needs to be
changed to use GCC_TARGET_ARCH.

Let me know if this proposal is clear or not. If it's not, I can cook
up a quick patch to show you the direction.

> diff --git a/package/binutils/Config.in.host b/package/binutils/Config.in.host
> index 21dc84e498..747bac57ed 100644
> --- a/package/binutils/Config.in.host
> +++ b/package/binutils/Config.in.host
> @@ -6,15 +6,18 @@ choice
>  	default BR2_BINUTILS_VERSION_2_28_X if BR2_ARM_INSTRUCTIONS_THUMB2
>  	default BR2_BINUTILS_VERSION_2_29_X if !BR2_arc
>  	default BR2_BINUTILS_VERSION_ARC if BR2_arc
> +	default BR2_BINUTILS_VERSION_2_30_X if BR2_riscv64

This is not really needed, since the first selectable entry is the one
chosen by default.

>  	help
>  	  Select the version of binutils you wish to use.
>  
>  config BR2_BINUTILS_VERSION_2_28_X
>  	bool "binutils 2.28.1"
>  	depends on !BR2_arc
> +	depends on !BR2_riscv64
>  
>  config BR2_BINUTILS_VERSION_2_29_X
>  	bool "binutils 2.29.1"
> +	depends on !BR2_riscv64
>  
>  config BR2_BINUTILS_VERSION_2_30_X
>  	bool "binutils 2.30"
> diff --git a/toolchain/toolchain-buildroot/Config.in b/toolchain/toolchain-buildroot/Config.in
> index 75e8191f46..6798448bda 100644
> --- a/toolchain/toolchain-buildroot/Config.in
> +++ b/toolchain/toolchain-buildroot/Config.in
> @@ -23,7 +23,7 @@ config BR2_TOOLCHAIN_BUILDROOT_VENDOR
>  choice
>  	prompt "C library"
>  	default BR2_TOOLCHAIN_BUILDROOT_UCLIBC
> -	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64
> +	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64 || BR2_riscv64

Ditto: since only glibc is supported on riscv64, it will automatically
be the default selection.

>  
>  config BR2_TOOLCHAIN_BUILDROOT_UCLIBC
>  	bool "uClibc-ng"
> @@ -46,14 +46,16 @@ config BR2_TOOLCHAIN_BUILDROOT_GLIBC
>  		   BR2_aarch64_be  || BR2_i386       || BR2_mips    || \
>  		   BR2_mipsel      || BR2_mips64     || BR2_mips64el|| \
>  		   BR2_powerpc     || BR2_powerpc64  || BR2_powerpc64le || \
> -		   BR2_sh          || BR2_sparc64    || BR2_x86_64 || \
> -		   BR2_microblaze  || BR2_nios2      || BR2_archs38
> +		   BR2_riscv64     || BR2_sh         || BR2_sparc64     || \
> +		   BR2_x86_64      || BR2_microblaze || BR2_nios2       || \
> +		   BR2_archs38
>  	depends on BR2_USE_MMU
>  	depends on !BR2_STATIC_LIBS
>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_2
>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_10 || !BR2_powerpc64le
>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5 || !BR2_MIPS_NAN_2008
>  	depends on !BR2_powerpc_SPE
> +	depends on BR2_RISCV_ISA_RVA || !BR2_riscv64
>  	select BR2_TOOLCHAIN_USES_GLIBC
>  	# our glibc.mk enables RPC support
>  	select BR2_TOOLCHAIN_HAS_NATIVE_RPC
> @@ -77,6 +79,10 @@ comment "glibc on MIPS w/ NAN2008 needs a toolchain w/ headers >= 4.5"
>  	depends on BR2_MIPS_NAN_2008
>  	depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5
>  
> +comment "glibc needs ISA extension A on riscv64"
> +	depends on BR2_riscv64
> +	depends on !BR2_RISCV_ISA_RVA

So, if the user selects a "Custom architecture", and then doesn't
enable BR2_RISCV_ISA_CUSTOM_RVA, he won't have any C library available
to build a toolchain for riscv64.

Provided this, I think we should not allow enabling/disabling RVA: we
should assume it's always enabled.

Could you fix those comments, and resubmit a new complete patch series,
with all patches you need for RISC-V 64 support ?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-02 19:40 ` Thomas Petazzoni
@ 2018-09-03 11:26   ` Mark Corbin
  2018-09-03 22:20     ` Arnout Vandecappelle
  2018-09-04 10:03   ` Mark Corbin
  1 sibling, 1 reply; 15+ messages in thread
From: Mark Corbin @ 2018-09-03 11:26 UTC (permalink / raw)
  To: buildroot

Hello Thomas

On 02/09/18 20:40, Thomas Petazzoni wrote:
> Hello Mark,
>
> On Fri, 31 Aug 2018 10:11:54 +0100, Mark Corbin wrote:
>> This enables a riscv64 system to be built with a Buildroot generated
>> toolchain (gcc >= 7.x, binutils >= 2.30, glibc only).
>>
>> This configuration has been used to successfully build a qemu-bootable
>> riscv-linux-4.15 kernel (https://github.com/riscv/riscv-linux.git).
>>
>> Signed-off-by: Mark Corbin <mark.corbin@embecosm.com>
> Thanks for this work on adding RISC-V support in Buildroot! As I
> already said on another patch, it would really, really be helpful if
> you sent a complete patch series instead of individual patches. Your
> patch series should include:
>
>  - The preliminary patches to support specifying a custom version of
>    the kernel headers
>  - This patch adding the RISC-V support
>  - The patch adding the qemu defconfig
>
> This way, the dependencies between the patches are clearly visible.
No problem.
>
> Could you add an entry in the DEVELOPERS file as well, listing all the
> files you're adding under your name/e-mail?
Ok.
>
>> +config BR2_riscv64
>> +	bool "RISCV64"
>> +	select BR2_ARCH_IS_64
>> +	select BR2_ARCH_HAS_MMU_OPTIONAL
> Does your patch support the situation where the MMU is not used? I.e,
> does the RISC-V toolchain support noMMU RISC-V ?
I think that changing this to mandatory makes sense for now.
>
>> diff --git a/arch/Config.in.riscv b/arch/Config.in.riscv
>> new file mode 100644
>> index 0000000000..0bee422581
>> --- /dev/null
>> +++ b/arch/Config.in.riscv
>> @@ -0,0 +1,104 @@
>> +# RISC-V CPU ISA extensions.
>> +
>> +config BR2_RISCV_ISA_RVI
>> +	bool
>> +	select BR2_ARCH_NEEDS_GCC_AT_LEAST_7
> Why is this select here instead of on the BR2_riscv64 option, since
> anyway there is not support at all for riscv64 before gcc 7.x ?
I think that I looked at examples from Config.in.mips. I will move this
to the BR2_riscv64 option as all riscv64 targets requires gcc >= 7.x
>
>> +choice
>> +	prompt "Target ABI"
>> +	default BR2_RISCV_ABI_LP64
>> +
>> +config BR2_RISCV_ABI_LP64
>> +	bool "lp64"
>> +	depends on BR2_ARCH_IS_64
>> +
>> +config BR2_RISCV_ABI_LP64F
>> +	bool "lp64f"
>> +	depends on BR2_ARCH_IS_64 && BR2_RISCV_ISA_RVF
>> +
>> +config BR2_RISCV_ABI_LP64D
>> +	bool "lp64d"
>> +	depends on BR2_ARCH_IS_64 && BR2_RISCV_ISA_RVD
> The BR2_ARCH_IS_64 conditions don't seem to be very useful, since only
> riscv64 is supported for now... and it is always 64-bit.
Originally I had added provision for 32-bit support, but then removed it
as I had no way to test it. I'll remove these dependencies.
>
>> +endchoice
>> +
>> +config BR2_ARCH
>> +	default "riscv64"
>> +
>> +config BR2_ENDIAN
>> +	default "LITTLE"
>> +
>> +config BR2_GCC_TARGET_ARCH
>> +#	Instruction set extension characters are appended by arch/arch.mk.riscv
>> +	default "rv64i"
>> +
>> +config BR2_GCC_TARGET_ABI
>> +	default "lp64" if BR2_RISCV_ABI_LP64
>> +	default "lp64f" if BR2_RISCV_ABI_LP64F
>> +	default "lp64d" if BR2_RISCV_ABI_LP64D
>> +
>> +config BR2_READELF_ARCH_NAME
>> +	default "RISC-V"
>> +
>> diff --git a/arch/arch.mk.riscv b/arch/arch.mk.riscv
>> new file mode 100644
>> index 0000000000..ad2e26903b
>> --- /dev/null
>> +++ b/arch/arch.mk.riscv
>> @@ -0,0 +1,24 @@
>> +#
>> +# Append the appropriate RISC-V ISA extensions to the
>> +# BR2_GCC_TARGET_ARCH variable.
>> +#
>> +
>> +BR2_RISCV_GCC_ARCH = $(call qstrip,$(BR2_GCC_TARGET_ARCH))
>> +
>> +ifeq ($(BR2_RISCV_ISA_RVM),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)m
>> +endif
>> +ifeq ($(BR2_RISCV_ISA_RVA),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)a
>> +endif
>> +ifeq ($(BR2_RISCV_ISA_RVF),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)f
>> +endif
>> +ifeq ($(BR2_RISCV_ISA_RVD),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)d
>> +endif
>> +ifeq ($(BR2_RISCV_ISA_RVC),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)c
>> +endif
>> +
>> +BR2_GCC_TARGET_ARCH := $(patsubst %,"%",$(BR2_RISCV_GCC_ARCH))
> So, this is probably the part I'm the least happy with. I think it is
> the only occurrence where make logic would tweak BR2_* variables. I'm
> not sure I want to set a precedent for this, as it could become
> "strange" to see values of BR2_* options that don't match what is
> visible in the .config file.
>
> So, I think this will require a bit more rework. The solution I would
> propose is to introduce arch/arch.mk, which does:
>
> GCC_TARGET_ARCH = $(call qstrip,$(BR2_GCC_TARGET_ARCH))
> GCC_TARGET_ABI  = $(call qstrip,$(BR2_GCC_TARGET_ABI))
> GCC_TARGET_NAN  = $(call qstrip,$(BR2_GCC_TARGET_NAN))
> # etc. for all BR2_GCC_TARGET_* options listed in arch/Config.in
>
> -include $(sort $(wildcard arch/arch.mk.*))
>
> Then, in the main Makefile, you include arch/arch.mk instead of
> wildcard arch/arch.mk.*.
>
> Then, in arch/arch.mk.riscv, you can override GCC_TARGET_ARCH and al.
>
> Of course, every place where BR2_GCC_TARGET_ARCH was used needs to be
> changed to use GCC_TARGET_ARCH.
>
> Let me know if this proposal is clear or not. If it's not, I can cook
> up a quick patch to show you the direction.
I think that I understand what you are suggesting. I have a couple of
questions...

a) I guess that I will have to replace every existing BR2_GCC_TARGET_...
in the Buildroot configurations with the equivalent GCC_TARGET_... ?
This would cover a few packages and the external toolchain.

b) Would it be reasonable to add quotes back to the various GCC_TARGET_
variables once the arch.mk.* files have been included? It seems to me
that the existing use of the BR2_GCC_TARGET_ variables expects them to
be quoted.

>> diff --git a/package/binutils/Config.in.host b/package/binutils/Config.in.host
>> index 21dc84e498..747bac57ed 100644
>> --- a/package/binutils/Config.in.host
>> +++ b/package/binutils/Config.in.host
>> @@ -6,15 +6,18 @@ choice
>>  	default BR2_BINUTILS_VERSION_2_28_X if BR2_ARM_INSTRUCTIONS_THUMB2
>>  	default BR2_BINUTILS_VERSION_2_29_X if !BR2_arc
>>  	default BR2_BINUTILS_VERSION_ARC if BR2_arc
>> +	default BR2_BINUTILS_VERSION_2_30_X if BR2_riscv64
> This is not really needed, since the first selectable entry is the one
> chosen by default.
I'll remove this.
>>  	help
>>  	  Select the version of binutils you wish to use.
>>  
>>  config BR2_BINUTILS_VERSION_2_28_X
>>  	bool "binutils 2.28.1"
>>  	depends on !BR2_arc
>> +	depends on !BR2_riscv64
>>  
>>  config BR2_BINUTILS_VERSION_2_29_X
>>  	bool "binutils 2.29.1"
>> +	depends on !BR2_riscv64
>>  
>>  config BR2_BINUTILS_VERSION_2_30_X
>>  	bool "binutils 2.30"
>> diff --git a/toolchain/toolchain-buildroot/Config.in b/toolchain/toolchain-buildroot/Config.in
>> index 75e8191f46..6798448bda 100644
>> --- a/toolchain/toolchain-buildroot/Config.in
>> +++ b/toolchain/toolchain-buildroot/Config.in
>> @@ -23,7 +23,7 @@ config BR2_TOOLCHAIN_BUILDROOT_VENDOR
>>  choice
>>  	prompt "C library"
>>  	default BR2_TOOLCHAIN_BUILDROOT_UCLIBC
>> -	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64
>> +	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64 || BR2_riscv64
> Ditto: since only glibc is supported on riscv64, it will automatically
> be the default selection.
I'll remove this too.
>>  
>>  config BR2_TOOLCHAIN_BUILDROOT_UCLIBC
>>  	bool "uClibc-ng"
>> @@ -46,14 +46,16 @@ config BR2_TOOLCHAIN_BUILDROOT_GLIBC
>>  		   BR2_aarch64_be  || BR2_i386       || BR2_mips    || \
>>  		   BR2_mipsel      || BR2_mips64     || BR2_mips64el|| \
>>  		   BR2_powerpc     || BR2_powerpc64  || BR2_powerpc64le || \
>> -		   BR2_sh          || BR2_sparc64    || BR2_x86_64 || \
>> -		   BR2_microblaze  || BR2_nios2      || BR2_archs38
>> +		   BR2_riscv64     || BR2_sh         || BR2_sparc64     || \
>> +		   BR2_x86_64      || BR2_microblaze || BR2_nios2       || \
>> +		   BR2_archs38
>>  	depends on BR2_USE_MMU
>>  	depends on !BR2_STATIC_LIBS
>>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_2
>>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_10 || !BR2_powerpc64le
>>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5 || !BR2_MIPS_NAN_2008
>>  	depends on !BR2_powerpc_SPE
>> +	depends on BR2_RISCV_ISA_RVA || !BR2_riscv64
>>  	select BR2_TOOLCHAIN_USES_GLIBC
>>  	# our glibc.mk enables RPC support
>>  	select BR2_TOOLCHAIN_HAS_NATIVE_RPC
>> @@ -77,6 +79,10 @@ comment "glibc on MIPS w/ NAN2008 needs a toolchain w/ headers >= 4.5"
>>  	depends on BR2_MIPS_NAN_2008
>>  	depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5
>>  
>> +comment "glibc needs ISA extension A on riscv64"
>> +	depends on BR2_riscv64
>> +	depends on !BR2_RISCV_ISA_RVA
> So, if the user selects a "Custom architecture", and then doesn't
> enable BR2_RISCV_ISA_CUSTOM_RVA, he won't have any C library available
> to build a toolchain for riscv64.
>
> Provided this, I think we should not allow enabling/disabling RVA: we
> should assume it's always enabled.
I will force the selection of RVA for now, but will still show it in the
architecture options. It is possible that it could be come optional at
some point in the future.
>
> Could you fix those comments, and resubmit a new complete patch series,
> with all patches you need for RISC-V 64 support ?
I will. Thanks for the feedback.

Mark
>
> Thanks a lot!
>
> Thomas

-- 
Mark Corbin
Embecosm Ltd.
https://www.embecosm.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-03 11:26   ` Mark Corbin
@ 2018-09-03 22:20     ` Arnout Vandecappelle
  2018-09-04  7:49       ` Thomas Petazzoni
  0 siblings, 1 reply; 15+ messages in thread
From: Arnout Vandecappelle @ 2018-09-03 22:20 UTC (permalink / raw)
  To: buildroot

 Hi Mark,

 I have a few additional comments, I'll include them together with some
reactions to Thomas's statements.

On 03/09/2018 13:26, Mark Corbin wrote:
> Hello Thomas
> 
> On 02/09/18 20:40, Thomas Petazzoni wrote:
>> Hello Mark,
>>
>> On Fri, 31 Aug 2018 10:11:54 +0100, Mark Corbin wrote:
>>> This enables a riscv64 system to be built with a Buildroot generated
>>> toolchain (gcc >= 7.x, binutils >= 2.30, glibc only).
>>>
>>> This configuration has been used to successfully build a qemu-bootable
>>> riscv-linux-4.15 kernel (https://github.com/riscv/riscv-linux.git).
>>>
>>> Signed-off-by: Mark Corbin <mark.corbin@embecosm.com>
>> Thanks for this work on adding RISC-V support in Buildroot! As I
>> already said on another patch, it would really, really be helpful if
>> you sent a complete patch series instead of individual patches. Your
>> patch series should include:
>>
>>  - The preliminary patches to support specifying a custom version of
>>    the kernel headers
>>  - This patch adding the RISC-V support
>>  - The patch adding the qemu defconfig
>>
>> This way, the dependencies between the patches are clearly visible.
> No problem.
>>
>> Could you add an entry in the DEVELOPERS file as well, listing all the
>> files you're adding under your name/e-mail?
> Ok.
>>
>>> +config BR2_riscv64

 Since the kernel uses "riscv" for both the 32- and 64-bit version, I'd prefer
if we'd only have a single "riscv" architecture in Buildroot that will
eventually cover the 32-bit version as well.

 So I'd call it "riscv" instead of "riscv64" everywhere. For now, you could add
a blind (promptless) BR2_RISCV_64 option, which later can become part of a
choice between 32-bit and 64-bit.

 However, this is unprecedented in Buildroot (right now all 64-bit architectures
have a separate symbol) so the other developers may disagree. But for me,
splitting all those architectures was a mistake (except for some, like arm64,
which really are a different ISA).

>>> +	bool "RISCV64"
>>> +	select BR2_ARCH_IS_64
>>> +	select BR2_ARCH_HAS_MMU_OPTIONAL
>> Does your patch support the situation where the MMU is not used? I.e,
>> does the RISC-V toolchain support noMMU RISC-V ?
> I think that changing this to mandatory makes sense for now.
>>
>>> diff --git a/arch/Config.in.riscv b/arch/Config.in.riscv
>>> new file mode 100644
>>> index 0000000000..0bee422581
>>> --- /dev/null
>>> +++ b/arch/Config.in.riscv
>>> @@ -0,0 +1,104 @@
>>> +# RISC-V CPU ISA extensions.
>>> +
>>> +config BR2_RISCV_ISA_RVI
>>> +	bool
>>> +	select BR2_ARCH_NEEDS_GCC_AT_LEAST_7
>> Why is this select here instead of on the BR2_riscv64 option, since
>> anyway there is not support at all for riscv64 before gcc 7.x ?
> I think that I looked at examples from Config.in.mips. I will move this
> to the BR2_riscv64 option as all riscv64 targets requires gcc >= 7.x
>>
>>> +choice
>>> +	prompt "Target ABI"
>>> +	default BR2_RISCV_ABI_LP64
>>> +
>>> +config BR2_RISCV_ABI_LP64
>>> +	bool "lp64"
>>> +	depends on BR2_ARCH_IS_64
>>> +
>>> +config BR2_RISCV_ABI_LP64F
>>> +	bool "lp64f"
>>> +	depends on BR2_ARCH_IS_64 && BR2_RISCV_ISA_RVF
>>> +
>>> +config BR2_RISCV_ABI_LP64D
>>> +	bool "lp64d"
>>> +	depends on BR2_ARCH_IS_64 && BR2_RISCV_ISA_RVD
>> The BR2_ARCH_IS_64 conditions don't seem to be very useful, since only
>> riscv64 is supported for now... and it is always 64-bit.
> Originally I had added provision for 32-bit support, but then removed it
> as I had no way to test it. I'll remove these dependencies.

 So, if it becomes "riscv" instead of "riscv64", then it does make sense to have
the _IS_64 conditions.

>>
>>> +endchoice
>>> +
>>> +config BR2_ARCH
>>> +	default "riscv64"
>>> +
>>> +config BR2_ENDIAN
>>> +	default "LITTLE"
>>> +
>>> +config BR2_GCC_TARGET_ARCH
>>> +#	Instruction set extension characters are appended by arch/arch.mk.riscv
>>> +	default "rv64i"

 I would construct the entire string in make, and leave the symbol undefined in
.config.

>>> +
>>> +config BR2_GCC_TARGET_ABI
>>> +	default "lp64" if BR2_RISCV_ABI_LP64
>>> +	default "lp64f" if BR2_RISCV_ABI_LP64F
>>> +	default "lp64d" if BR2_RISCV_ABI_LP64D
>>> +
>>> +config BR2_READELF_ARCH_NAME
>>> +	default "RISC-V"
>>> +
>>> diff --git a/arch/arch.mk.riscv b/arch/arch.mk.riscv
>>> new file mode 100644
>>> index 0000000000..ad2e26903b
>>> --- /dev/null
>>> +++ b/arch/arch.mk.riscv
>>> @@ -0,0 +1,24 @@
>>> +#
>>> +# Append the appropriate RISC-V ISA extensions to the
>>> +# BR2_GCC_TARGET_ARCH variable.
>>> +#
>>> +
>>> +BR2_RISCV_GCC_ARCH = $(call qstrip,$(BR2_GCC_TARGET_ARCH))

 It's good to have an additional variable here, but:

1. remove the BR2_ part - that's only for .config symbols (and user-visible
environment variables);

2. since you use := below, use := here as well.

>>> +
>>> +ifeq ($(BR2_RISCV_ISA_RVM),y)
>>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)m
>>> +endif
>>> +ifeq ($(BR2_RISCV_ISA_RVA),y)
>>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)a
>>> +endif
>>> +ifeq ($(BR2_RISCV_ISA_RVF),y)
>>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)f
>>> +endif
>>> +ifeq ($(BR2_RISCV_ISA_RVD),y)
>>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)d
>>> +endif
>>> +ifeq ($(BR2_RISCV_ISA_RVC),y)
>>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)c
>>> +endif
>>> +
>>> +BR2_GCC_TARGET_ARCH := $(patsubst %,"%",$(BR2_RISCV_GCC_ARCH))
>> So, this is probably the part I'm the least happy with. I think it is
>> the only occurrence where make logic would tweak BR2_* variables. I'm
>> not sure I want to set a precedent for this, as it could become
>> "strange" to see values of BR2_* options that don't match what is
>> visible in the .config file.
>>
>> So, I think this will require a bit more rework. The solution I would
>> propose is to introduce arch/arch.mk, which does:
>>
>> GCC_TARGET_ARCH = $(call qstrip,$(BR2_GCC_TARGET_ARCH))
>> GCC_TARGET_ABI  = $(call qstrip,$(BR2_GCC_TARGET_ABI))
>> GCC_TARGET_NAN  = $(call qstrip,$(BR2_GCC_TARGET_NAN))
>> # etc. for all BR2_GCC_TARGET_* options listed in arch/Config.in

 I like this proposal!

>>
>> -include $(sort $(wildcard arch/arch.mk.*))
>>
>> Then, in the main Makefile, you include arch/arch.mk instead of
>> wildcard arch/arch.mk.*.
>>
>> Then, in arch/arch.mk.riscv, you can override GCC_TARGET_ARCH and al.
>>
>> Of course, every place where BR2_GCC_TARGET_ARCH was used needs to be
>> changed to use GCC_TARGET_ARCH.
>>
>> Let me know if this proposal is clear or not. If it's not, I can cook
>> up a quick patch to show you the direction.
> I think that I understand what you are suggesting. I have a couple of
> questions...
> 
> a) I guess that I will have to replace every existing BR2_GCC_TARGET_...
> in the Buildroot configurations with the equivalent GCC_TARGET_... ?
> This would cover a few packages and the external toolchain.

 Yes (as Thomas wrote a few lines above).

> 
> b) Would it be reasonable to add quotes back to the various GCC_TARGET_
> variables once the arch.mk.* files have been included? It seems to me
> that the existing use of the BR2_GCC_TARGET_ variables expects them to
> be quoted.

 No. The convention is to always qstrip the .config variables, and add the
quotes back where needed. This makes sure that even if you override the option
on the command line, the quotes are there.

 That said, all GCC_TARGET_* variables *must* be either a single word or empty,
so there is no need to add quotes.

> 
>>> diff --git a/package/binutils/Config.in.host b/package/binutils/Config.in.host
>>> index 21dc84e498..747bac57ed 100644
>>> --- a/package/binutils/Config.in.host
>>> +++ b/package/binutils/Config.in.host
>>> @@ -6,15 +6,18 @@ choice
>>>  	default BR2_BINUTILS_VERSION_2_28_X if BR2_ARM_INSTRUCTIONS_THUMB2
>>>  	default BR2_BINUTILS_VERSION_2_29_X if !BR2_arc
>>>  	default BR2_BINUTILS_VERSION_ARC if BR2_arc
>>> +	default BR2_BINUTILS_VERSION_2_30_X if BR2_riscv64
>> This is not really needed, since the first selectable entry is the one
>> chosen by default.
> I'll remove this.

 Yes, remove this, but...

>>>  	help
>>>  	  Select the version of binutils you wish to use.
>>>  
>>>  config BR2_BINUTILS_VERSION_2_28_X
>>>  	bool "binutils 2.28.1"
>>>  	depends on !BR2_arc
>>> +	depends on !BR2_riscv64
>>>  
>>>  config BR2_BINUTILS_VERSION_2_29_X
>>>  	bool "binutils 2.29.1"
>>> +	depends on !BR2_riscv64
>>>  
>>>  config BR2_BINUTILS_VERSION_2_30_X
>>>  	bool "binutils 2.30"
>>> diff --git a/toolchain/toolchain-buildroot/Config.in b/toolchain/toolchain-buildroot/Config.in
>>> index 75e8191f46..6798448bda 100644
>>> --- a/toolchain/toolchain-buildroot/Config.in
>>> +++ b/toolchain/toolchain-buildroot/Config.in
>>> @@ -23,7 +23,7 @@ config BR2_TOOLCHAIN_BUILDROOT_VENDOR
>>>  choice
>>>  	prompt "C library"
>>>  	default BR2_TOOLCHAIN_BUILDROOT_UCLIBC
>>> -	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64
>>> +	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64 || BR2_riscv64
>> Ditto: since only glibc is supported on riscv64, it will automatically
>> be the default selection.
> I'll remove this too.

 ... here it makes sense to have an explicit default.

 Indeed, for the binutils version, there is a natural order, and when the overal
default changes to 2.31 we want riscv to follow. For the C library, however,
there is no natural order, so (IMO) it makes sense to explicitly select a
default. Indeed, we add an explicit default for uClibc even though it's the
first option.

 Yes, glibc is the only possibility so a default isn't really needed. But if
musl support is added (before uClibc support) then we do want an explicit
default for glibc...

 Again, my colleagues may be of a different opinion. And it's mostly
bikeshedding anyway.

>>>  
>>>  config BR2_TOOLCHAIN_BUILDROOT_UCLIBC
>>>  	bool "uClibc-ng"
>>> @@ -46,14 +46,16 @@ config BR2_TOOLCHAIN_BUILDROOT_GLIBC
>>>  		   BR2_aarch64_be  || BR2_i386       || BR2_mips    || \
>>>  		   BR2_mipsel      || BR2_mips64     || BR2_mips64el|| \
>>>  		   BR2_powerpc     || BR2_powerpc64  || BR2_powerpc64le || \
>>> -		   BR2_sh          || BR2_sparc64    || BR2_x86_64 || \
>>> -		   BR2_microblaze  || BR2_nios2      || BR2_archs38
>>> +		   BR2_riscv64     || BR2_sh         || BR2_sparc64     || \
>>> +		   BR2_x86_64      || BR2_microblaze || BR2_nios2       || \
>>> +		   BR2_archs38
>>>  	depends on BR2_USE_MMU
>>>  	depends on !BR2_STATIC_LIBS
>>>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_2
>>>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_10 || !BR2_powerpc64le
>>>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5 || !BR2_MIPS_NAN_2008
>>>  	depends on !BR2_powerpc_SPE
>>> +	depends on BR2_RISCV_ISA_RVA || !BR2_riscv64
>>>  	select BR2_TOOLCHAIN_USES_GLIBC
>>>  	# our glibc.mk enables RPC support
>>>  	select BR2_TOOLCHAIN_HAS_NATIVE_RPC
>>> @@ -77,6 +79,10 @@ comment "glibc on MIPS w/ NAN2008 needs a toolchain w/ headers >= 4.5"
>>>  	depends on BR2_MIPS_NAN_2008
>>>  	depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5
>>>  
>>> +comment "glibc needs ISA extension A on riscv64"
>>> +	depends on BR2_riscv64
>>> +	depends on !BR2_RISCV_ISA_RVA
>> So, if the user selects a "Custom architecture", and then doesn't
>> enable BR2_RISCV_ISA_CUSTOM_RVA, he won't have any C library available
>> to build a toolchain for riscv64.
>>
>> Provided this, I think we should not allow enabling/disabling RVA: we
>> should assume it's always enabled.
> I will force the selection of RVA for now, but will still show it in the
> architecture options. It is possible that it could be come optional at
> some point in the future.

 I would keep the option and make it blind (i.e. remove the prompt).

 Keep the conditions in the C library selection as well, of course. The comment
will never be shown, but it's already there fo when the atomics extension does
become optional.

 Speaking of which, what would be the effect of this atomics option on the
_HAS_SYNC options?

>>
>> Could you fix those comments, and resubmit a new complete patch series,
>> with all patches you need for RISC-V 64 support ?
> I will. Thanks for the feedback.
> 
> Mark
>>
>> Thanks a lot!
>>
>> Thomas
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-08-31  9:11 [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture Mark Corbin
  2018-09-02 19:40 ` Thomas Petazzoni
@ 2018-09-03 23:00 ` Arnout Vandecappelle
  2018-09-04 13:10   ` Mark Corbin
  1 sibling, 1 reply; 15+ messages in thread
From: Arnout Vandecappelle @ 2018-09-03 23:00 UTC (permalink / raw)
  To: buildroot



On 31/08/2018 11:11, Mark Corbin wrote:
> This enables a riscv64 system to be built with a Buildroot generated
> toolchain (gcc >= 7.x, binutils >= 2.30, glibc only).
> 
> This configuration has been used to successfully build a qemu-bootable
> riscv-linux-4.15 kernel (https://github.com/riscv/riscv-linux.git).
> 
> Signed-off-by: Mark Corbin <mark.corbin@embecosm.com>
> ---
>  Makefile                                |   1 +
>  arch/Config.in                          |  15 ++++
>  arch/Config.in.riscv                    | 104 ++++++++++++++++++++++++
>  arch/arch.mk.riscv                      |  24 ++++++
>  package/binutils/Config.in.host         |   3 +
>  toolchain/toolchain-buildroot/Config.in |  12 ++-

 One more thing that is missing: in package/linux-headers/Config.in.host, you
have to exclude all the linux versions that don't support riscv yet (i.e.
everything before 4.15).

 To simplify that job, you first might want (in separate patches) some
deprecated versions: 4.10, 4.11, 4.12, 4.13.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-03 22:20     ` Arnout Vandecappelle
@ 2018-09-04  7:49       ` Thomas Petazzoni
  2018-09-04  8:51         ` Arnout Vandecappelle
  2018-09-04 14:50         ` Mark Corbin
  0 siblings, 2 replies; 15+ messages in thread
From: Thomas Petazzoni @ 2018-09-04  7:49 UTC (permalink / raw)
  To: buildroot

Hello,

On Tue, 4 Sep 2018 00:20:57 +0200, Arnout Vandecappelle wrote:

> >>> +config BR2_riscv64  
> 
>  Since the kernel uses "riscv" for both the 32- and 64-bit version, I'd prefer
> if we'd only have a single "riscv" architecture in Buildroot that will
> eventually cover the 32-bit version as well.
> 
>  So I'd call it "riscv" instead of "riscv64" everywhere. For now, you could add
> a blind (promptless) BR2_RISCV_64 option, which later can become part of a
> choice between 32-bit and 64-bit.
> 
>  However, this is unprecedented in Buildroot (right now all 64-bit architectures
> have a separate symbol) so the other developers may disagree. But for me,
> splitting all those architectures was a mistake (except for some, like arm64,
> which really are a different ISA).

I am fine with this proposal, especially if the kernel uses "riscv" for
both the 32 bit and 64 bit platforms.

Apparently, the architecture part of the tuple is going to be
different, but that's fine.

So, OK for BR2_riscv.

However Arnout, this means it's going to be handled different than x86.
For x86, we have separate i386 and x86-64 top-level architectures in
menuconfig, and then internally, we share the list of CPUs between
both. For RISC-V, you want a single top-level architecture, and a
sub-option to select between 32 and 64 bit.

But I'm fine with your proposal even with this slight inconsistency.

> >>> +config BR2_GCC_TARGET_ARCH
> >>> +#	Instruction set extension characters are appended by arch/arch.mk.riscv
> >>> +	default "rv64i"  
> 
>  I would construct the entire string in make, and leave the symbol undefined in
> .config.

Yes, agreed.

> > a) I guess that I will have to replace every existing BR2_GCC_TARGET_...
> > in the Buildroot configurations with the equivalent GCC_TARGET_... ?
> > This would cover a few packages and the external toolchain.  
> 
>  Yes (as Thomas wrote a few lines above).

Yes, the other places where BR2_GCC_TARGET_... are used should be
changed to use the new GCC_TARGET_... options.

> > b) Would it be reasonable to add quotes back to the various GCC_TARGET_
> > variables once the arch.mk.* files have been included? It seems to me
> > that the existing use of the BR2_GCC_TARGET_ variables expects them to
> > be quoted.  
> 
>  No. The convention is to always qstrip the .config variables, and add the
> quotes back where needed. This makes sure that even if you override the option
> on the command line, the quotes are there.
> 
>  That said, all GCC_TARGET_* variables *must* be either a single word or empty,
> so there is no need to add quotes.

ACK.

> >>> diff --git a/toolchain/toolchain-buildroot/Config.in b/toolchain/toolchain-buildroot/Config.in
> >>> index 75e8191f46..6798448bda 100644
> >>> --- a/toolchain/toolchain-buildroot/Config.in
> >>> +++ b/toolchain/toolchain-buildroot/Config.in
> >>> @@ -23,7 +23,7 @@ config BR2_TOOLCHAIN_BUILDROOT_VENDOR
> >>>  choice
> >>>  	prompt "C library"
> >>>  	default BR2_TOOLCHAIN_BUILDROOT_UCLIBC
> >>> -	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64
> >>> +	default BR2_TOOLCHAIN_BUILDROOT_GLIBC if BR2_powerpc64 || BR2_riscv64  
> >> Ditto: since only glibc is supported on riscv64, it will automatically
> >> be the default selection.  
> > I'll remove this too.  
> 
>  ... here it makes sense to have an explicit default.
> 
>  Indeed, for the binutils version, there is a natural order, and when the overal
> default changes to 2.31 we want riscv to follow. For the C library, however,
> there is no natural order, so (IMO) it makes sense to explicitly select a
> default. Indeed, we add an explicit default for uClibc even though it's the
> first option.
> 
>  Yes, glibc is the only possibility so a default isn't really needed. But if
> musl support is added (before uClibc support) then we do want an explicit
> default for glibc...
> 
>  Again, my colleagues may be of a different opinion. And it's mostly
> bikeshedding anyway.

I won't bikeshed the bikesheding, but I don't find these default very
useful, if only because they are not consistent.

Take BR2_nios2, it is only supported by glibc, but it isn't part of
this "select glibc if BR2_powerpc64". Ditto BR2_sparc64. Ditto
BR2_powerpc64le, etc.

I also believe there is a natural order in the C libraries:

 - uClibc is first, because that has traditionally been the default C
   library in Buildroot, so if it supports the currently selected
   architecture, it should be our default.

 - glibc, because it's otherwise the natural default.

 - musl, coming last, as an alternate option.

So to me, it's very much like the binutils version selection: there is
no need to have explicit "default", as they would be annoying to
maintain, as shown by the current inconsistencies.

> >> Provided this, I think we should not allow enabling/disabling RVA: we
> >> should assume it's always enabled.  
> > I will force the selection of RVA for now, but will still show it in the
> > architecture options. It is possible that it could be come optional at
> > some point in the future.  
> 
>  I would keep the option and make it blind (i.e. remove the prompt).
> 
>  Keep the conditions in the C library selection as well, of course. The comment
> will never be shown, but it's already there fo when the atomics extension does
> become optional.

Meh, I'm not so sure. Is it likely that glibc adds support for
platforms without the atomic operations ? I'm not so sure. So I'd
rather not support it all for now, and add it when/if that becomes
available.

Perhaps we could make the option visible, but forcefully selected by
the BR2_riscv option, so that in practice it cannot be disabled.

>  Speaking of which, what would be the effect of this atomics option on the
> _HAS_SYNC options?

Ah, that's a good point, all the BR2_TOOLCHAIN_HAS_SYNC_{1,2,4,8}
options need to be updated.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-04  7:49       ` Thomas Petazzoni
@ 2018-09-04  8:51         ` Arnout Vandecappelle
  2018-09-04 14:44           ` Mark Corbin
  2018-09-04 14:50         ` Mark Corbin
  1 sibling, 1 reply; 15+ messages in thread
From: Arnout Vandecappelle @ 2018-09-04  8:51 UTC (permalink / raw)
  To: buildroot



On 04/09/2018 09:49, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 4 Sep 2018 00:20:57 +0200, Arnout Vandecappelle wrote:
> 
>>>>> +config BR2_riscv64  
>>
>>  Since the kernel uses "riscv" for both the 32- and 64-bit version, I'd prefer
>> if we'd only have a single "riscv" architecture in Buildroot that will
>> eventually cover the 32-bit version as well.
>>
>>  So I'd call it "riscv" instead of "riscv64" everywhere. For now, you could add
>> a blind (promptless) BR2_RISCV_64 option, which later can become part of a
>> choice between 32-bit and 64-bit.
>>
>>  However, this is unprecedented in Buildroot (right now all 64-bit architectures
>> have a separate symbol) so the other developers may disagree. But for me,
>> splitting all those architectures was a mistake (except for some, like arm64,
>> which really are a different ISA).
> 
> I am fine with this proposal, especially if the kernel uses "riscv" for
> both the 32 bit and 64 bit platforms.
> 
> Apparently, the architecture part of the tuple is going to be
> different, but that's fine.
> 
> So, OK for BR2_riscv.
> 
> However Arnout, this means it's going to be handled different than x86.
> For x86, we have separate i386 and x86-64 top-level architectures in
> menuconfig, and then internally, we share the list of CPUs between
> both. For RISC-V, you want a single top-level architecture, and a
> sub-option to select between 32 and 64 bit.

 Not just x86, also mips, powerpc and sparc (in the kernel it's one arch, in
Buildroot they are separate). But so I think it's a mistake of Buildroot to
treat these as separate. Just look at how often we have the same condition
repeated for the 32-bit and 64-bit variant of these architectures all over the
place.

 The same could be said about the endianness options BTW, but fortunately RISC-V
is always little-endian :-)


> But I'm fine with your proposal even with this slight inconsistency.
> 
[snip]
>>>> Ditto: since only glibc is supported on riscv64, it will automatically
>>>> be the default selection.  
>>> I'll remove this too.  
>>
>>  ... here it makes sense to have an explicit default.
>>
>>  Indeed, for the binutils version, there is a natural order, and when the overal
>> default changes to 2.31 we want riscv to follow. For the C library, however,
>> there is no natural order, so (IMO) it makes sense to explicitly select a
>> default. Indeed, we add an explicit default for uClibc even though it's the
>> first option.
>>
>>  Yes, glibc is the only possibility so a default isn't really needed. But if
>> musl support is added (before uClibc support) then we do want an explicit
>> default for glibc...
>>
>>  Again, my colleagues may be of a different opinion. And it's mostly
>> bikeshedding anyway.
> 
> I won't bikeshed the bikesheding, but I don't find these default very
> useful, if only because they are not consistent.

 Ah, indeed, it's not consistent. That's not good.

> Take BR2_nios2, it is only supported by glibc, but it isn't part of
> this "select glibc if BR2_powerpc64". Ditto BR2_sparc64. Ditto
> BR2_powerpc64le, etc.

 OK, so indeed if we don't want to add to the inconsistency, there should not be
a default.

> 
> I also believe there is a natural order in the C libraries:

 Fair enough.

 So, remove the explicit default after all.

> 
>  - uClibc is first, because that has traditionally been the default C
>    library in Buildroot, so if it supports the currently selected
>    architecture, it should be our default.
> 
>  - glibc, because it's otherwise the natural default.
> 
>  - musl, coming last, as an alternate option.
> 
> So to me, it's very much like the binutils version selection: there is
> no need to have explicit "default", as they would be annoying to
> maintain, as shown by the current inconsistencies.
> 
>>>> Provided this, I think we should not allow enabling/disabling RVA: we
>>>> should assume it's always enabled.  
>>> I will force the selection of RVA for now, but will still show it in the
>>> architecture options. It is possible that it could be come optional at
>>> some point in the future.  
>>
>>  I would keep the option and make it blind (i.e. remove the prompt).
>>
>>  Keep the conditions in the C library selection as well, of course. The comment
>> will never be shown, but it's already there fo when the atomics extension does
>> become optional.
> 
> Meh, I'm not so sure. Is it likely that glibc adds support for
> platforms without the atomic operations ? I'm not so sure. So I'd
> rather not support it all for now, and add it when/if that becomes
> available.

 I don't think it's likely that glibc will add support for non-atomic variants.
I do think it's likely that uClibc will do that. When uClibc for riscv is added
(and it supports non-atomic), we *do* want to see the comment for glibc.

 However, I'm going to back off from my earlier position. The rule in Buildroot
is: don't add anything that is not useful right away, only add it when it is
actually going to be used.

 The RVA option itself is used, but the comment isn't (it can never become
visible). So drop the comment.


> Perhaps we could make the option visible, but forcefully selected by
> the BR2_riscv option, so that in practice it cannot be disabled.

 Well, that was your original suggestion, and my suggestion was: make the option
invisible. I really don't see the point of a user-visible option that you can't
(de)select.

 Regards,
 Arnout


> 
>>  Speaking of which, what would be the effect of this atomics option on the
>> _HAS_SYNC options?
> 
> Ah, that's a good point, all the BR2_TOOLCHAIN_HAS_SYNC_{1,2,4,8}
> options need to be updated.
> 
> Best regards,
> 
> Thomas
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-02 19:40 ` Thomas Petazzoni
  2018-09-03 11:26   ` Mark Corbin
@ 2018-09-04 10:03   ` Mark Corbin
  2018-09-04 11:24     ` Thomas Petazzoni
  1 sibling, 1 reply; 15+ messages in thread
From: Mark Corbin @ 2018-09-04 10:03 UTC (permalink / raw)
  To: buildroot

Hello Thomas

On 02/09/18 20:40, Thomas Petazzoni wrote:
>
>> +endchoice
>> +
>> +config BR2_ARCH
>> +	default "riscv64"
>> +
>> +config BR2_ENDIAN
>> +	default "LITTLE"
>> +
>> +config BR2_GCC_TARGET_ARCH
>> +#	Instruction set extension characters are appended by arch/arch.mk.riscv
>> +	default "rv64i"
>> +
>> +config BR2_GCC_TARGET_ABI
>> +	default "lp64" if BR2_RISCV_ABI_LP64
>> +	default "lp64f" if BR2_RISCV_ABI_LP64F
>> +	default "lp64d" if BR2_RISCV_ABI_LP64D
>> +
>> +config BR2_READELF_ARCH_NAME
>> +	default "RISC-V"
>> +
>> diff --git a/arch/arch.mk.riscv b/arch/arch.mk.riscv
>> new file mode 100644
>> index 0000000000..ad2e26903b
>> --- /dev/null
>> +++ b/arch/arch.mk.riscv
>> @@ -0,0 +1,24 @@
>> +#
>> +# Append the appropriate RISC-V ISA extensions to the
>> +# BR2_GCC_TARGET_ARCH variable.
>> +#
>> +
>> +BR2_RISCV_GCC_ARCH = $(call qstrip,$(BR2_GCC_TARGET_ARCH))
>> +
>> +ifeq ($(BR2_RISCV_ISA_RVM),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)m
>> +endif
>> +ifeq ($(BR2_RISCV_ISA_RVA),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)a
>> +endif
>> +ifeq ($(BR2_RISCV_ISA_RVF),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)f
>> +endif
>> +ifeq ($(BR2_RISCV_ISA_RVD),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)d
>> +endif
>> +ifeq ($(BR2_RISCV_ISA_RVC),y)
>> +BR2_RISCV_GCC_ARCH := $(BR2_RISCV_GCC_ARCH)c
>> +endif
>> +
>> +BR2_GCC_TARGET_ARCH := $(patsubst %,"%",$(BR2_RISCV_GCC_ARCH))
> So, this is probably the part I'm the least happy with. I think it is
> the only occurrence where make logic would tweak BR2_* variables. I'm
> not sure I want to set a precedent for this, as it could become
> "strange" to see values of BR2_* options that don't match what is
> visible in the .config file.
>
> So, I think this will require a bit more rework. The solution I would
> propose is to introduce arch/arch.mk, which does:
>
> GCC_TARGET_ARCH = $(call qstrip,$(BR2_GCC_TARGET_ARCH))
> GCC_TARGET_ABI  = $(call qstrip,$(BR2_GCC_TARGET_ABI))
> GCC_TARGET_NAN  = $(call qstrip,$(BR2_GCC_TARGET_NAN))
> # etc. for all BR2_GCC_TARGET_* options listed in arch/Config.in
There is a name clash with using GCC_TARGET_* - some of these names are
already used in packages/gcc/gcc.mk.

GCC_TARGET_FPU = $(call qstrip,$(BR2_GCC_TARGET_FPU))
ifneq ($(GCC_TARGET_FPU),)
HOST_GCC_COMMON_CONF_OPTS += --with-fpu=$(GCC_TARGET_FPU)
endif

GCC_TARGET_FLOAT_ABI = $(call qstrip,$(BR2_GCC_TARGET_FLOAT_ABI))
ifneq ($(GCC_TARGET_FLOAT_ABI),)
HOST_GCC_COMMON_CONF_OPTS += --with-float=$(GCC_TARGET_FLOAT_ABI)
endif

GCC_TARGET_MODE = $(call qstrip,$(BR2_GCC_TARGET_MODE))
ifneq ($(GCC_TARGET_MODE),)
HOST_GCC_COMMON_CONF_OPTS += --with-mode=$(GCC_TARGET_MODE)
endif

Anybody have any thoughts for a new prefix for either set of variables?

Thanks

Mark


-- 
Mark Corbin
Embecosm Ltd.
https://www.embecosm.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-04 10:03   ` Mark Corbin
@ 2018-09-04 11:24     ` Thomas Petazzoni
  0 siblings, 0 replies; 15+ messages in thread
From: Thomas Petazzoni @ 2018-09-04 11:24 UTC (permalink / raw)
  To: buildroot

Hello,

On Tue, 4 Sep 2018 11:03:22 +0100, Mark Corbin wrote:

> There is a name clash with using GCC_TARGET_* - some of these names are
> already used in packages/gcc/gcc.mk.
> 
> GCC_TARGET_FPU = $(call qstrip,$(BR2_GCC_TARGET_FPU))

Well, just get rid of this line, since it's now going to be in
arch/arch.mk.

> ifneq ($(GCC_TARGET_FPU),)
> HOST_GCC_COMMON_CONF_OPTS += --with-fpu=$(GCC_TARGET_FPU)
> endif
> 
> GCC_TARGET_FLOAT_ABI = $(call qstrip,$(BR2_GCC_TARGET_FLOAT_ABI))

Ditto.

> ifneq ($(GCC_TARGET_FLOAT_ABI),)
> HOST_GCC_COMMON_CONF_OPTS += --with-float=$(GCC_TARGET_FLOAT_ABI)
> endif
> 
> GCC_TARGET_MODE = $(call qstrip,$(BR2_GCC_TARGET_MODE))

Ditto.

> ifneq ($(GCC_TARGET_MODE),)
> HOST_GCC_COMMON_CONF_OPTS += --with-mode=$(GCC_TARGET_MODE)
> endif
> 
> Anybody have any thoughts for a new prefix for either set of variables?

Not needed: the whole idea of the change is to move those line setting
GCC_TARGET_* variables from package/gcc/ to arch/arch.mk.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-03 23:00 ` Arnout Vandecappelle
@ 2018-09-04 13:10   ` Mark Corbin
  2018-09-04 21:03     ` Arnout Vandecappelle
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Corbin @ 2018-09-04 13:10 UTC (permalink / raw)
  To: buildroot

Hello Arnout

On 04/09/18 00:00, Arnout Vandecappelle wrote:
>
> On 31/08/2018 11:11, Mark Corbin wrote:
>> This enables a riscv64 system to be built with a Buildroot generated
>> toolchain (gcc >= 7.x, binutils >= 2.30, glibc only).
>>
>> This configuration has been used to successfully build a qemu-bootable
>> riscv-linux-4.15 kernel (https://github.com/riscv/riscv-linux.git).
>>
>> Signed-off-by: Mark Corbin <mark.corbin@embecosm.com>
>> ---
>>  Makefile                                |   1 +
>>  arch/Config.in                          |  15 ++++
>>  arch/Config.in.riscv                    | 104 ++++++++++++++++++++++++
>>  arch/arch.mk.riscv                      |  24 ++++++
>>  package/binutils/Config.in.host         |   3 +
>>  toolchain/toolchain-buildroot/Config.in |  12 ++-
>  One more thing that is missing: in package/linux-headers/Config.in.host, you
> have to exclude all the linux versions that don't support riscv yet (i.e.
> everything before 4.15).
Is it worth doing this when you actually have to select a custom version
of headers anyway? This makes more sense to me when the necessary riscv
support makes it upstream. I think that it would be a bit messy at the
moment as I should add a restriction to stop you choosing *any* mainline
kernel version...

config BR2_KERNEL_HEADERS_4_1
??????? bool "Linux 4.1.x kernel headers"
??????? select BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_1
??????? depends !BR2_riscv
.
.
etc.

...and also restrict your choice of versions for custom headers pre-4.15...

config BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_4_14
??????? bool "4.14.x"
??????? select BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
??????? depends on !BR2_riscv

.
.
etc.

Quite happy to do this if that is what is wanted.

>
>  To simplify that job, you first might want (in separate patches) some
> deprecated versions: 4.10, 4.11, 4.12, 4.13.
I'm not sure that I'd want to start deprecating header versions that
might still be being used by people.

Regards

Mark

>  Regards,
>  Arnout
>

-- 

*Mark Corbin*
Embedded Operating Systems Lead
Phone: +44 1590 610184?????Mobile: +44 7765 703479
Email: mark.corbin at embecosm.com
<mailto:mark.corbin@embecosm.com>?????Web: https://www.embecosm.com

Embecosm Logo

Embecosm Ltd., Palamos House #208, 66/67 High Street, Lymington, SO41
9AL, UK
Company No. 6577021 (England & Wales).
--
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20180904/7b005ba0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: logo.png
Type: image/png
Size: 8442 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20180904/7b005ba0/attachment.png>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-04  8:51         ` Arnout Vandecappelle
@ 2018-09-04 14:44           ` Mark Corbin
  0 siblings, 0 replies; 15+ messages in thread
From: Mark Corbin @ 2018-09-04 14:44 UTC (permalink / raw)
  To: buildroot


On 04/09/18 09:51, Arnout Vandecappelle wrote:
>
>>>>> Provided this, I think we should not allow enabling/disabling RVA: we
>>>>> should assume it's always enabled.  
>>>> I will force the selection of RVA for now, but will still show it in the
>>>> architecture options. It is possible that it could be come optional at
>>>> some point in the future.  
>>>  I would keep the option and make it blind (i.e. remove the prompt).
>>>
>>>  Keep the conditions in the C library selection as well, of course. The comment
>>> will never be shown, but it's already there fo when the atomics extension does
>>> become optional.
>> Meh, I'm not so sure. Is it likely that glibc adds support for
>> platforms without the atomic operations ? I'm not so sure. So I'd
>> rather not support it all for now, and add it when/if that becomes
>> available.
>  I don't think it's likely that glibc will add support for non-atomic variants.
> I do think it's likely that uClibc will do that. When uClibc for riscv is added
> (and it supports non-atomic), we *do* want to see the comment for glibc.
>
>  However, I'm going to back off from my earlier position. The rule in Buildroot
> is: don't add anything that is not useful right away, only add it when it is
> actually going to be used.
>
>  The RVA option itself is used, but the comment isn't (it can never become
> visible). So drop the comment.
>
>
>> Perhaps we could make the option visible, but forcefully selected by
>> the BR2_riscv option, so that in practice it cannot be disabled.
>  Well, that was your original suggestion, and my suggestion was: make the option
> invisible. I really don't see the point of a user-visible option that you can't
> (de)select.
>
>  Regards,
>  Arnout

I'm in favour of a user-visible option that you can't disable...I like
the fact that I can see which optional architecture ISA extensions have
been selected/used to build the toolchain and filesystem (without having
to look in .config).

*** Instruction Set Extensions ***
[ ] Integer Multiplication and Division (M) (NEW)
-*- Atomic Instructions (A)
[ ] Single-precision Floating-point (F) (NEW)
[ ] Compressed Instructions (C) (NEW)

Regards

Mark
-- 
Mark Corbin
Embecosm Ltd.
https://www.embecosm.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-04  7:49       ` Thomas Petazzoni
  2018-09-04  8:51         ` Arnout Vandecappelle
@ 2018-09-04 14:50         ` Mark Corbin
  1 sibling, 0 replies; 15+ messages in thread
From: Mark Corbin @ 2018-09-04 14:50 UTC (permalink / raw)
  To: buildroot


On 04/09/18 08:49, Thomas Petazzoni wrote:
> Speaking of which, what would be the effect of this atomics option on the
>> _HAS_SYNC options?
> Ah, that's a good point, all the BR2_TOOLCHAIN_HAS_SYNC_{1,2,4,8}
> options need to be updated.

The default settings for BR2_TOOLCHAIN_HAS_SYNC_{1,2,4,8} are good (i.e.
the toolchain supports all those data sizes).

Regards

Mark

>
> Best regards,
>
> Thomas

-- 
Mark Corbin
Embecosm Ltd.
https://www.embecosm.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-04 13:10   ` Mark Corbin
@ 2018-09-04 21:03     ` Arnout Vandecappelle
  2018-09-05 10:09       ` Mark Corbin
  0 siblings, 1 reply; 15+ messages in thread
From: Arnout Vandecappelle @ 2018-09-04 21:03 UTC (permalink / raw)
  To: buildroot



On 04/09/2018 15:10, Mark Corbin wrote:
> Hello Arnout
> 
> On 04/09/18 00:00, Arnout Vandecappelle wrote:
>> On 31/08/2018 11:11, Mark Corbin wrote:
>>> This enables a riscv64 system to be built with a Buildroot generated
>>> toolchain (gcc >= 7.x, binutils >= 2.30, glibc only).
>>>
>>> This configuration has been used to successfully build a qemu-bootable
>>> riscv-linux-4.15 kernel (https://github.com/riscv/riscv-linux.git).
>>>
>>> Signed-off-by: Mark Corbin <mark.corbin@embecosm.com>
>>> ---
>>>  Makefile                                |   1 +
>>>  arch/Config.in                          |  15 ++++
>>>  arch/Config.in.riscv                    | 104 ++++++++++++++++++++++++
>>>  arch/arch.mk.riscv                      |  24 ++++++
>>>  package/binutils/Config.in.host         |   3 +
>>>  toolchain/toolchain-buildroot/Config.in |  12 ++-
>>  One more thing that is missing: in package/linux-headers/Config.in.host, you
>> have to exclude all the linux versions that don't support riscv yet (i.e.
>> everything before 4.15).
> Is it worth doing this when you actually have to select a custom version of
> headers anyway?

 In that case, you have to disable *all* choices except for _AS_KERNEL and _VERSION.

 I didn't realise that upstream riscv support was broken. I saw that Linus added
riscv in v4.15 so I assumed that it could be used.

 So I pulled a diff, the differences between upstream and riscv in uapi are
really minor. And there are no longer any differences between v4.18.5 and the
riscv-linux-4.18 branch. So I would tend to say that the upstream v4.15 should
work as well. Of course, I haven't tried...

> This makes more sense to me when the necessary riscv support
> makes it upstream. I think that it would be a bit messy at the moment as I
> should add a restriction to stop you choosing *any* mainline kernel version...
> 
> config BR2_KERNEL_HEADERS_4_1
> ??????? bool "Linux 4.1.x kernel headers"
> ??????? select BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_1
> ??????? depends !BR2_riscv
> .
> .
> etc.
> 
> ...and also restrict your choice of versions for custom headers pre-4.15...
> 
> config BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_4_14
> ??????? bool "4.14.x"
> ??????? select BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
> ??????? depends on !BR2_riscv

 If you look at the existing code, you'll see that we do add the exclusions for
the upstream headers choice, but not for the custom headers. That's because the
custom headers is supposed to be used for a vendor fork which *does* support
that arch already. Case in point: you'd exclude the upstream 4.15 choice, but
you'd still want to be able to build with the riscv-linux-4.15 tree.

> 
> .
> .
> etc.
> 
> Quite happy to do this if that is what is wanted.
> 
>>  To simplify that job, you first might want (in separate patches) some
>> deprecated versions: 4.10, 4.11, 4.12, 4.13.
> I'm not sure that I'd want to start deprecating header versions that might still
> be being used by people.

 The version choices we offer are kind of "sanctioned", so we only really want
the versions that are still supported by the stable tree. People who need
something different can always choose a custom version. It does make sense to
keep a few additional ones, e.g. 4.1 because it is used in some distros and by a
current NXP release.

 Hm, I notice now that a year ago Gustavo removed 3.18 and 3.16 even though
those are still getting patches... So sometimes the removal is a little too
aggressive...

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-04 21:03     ` Arnout Vandecappelle
@ 2018-09-05 10:09       ` Mark Corbin
  2018-09-07 19:26         ` Arnout Vandecappelle
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Corbin @ 2018-09-05 10:09 UTC (permalink / raw)
  To: buildroot

Hello Arnout

On 04/09/18 22:03, Arnout Vandecappelle wrote:
>
> On 04/09/2018 15:10, Mark Corbin wrote:
>> Hello Arnout
>>
>> On 04/09/18 00:00, Arnout Vandecappelle wrote:
>>> On 31/08/2018 11:11, Mark Corbin wrote:
>>>> This enables a riscv64 system to be built with a Buildroot generated
>>>> toolchain (gcc >= 7.x, binutils >= 2.30, glibc only).
>>>>
>>>> This configuration has been used to successfully build a qemu-bootable
>>>> riscv-linux-4.15 kernel (https://github.com/riscv/riscv-linux.git).
>>>>
>>>> Signed-off-by: Mark Corbin <mark.corbin@embecosm.com>
>>>> ---
>>>>  Makefile                                |   1 +
>>>>  arch/Config.in                          |  15 ++++
>>>>  arch/Config.in.riscv                    | 104 ++++++++++++++++++++++++
>>>>  arch/arch.mk.riscv                      |  24 ++++++
>>>>  package/binutils/Config.in.host         |   3 +
>>>>  toolchain/toolchain-buildroot/Config.in |  12 ++-
>>>  One more thing that is missing: in package/linux-headers/Config.in.host, you
>>> have to exclude all the linux versions that don't support riscv yet (i.e.
>>> everything before 4.15).
>> Is it worth doing this when you actually have to select a custom version of
>> headers anyway?
>  In that case, you have to disable *all* choices except for _AS_KERNEL and _VERSION.
>
>  I didn't realise that upstream riscv support was broken. I saw that Linus added
> riscv in v4.15 so I assumed that it could be used.
>
>  So I pulled a diff, the differences between upstream and riscv in uapi are
> really minor. And there are no longer any differences between v4.18.5 and the
> riscv-linux-4.18 branch. So I would tend to say that the upstream v4.15 should
> work as well. Of course, I haven't tried...

I couldn't get the upstream 4.15 and 4.17 kernels to boot under qemu, so
I asked about the status of kernel support on the riscv sw-dev mailing
list. Apparently the existing upstream kernels do not support qemu
(which is the first target that I am adding). The developers are
expecting the necessary changes to make it into the 4.19 kernel release.

In this case I think it makes sense for me to disable all choices except
_AS_KERNEL, _CUSTOM_TARBALL and CUSTOM_GIT.

>
>> This makes more sense to me when the necessary riscv support
>> makes it upstream. I think that it would be a bit messy at the moment as I
>> should add a restriction to stop you choosing *any* mainline kernel version...
>>
>> config BR2_KERNEL_HEADERS_4_1
>> ??????? bool "Linux 4.1.x kernel headers"
>> ??????? select BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_1
>> ??????? depends !BR2_riscv
>> .
>> .
>> etc.
>>
>> ...and also restrict your choice of versions for custom headers pre-4.15...
>>
>> config BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_4_14
>> ??????? bool "4.14.x"
>> ??????? select BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
>> ??????? depends on !BR2_riscv
>  If you look at the existing code, you'll see that we do add the exclusions for
> the upstream headers choice, but not for the custom headers. That's because the
> custom headers is supposed to be used for a vendor fork which *does* support
> that arch already. Case in point: you'd exclude the upstream 4.15 choice, but
> you'd still want to be able to build with the riscv-linux-4.15 tree.

So should I allow any version of custom headers to be selected even
though only 4.15 is relevant for riscv?

Regards

Mark

-- 
Mark Corbin
Embecosm Ltd.
https://www.embecosm.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture
  2018-09-05 10:09       ` Mark Corbin
@ 2018-09-07 19:26         ` Arnout Vandecappelle
  0 siblings, 0 replies; 15+ messages in thread
From: Arnout Vandecappelle @ 2018-09-07 19:26 UTC (permalink / raw)
  To: buildroot



On 05/09/2018 12:09, Mark Corbin wrote:
> Hello Arnout
> 
> On 04/09/18 22:03, Arnout Vandecappelle wrote:
>>
>> On 04/09/2018 15:10, Mark Corbin wrote:
>>> Hello Arnout
>>>
>>> On 04/09/18 00:00, Arnout Vandecappelle wrote:
>>>> On 31/08/2018 11:11, Mark Corbin wrote:
>>>>> This enables a riscv64 system to be built with a Buildroot generated
>>>>> toolchain (gcc >= 7.x, binutils >= 2.30, glibc only).
>>>>>
>>>>> This configuration has been used to successfully build a qemu-bootable
>>>>> riscv-linux-4.15 kernel (https://github.com/riscv/riscv-linux.git).
>>>>>
>>>>> Signed-off-by: Mark Corbin <mark.corbin@embecosm.com>
>>>>> ---
>>>>>  Makefile                                |   1 +
>>>>>  arch/Config.in                          |  15 ++++
>>>>>  arch/Config.in.riscv                    | 104 ++++++++++++++++++++++++
>>>>>  arch/arch.mk.riscv                      |  24 ++++++
>>>>>  package/binutils/Config.in.host         |   3 +
>>>>>  toolchain/toolchain-buildroot/Config.in |  12 ++-
>>>>  One more thing that is missing: in package/linux-headers/Config.in.host, you
>>>> have to exclude all the linux versions that don't support riscv yet (i.e.
>>>> everything before 4.15).
>>> Is it worth doing this when you actually have to select a custom version of
>>> headers anyway?
>>  In that case, you have to disable *all* choices except for _AS_KERNEL and _VERSION.
>>
>>  I didn't realise that upstream riscv support was broken. I saw that Linus added
>> riscv in v4.15 so I assumed that it could be used.
>>
>>  So I pulled a diff, the differences between upstream and riscv in uapi are
>> really minor. And there are no longer any differences between v4.18.5 and the
>> riscv-linux-4.18 branch. So I would tend to say that the upstream v4.15 should
>> work as well. Of course, I haven't tried...
> 
> I couldn't get the upstream 4.15 and 4.17 kernels to boot under qemu, so
> I asked about the status of kernel support on the riscv sw-dev mailing
> list. Apparently the existing upstream kernels do not support qemu
> (which is the first target that I am adding). The developers are
> expecting the necessary changes to make it into the 4.19 kernel release.

 Yes, you do not want to use the upstream 4.15 *kernel*. But that doesn't mean
you should exclude the upstream 4.15 *headers*.

> In this case I think it makes sense for me to disable all choices except
> _AS_KERNEL, _CUSTOM_TARBALL and CUSTOM_GIT.
> 
>>
>>> This makes more sense to me when the necessary riscv support
>>> makes it upstream. I think that it would be a bit messy at the moment as I
>>> should add a restriction to stop you choosing *any* mainline kernel version...
>>>
>>> config BR2_KERNEL_HEADERS_4_1
>>> ??????? bool "Linux 4.1.x kernel headers"
>>> ??????? select BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_1
>>> ??????? depends !BR2_riscv
>>> .
>>> .
>>> etc.
>>>
>>> ...and also restrict your choice of versions for custom headers pre-4.15...
>>>
>>> config BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_4_14
>>> ??????? bool "4.14.x"
>>> ??????? select BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
>>> ??????? depends on !BR2_riscv
>>  If you look at the existing code, you'll see that we do add the exclusions for
>> the upstream headers choice, but not for the custom headers. That's because the
>> custom headers is supposed to be used for a vendor fork which *does* support
>> that arch already. Case in point: you'd exclude the upstream 4.15 choice, but
>> you'd still want to be able to build with the riscv-linux-4.15 tree.
> 
> So should I allow any version of custom headers to be selected even
> though only 4.15 is relevant for riscv?

 Yes. In other words, don't add dependencies at all for the custom choices. We
don't do it for any of the other architectures either.

 Regards,
 Arnout

> 
> Regards
> 
> Mark
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-09-07 19:26 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-31  9:11 [Buildroot] [PATCH 1/1] arch: add support for RISC-V 64-bit (riscv64) architecture Mark Corbin
2018-09-02 19:40 ` Thomas Petazzoni
2018-09-03 11:26   ` Mark Corbin
2018-09-03 22:20     ` Arnout Vandecappelle
2018-09-04  7:49       ` Thomas Petazzoni
2018-09-04  8:51         ` Arnout Vandecappelle
2018-09-04 14:44           ` Mark Corbin
2018-09-04 14:50         ` Mark Corbin
2018-09-04 10:03   ` Mark Corbin
2018-09-04 11:24     ` Thomas Petazzoni
2018-09-03 23:00 ` Arnout Vandecappelle
2018-09-04 13:10   ` Mark Corbin
2018-09-04 21:03     ` Arnout Vandecappelle
2018-09-05 10:09       ` Mark Corbin
2018-09-07 19:26         ` Arnout Vandecappelle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.