[v2,06/10] boot/arm-trusted-firmware: Add RCW support
diff mbox series

Message ID 20191121102324.35225-7-jerry.huang@nxp.com
State Changes Requested
Headers show
Series
  • new board ls1028ardb introduced
Related show

Commit Message

Jerry Huang Nov. 21, 2019, 10:23 a.m. UTC
NXP Layerscape platforms use RCW (Reset Configure Word) to
setup clocking and IO allocations and then launches the next stage of boot.
RCW needs ATF as well as uboot.
Option BR2_PACKAGE_HOST_RCW_ATF is used to determine if ATF is used for RCW.

Signed-off-by: Changming Huang <jerry.huang@nxp.com>
---
changes since v1:
1. Use option BR2_PACKAGE_HOST_RCW_ATF as the condition for RCW used by ATF.
---
 boot/arm-trusted-firmware/arm-trusted-firmware.mk | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Sergey Matyukevich Nov. 22, 2019, 9:19 a.m. UTC | #1
> diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> index 2133d39e6d..2bca8109f1 100644
> --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> @@ -92,6 +92,15 @@ endif
>  
>  ARM_TRUSTED_FIRMWARE_MAKE_TARGETS = all
>  
> +ifeq ($(BR2_PACKAGE_HOST_RCW_ATF),y)
> +RCW_BOOT_MODE = $(call qstrip,$(BR2_PACKAGE_HOST_RCW_BOOT_MODE))
> +RCW_PATH = $(call qstrip,$(BR2_PACKAGE_HOST_RCW_BIN))
> +RCW_FILE = $(lastword $(subst /, ,$(RCW_PATH)))
> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += BOOT_MODE=$(RCW_BOOT_MODE) RCW=$(BINARIES_DIR)/$(RCW_FILE)
> +ARM_TRUSTED_FIRMWARE_MAKE_TARGETS += pbl
> +ARM_TRUSTED_FIRMWARE_DEPENDENCIES += host-fsl-qoriq-rcw
> +endif
> +
>  ifeq ($(BR2_TARGET_ARM_TRUSTED_FIRMWARE_FIP),y)
>  ARM_TRUSTED_FIRMWARE_MAKE_TARGETS += fip
>  ARM_TRUSTED_FIRMWARE_DEPENDENCIES += host-openssl

Hi Jerry, Thomas, and all

This approach looks reasonable to me: if we enable RCW_ATF in board config,
then arm-trusted-firmware build process is modified in accordance
with that choice.

However I am slightly worried about naming.  There was a discussion around the
first version of the patch set regarding too generic name for RCW package.
IIUC here we have the same issue here. Option BR2_PACKAGE_HOST_RCW_ATF
looks fairly generic, but it selects fsl-qoriq specific dependencies.

Thoughts ? Comments ?

Regards,
Sergey
Jerry Huang Nov. 22, 2019, 9:31 a.m. UTC | #2
Best Regards
Jerry Huang

