[Buildroot] [PATCH 1/3] arch: riscv: Add a RISC-V Platform option

[Buildroot] [PATCH 1/3] arch: riscv: Add a RISC-V Platform option

[Alistair Francis]

Add a RISC-V Platform option. At the moment only QEMU platforms are
supported, but in the future this can be extended to include real
hardware platforms (such as the Unleashed board).

My goal is that this platform will set default config options (such as
the default defconfig for the kernel) based on the platform value.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 arch/Config.in.riscv | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/Config.in.riscv b/arch/Config.in.riscv
index 097719e846..c97fa5b703 100644
--- a/arch/Config.in.riscv
+++ b/arch/Config.in.riscv
@@ -108,6 +108,22 @@ config BR2_RISCV_ABI_LP64D
 	depends on BR2_ARCH_IS_64 && BR2_RISCV_ISA_RVD
 endchoice
 
+choice
+	prompt "Target Platform"
+	default BR2_RISCV_QEMU_VIRT
+	help
+	  The RISC-V Platform to target.
+
+config BR2_RISCV_QEMU_VIRT
+	bool "qemu/virt"
+
+config BR2_RISCV_QEMU_SIFIVE_U
+	bool "qemu/sifive_u"
+
+config BR2_RISCV_UNKNOWN
+	bool "unknown"
+endchoice
+
 config BR2_ARCH
 	default "riscv32" if !BR2_ARCH_IS_64
 	default "riscv64" if BR2_ARCH_IS_64
-- 
2.21.0

[Buildroot] [PATCH 2/3] boot: riscv: Initial commit of OpenSBI

[Alistair Francis]

OpenSBI is a much improved alternative to BLL (riscv-pk). Add OpenSBI
support to buildroot.

OpenSBI support uses the new RISC-V "Target Platform" config to build
the specified platform.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 DEVELOPERS              |  1 +
 boot/Config.in          |  1 +
 boot/opensbi/Config.in  | 25 +++++++++++++++++++++++++
 boot/opensbi/opensbi.mk | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+)
 create mode 100644 boot/opensbi/Config.in
 create mode 100644 boot/opensbi/opensbi.mk

diff --git a/DEVELOPERS b/DEVELOPERS
index 8a57cb2e23..a8978856ad 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -1383,6 +1383,7 @@ F:	arch/arch.mk.riscv
 F:	arch/Config.in.riscv
 F:	board/qemu/riscv32-virt/
 F:	board/qemu/riscv64-virt/
+F:	boot/opensbi/
 F:	boot/riscv-pk/
 F:	configs/qemu_riscv32_virt_defconfig
 F:	configs/qemu_riscv64_virt_defconfig
diff --git a/boot/Config.in b/boot/Config.in
index 74481e7545..97bd3de6e9 100644
--- a/boot/Config.in
+++ b/boot/Config.in
@@ -14,6 +14,7 @@ source "boot/lpc32xxcdl/Config.in"
 source "boot/mv-ddr-marvell/Config.in"
 source "boot/mxs-bootlets/Config.in"
 source "boot/optee-os/Config.in"
+source "boot/opensbi/Config.in"
 source "boot/riscv-pk/Config.in"
 source "boot/s500-bootloader/Config.in"
 source "boot/shim/Config.in"
