[v2,1/2] boot/opensbi: Add support for including Linux payload
diff mbox series

Message ID 20190722204447.7935-1-alistair.francis@wdc.com
State Accepted
Headers show
Series
  • [v2,1/2] boot/opensbi: Add support for including Linux payload
Related show

Commit Message

Alistair Francis July 22, 2019, 8:44 p.m. UTC
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 boot/opensbi/Config.in  | 10 ++++++++++
 boot/opensbi/opensbi.mk | 26 +++++++++++++++++++++-----
 2 files changed, 31 insertions(+), 5 deletions(-)

Comments

Yann E. MORIN July 28, 2019, 9:50 a.m. UTC | #1
Allistair, All,

On 2019-07-22 13:44 -0700, Alistair Francis spake thusly:
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
[--SNIP--]
> diff --git a/boot/opensbi/opensbi.mk b/boot/opensbi/opensbi.mk
> index 83552a5442..14657cac06 100644
> --- a/boot/opensbi/opensbi.mk
> +++ b/boot/opensbi/opensbi.mk
> @@ -19,19 +19,35 @@ ifneq ($(OPENSBI_PLAT),)
>  OPENSBI_MAKE_ENV += PLATFORM=$(OPENSBI_PLAT)
>  endif
>  
> +OPENSBI_LINUX_PAYLOAD = $(call qstrip,$(BR2_TARGET_OPENSBI_LINUX_PAYLOAD))
> +ifeq ($(OPENSBI_LINUX_PAYLOAD), y)
> +OPENSBI_DEPENDENCIES = linux
> +OPENSBI_MAKE_ENV += FW_PAYLOAD_PATH="$(BINARIES_DIR)/Image"
> +endif

In Kconfig, OPENSBI_LINUX_PAYLOAD is conditional to OPENSBI_PLAT != ""
so I would expect to see the same layout in the .mk (see below for what
I mean).