> -----Original Message-----
> From: Sergey Matyukevich <geomatsi@gmail.com>
> Sent: Friday, November 22, 2019 5:20 PM
> To: Jerry Huang <jerry.huang@nxp.com>
> Cc: buildroot@busybox.net; matthew.weber@collins.com; michael@walle.cc;
> thomas.petazzoni@bootlin.com
> Subject: [EXT] Re: [PATCH v2 06/10] boot/arm-trusted-firmware: Add RCW
> support
> 
> Caution: EXT Email
> 
> > diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > index 2133d39e6d..2bca8109f1 100644
> > --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > @@ -92,6 +92,15 @@ endif
> >
> >  ARM_TRUSTED_FIRMWARE_MAKE_TARGETS = all
> >
> > +ifeq ($(BR2_PACKAGE_HOST_RCW_ATF),y)
> > +RCW_BOOT_MODE = $(call
> qstrip,$(BR2_PACKAGE_HOST_RCW_BOOT_MODE))
> > +RCW_PATH = $(call qstrip,$(BR2_PACKAGE_HOST_RCW_BIN))
> > +RCW_FILE = $(lastword $(subst /, ,$(RCW_PATH)))
> > +ARM_TRUSTED_FIRMWARE_MAKE_OPTS +=
> BOOT_MODE=$(RCW_BOOT_MODE)
> > +RCW=$(BINARIES_DIR)/$(RCW_FILE)
> ARM_TRUSTED_FIRMWARE_MAKE_TARGETS +=
> > +pbl ARM_TRUSTED_FIRMWARE_DEPENDENCIES += host-fsl-qoriq-rcw endif
> > +
> >  ifeq ($(BR2_TARGET_ARM_TRUSTED_FIRMWARE_FIP),y)
> >  ARM_TRUSTED_FIRMWARE_MAKE_TARGETS += fip
> > ARM_TRUSTED_FIRMWARE_DEPENDENCIES += host-openssl
> 
> Hi Jerry, Thomas, and all
> 
> This approach looks reasonable to me: if we enable RCW_ATF in board config,
> then arm-trusted-firmware build process is modified in accordance with that
> choice.
> 
> However I am slightly worried about naming.  There was a discussion around
> the first version of the patch set regarding too generic name for RCW package.
> IIUC here we have the same issue here. Option BR2_PACKAGE_HOST_RCW_ATF
> looks fairly generic, but it selects fsl-qoriq specific dependencies.
> 
> Thoughts ? Comments ?
Currently, just Freescale/NXP QorIQ platforms use RCW to configure the hardware.
Maybe, who can suggest one more reasonable name for that (BR2_PACKAGE_HOST_RCW_ATF) ?


> Regards,
> Sergey
Thomas Petazzoni Nov. 25, 2019, 10:09 p.m. UTC | #3
Hello,

On Thu, 21 Nov 2019 18:23:20 +0800
Changming Huang <jerry.huang@nxp.com> wrote:

> diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> index 2133d39e6d..2bca8109f1 100644
> --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> @@ -92,6 +92,15 @@ endif
>  
>  ARM_TRUSTED_FIRMWARE_MAKE_TARGETS = all
>  
> +ifeq ($(BR2_PACKAGE_HOST_RCW_ATF),y)

This option should be introduced by this patch.

Overall, I'm not super happy with how arm-trusted-firmware works. In
some cases, it's a sub-option of arm-trusted-firmware that causes it to
use some additional package (for example OPTEE as BL32, U-Boot as
BL33), but sometimes it's the implicit fact of having another package
enabled (mv-ddr-marvell, vexpress-firmware), that causes
arm-trusted-firmware to use it. Not great.

So I'm not sure how to proceed with RCW.

> +RCW_BOOT_MODE = $(call qstrip,$(BR2_PACKAGE_HOST_RCW_BOOT_MODE))
> +RCW_PATH = $(call qstrip,$(BR2_PACKAGE_HOST_RCW_BIN))
> +RCW_FILE = $(lastword $(subst /, ,$(RCW_PATH)))

Those variables names should be prefixed with the package name, i.e ARM_TRUSTED_FIRMWARE.

> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += BOOT_MODE=$(RCW_BOOT_MODE) RCW=$(BINARIES_DIR)/$(RCW_FILE)

These BOOT_MODE and RCW options are not supported in ATF upstream,
apparently only in the NXP fork. I'm not sure we want explicit support
for vendor-specific things. Should we look at providing an
arm-trusted-firmware that can more easily be customized through options?

That's really an open discussion, I don't yet have a clear idea on how
to handle that.

Thomas
Jerry Huang Nov. 26, 2019, 7:06 a.m. UTC | #4
Hi, Thomas,
Some comment in lines, please review them, thanks a lot.

Best Regards
Jerry Huang

> -----Original Message-----
> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Sent: Tuesday, November 26, 2019 6:10 AM
> To: Jerry Huang <jerry.huang@nxp.com>
> Cc: buildroot@busybox.net; michael@walle.cc; matthew.weber@collins.com;
> geomatsi@gmail.com
> Subject: [EXT] Re: [Buildroot] [PATCH v2 06/10] boot/arm-trusted-firmware: Add
> RCW support
> 
> Caution: EXT Email
> 
> Hello,
> 
> On Thu, 21 Nov 2019 18:23:20 +0800
> Changming Huang <jerry.huang@nxp.com> wrote:
> 
> > diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > index 2133d39e6d..2bca8109f1 100644
> > --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > @@ -92,6 +92,15 @@ endif
> >
> >  ARM_TRUSTED_FIRMWARE_MAKE_TARGETS = all
> >
> > +ifeq ($(BR2_PACKAGE_HOST_RCW_ATF),y)
> 
> This option should be introduced by this patch.
Sure,

