[2/3] boot: riscv: Initial commit of OpenSBI
diff mbox series

Message ID 20190315230540.4311-2-alistair.francis@wdc.com
State Changes Requested
Headers show
Series
  • Untitled series #97451
Related show

Commit Message

Alistair Francis March 15, 2019, 11:06 p.m. UTC
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

Comments

Thomas Petazzoni March 16, 2019, 7:41 a.m. UTC | #1
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
Alistair Francis March 18, 2019, 8:36 p.m. UTC | #2
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
Thomas Petazzoni March 18, 2019, 8:46 p.m. UTC | #3
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
Alistair Francis March 18, 2019, 9:02 p.m. UTC | #4
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

Patch
diff mbox series

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))