>  define OPENSBI_BUILD_CMDS
>  	$(TARGET_MAKE_ENV) $(OPENSBI_MAKE_ENV) $(MAKE) -C $(@D)
>  endef
>  
>  ifneq ($(OPENSBI_PLAT),)
>  OPENSBI_INSTALL_IMAGES = YES
> +OPENSBI_INSTALL_IMAGES_CMDS_PLAT = \
> +	$(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; \
> +	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.bin $(BINARIES_DIR)/fw_dynamic.bin; \
> +	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.elf $(BINARIES_DIR)/fw_dynamic.elf;
> +endif
> +
> +ifeq ($(OPENSBI_LINUX_PAYLOAD), y)
> +OPENSBI_INSTALL_IMAGES_CMDS_PAYLOAD = \
> +	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_payload.bin $(BINARIES_DIR)/fw_payload.bin; \
> +	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_payload.elf $(BINARIES_DIR)/fw_payload.elf;
> +endif

Why can't we have something like (untested):

    OPENSBI_FW_DIR = $(@D)/build/platform/$(OPENSBI_PLAT)/firmware

    ifneq ($(OPENSBI_PLAT),)
    OPENSBI_FW_FILES = fw_jump.bin fw_jump.elf fw_dynamic.bin fw_dynamic.elf
    ifeq ($(OPENSBI_LINUX_PAYLOAD),y)
    OPENSBI_FW_FILES += fw_jump.bin fw_jump.elf fw_dynamic.bin fw_dynamic.elf
    endif
    endif

    define OPENSBI_INSTALL_IMAGES_CMDS
        $(foreach fw,$(OPENSBI_FW_FILES), \
            $(INSTALL) -m 0644 -D $(OPENSBI_FW_DIR)/$(fw) $(BINARIES_DIR)/$(fw)
        )
    endef

(notice also how OPENSBI_LINUX_PAYLOAD is inside the OPENSBI_PLAT != ""
block, like it is in Config.in.)

Regards,
Yann E. MORIN.

>  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
> -	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.bin $(BINARIES_DIR)/fw_dynamic.bin
> -	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.elf $(BINARIES_DIR)/fw_dynamic.elf
> +	$(OPENSBI_INSTALL_IMAGES_CMDS_PLAT)
> +	$(OPENSBI_INSTALL_IMAGES_CMDS_PAYLOAD)
>  endef
> -endif
>  
>  # libsbi.a is not a library meant to be linked in user-space code, but
>  # with bare metal code, which is why we don't install it in
> -- 
> 2.22.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Thomas Petazzoni Aug. 3, 2019, 8:37 a.m. UTC | #2
Hello,

On Mon, 22 Jul 2019 13:44:46 -0700
Alistair Francis <alistair.francis@wdc.com> wrote:

> +if BR2_TARGET_OPENSBI_PLAT != ""

I move this if to a simple "depends on" under the
BR2_TARGET_OPENSBI_LINUX_PAYLOAD optio.

> +config BR2_TARGET_OPENSBI_LINUX_PAYLOAD
> +	bool "Include Linux as OpenSBI Payload"
> +	depends on BR2_LINUX_KERNEL
> +	depends on BR2_LINUX_KERNEL_IMAGE
> +	depends on !BR2_TARGET_OPENSBI_LIBRARY_ONLY

This option doesn't exist anywhere, so I dropped this depends on.

> +OPENSBI_LINUX_PAYLOAD = $(call qstrip,$(BR2_TARGET_OPENSBI_LINUX_PAYLOAD))

qstrip is only useful for string options. For boolean options, it's
useless, so I dropped this and simply used
BR2_TARGET_OPENSBI_LINUX_PAYLOAD everywhere.

> +ifeq ($(OPENSBI_LINUX_PAYLOAD), y)

No space before "y".

> +OPENSBI_DEPENDENCIES = linux

+= for this addition, so that we don't mistakenly overwrite any
previous dependency that might have been added.

> +OPENSBI_MAKE_ENV += FW_PAYLOAD_PATH="$(BINARIES_DIR)/Image"
> +endif
> +
>  define OPENSBI_BUILD_CMDS
>  	$(TARGET_MAKE_ENV) $(OPENSBI_MAKE_ENV) $(MAKE) -C $(@D)
>  endef
>  
>  ifneq ($(OPENSBI_PLAT),)
>  OPENSBI_INSTALL_IMAGES = YES
> +OPENSBI_INSTALL_IMAGES_CMDS_PLAT = \
> +	$(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; \
> +	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.bin $(BINARIES_DIR)/fw_dynamic.bin; \
> +	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.elf $(BINARIES_DIR)/fw_dynamic.elf;
> +endif
> +
> +ifeq ($(OPENSBI_LINUX_PAYLOAD), y)
> +OPENSBI_INSTALL_IMAGES_CMDS_PAYLOAD = \
> +	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_payload.bin $(BINARIES_DIR)/fw_payload.bin; \
> +	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_payload.elf $(BINARIES_DIR)/fw_payload.elf;
> +endif
> +
>  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
> -	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.bin $(BINARIES_DIR)/fw_dynamic.bin
> -	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.elf $(BINARIES_DIR)/fw_dynamic.elf
> +	$(OPENSBI_INSTALL_IMAGES_CMDS_PLAT)
> +	$(OPENSBI_INSTALL_IMAGES_CMDS_PAYLOAD)

As suggested by Yann, I've refactored all of that with a loop over a
OPENSBI_FW_IMAGES variable that contains jump, dynamic and optionally
payload.

I fixed up these different aspects, and merged the patch. See the final
commit at:

  https://git.buildroot.org/buildroot/commit/?id=9b5b7165deb91bad3cdb1ea0dd8f940eff75f3a9

Thanks!

Thomas
Alistair Francis Aug. 6, 2019, 12:09 a.m. UTC | #3
On Sat, Aug 3, 2019 at 1:37 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Mon, 22 Jul 2019 13:44:46 -0700
> Alistair Francis <alistair.francis@wdc.com> wrote:
>
> > +if BR2_TARGET_OPENSBI_PLAT != ""
>
> I move this if to a simple "depends on" under the
> BR2_TARGET_OPENSBI_LINUX_PAYLOAD optio.
>
> > +config BR2_TARGET_OPENSBI_LINUX_PAYLOAD
> > +     bool "Include Linux as OpenSBI Payload"
> > +     depends on BR2_LINUX_KERNEL
> > +     depends on BR2_LINUX_KERNEL_IMAGE
> > +     depends on !BR2_TARGET_OPENSBI_LIBRARY_ONLY
>
> This option doesn't exist anywhere, so I dropped this depends on.
>
> > +OPENSBI_LINUX_PAYLOAD = $(call qstrip,$(BR2_TARGET_OPENSBI_LINUX_PAYLOAD))
>
> qstrip is only useful for string options. For boolean options, it's
> useless, so I dropped this and simply used
> BR2_TARGET_OPENSBI_LINUX_PAYLOAD everywhere.
>
> > +ifeq ($(OPENSBI_LINUX_PAYLOAD), y)
>
> No space before "y".
>
> > +OPENSBI_DEPENDENCIES = linux
>
> += for this addition, so that we don't mistakenly overwrite any
> previous dependency that might have been added.
>
> > +OPENSBI_MAKE_ENV += FW_PAYLOAD_PATH="$(BINARIES_DIR)/Image"
> > +endif
> > +
> >  define OPENSBI_BUILD_CMDS
> >       $(TARGET_MAKE_ENV) $(OPENSBI_MAKE_ENV) $(MAKE) -C $(@D)
> >  endef
> >
> >  ifneq ($(OPENSBI_PLAT),)
> >  OPENSBI_INSTALL_IMAGES = YES
> > +OPENSBI_INSTALL_IMAGES_CMDS_PLAT = \
> > +     $(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; \
> > +     $(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.bin $(BINARIES_DIR)/fw_dynamic.bin; \
> > +     $(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.elf $(BINARIES_DIR)/fw_dynamic.elf;
> > +endif
> > +
> > +ifeq ($(OPENSBI_LINUX_PAYLOAD), y)
> > +OPENSBI_INSTALL_IMAGES_CMDS_PAYLOAD = \
> > +     $(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_payload.bin $(BINARIES_DIR)/fw_payload.bin; \
> > +     $(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_payload.elf $(BINARIES_DIR)/fw_payload.elf;
> > +endif
> > +
> >  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
> > -     $(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.bin $(BINARIES_DIR)/fw_dynamic.bin
> > -     $(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.elf $(BINARIES_DIR)/fw_dynamic.elf
> > +     $(OPENSBI_INSTALL_IMAGES_CMDS_PLAT)
> > +     $(OPENSBI_INSTALL_IMAGES_CMDS_PAYLOAD)
>
> As suggested by Yann, I've refactored all of that with a loop over a
> OPENSBI_FW_IMAGES variable that contains jump, dynamic and optionally
> payload.
>
> I fixed up these different aspects, and merged the patch. See the final
> commit at:
>
>   https://git.buildroot.org/buildroot/commit/?id=9b5b7165deb91bad3cdb1ea0dd8f940eff75f3a9

Thanks for that Thomas. Sorry I didn't update it myself, I was travelling.

Alistair

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

Patch
diff mbox series

diff --git a/boot/opensbi/Config.in b/boot/opensbi/Config.in
index 5f3cc13312..686aac8798 100644
--- a/boot/opensbi/Config.in
+++ b/boot/opensbi/Config.in
@@ -22,4 +22,14 @@  config BR2_TARGET_OPENSBI_PLAT
 	  library libsbi.a is built. If a platform is specified then
 	  the platform specific static library libplatsbi.a and firmware
 	  examples are built.
+
+if BR2_TARGET_OPENSBI_PLAT != ""
+config BR2_TARGET_OPENSBI_LINUX_PAYLOAD
+	bool "Include Linux as OpenSBI Payload"
+	depends on BR2_LINUX_KERNEL
+	depends on BR2_LINUX_KERNEL_IMAGE
+	depends on !BR2_TARGET_OPENSBI_LIBRARY_ONLY
+	help
+	  Build OpenSBI with the Linux kernel as a Payload.
+endif
 endif
diff --git a/boot/opensbi/opensbi.mk b/boot/opensbi/opensbi.mk
index 83552a5442..14657cac06 100644
--- a/boot/opensbi/opensbi.mk
+++ b/boot/opensbi/opensbi.mk
@@ -19,19 +19,35 @@  ifneq ($(OPENSBI_PLAT),)
 OPENSBI_MAKE_ENV += PLATFORM=$(OPENSBI_PLAT)
 endif
 
+OPENSBI_LINUX_PAYLOAD = $(call qstrip,$(BR2_TARGET_OPENSBI_LINUX_PAYLOAD))
+ifeq ($(OPENSBI_LINUX_PAYLOAD), y)
+OPENSBI_DEPENDENCIES = linux
+OPENSBI_MAKE_ENV += FW_PAYLOAD_PATH="$(BINARIES_DIR)/Image"
+endif
+
 define OPENSBI_BUILD_CMDS
 	$(TARGET_MAKE_ENV) $(OPENSBI_MAKE_ENV) $(MAKE) -C $(@D)
 endef
 
 ifneq ($(OPENSBI_PLAT),)
 OPENSBI_INSTALL_IMAGES = YES
+OPENSBI_INSTALL_IMAGES_CMDS_PLAT = \
+	$(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; \
+	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.bin $(BINARIES_DIR)/fw_dynamic.bin; \
+	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.elf $(BINARIES_DIR)/fw_dynamic.elf;
+endif
+
+ifeq ($(OPENSBI_LINUX_PAYLOAD), y)
+OPENSBI_INSTALL_IMAGES_CMDS_PAYLOAD = \
+	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_payload.bin $(BINARIES_DIR)/fw_payload.bin; \
+	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_payload.elf $(BINARIES_DIR)/fw_payload.elf;
+endif
+
 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
-	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.bin $(BINARIES_DIR)/fw_dynamic.bin
-	$(INSTALL) -m 0644 -D $(@D)/build/platform/$(OPENSBI_PLAT)/firmware/fw_dynamic.elf $(BINARIES_DIR)/fw_dynamic.elf
+	$(OPENSBI_INSTALL_IMAGES_CMDS_PLAT)
+	$(OPENSBI_INSTALL_IMAGES_CMDS_PAYLOAD)
 endef
-endif
 
 # libsbi.a is not a library meant to be linked in user-space code, but
 # with bare metal code, which is why we don't install it in