| Message ID | 20171112201646.15558-2-geomatsi@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for MacchiatoBin board | expand |
Hi Sergey, On Sun, Nov 12, 2017 at 11:16:42PM +0300, Sergey Matyukevich wrote: > This package adds SCP BL2 firmware for Marvell Armada 7040 and 8040 SoCs. > > Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com> > --- > boot/Config.in | 1 + > boot/armada-firmware/Config.in | 19 +++++++++++++++++++ > boot/armada-firmware/armada-firmware.hash | 2 ++ > boot/armada-firmware/armada-firmware.mk | 18 ++++++++++++++++++ > 4 files changed, 40 insertions(+) > create mode 100644 boot/armada-firmware/Config.in > create mode 100644 boot/armada-firmware/armada-firmware.hash > create mode 100644 boot/armada-firmware/armada-firmware.mk > > diff --git a/boot/Config.in b/boot/Config.in > index 2f46c8546e..0ffbd7288b 100644 > --- a/boot/Config.in > +++ b/boot/Config.in > @@ -17,5 +17,6 @@ source "boot/ts4800-mbrboot/Config.in" > source "boot/uboot/Config.in" > source "boot/vexpress-firmware/Config.in" > source "boot/xloader/Config.in" > +source "boot/armada-firmware/Config.in" > > endmenu > diff --git a/boot/armada-firmware/Config.in b/boot/armada-firmware/Config.in > new file mode 100644 > index 0000000000..530b5622c8 > --- /dev/null > +++ b/boot/armada-firmware/Config.in > @@ -0,0 +1,19 @@ > +config BR2_TARGET_ARMADA_FIRMWARE > + bool "armada-firmware" > + depends on BR2_aarch64 > + help > + Marvell Armada SCP BL2 firmware images. > + > +if BR2_TARGET_ARMADA_FIRMWARE > + > +config BR2_TARGET_ARMADA_FIRMWARE_IMAGE > + string "Armada SCP BL2 image name" > + help > + Armada SCP BL2 firmware image name. This symbol is not used here, which is quite unusual. Instead, you set this symbol in the defconfig, and use it in the ATF package. But since we'll use only one of the 7k/8k images (right?), I suggest to add a 7k/8k choice, and make BR2_TARGET_ARMADA_FIRMWARE_IMAGE a blink symbol that is set (default) according to the choice. Something like (abbreviated; untested): choice prompt "Armada platform" config BR2_PACKAGE_ARMADA_8040 bool "8040" config BR2_PACKAGE_ARMADA_7040 bool "7040" endchoice config BR2_TARGET_ARMADA_FIRMWARE_IMAGE string default "mrvl_scp_bl2_8040.img" if BR2_PACKAGE_ARMADA_8040 default "mrvl_scp_bl2_7040.img" if BR2_PACKAGE_ARMADA_7040 > +config BR2_TARGET_ARMADA_FIRMWARE_VERSION > + string "Armada SCP BL2 image version" > + help > + Armada SCP BL2 firmware image version. > + > +endif > diff --git a/boot/armada-firmware/armada-firmware.hash b/boot/armada-firmware/armada-firmware.hash > new file mode 100644 > index 0000000000..5eeca9d6ea > --- /dev/null > +++ b/boot/armada-firmware/armada-firmware.hash > @@ -0,0 +1,2 @@ > +# Locally calculated > +sha256 b310443c0d51d07b7c11597548685ae608b8478eee7095925427da6ab71e0168 armada-firmware-binaries-marvell-armada-17.10.tar.gz > diff --git a/boot/armada-firmware/armada-firmware.mk b/boot/armada-firmware/armada-firmware.mk > new file mode 100644 > index 0000000000..d75dc7cb6c > --- /dev/null > +++ b/boot/armada-firmware/armada-firmware.mk > @@ -0,0 +1,18 @@ > +################################################################################ > +# > +# Marvell Armada SCP BL2 firmware images > +# > +################################################################################ > + > +ARMADA_FIRMWARE_VERSION = $(call qstrip,$(BR2_TARGET_ARMADA_FIRMWARE_VERSION)) Having a configurable version is an exception of very few packages (linux, uboot, ATF). Other packages should set to a static version string or a commit id when there is no official version release. > +ARMADA_FIRMWARE_SITE = $(call github,MarvellEmbeddedProcessors,binaries-marvell,$(ARMADA_FIRMWARE_VERSION)) > +ARMADA_FIRMWARE_LICENSE = Proprietary These binaries come with no license, so you should also add ARMADA_FIRMWARE_REDISTRIBUTE = NO. > + > +ARMADA_FIRMWARE_INSTALL_IMAGES = YES > + > +define ARMADA_FIRMWARE_INSTALL_IMAGES_CMDS > + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_7040.img $(BINARIES_DIR)/mrvl_scp_bl2_7040.img > + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_8040.img $(BINARIES_DIR)/mrvl_scp_bl2_8040.img > +endef > + > +$(eval $(generic-package)) baruch
Hi Baruch, Thanks a lot for your thorough review ! > > new file mode 100644 > > index 0000000000..530b5622c8 > > --- /dev/null > > +++ b/boot/armada-firmware/Config.in > > @@ -0,0 +1,19 @@ > > +config BR2_TARGET_ARMADA_FIRMWARE > > + bool "armada-firmware" > > + depends on BR2_aarch64 > > + help > > + Marvell Armada SCP BL2 firmware images. > > + > > +if BR2_TARGET_ARMADA_FIRMWARE > > + > > +config BR2_TARGET_ARMADA_FIRMWARE_IMAGE > > + string "Armada SCP BL2 image name" > > + help > > + Armada SCP BL2 firmware image name. > > This symbol is not used here, which is quite unusual. Instead, you set this > symbol in the defconfig, and use it in the ATF package. But since we'll use > only one of the 7k/8k images (right?), I suggest to add a 7k/8k choice, and > make BR2_TARGET_ARMADA_FIRMWARE_IMAGE a blink symbol that is set (default) > according to the choice. > > Something like (abbreviated; untested): > > choice > prompt "Armada platform" > > config BR2_PACKAGE_ARMADA_8040 > bool "8040" > > config BR2_PACKAGE_ARMADA_7040 > bool "7040" > > endchoice > > config BR2_TARGET_ARMADA_FIRMWARE_IMAGE > string > default "mrvl_scp_bl2_8040.img" if BR2_PACKAGE_ARMADA_8040 > default "mrvl_scp_bl2_7040.img" if BR2_PACKAGE_ARMADA_7040 Well, I used string field to specify firmware name because earlier versions of Armada firmware may have different names. For instance, branch 17.08 contains single firmware blob RTOSDemo-cm3.bin. So if we specify branch of Armada firmware repository, we have to specify firmware blob name as well. On the other hand, as you mentioned in subsequent comment, having a configurable version for this firmware is not a good idea. So if we set it to the static version (now the latest is 17.10), then we can use suggested 'choice' menu. Moreover, as you mentioned, it is not really needed for upstream BSP. I will check if it is needed for Marvell BSP. If not, then it should be safe to drop this package altogether. > > new file mode 100644 > > index 0000000000..d75dc7cb6c > > --- /dev/null > > +++ b/boot/armada-firmware/armada-firmware.mk > > @@ -0,0 +1,18 @@ > > +################################################################################ > > +# > > +# Marvell Armada SCP BL2 firmware images > > +# > > +################################################################################ > > + > > +ARMADA_FIRMWARE_VERSION = $(call qstrip,$(BR2_TARGET_ARMADA_FIRMWARE_VERSION)) > > Having a configurable version is an exception of very few packages (linux, > uboot, ATF). Other packages should set to a static version string or a commit > id when there is no official version release. Ok, will do. > > +ARMADA_FIRMWARE_SITE = $(call github,MarvellEmbeddedProcessors,binaries-marvell,$(ARMADA_FIRMWARE_VERSION)) > > +ARMADA_FIRMWARE_LICENSE = Proprietary > > These binaries come with no license, so you should also add > ARMADA_FIRMWARE_REDISTRIBUTE = NO. Ok, will do. Regards, Sergey
On 12-11-17 21:16, Sergey Matyukevich wrote: > This package adds SCP BL2 firmware for Marvell Armada 7040 and 8040 SoCs. > > Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com> > --- > boot/Config.in | 1 + > boot/armada-firmware/Config.in | 19 +++++++++++++++++++ > boot/armada-firmware/armada-firmware.hash | 2 ++ > boot/armada-firmware/armada-firmware.mk | 18 ++++++++++++++++++ > 4 files changed, 40 insertions(+) > create mode 100644 boot/armada-firmware/Config.in > create mode 100644 boot/armada-firmware/armada-firmware.hash > create mode 100644 boot/armada-firmware/armada-firmware.mk > > diff --git a/boot/Config.in b/boot/Config.in > index 2f46c8546e..0ffbd7288b 100644 > --- a/boot/Config.in > +++ b/boot/Config.in > @@ -17,5 +17,6 @@ source "boot/ts4800-mbrboot/Config.in" > source "boot/uboot/Config.in" > source "boot/vexpress-firmware/Config.in" > source "boot/xloader/Config.in" > +source "boot/armada-firmware/Config.in" Please keep things alphabetic. However, I don't think the boot loader menu is appropriate. Yes, it's an image that is used by the bootloader, but really it is a piece of firmware, not really related to booting the SoC. > > endmenu > diff --git a/boot/armada-firmware/Config.in b/boot/armada-firmware/Config.in > new file mode 100644 > index 0000000000..530b5622c8 > --- /dev/null > +++ b/boot/armada-firmware/Config.in > @@ -0,0 +1,19 @@ > +config BR2_TARGET_ARMADA_FIRMWARE > + bool "armada-firmware" > + depends on BR2_aarch64 > + help > + Marvell Armada SCP BL2 firmware images. Could you write a little more help text? Like, that it's the firmware for the management coprocessor and that it is needed for power management. The first paragraph of [1] can be a source of inspiration. We normally also want an upstream URL. [1] might be usable but I'm not sure. > + > +if BR2_TARGET_ARMADA_FIRMWARE > + > +config BR2_TARGET_ARMADA_FIRMWARE_IMAGE > + string "Armada SCP BL2 image name" > + help > + Armada SCP BL2 firmware image name. > + > +config BR2_TARGET_ARMADA_FIRMWARE_VERSION > + string "Armada SCP BL2 image version" > + help > + Armada SCP BL2 firmware image version. > + > +endif > diff --git a/boot/armada-firmware/armada-firmware.hash b/boot/armada-firmware/armada-firmware.hash > new file mode 100644 > index 0000000000..5eeca9d6ea > --- /dev/null > +++ b/boot/armada-firmware/armada-firmware.hash > @@ -0,0 +1,2 @@ > +# Locally calculated > +sha256 b310443c0d51d07b7c11597548685ae608b8478eee7095925427da6ab71e0168 armada-firmware-binaries-marvell-armada-17.10.tar.gz > diff --git a/boot/armada-firmware/armada-firmware.mk b/boot/armada-firmware/armada-firmware.mk > new file mode 100644 > index 0000000000..d75dc7cb6c > --- /dev/null > +++ b/boot/armada-firmware/armada-firmware.mk > @@ -0,0 +1,18 @@ > +################################################################################ > +# > +# Marvell Armada SCP BL2 firmware images > +# > +################################################################################ > + > +ARMADA_FIRMWARE_VERSION = $(call qstrip,$(BR2_TARGET_ARMADA_FIRMWARE_VERSION)) > +ARMADA_FIRMWARE_SITE = $(call github,MarvellEmbeddedProcessors,binaries-marvell,$(ARMADA_FIRMWARE_VERSION)) We try to keep the upstream name as the package name. Is there anything wrong with binaries-marvell? > +ARMADA_FIRMWARE_LICENSE = Proprietary > + > +ARMADA_FIRMWARE_INSTALL_IMAGES = YES > + > +define ARMADA_FIRMWARE_INSTALL_IMAGES_CMDS > + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_7040.img $(BINARIES_DIR)/mrvl_scp_bl2_7040.img > + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_8040.img $(BINARIES_DIR)/mrvl_scp_bl2_8040.img I don't think the name is really relevant, is it? So maybe you could always copy it to the same file in BINARIES_DIR so on the ATF side you don't need to access variables of this package. Regards, Arnout > +endef > + > +$(eval $(generic-package)) > [1] http://wiki.macchiatobin.net/tiki-index.php?page=Enable+Power+Management+on+MACCHIATObin#Bootloader
Hi Arnout, Thanks for the review ! > > This package adds SCP BL2 firmware for Marvell Armada 7040 and 8040 SoCs. ... > > diff --git a/boot/Config.in b/boot/Config.in > > index 2f46c8546e..0ffbd7288b 100644 > > --- a/boot/Config.in > > +++ b/boot/Config.in > > @@ -17,5 +17,6 @@ source "boot/ts4800-mbrboot/Config.in" > > source "boot/uboot/Config.in" > > source "boot/vexpress-firmware/Config.in" > > source "boot/xloader/Config.in" > > +source "boot/armada-firmware/Config.in" > > Please keep things alphabetic. However, I don't think the boot loader menu is > appropriate. Yes, it's an image that is used by the bootloader, but really it is > a piece of firmware, not really related to booting the SoC. Will fix the order. However it looks like boot menu is the right place for this package. This package is similar to versatile-firmware. It is SCP_BL2 image which is used by ATF to boot SCP coprocessor. A bit more details are available in ATF design document: https://github.com/ARM-software/arm-trusted-firmware/blob/master/docs/firmware-design.rst#scp-bl2-system-control-processor-firmware-image-load > > diff --git a/boot/armada-firmware/Config.in b/boot/armada-firmware/Config.in > > new file mode 100644 > > index 0000000000..530b5622c8 > > --- /dev/null > > +++ b/boot/armada-firmware/Config.in > > @@ -0,0 +1,19 @@ > > +config BR2_TARGET_ARMADA_FIRMWARE > > + bool "armada-firmware" > > + depends on BR2_aarch64 > > + help > > + Marvell Armada SCP BL2 firmware images. > > Could you write a little more help text? Like, that it's the firmware for the > management coprocessor and that it is needed for power management. The first > paragraph of [1] can be a source of inspiration. > > We normally also want an upstream URL. [1] might be usable but I'm not sure. Sure, I will add more details about this package. > > diff --git a/boot/armada-firmware/armada-firmware.mk b/boot/armada-firmware/armada-firmware.mk > > new file mode 100644 > > index 0000000000..d75dc7cb6c > > --- /dev/null > > +++ b/boot/armada-firmware/armada-firmware.mk > > @@ -0,0 +1,18 @@ > > +################################################################################ > > +# > > +# Marvell Armada SCP BL2 firmware images > > +# > > +################################################################################ > > + > > +ARMADA_FIRMWARE_VERSION = $(call qstrip,$(BR2_TARGET_ARMADA_FIRMWARE_VERSION)) > > +ARMADA_FIRMWARE_SITE = $(call github,MarvellEmbeddedProcessors,binaries-marvell,$(ARMADA_FIRMWARE_VERSION)) > > We try to keep the upstream name as the package name. Is there anything wrong > with binaries-marvell? Ok, makes sense. Will do. > > +ARMADA_FIRMWARE_LICENSE = Proprietary > > + > > +ARMADA_FIRMWARE_INSTALL_IMAGES = YES > > + > > +define ARMADA_FIRMWARE_INSTALL_IMAGES_CMDS > > + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_7040.img $(BINARIES_DIR)/mrvl_scp_bl2_7040.img > > + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_8040.img $(BINARIES_DIR)/mrvl_scp_bl2_8040.img > > I don't think the name is really relevant, is it? So maybe you could always > copy it to the same file in BINARIES_DIR so on the ATF side you don't need to > access variables of this package. This file depends on platform. Now we are adding MacchiatoBin, but another interesting networking board called EspressoBin from the same vendor is on the horizon. So it makes sense to keep things a little bit more flexible. It looks like, together with review comments from Baruch, this brings us to the following implementation: - choice submenu for available platforms in armada-firmware config menu .. config BR2_TARGET_ARMADA_FIRMWARE_IMAGE string default "mrvl_scp_bl2_8040.img" if BR2_PACKAGE_ARMADA_8040 default "mrvl_scp_bl2_7040.img" if BR2_PACKAGE_ARMADA_7040 - copy only selected BR2_TARGET_ARMADA_FIRMWARE_IMAGE to BINARIES_DIR - use the same BR2_TARGET_ARMADA_FIRMWARE_FIRMWARE variable in ATF package to specify correct firmware Regards, Sergey
On 15-11-17 22:15, Sergey Matyukevich wrote: > Hi Arnout, > > Thanks for the review ! > >>> This package adds SCP BL2 firmware for Marvell Armada 7040 and 8040 SoCs. > > ... > >>> diff --git a/boot/Config.in b/boot/Config.in >>> index 2f46c8546e..0ffbd7288b 100644 >>> --- a/boot/Config.in >>> +++ b/boot/Config.in >>> @@ -17,5 +17,6 @@ source "boot/ts4800-mbrboot/Config.in" >>> source "boot/uboot/Config.in" >>> source "boot/vexpress-firmware/Config.in" >>> source "boot/xloader/Config.in" >>> +source "boot/armada-firmware/Config.in" >> >> Please keep things alphabetic. However, I don't think the boot loader menu is >> appropriate. Yes, it's an image that is used by the bootloader, but really it is >> a piece of firmware, not really related to booting the SoC. > > Will fix the order. However it looks like boot menu is the right place for this > package. This package is similar to versatile-firmware. It is SCP_BL2 image > which is used by ATF to boot SCP coprocessor. A bit more details are available > in ATF design document: > https://github.com/ARM-software/arm-trusted-firmware/blob/master/docs/firmware-design.rst#scp-bl2-system-control-processor-firmware-image-load As I understand it, it should also be possible to load the SCP firmware after boot. Maybe not on all platforms. But even if it weren't I wouldn't consider it a bootloader. And the same goes for vexpress-firmware by the way. That said, it's not such a big deal for me. [snip] >>> +define ARMADA_FIRMWARE_INSTALL_IMAGES_CMDS >>> + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_7040.img $(BINARIES_DIR)/mrvl_scp_bl2_7040.img >>> + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_8040.img $(BINARIES_DIR)/mrvl_scp_bl2_8040.img >> >> I don't think the name is really relevant, is it? So maybe you could always >> copy it to the same file in BINARIES_DIR so on the ATF side you don't need to >> access variables of this package. > > This file depends on platform. Now we are adding MacchiatoBin, but another > interesting networking board called EspressoBin from the same vendor is on the > horizon. So it makes sense to keep things a little bit more flexible. > > It looks like, together with review comments from Baruch, this brings us > to the following implementation: > - choice submenu for available platforms in armada-firmware config menu > .. > config BR2_TARGET_ARMADA_FIRMWARE_IMAGE > string > default "mrvl_scp_bl2_8040.img" if BR2_PACKAGE_ARMADA_8040 > default "mrvl_scp_bl2_7040.img" if BR2_PACKAGE_ARMADA_7040 > > - copy only selected BR2_TARGET_ARMADA_FIRMWARE_IMAGE to BINARIES_DIR > > - use the same BR2_TARGET_ARMADA_FIRMWARE_FIRMWARE variable in ATF package > to specify correct firmware My point was: you can copy the image to $(BINARIES_DIR)/scp-fw.bin, then in the ATF package you don't need to specify the firmware name but just always use scp-fw.bin. A further step would be to introduce an arm-scp-firmware (or atf-scp-firmware) virtual package (implemented by vexpress-firmware and armada-firmware). That way no change is needed in ATF to add an additional firmware package, and it's possible to add new firmware in BR2_EXTERNAL. Regards, Arnout
diff --git a/boot/Config.in b/boot/Config.in index 2f46c8546e..0ffbd7288b 100644 --- a/boot/Config.in +++ b/boot/Config.in @@ -17,5 +17,6 @@ source "boot/ts4800-mbrboot/Config.in" source "boot/uboot/Config.in" source "boot/vexpress-firmware/Config.in" source "boot/xloader/Config.in" +source "boot/armada-firmware/Config.in" endmenu diff --git a/boot/armada-firmware/Config.in b/boot/armada-firmware/Config.in new file mode 100644 index 0000000000..530b5622c8 --- /dev/null +++ b/boot/armada-firmware/Config.in @@ -0,0 +1,19 @@ +config BR2_TARGET_ARMADA_FIRMWARE + bool "armada-firmware" + depends on BR2_aarch64 + help + Marvell Armada SCP BL2 firmware images. + +if BR2_TARGET_ARMADA_FIRMWARE + +config BR2_TARGET_ARMADA_FIRMWARE_IMAGE + string "Armada SCP BL2 image name" + help + Armada SCP BL2 firmware image name. + +config BR2_TARGET_ARMADA_FIRMWARE_VERSION + string "Armada SCP BL2 image version" + help + Armada SCP BL2 firmware image version. + +endif diff --git a/boot/armada-firmware/armada-firmware.hash b/boot/armada-firmware/armada-firmware.hash new file mode 100644 index 0000000000..5eeca9d6ea --- /dev/null +++ b/boot/armada-firmware/armada-firmware.hash @@ -0,0 +1,2 @@ +# Locally calculated +sha256 b310443c0d51d07b7c11597548685ae608b8478eee7095925427da6ab71e0168 armada-firmware-binaries-marvell-armada-17.10.tar.gz diff --git a/boot/armada-firmware/armada-firmware.mk b/boot/armada-firmware/armada-firmware.mk new file mode 100644 index 0000000000..d75dc7cb6c --- /dev/null +++ b/boot/armada-firmware/armada-firmware.mk @@ -0,0 +1,18 @@ +################################################################################ +# +# Marvell Armada SCP BL2 firmware images +# +################################################################################ + +ARMADA_FIRMWARE_VERSION = $(call qstrip,$(BR2_TARGET_ARMADA_FIRMWARE_VERSION)) +ARMADA_FIRMWARE_SITE = $(call github,MarvellEmbeddedProcessors,binaries-marvell,$(ARMADA_FIRMWARE_VERSION)) +ARMADA_FIRMWARE_LICENSE = Proprietary + +ARMADA_FIRMWARE_INSTALL_IMAGES = YES + +define ARMADA_FIRMWARE_INSTALL_IMAGES_CMDS + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_7040.img $(BINARIES_DIR)/mrvl_scp_bl2_7040.img + $(INSTALL) -D -m 0644 $(@D)/mrvl_scp_bl2_8040.img $(BINARIES_DIR)/mrvl_scp_bl2_8040.img +endef + +$(eval $(generic-package))
This package adds SCP BL2 firmware for Marvell Armada 7040 and 8040 SoCs. Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com> --- boot/Config.in | 1 + boot/armada-firmware/Config.in | 19 +++++++++++++++++++ boot/armada-firmware/armada-firmware.hash | 2 ++ boot/armada-firmware/armada-firmware.mk | 18 ++++++++++++++++++ 4 files changed, 40 insertions(+) create mode 100644 boot/armada-firmware/Config.in create mode 100644 boot/armada-firmware/armada-firmware.hash create mode 100644 boot/armada-firmware/armada-firmware.mk