diff --git a/boot/opensbi/Config.in b/boot/opensbi/Config.in
new file mode 100644
index 0000000000..01ee342215
--- /dev/null
+++ b/boot/opensbi/Config.in
@@ -0,0 +1,25 @@
+config BR2_TARGET_OPENSBI
+	bool "OpenSBI"
+	depends on BR2_riscv
+	help
+	  OpenSBI aims to provide an open-source and extensible
+	  implementation of the RISC-V SBI specification for a platform
+	  specific firmware (M-mode) and a general purpose OS, hypervisor
+	  or bootloader (S-mode or HS-mode). OpenSBI implementation can
+	  be easily extended by RISC-V platform or System-on-Chip vendors
+	  to fit a particular hadware configuration.
+
+	  https://github.com/riscv/opensbi.git
+
+if BR2_TARGET_OPENSBI
+config BR2_TARGET_OPENSBI_USE_PLAT
+	bool "Build OpenSBI for Platform"
+	depends on BR2_TARGET_OPENSBI
+
+config BR2_TARGET_OPENSBI_PLAT
+	string "OpenSBI Platform"
+	depends on BR2_TARGET_OPENSBI_USE_PLAT
+	default "qemu/virt" if BR2_RISCV_QEMU_VIRT
+	default "qemu/sifive_u" if BR2_RISCV_QEMU_SIFIVE_U
+	default ""
+endif
diff --git a/boot/opensbi/opensbi.mk b/boot/opensbi/opensbi.mk
new file mode 100644
index 0000000000..9da4e7f44e
--- /dev/null
+++ b/boot/opensbi/opensbi.mk
@@ -0,0 +1,32 @@
+################################################################################
+#
+# OpenSBI
+#
+################################################################################
+
+OPENSBI_VERSION = ca20ac0cd4c099006d4eea4d9ac7bd7b58e2ae0f
+OPENSBI_SITE = git://github.com/riscv/opensbi.git
+OPENSBI_LICENSE = BSD-2-Clause
+OPENSBI_LICENSE_FILES = COPYING.BSD
+OPENSBI_INSTALL_IMAGES = YES
+
+OPENSBI_MAKE_ENV = \
+	CROSS_COMPILE=$(TARGET_CROSS)
+
+ifeq ($(BR2_TARGET_OPENSBI_USE_PLAT),y)
+	OPENSBI_PLAT = $(call qstrip,$(BR2_TARGET_UBOOT_BOARD_DEFCONFIG))
+	OPENSBI_MAKE_ENV += PLATFORM=$(OPENSBI_PLAT)
+endif
+
+define OPENSBI_BUILD_CMDS
+	$(TARGET_MAKE_ENV) $(OPENSBI_MAKE_ENV) $(MAKE) -C $(@D)
+endef
+
+ifeq ($(BR2_TARGET_OPENSBI_USE_PLAT),y)
+define OPENSBI_INSTALL_IMAGES_CMDS
+	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_jump.bin $(BINARIES_DIR)/fw_jump.bin
+	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_jump.elf $(BINARIES_DIR)/fw_jump.elf
+endef
+endif
+
+$(eval $(generic-package))
-- 
2.21.0

[Buildroot] [PATCH 3/3] boot: riscv: Remove riscv-pk

[Alistair Francis]

Now that OpenSBI is supported remove BBL (riscv-pk).

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 DEVELOPERS                |  1 -
 boot/Config.in            |  1 -
 boot/riscv-pk/Config.in   | 14 --------------
 boot/riscv-pk/riscv-pk.mk | 32 --------------------------------
 4 files changed, 48 deletions(-)
 delete mode 100644 boot/riscv-pk/Config.in
 delete mode 100644 boot/riscv-pk/riscv-pk.mk

diff --git a/DEVELOPERS b/DEVELOPERS
index a8978856ad..246bb8b471 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -1384,7 +1384,6 @@ F:	arch/Config.in.riscv
 F:	board/qemu/riscv32-virt/
 F:	board/qemu/riscv64-virt/
 F:	boot/opensbi/
-F:	boot/riscv-pk/
 F:	configs/qemu_riscv32_virt_defconfig
 F:	configs/qemu_riscv64_virt_defconfig
 
diff --git a/boot/Config.in b/boot/Config.in
index 97bd3de6e9..228868de8c 100644
--- a/boot/Config.in
+++ b/boot/Config.in
@@ -15,7 +15,6 @@ source "boot/mv-ddr-marvell/Config.in"
 source "boot/mxs-bootlets/Config.in"
 source "boot/optee-os/Config.in"
 source "boot/opensbi/Config.in"
-source "boot/riscv-pk/Config.in"
 source "boot/s500-bootloader/Config.in"
 source "boot/shim/Config.in"
 source "boot/syslinux/Config.in"
