[v2,2/2] boot/opensbi: Implement a choice for the OpenSBI Platform
diff mbox series

Message ID 20190709223649.25630-2-alistair.francis@wdc.com
State Rejected
Headers show
Series
  • Untitled series #118641
Related show

Commit Message

Alistair Francis July 9, 2019, 10:36 p.m. UTC
Instead of requiring users to look at the OpenSBI source code and
determine the platform string let's use a choice option to allow them to
specify the platform they would like to target. We still allow them to
specify a custom string if they want to.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 boot/opensbi/Config.in              | 25 ++++++++++++++++++++++---
 boot/opensbi/opensbi.mk             | 11 ++++++++++-
 configs/qemu_riscv32_virt_defconfig |  2 +-
 configs/qemu_riscv64_virt_defconfig |  2 +-
 4 files changed, 34 insertions(+), 6 deletions(-)

Comments

Thomas Petazzoni July 20, 2019, 7:50 p.m. UTC | #1
Hello Alistair,

On Tue,  9 Jul 2019 15:36:49 -0700
Alistair Francis <alistair.francis@wdc.com> wrote:

> Instead of requiring users to look at the OpenSBI source code and
> determine the platform string let's use a choice option to allow them to
> specify the platform they would like to target. We still allow them to
> specify a custom string if they want to.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

I have not applied this patch, because we don't do this anywhere in
Buildroot. For things like U-Boot, Linux or Barebox, the name of the
platform/configuration is always a free-form string. Having an explicit
list of platform is difficult to maintain as the list of platforms can
be huge. Also the list of supported platforms can differ from one
version to the other (in the case where the version itself is
configurable). In addition, how do you draw the line between platforms
that should be explicitly listed in the Config.in choice, and the ones
for which users should use the free-form string option.

But again, the main reason for rejecting is that in exactly this
situation in other packages in Buildroot, we use a simple free-form
string.

Best regards,

Thomas
Alistair Francis July 22, 2019, 8:45 p.m. UTC | #2
On Sat, Jul 20, 2019 at 12:50 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Alistair,
>
> On Tue,  9 Jul 2019 15:36:49 -0700
> Alistair Francis <alistair.francis@wdc.com> wrote:
>
> > Instead of requiring users to look at the OpenSBI source code and
> > determine the platform string let's use a choice option to allow them to
> > specify the platform they would like to target. We still allow them to
> > specify a custom string if they want to.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>
> I have not applied this patch, because we don't do this anywhere in
> Buildroot. For things like U-Boot, Linux or Barebox, the name of the
> platform/configuration is always a free-form string. Having an explicit
> list of platform is difficult to maintain as the list of platforms can
> be huge. Also the list of supported platforms can differ from one
> version to the other (in the case where the version itself is
> configurable). In addition, how do you draw the line between platforms
> that should be explicitly listed in the Config.in choice, and the ones
> for which users should use the free-form string option.
>
> But again, the main reason for rejecting is that in exactly this
> situation in other packages in Buildroot, we use a simple free-form
> string.

No worries. As that was the main problem Mark had with the HiFive
Unleashed patches these should be good to merge.

I just sent a v2 with an updated readme to the list.

Alistair

>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

Patch
diff mbox series

diff --git a/boot/opensbi/Config.in b/boot/opensbi/Config.in
index 5f3cc13312..997c79f9b0 100644
--- a/boot/opensbi/Config.in
+++ b/boot/opensbi/Config.in
@@ -13,13 +13,32 @@  config BR2_TARGET_OPENSBI
 	  https://github.com/riscv/opensbi.git
 
 if BR2_TARGET_OPENSBI
-config BR2_TARGET_OPENSBI_PLAT
-	string "OpenSBI Platform"
-	default ""
+
+choice
+	prompt "OpenSBI Platform"
 	help
 	  Specifies the OpenSBI platform to build. If no platform is
 	  specified only the OpenSBI platform independent static
 	  library libsbi.a is built. If a platform is specified then
 	  the platform specific static library libplatsbi.a and firmware
 	  examples are built.
+
+config BR2_TARGET_OPENSBI_LIBRARY_ONLY
+	bool "Library Only"
+
+config BR2_TARGET_OPENSBI_QEMU_VIRT
+	bool "QEMU Virt"
+
+config BR2_TARGET_OPENSBI_QEMU_SIFIVE_U
+	bool "QEMU SiFive U"
+
+config BR2_TARGET_OPENSBI_CUSTOM_PLATFORM
+	bool "Custom Platform"
+
+endchoice
+
+config BR2_TARGET_OPENSBI_CUSTOM_PLATFORM_VALUE
+	string "OpenSBI Platform String"
+	depends on BR2_TARGET_OPENSBI_CUSTOM_PLATFORM
+
 endif
diff --git a/boot/opensbi/opensbi.mk b/boot/opensbi/opensbi.mk
index 83552a5442..78da22aa1f 100644
--- a/boot/opensbi/opensbi.mk
+++ b/boot/opensbi/opensbi.mk
@@ -14,7 +14,16 @@  OPENSBI_INSTALL_STAGING = YES
 OPENSBI_MAKE_ENV = \
 	CROSS_COMPILE=$(TARGET_CROSS)
 
-OPENSBI_PLAT = $(call qstrip,$(BR2_TARGET_OPENSBI_PLAT))
+ifeq ($(BR2_TARGET_OPENSBI_QEMU_VIRT),y)
+OPENSBI_PLAT = qemu/virt
+else ifeq ($(BR2_TARGET_OPENSBI_QEMU_SIFIVE_U),y)
+OPENSBI_PLAT = qemu/sifive_u
+else ifeq ($(BR2_TARGET_OPENSBI_SIFIVE_FU540),y)
+OPENSBI_PLAT = sifive/fu540
+else ifeq ($(BR2_TARGET_OPENSBI_CUSTOM_PLATFORM),y)
+OPENSBI_PLAT = $(call qstrip,$(BR2_TARGET_OPENSBI_CUSTOM_PLATFORM_VALUE))
+endif
+
 ifneq ($(OPENSBI_PLAT),)
 OPENSBI_MAKE_ENV += PLATFORM=$(OPENSBI_PLAT)
 endif
diff --git a/configs/qemu_riscv32_virt_defconfig b/configs/qemu_riscv32_virt_defconfig
index a1a8c5fd20..ebe3e72135 100644
--- a/configs/qemu_riscv32_virt_defconfig
+++ b/configs/qemu_riscv32_virt_defconfig
@@ -24,4 +24,4 @@  BR2_LINUX_KERNEL_IMAGE=y
 # Bootloader
 BR2_TARGET_OPENSBI=y
 BR2_TARGET_OPENSBI_USE_PLAT=y
-BR2_TARGET_OPENSBI_PLAT="qemu/virt"
+BR2_TARGET_OPENSBI_QEMU_VIRT=y
diff --git a/configs/qemu_riscv64_virt_defconfig b/configs/qemu_riscv64_virt_defconfig
index c0b1a43925..16e0e332f7 100644
--- a/configs/qemu_riscv64_virt_defconfig
+++ b/configs/qemu_riscv64_virt_defconfig
@@ -23,4 +23,4 @@  BR2_LINUX_KERNEL_IMAGE=y
 # Bootloader
 BR2_TARGET_OPENSBI=y
 BR2_TARGET_OPENSBI_USE_PLAT=y
-BR2_TARGET_OPENSBI_PLAT="qemu/virt"
+BR2_TARGET_OPENSBI_QEMU_VIRT=y