| Message ID | 20190320003706.1126-2-alistair.francis@wdc.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v5,1/4] board/qemu/riscv32-virt: Update linux config | expand |
Hello Francis, You are respining the patches too fast, without answering some of the questions, and therefore there's still an issue, see below. On Wed, 20 Mar 2019 00:38:05 +0000 Alistair Francis <Alistair.Francis@wdc.com> wrote: > +ifneq ($(OPENSBI_PLAT),) > +OPENSBI_PLAT_INSTALL = \ > + $(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 > +endif > + > +# Install libsbi.a in the host lib dir so it can be linked in the future. > +define OPENSBI_INSTALL_IMAGES_CMDS > + $(OPENSBI_PLAT_INSTALL) > + $(INSTALL) -m 0644 -D $(@D)/build/lib/libsbi.a $(HOST_DIR)/usr/lib I suppose that libsbi.a contains code cross-compiled for RISC-V, so installing it in $(HOST_DIR) is wrong in this case. It should go in $(STAGING_DIR). The second question, which I asked in my review of your v4 is whether this library is meant to be linked into bare-metal code (which is what I would expect for something related to firmware) or to Linux user-space applications. In the latter case, installing to $(STAGING_DIR)/usr/lib is the right thing to do. In the former case however, it should probably be installed elsewhere, maybe $(STAGING_DIR)/usr/share/opensbi (but I'm not a FHS expert). Also, when -D is used, the destination path should include the filename. Best regards, Thomas
On Wed, Mar 20, 2019 at 2:12 PM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello Francis, > > You are respining the patches too fast, without answering some of the > questions, and therefore there's still an issue, see below. Sorry, I didn't realise I didn't respond to a question. > > On Wed, 20 Mar 2019 00:38:05 +0000 > Alistair Francis <Alistair.Francis@wdc.com> wrote: > > > +ifneq ($(OPENSBI_PLAT),) > > +OPENSBI_PLAT_INSTALL = \ > > + $(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 > > +endif > > + > > +# Install libsbi.a in the host lib dir so it can be linked in the future. > > +define OPENSBI_INSTALL_IMAGES_CMDS > > + $(OPENSBI_PLAT_INSTALL) > > + $(INSTALL) -m 0644 -D $(@D)/build/lib/libsbi.a $(HOST_DIR)/usr/lib > > I suppose that libsbi.a contains code cross-compiled for RISC-V, so > installing it in $(HOST_DIR) is wrong in this case. It should go in > $(STAGING_DIR). Yes, it does contain cross compiled code. I have changed it to the STAGING_DIR. > > The second question, which I asked in my review of your v4 is whether > this library is meant to be linked into bare-metal code (which is what Sorry, I missed your question. It is meant to be linked into bare metal code. > I would expect for something related to firmware) or to Linux > user-space applications. In the latter case, installing to > $(STAGING_DIR)/usr/lib is the right thing to do. In the former case > however, it should probably be installed elsewhere, maybe > $(STAGING_DIR)/usr/share/opensbi (but I'm not a FHS expert). $(STAGING_DIR)/usr/share/opensbi looks good to me. I will use that. > > Also, when -D is used, the destination path should include the filename. Ok, fixed. Alistair > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hello, On Wed, 20 Mar 2019 14:28:59 -0700 Alistair Francis <alistair23@gmail.com> wrote: > > I would expect for something related to firmware) or to Linux > > user-space applications. In the latter case, installing to > > $(STAGING_DIR)/usr/lib is the right thing to do. In the former case > > however, it should probably be installed elsewhere, maybe > > $(STAGING_DIR)/usr/share/opensbi (but I'm not a FHS expert). > > $(STAGING_DIR)/usr/share/opensbi looks good to me. I will use that. Then it should go to a OPENSBI_INSTALL_STAGING_CMDS variable, not in OPENSBI_INSTALL_IMAGES_CMDS. Thanks! Thomas
Hello Thomas On 20/03/2019 21:12, Thomas Petazzoni wrote: > Hello Francis, > > You are respining the patches too fast, without answering some of the > questions, and therefore there's still an issue, see below. Just a heads-up that things have got a bit out of sync for RISC-V architecture on the master branch... a) commit 'allow BR2_LINUX_KERNEL_IMAGE on RISC-V' (a3a4d4d4d307fd21f19ae43e77ac21b85adad7f2) is causing riscv builds to fail for the existing qemu defconfigs because OpenSBI hasn't been added yet and riscv-pk expects a 'vmlinux' file. b) commit 'configs/qemu_riscv32_virt: upgrade to 4.20 kernel' (e92d5624236e47e1abe44f2ace24fceb1882c0a5) results in a kernel that won't boot with QEMU due to lack of OpenSBI and/or required kernel configuration updates. Might be worth backing these out if the other updates are going to take a while? Regards Mark
Hello Mark, Thanks for the heads-up! On Thu, 21 Mar 2019 16:36:22 +0000 Mark Corbin <mark.corbin@embecosm.com> wrote: > a) commit 'allow BR2_LINUX_KERNEL_IMAGE on RISC-V' > (a3a4d4d4d307fd21f19ae43e77ac21b85adad7f2) is causing riscv builds to > fail for the existing qemu defconfigs because OpenSBI hasn't been added > yet and riscv-pk expects a 'vmlinux' file. This one should be fixed by adding: BR2_LINUX_KERNEL_VMLINUX=y in the defconfig. > b) commit 'configs/qemu_riscv32_virt: upgrade to 4.20 kernel' > (e92d5624236e47e1abe44f2ace24fceb1882c0a5) results in a kernel that > won't boot with QEMU due to lack of OpenSBI and/or required kernel > configuration updates. Which configuration updates are needed ? The ones that Alistair was pushing ? If they were need for the 4.20 bump to work, then they should have been part of the same commit :-/ Anyway, Alistair has been respinning his series very frequently, so I assume we can get the remainder of the work merged very soon. Best regards, Thomas
On Thu, Mar 21, 2019 at 6:57 AM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello Mark, > > Thanks for the heads-up! I am on holidays for the next few days so I won't be able to send any more patches until next week. > > On Thu, 21 Mar 2019 16:36:22 +0000 > Mark Corbin <mark.corbin@embecosm.com> wrote: > > > a) commit 'allow BR2_LINUX_KERNEL_IMAGE on RISC-V' > > (a3a4d4d4d307fd21f19ae43e77ac21b85adad7f2) is causing riscv builds to > > fail for the existing qemu defconfigs because OpenSBI hasn't been added > > yet and riscv-pk expects a 'vmlinux' file. > > This one should be fixed by adding: > > BR2_LINUX_KERNEL_VMLINUX=y > > in the defconfig. I didn't realise that enabling Image would change the default. > > > b) commit 'configs/qemu_riscv32_virt: upgrade to 4.20 kernel' > > (e92d5624236e47e1abe44f2ace24fceb1882c0a5) results in a kernel that > > won't boot with QEMU due to lack of OpenSBI and/or required kernel > > configuration updates. > > Which configuration updates are needed ? The ones that Alistair was > pushing ? If they were need for the 4.20 bump to work, then they should > have been part of the same commit :-/ Sorry about this. I tested them with a custom config that wasn't commuted in the same patch. > > Anyway, Alistair has been respinning his series very frequently, so I > assume we can get the remainder of the work merged very soon. Are there any comments on the latest series? Or can they be merged to fix this. Alistair > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/DEVELOPERS b/DEVELOPERS index 3e0ac08e11..91eda42949 100644 --- a/DEVELOPERS +++ b/DEVELOPERS @@ -122,6 +122,7 @@ F: package/kvazaar/ F: package/v4l2loopback/ N: Alistair Francis <alistair@alistair23.me> +F: boot/opensbi/ F: package/xen/ N: Alvaro G. M <alvaro.gamez@hazent.com> 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..5f3cc13312 --- /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_PLAT + string "OpenSBI Platform" + default "" + 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. +endif diff --git a/boot/opensbi/opensbi.mk b/boot/opensbi/opensbi.mk new file mode 100644 index 0000000000..341e2dcb19 --- /dev/null +++ b/boot/opensbi/opensbi.mk @@ -0,0 +1,37 @@ +################################################################################ +# +# opensbi +# +################################################################################ + +OPENSBI_VERSION = v0.3 +OPENSBI_SITE = $(call github,riscv,opensbi,$(OPENSBI_VERSION)) +OPENSBI_LICENSE = BSD-2-Clause +OPENSBI_LICENSE_FILES = COPYING.BSD +OPENSBI_INSTALL_IMAGES = YES + +OPENSBI_MAKE_ENV = \ + CROSS_COMPILE=$(TARGET_CROSS) + +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 + +ifneq ($(OPENSBI_PLAT),) +OPENSBI_PLAT_INSTALL = \ + $(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 +endif + +# Install libsbi.a in the host lib dir so it can be linked in the future. +define OPENSBI_INSTALL_IMAGES_CMDS + $(OPENSBI_PLAT_INSTALL) + $(INSTALL) -m 0644 -D $(@D)/build/lib/libsbi.a $(HOST_DIR)/usr/lib +endef + +$(eval $(generic-package))
OpenSBI is a much improved alternative to BBL (riscv-pk). Add OpenSBI support to buildroot. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- DEVELOPERS | 1 + boot/Config.in | 1 + boot/opensbi/Config.in | 25 +++++++++++++++++++++++++ boot/opensbi/opensbi.mk | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+) create mode 100644 boot/opensbi/Config.in create mode 100644 boot/opensbi/opensbi.mk