diff --git a/boot/riscv-pk/Config.in b/boot/riscv-pk/Config.in
deleted file mode 100644
index b4fe36590f..0000000000
--- a/boot/riscv-pk/Config.in
+++ /dev/null
@@ -1,14 +0,0 @@
-comment "riscv-pk needs a Linux kernel to be built"
-	depends on BR2_riscv
-	depends on !BR2_LINUX_KERNEL
-
-config BR2_TARGET_RISCV_PK
-	bool "riscv-pk"
-	depends on BR2_riscv
-	depends on BR2_LINUX_KERNEL
-	help
-	  The RISC-V Proxy Kernel (pk) package contains the Berkeley
-	  Boot Loader (BBL) which has been designed to boot a Linux
-	  kernel on a RISC-V processor.
-
-	  https://github.com/riscv/riscv-pk.git
diff --git a/boot/riscv-pk/riscv-pk.mk b/boot/riscv-pk/riscv-pk.mk
deleted file mode 100644
index 0ab5879ee4..0000000000
--- a/boot/riscv-pk/riscv-pk.mk
+++ /dev/null
@@ -1,32 +0,0 @@
-################################################################################
-#
-# riscv-pk
-#
-################################################################################
-
-RISCV_PK_VERSION = 706cc77c369fd3e4734b5a6aa813d421347f1814
-RISCV_PK_SITE = git://github.com/riscv/riscv-pk.git
-RISCV_PK_LICENSE = BSD-3-Clause
-RISCV_PK_LICENSE_FILES = LICENSE
-RISCV_PK_DEPENDENCIES = linux
-RISCV_PK_SUBDIR = build
-RISCV_PK_INSTALL_IMAGES = YES
-
-define RISCV_PK_CONFIGURE_CMDS
-	mkdir -p $(@D)/build
-	(cd $(@D)/build; \
-		$(TARGET_CONFIGURE_OPTS) ../configure \
-		--host=$(GNU_TARGET_NAME) \
-		--with-payload=$(BINARIES_DIR)/vmlinux \
-	)
-endef
-
-define RISCV_PK_BUILD_CMDS
-	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)/build bbl
-endef
-
-define RISCV_PK_INSTALL_IMAGES_CMDS
-	$(INSTALL) -D -m 0755 $(@D)/build/bbl $(BINARIES_DIR)/bbl
-endef
-
-$(eval $(generic-package))
-- 
2.21.0

[Buildroot] [PATCH 1/3] arch: riscv: Add a RISC-V Platform option

[Thomas Petazzoni]

Hello Alistair,

Thanks for your contribution.

On Fri, 15 Mar 2019 23:06:38 +0000
Alistair Francis <Alistair.Francis@wdc.com> wrote:

> Add a RISC-V Platform option. At the moment only QEMU platforms are
> supported, but in the future this can be extended to include real
> hardware platforms (such as the Unleashed board).
> 
> My goal is that this platform will set default config options (such as
> the default defconfig for the kernel) based on the platform value.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

We never describe each and every board using Buildroot options. If we
were to do that, we would have gazillion of options for the thousands
of boards that exist.

So, we are not going to accept this patch. What we do however is add a
number of defconfigs for well known platforms, just like:

  configs/qemu_riscv32_virt_defconfig
  configs/qemu_riscv64_virt_defconfig

Best regards,

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

[Buildroot] [PATCH 2/3] boot: riscv: Initial commit of OpenSBI

[Thomas Petazzoni]

Hello,

On Fri, 15 Mar 2019 23:06:38 +0000
Alistair Francis <Alistair.Francis@wdc.com> wrote:

> index 8a57cb2e23..a8978856ad 100644
> --- a/DEVELOPERS
> +++ b/DEVELOPERS
> @@ -1383,6 +1383,7 @@ F:	arch/arch.mk.riscv
>  F:	arch/Config.in.riscv
>  F:	board/qemu/riscv32-virt/
>  F:	board/qemu/riscv64-virt/
> +F:	boot/opensbi/

Why is this added under the name of Mark Corbin, and not yours? Is Mark
fine with this? Why don't you add opensbi under your entry in the
DEVELOPERS file?