> Overall, I'm not super happy with how arm-trusted-firmware works. In some
> cases, it's a sub-option of arm-trusted-firmware that causes it to use some
> additional package (for example OPTEE as BL32, U-Boot as BL33), but
> sometimes it's the implicit fact of having another package enabled
> (mv-ddr-marvell, vexpress-firmware), that causes arm-trusted-firmware to use it.
> Not great.
> 
> So I'm not sure how to proceed with RCW.
Yes, it uses some additional package for booting.
But, if it does not use them (U-Boot as BL33, OPTEE as BL32, RCW as BL2), what can arm-trusted-firmware do?

> > +RCW_BOOT_MODE = $(call
> qstrip,$(BR2_PACKAGE_HOST_RCW_BOOT_MODE))
> > +RCW_PATH = $(call qstrip,$(BR2_PACKAGE_HOST_RCW_BIN))
> > +RCW_FILE = $(lastword $(subst /, ,$(RCW_PATH)))
> 
> Those variables names should be prefixed with the package name, i.e
> ARM_TRUSTED_FIRMWARE.
Sure,

> > +ARM_TRUSTED_FIRMWARE_MAKE_OPTS +=
> BOOT_MODE=$(RCW_BOOT_MODE)
> > +RCW=$(BINARIES_DIR)/$(RCW_FILE)
> 
> These BOOT_MODE and RCW options are not supported in ATF upstream,
> apparently only in the NXP fork. I'm not sure we want explicit support for
> vendor-specific things. Should we look at providing an arm-trusted-firmware
> that can more easily be customized through options?

Maybe we can provide three options
ARM_TRUSTED_FIRMWARE_EXTRA_OPTS: the custom OPTS
ARM_TRUSTED_FIRMWARE_EXTRA_TARGETS: the custom TARGETS
ARM_TRUSTED_FIRMWARE_EXTRA_DEPENDENCIES: the custom dependency

Which are defined in defconfig file or empty,
For example, NXP QorIQ:
ARM_TRUSTED_FIRMWARE_EXTRA_OPTS = " BOOT_MODE=sd RCW=$(BINARIES_DIR)/ rcw_1300_sdboot.bin"
ARM_TRUSTED_FIRMWARE_EXTRA_TARGETS = " pbl"
ARM_TRUSTED_FIRMWARE_EXTRA_DEPENDENCIES = "host-qoriq-rcw"
Or Marvell DDR:
ARM_TRUSTED_FIRMWARE_EXTRA_OPTS = " SCP_BL2=$(BINARIES_DIR)/scp-fw.bin"
ARM_TRUSTED_FIRMWARE_EXTRA_DEPENDENCIES = "mv-ddr-marvell"

we can use them as below without detecting the condition in ATF makefile:
ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ARM_TRUSTED_FIRMWARE_EXTRA_OPTS
ARM_TRUSTED_FIRMWARE_MAKE_TARGETS += ARM_TRUSTED_FIRMWARE_EXTRA_TARGETS
ARM_TRUSTED_FIRMWARE_DEPENDENCIES += ARM_TRUSTED_FIRMWARE_EXTRA_DEPENDENCIES

We can put anything we want into these options, and the ATF makefile is very clean.

How do you think about them?

