| Message ID | 20191121102324.35225-7-jerry.huang@nxp.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | new board ls1028ardb introduced | expand |
> 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
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
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
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&data=02%7C01%7Cjerry.huang%40nxp.com%7C74c02743671b4e18a > b5c08d771f42b8f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637 > 103165822614341&sdata=rcEnN%2B%2FlfMpj0iwpikulfsCH0v813TaG86K > GsE3eGTo%3D&reserved=0
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&data=02%7C01%7Cjerry.huang%40nxp.com%7C74c02743671b4e18a > > > b5c08d771f42b8f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637 > > > 103165822614341&sdata=rcEnN%2B%2FlfMpj0iwpikulfsCH0v813TaG86K > > GsE3eGTo%3D&reserved=0
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
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(+)