> diff --git a/boot/opensbi/Config.in b/boot/opensbi/Config.in
> new file mode 100644
> index 0000000000..01ee342215
> --- /dev/null
> +++ b/boot/opensbi/Config.in
> @@ -0,0 +1,25 @@
> +config BR2_TARGET_OPENSBI
> +	bool "OpenSBI"

All lower-case "opensbi".

> +	depends on BR2_riscv
> +	help
> +	  OpenSBI aims to provide an open-source and extensible
> +	  implementation of the RISC-V SBI specification for a platform
> +	  specific firmware (M-mode) and a general purpose OS, hypervisor
> +	  or bootloader (S-mode or HS-mode). OpenSBI implementation can
> +	  be easily extended by RISC-V platform or System-on-Chip vendors
> +	  to fit a particular hadware configuration.
> +
> +	  https://github.com/riscv/opensbi.git
> +
> +if BR2_TARGET_OPENSBI
> +config BR2_TARGET_OPENSBI_USE_PLAT
> +	bool "Build OpenSBI for Platform"
> +	depends on BR2_TARGET_OPENSBI

This "depends on" is not needed, you are already inside a if
BR2_TARGET_OPENSBI...endif block.

But this option is not needed: you can simply decide whether you want
to do a "platform" build depending on whether the
BR2_TARGET_OPENSBI_PLAT option is empty or not.

> +config BR2_TARGET_OPENSBI_PLAT
> +	string "OpenSBI Platform"
> +	depends on BR2_TARGET_OPENSBI_USE_PLAT
> +	default "qemu/virt" if BR2_RISCV_QEMU_VIRT
> +	default "qemu/sifive_u" if BR2_RISCV_QEMU_SIFIVE_U

Instead of this, you should simply make it a string option, and it's
the responsibility of the user to fill it in with the right value.
Example defconfigs can help users for well-known platforms. That's how
we do things for Linux, U-Boot, and all other platform-specific
components.

> diff --git a/boot/opensbi/opensbi.mk b/boot/opensbi/opensbi.mk
> new file mode 100644
> index 0000000000..9da4e7f44e
> --- /dev/null
> +++ b/boot/opensbi/opensbi.mk
> @@ -0,0 +1,32 @@
> +################################################################################
> +#
> +# OpenSBI
> +#
> +################################################################################
> +
> +OPENSBI_VERSION = ca20ac0cd4c099006d4eea4d9ac7bd7b58e2ae0f
> +OPENSBI_SITE = git://github.com/riscv/opensbi.git
> +OPENSBI_LICENSE = BSD-2-Clause
> +OPENSBI_LICENSE_FILES = COPYING.BSD
> +OPENSBI_INSTALL_IMAGES = YES
> +
> +OPENSBI_MAKE_ENV = \
> +	CROSS_COMPILE=$(TARGET_CROSS)
> +
> +ifeq ($(BR2_TARGET_OPENSBI_USE_PLAT),y)
> +	OPENSBI_PLAT = $(call qstrip,$(BR2_TARGET_UBOOT_BOARD_DEFCONFIG))

So you're re-using the name of the U-Boot defconfig ? Aren't you
supposed to use BR2_TARGET_OPENSBI_PLAT here ?

> +	OPENSBI_MAKE_ENV += PLATFORM=$(OPENSBI_PLAT)
> +endif

I believe this block of code should instead be:

OPENSBI_PLAT = $(call qstrip,$(BR2_TARGET_OPENSBI_PLAT))
ifneq ($(OPENSBI_PLAT),)
OPENSBI_MAKE_ENV += PLATFORM=$(OPENSBI_PLAT)
endif

> +define OPENSBI_BUILD_CMDS
> +	$(TARGET_MAKE_ENV) $(OPENSBI_MAKE_ENV) $(MAKE) -C $(@D)
> +endef
> +
> +ifeq ($(BR2_TARGET_OPENSBI_USE_PLAT),y)

So when there's no platform defined, nothing gets installed ? This
looks weird.

I'm not sure how much it makes sense for a build without a platform
defined. Shouldn't we do like U-Boot/Linux and simply disallow building
with an undefined platform/configuration ?