> That's really an open discussion, I don't yet have a clear idea on how to handle
> that.
> 
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin.c
> om&amp;data=02%7C01%7Cjerry.huang%40nxp.com%7C74c02743671b4e18a
> b5c08d771f42b8f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637
> 103165822614341&amp;sdata=rcEnN%2B%2FlfMpj0iwpikulfsCH0v813TaG86K
> GsE3eGTo%3D&amp;reserved=0
Jerry Huang Nov. 26, 2019, 7:34 a.m. UTC | #5
I double check the "boot/arm-trusted-firmware/Config.in",
There is one option BR2_TARGET_ARM_TRUSTED_FIRMWARE_ADDITIONAL_VARIABLES,
I think it can be used to replace my previous mail mentioned: ARM_TRUSTED_FIRMWARE_EXTRA_OPTS.
Add now this option is added to ARM_TRUSTED_FIRMWARE_MAKE_OPTS

So in order to keep consistent with the context, two new options are defined:
BR2_TARGET_ARM_TRUSTED_FIRMWARE_ADDITIONAL_TARGETS
BR2_TARGET_ARM_TRUSTED_FIRMWARE_ADDITIONAL_ DEPENDENCIES

ARM_TRUSTED_FIRMWARE_MAKE_TARGETS += BR2_TARGET_ARM_TRUSTED_FIRMWARE_ADDITIONAL_TARGETS
ARM_TRUSTED_FIRMWARE_DEPENDENCIES += BR2_TARGET_ARM_TRUSTED_FIRMWARE_ADDITIONAL_ DEPENDENCIES

Best Regards
Jerry Huang

> -----Original Message-----
> From: Jerry Huang
> Sent: Tuesday, November 26, 2019 3:07 PM
> To: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: buildroot@busybox.net; michael@walle.cc; matthew.weber@collins.com;
> geomatsi@gmail.com
> Subject: RE: [EXT] Re: [Buildroot] [PATCH v2 06/10] boot/arm-trusted-firmware:
> Add RCW support
> 
> Hi, Thomas,
> Some comment in lines, please review them, thanks a lot.
> 
> Best Regards
> Jerry Huang
> 
> > -----Original Message-----
> > From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > Sent: Tuesday, November 26, 2019 6:10 AM
> > To: Jerry Huang <jerry.huang@nxp.com>
> > Cc: buildroot@busybox.net; michael@walle.cc;
> > matthew.weber@collins.com; geomatsi@gmail.com
> > Subject: [EXT] Re: [Buildroot] [PATCH v2 06/10]
> > boot/arm-trusted-firmware: Add RCW support
> >
> > Caution: EXT Email
> >
> > Hello,
> >
> > On Thu, 21 Nov 2019 18:23:20 +0800
> > Changming Huang <jerry.huang@nxp.com> wrote:
> >
> > > diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > > b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > > index 2133d39e6d..2bca8109f1 100644
> > > --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > > +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> > > @@ -92,6 +92,15 @@ endif
> > >
> > >  ARM_TRUSTED_FIRMWARE_MAKE_TARGETS = all
> > >
> > > +ifeq ($(BR2_PACKAGE_HOST_RCW_ATF),y)
> >
> > This option should be introduced by this patch.
> Sure,
> 
> > Overall, I'm not super happy with how arm-trusted-firmware works. In
> > some cases, it's a sub-option of arm-trusted-firmware that causes it
> > to use some additional package (for example OPTEE as BL32, U-Boot as
> > BL33), but sometimes it's the implicit fact of having another package
> > enabled (mv-ddr-marvell, vexpress-firmware), that causes
> arm-trusted-firmware to use it.
> > Not great.
> >
> > So I'm not sure how to proceed with RCW.
> Yes, it uses some additional package for booting.
> But, if it does not use them (U-Boot as BL33, OPTEE as BL32, RCW as BL2), what
> can arm-trusted-firmware do?
> 
> > > +RCW_BOOT_MODE = $(call
> > qstrip,$(BR2_PACKAGE_HOST_RCW_BOOT_MODE))
> > > +RCW_PATH = $(call qstrip,$(BR2_PACKAGE_HOST_RCW_BIN))
> > > +RCW_FILE = $(lastword $(subst /, ,$(RCW_PATH)))
> >
> > Those variables names should be prefixed with the package name, i.e
> > ARM_TRUSTED_FIRMWARE.
> Sure,
> 
> > > +ARM_TRUSTED_FIRMWARE_MAKE_OPTS +=
> > BOOT_MODE=$(RCW_BOOT_MODE)
> > > +RCW=$(BINARIES_DIR)/$(RCW_FILE)
> >
> > These BOOT_MODE and RCW options are not supported in ATF upstream,
> > apparently only in the NXP fork. I'm not sure we want explicit support
> > for vendor-specific things. Should we look at providing an
> > arm-trusted-firmware that can more easily be customized through options?
> 
> Maybe we can provide three options
> ARM_TRUSTED_FIRMWARE_EXTRA_OPTS: the custom OPTS
> ARM_TRUSTED_FIRMWARE_EXTRA_TARGETS: the custom TARGETS
> ARM_TRUSTED_FIRMWARE_EXTRA_DEPENDENCIES: the custom dependency
> 
> Which are defined in defconfig file or empty, For example, NXP QorIQ:
> ARM_TRUSTED_FIRMWARE_EXTRA_OPTS = " BOOT_MODE=sd
> RCW=$(BINARIES_DIR)/ rcw_1300_sdboot.bin"
> ARM_TRUSTED_FIRMWARE_EXTRA_TARGETS = " pbl"
> ARM_TRUSTED_FIRMWARE_EXTRA_DEPENDENCIES = "host-qoriq-rcw"
> Or Marvell DDR:
> ARM_TRUSTED_FIRMWARE_EXTRA_OPTS = "
> SCP_BL2=$(BINARIES_DIR)/scp-fw.bin"
> ARM_TRUSTED_FIRMWARE_EXTRA_DEPENDENCIES = "mv-ddr-marvell"
> 
> we can use them as below without detecting the condition in ATF makefile:
> ARM_TRUSTED_FIRMWARE_MAKE_OPTS +=
> ARM_TRUSTED_FIRMWARE_EXTRA_OPTS
> ARM_TRUSTED_FIRMWARE_MAKE_TARGETS +=
> ARM_TRUSTED_FIRMWARE_EXTRA_TARGETS
> ARM_TRUSTED_FIRMWARE_DEPENDENCIES +=
> ARM_TRUSTED_FIRMWARE_EXTRA_DEPENDENCIES
> 
> We can put anything we want into these options, and the ATF makefile is very
> clean.
> 
> How do you think about them?
> 
> > That's really an open discussion, I don't yet have a clear idea on how
> > to handle that.
> >
> > Thomas
> > --
> > Thomas Petazzoni, CTO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fboot
> > lin.c
> om&amp;data=02%7C01%7Cjerry.huang%40nxp.com%7C74c02743671b4e18a
> >
> b5c08d771f42b8f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637
> >
> 103165822614341&amp;sdata=rcEnN%2B%2FlfMpj0iwpikulfsCH0v813TaG86K
> > GsE3eGTo%3D&amp;reserved=0