Thanks,

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

[Buildroot] [PATCH 3/3] boot: riscv: Remove riscv-pk

[Thomas Petazzoni]

Hello,

On Fri, 15 Mar 2019 23:06:39 +0000
Alistair Francis <Alistair.Francis@wdc.com> wrote:

> Now that OpenSBI is supported remove BBL (riscv-pk).
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

This commit is not enough: you do not update the
qemu_riscv64_virt_defconfig and qemu_riscv32_virt_defconfig, which use
riscv-pk. They should be converted to use opensbi instead, and of
course be runtime tested under Qemu.

And of course, it'd be nice if Mark Corbin, who added riscv-pk, could
give his opinion about the replacement of riscv-pk by opensbi.

Thanks!

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

[Buildroot] [PATCH 3/3] boot: riscv: Remove riscv-pk

[Mark Corbin]

Hello

On 16/03/2019 07:43, Thomas Petazzoni wrote:
> Hello,
>
> On Fri, 15 Mar 2019 23:06:39 +0000
> Alistair Francis <Alistair.Francis@wdc.com> wrote:
>
>> Now that OpenSBI is supported remove BBL (riscv-pk).
>>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> This commit is not enough: you do not update the
> qemu_riscv64_virt_defconfig and qemu_riscv32_virt_defconfig, which use
> riscv-pk. They should be converted to use opensbi instead, and of
> course be runtime tested under Qemu.
>
> And of course, it'd be nice if Mark Corbin, who added riscv-pk, could
> give his opinion about the replacement of riscv-pk by opensbi.

I don't think that removing support for riscv-pk at this stage is
desirable. I imagine that the majority (if not all) RISC-V systems have
been using riscv-pk (BBL) up until this point, including the HiFive
Unleashed board, so I expect that there are a lot of systems and people
with time invested in using it. That said, I always imagined a migration
path from riscv-pk to an alternative such as U-Boot (or OpenSBI or
whatever).

As the first release of OpenSBI was only at the end of January 2019, I
would strongly suggest that we keep riscv-pk for a period of time
(possibly commenting the intention to eventually deprecate it in favour
of OpenSBI). This will give OpenSBI time to mature and users of riscv-pk
a period to evaluate it and consider migrating.

I do think that it would be a good start to test the existing QEMU virt
configurations with OpenSBI and then update them as necessary.

Regards

Mark

>
> Thanks!
>
> Thomas

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

[Buildroot] [PATCH 3/3] boot: riscv: Remove riscv-pk

[Alistair Francis]

On Sat, Mar 16, 2019 at 12:43 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Fri, 15 Mar 2019 23:06:39 +0000
> Alistair Francis <Alistair.Francis@wdc.com> wrote:
>
> > Now that OpenSBI is supported remove BBL (riscv-pk).
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>
> This commit is not enough: you do not update the
> qemu_riscv64_virt_defconfig and qemu_riscv32_virt_defconfig, which use
> riscv-pk. They should be converted to use opensbi instead, and of
> course be runtime tested under Qemu.

Ah ok. I didn't think about the defconfigs. I'll update those as well
as the instructions.

Alistair

>
> And of course, it'd be nice if Mark Corbin, who added riscv-pk, could
> give his opinion about the replacement of riscv-pk by opensbi.
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

[Buildroot] [PATCH 3/3] boot: riscv: Remove riscv-pk

[Alistair Francis]

On Mon, Mar 18, 2019 at 3:45 AM Mark Corbin <mark.corbin@embecosm.com> wrote:
>
> Hello
>
> On 16/03/2019 07:43, Thomas Petazzoni wrote:
> > Hello,
> >
> > On Fri, 15 Mar 2019 23:06:39 +0000
> > Alistair Francis <Alistair.Francis@wdc.com> wrote:
> >
> >> Now that OpenSBI is supported remove BBL (riscv-pk).
> >>
> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > This commit is not enough: you do not update the
> > qemu_riscv64_virt_defconfig and qemu_riscv32_virt_defconfig, which use
> > riscv-pk. They should be converted to use opensbi instead, and of
> > course be runtime tested under Qemu.
> >
> > And of course, it'd be nice if Mark Corbin, who added riscv-pk, could
> > give his opinion about the replacement of riscv-pk by opensbi.
>
> I don't think that removing support for riscv-pk at this stage is
> desirable. I imagine that the majority (if not all) RISC-V systems have
> been using riscv-pk (BBL) up until this point, including the HiFive
> Unleashed board, so I expect that there are a lot of systems and people
> with time invested in using it. That said, I always imagined a migration
> path from riscv-pk to an alternative such as U-Boot (or OpenSBI or
> whatever).

I agree that up until OpenSBI almost all boards would have been using
BBL. The problem with BBL is that people were never investing time
into it. There was never a formal release and the code base is
difficult to maintain. I think that the sooner we can replace it the
better. Yocto/OpenEmbedded has fully swapped and Fedora is looking at
swapping as well.

OpenSBI allows us to not compile the kernel into the boot loader when
booting with QEMU. It also allows us to boot directly into u-boot on
QEMU and on hardware (with u-boot 2019.04). I think leaving it in
confuses people who might continue to use it as that is what they have
seen before, even though it is a bad choice.

>
> As the first release of OpenSBI was only at the end of January 2019, I
> would strongly suggest that we keep riscv-pk for a period of time
> (possibly commenting the intention to eventually deprecate it in favour
> of OpenSBI). This will give OpenSBI time to mature and users of riscv-pk
> a period to evaluate it and consider migrating.
>
> I do think that it would be a good start to test the existing QEMU virt
> configurations with OpenSBI and then update them as necessary.

Yep, I'll update the configurations in v2 as well as the
documentation. OpenSBI is well tested on QEMU in general.

Alistair

>
> Regards
>
> Mark
>
> >
> > Thanks!
> >
> > Thomas
>
> --
> Mark Corbin
> Embecosm Ltd.
> https://www.embecosm.com

[Buildroot] [PATCH 1/3] arch: riscv: Add a RISC-V Platform option

[Alistair Francis]

On Sat, Mar 16, 2019 at 12:36 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Alistair,
>
> Thanks for your contribution.
>
> On Fri, 15 Mar 2019 23:06:38 +0000
> Alistair Francis <Alistair.Francis@wdc.com> wrote:
>
> > Add a RISC-V Platform option. At the moment only QEMU platforms are
> > supported, but in the future this can be extended to include real
> > hardware platforms (such as the Unleashed board).
> >
> > My goal is that this platform will set default config options (such as
> > the default defconfig for the kernel) based on the platform value.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>
> We never describe each and every board using Buildroot options. If we
> were to do that, we would have gazillion of options for the thousands
> of boards that exist.

Yep, that makes sense.

>
> So, we are not going to accept this patch. What we do however is add a
> number of defconfigs for well known platforms, just like:
>
>   configs/qemu_riscv32_virt_defconfig
>   configs/qemu_riscv64_virt_defconfig

Update in v2.

Alistair

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

[Buildroot] [PATCH 2/3] boot: riscv: Initial commit of OpenSBI

[Alistair Francis]

On Sat, Mar 16, 2019 at 12:41 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Fri, 15 Mar 2019 23:06:38 +0000
> Alistair Francis <Alistair.Francis@wdc.com> wrote:
>
> > index 8a57cb2e23..a8978856ad 100644
> > --- a/DEVELOPERS
> > +++ b/DEVELOPERS
> > @@ -1383,6 +1383,7 @@ F:      arch/arch.mk.riscv
> >  F:   arch/Config.in.riscv
> >  F:   board/qemu/riscv32-virt/
> >  F:   board/qemu/riscv64-virt/
> > +F:   boot/opensbi/
>
> Why is this added under the name of Mark Corbin, and not yours? Is Mark
> fine with this? Why don't you add opensbi under your entry in the
> DEVELOPERS file?

I was just keeping all the RISC-V ones together, but you are right.
I'll add it to mine.