Patch
diff mbox series

diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
index 2133d39e6d..2bca8109f1 100644
--- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
+++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
@@ -92,6 +92,15 @@  endif
 
 ARM_TRUSTED_FIRMWARE_MAKE_TARGETS = all
 
+ifeq ($(BR2_PACKAGE_HOST_RCW_ATF),y)
+RCW_BOOT_MODE = $(call qstrip,$(BR2_PACKAGE_HOST_RCW_BOOT_MODE))
+RCW_PATH = $(call qstrip,$(BR2_PACKAGE_HOST_RCW_BIN))
+RCW_FILE = $(lastword $(subst /, ,$(RCW_PATH)))
+ARM_TRUSTED_FIRMWARE_MAKE_OPTS += BOOT_MODE=$(RCW_BOOT_MODE) RCW=$(BINARIES_DIR)/$(RCW_FILE)
+ARM_TRUSTED_FIRMWARE_MAKE_TARGETS += pbl
+ARM_TRUSTED_FIRMWARE_DEPENDENCIES += host-fsl-qoriq-rcw
+endif
+
 ifeq ($(BR2_TARGET_ARM_TRUSTED_FIRMWARE_FIP),y)
 ARM_TRUSTED_FIRMWARE_MAKE_TARGETS += fip
 ARM_TRUSTED_FIRMWARE_DEPENDENCIES += host-openssl