>
>
> > diff --git a/boot/opensbi/Config.in b/boot/opensbi/Config.in
> > new file mode 100644
> > index 0000000000..01ee342215
> > --- /dev/null
> > +++ b/boot/opensbi/Config.in
> > @@ -0,0 +1,25 @@
> > +config BR2_TARGET_OPENSBI
> > +     bool "OpenSBI"
>
> All lower-case "opensbi".

Fixed

>
> > +     depends on BR2_riscv
> > +     help
> > +       OpenSBI aims to provide an open-source and extensible
> > +       implementation of the RISC-V SBI specification for a platform
> > +       specific firmware (M-mode) and a general purpose OS, hypervisor
> > +       or bootloader (S-mode or HS-mode). OpenSBI implementation can
> > +       be easily extended by RISC-V platform or System-on-Chip vendors
> > +       to fit a particular hadware configuration.
> > +
> > +       https://github.com/riscv/opensbi.git
> > +
> > +if BR2_TARGET_OPENSBI
> > +config BR2_TARGET_OPENSBI_USE_PLAT
> > +     bool "Build OpenSBI for Platform"
> > +     depends on BR2_TARGET_OPENSBI
>
> This "depends on" is not needed, you are already inside a if
> BR2_TARGET_OPENSBI...endif block.
>
> But this option is not needed: you can simply decide whether you want
> to do a "platform" build depending on whether the
> BR2_TARGET_OPENSBI_PLAT option is empty or not.

Ok, fixed.

>
> > +config BR2_TARGET_OPENSBI_PLAT
> > +     string "OpenSBI Platform"
> > +     depends on BR2_TARGET_OPENSBI_USE_PLAT
> > +     default "qemu/virt" if BR2_RISCV_QEMU_VIRT
> > +     default "qemu/sifive_u" if BR2_RISCV_QEMU_SIFIVE_U
>
> Instead of this, you should simply make it a string option, and it's
> the responsibility of the user to fill it in with the right value.
> Example defconfigs can help users for well-known platforms. That's how
> we do things for Linux, U-Boot, and all other platform-specific
> components.

Fixed as well.

>
> > diff --git a/boot/opensbi/opensbi.mk b/boot/opensbi/opensbi.mk
> > new file mode 100644
> > index 0000000000..9da4e7f44e
> > --- /dev/null
> > +++ b/boot/opensbi/opensbi.mk
> > @@ -0,0 +1,32 @@
> > +################################################################################
> > +#
> > +# OpenSBI
> > +#
> > +################################################################################
> > +
> > +OPENSBI_VERSION = ca20ac0cd4c099006d4eea4d9ac7bd7b58e2ae0f
> > +OPENSBI_SITE = git://github.com/riscv/opensbi.git
> > +OPENSBI_LICENSE = BSD-2-Clause
> > +OPENSBI_LICENSE_FILES = COPYING.BSD
> > +OPENSBI_INSTALL_IMAGES = YES
> > +
> > +OPENSBI_MAKE_ENV = \
> > +     CROSS_COMPILE=$(TARGET_CROSS)
> > +
> > +ifeq ($(BR2_TARGET_OPENSBI_USE_PLAT),y)
> > +     OPENSBI_PLAT = $(call qstrip,$(BR2_TARGET_UBOOT_BOARD_DEFCONFIG))
>
> So you're re-using the name of the U-Boot defconfig ? Aren't you
> supposed to use BR2_TARGET_OPENSBI_PLAT here ?

This was a typo.

>
> > +     OPENSBI_MAKE_ENV += PLATFORM=$(OPENSBI_PLAT)
> > +endif
>
> I believe this block of code should instead be:
>
> OPENSBI_PLAT = $(call qstrip,$(BR2_TARGET_OPENSBI_PLAT))
> ifneq ($(OPENSBI_PLAT),)
> OPENSBI_MAKE_ENV += PLATFORM=$(OPENSBI_PLAT)
> endif

Looks good to me, I'll use this.

>
> > +define OPENSBI_BUILD_CMDS
> > +     $(TARGET_MAKE_ENV) $(OPENSBI_MAKE_ENV) $(MAKE) -C $(@D)
> > +endef
> > +
> > +ifeq ($(BR2_TARGET_OPENSBI_USE_PLAT),y)
>
> So when there's no platform defined, nothing gets installed ? This
> looks weird.

If no platform is defined then OpenSBI will just build a library that
can be used by other projects. The idea here is to allow people to
build that library. At the moment though nothing is using it.

>
> I'm not sure how much it makes sense for a build without a platform
> defined. Shouldn't we do like U-Boot/Linux and simply disallow building
> with an undefined platform/configuration ?

I think it does make sense to allow building without a platform, but I
can disable that if required.

Alistair

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

[Buildroot] [PATCH 2/3] boot: riscv: Initial commit of OpenSBI

[Thomas Petazzoni]

Hello,

On Mon, 18 Mar 2019 13:36:53 -0700
Alistair Francis <alistair23@gmail.com> wrote:

> > So when there's no platform defined, nothing gets installed ? This
> > looks weird.  
> 
> If no platform is defined then OpenSBI will just build a library that
> can be used by other projects. The idea here is to allow people to
> build that library. At the moment though nothing is using it.

OK, then you can probably keep the "build without platform" thing, but
the Config.in help text of the string option used to define the
platform should be improved to explain what happens when no platform is
defined.

Thanks!

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

[Buildroot] [PATCH 2/3] boot: riscv: Initial commit of OpenSBI

[Alistair Francis]

On Mon, Mar 18, 2019 at 1:46 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Mon, 18 Mar 2019 13:36:53 -0700
> Alistair Francis <alistair23@gmail.com> wrote:
>
> > > So when there's no platform defined, nothing gets installed ? This
> > > looks weird.
> >
> > If no platform is defined then OpenSBI will just build a library that
> > can be used by other projects. The idea here is to allow people to
> > build that library. At the moment though nothing is using it.
>
> OK, then you can probably keep the "build without platform" thing, but
> the Config.in help text of the string option used to define the
> platform should be improved to explain what happens when no platform is
> defined.

Done! V2 sent.

Alistair

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

[Buildroot] [PATCH 3/3] boot: riscv: Remove riscv-pk

[Mark Corbin]

Hello Alistair

On 18/03/2019 17:51, Alistair Francis wrote:
> On Mon, Mar 18, 2019 at 3:45 AM Mark Corbin <mark.corbin@embecosm.com> wrote:
>> Hello
>>
>> On 16/03/2019 07:43, Thomas Petazzoni wrote:
>>> Hello,
>>>
>>> On Fri, 15 Mar 2019 23:06:39 +0000
>>> Alistair Francis <Alistair.Francis@wdc.com> wrote:
>>>
>>>> Now that OpenSBI is supported remove BBL (riscv-pk).
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>> This commit is not enough: you do not update the
>>> qemu_riscv64_virt_defconfig and qemu_riscv32_virt_defconfig, which use
>>> riscv-pk. They should be converted to use opensbi instead, and of
>>> course be runtime tested under Qemu.
>>>
>>> And of course, it'd be nice if Mark Corbin, who added riscv-pk, could
>>> give his opinion about the replacement of riscv-pk by opensbi.
>> I don't think that removing support for riscv-pk at this stage is
>> desirable. I imagine that the majority (if not all) RISC-V systems have
>> been using riscv-pk (BBL) up until this point, including the HiFive
>> Unleashed board, so I expect that there are a lot of systems and people
>> with time invested in using it. That said, I always imagined a migration
>> path from riscv-pk to an alternative such as U-Boot (or OpenSBI or
>> whatever).
> I agree that up until OpenSBI almost all boards would have been using
> BBL. The problem with BBL is that people were never investing time
> into it. There was never a formal release and the code base is
> difficult to maintain. I think that the sooner we can replace it the
> better. Yocto/OpenEmbedded has fully swapped and Fedora is looking at
> swapping as well.
I'd forgotten that riscv-pk support is of course in the current LTS
release for those who want it...so I think that it does make sense to
drop it at this point.

Regards

Mark

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