Hi Kever,

On Sat, 8 Aug 2020 at 04:59, Kever Yang kever.yang@rock-chips.com wrote:

Hi Simon,

在 2020/7/20 上午3:55, Simon Glass 写道:

...

Bob is a Chromebook and can boot from SPI flash. Add it to the condition check for this.

Signed-off-by: Simon Glass sjg@chromium.org

(no changes since v1)

arch/arm/mach-rockchip/spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c index cddf4fd3d5..f148d48b6a 100644 --- a/arch/arm/mach-rockchip/spl.c +++ b/arch/arm/mach-rockchip/spl.c @@ -54,7 +54,8 @@ u32 spl_boot_device(void) #if defined(CONFIG_TARGET_CHROMEBOOK_JERRY) || \ defined(CONFIG_TARGET_CHROMEBIT_MICKEY) || \ defined(CONFIG_TARGET_CHROMEBOOK_MINNIE) || \

•         defined(CONFIG_TARGET_CHROMEBOOK_SPEEDY)
  

•         defined(CONFIG_TARGET_CHROMEBOOK_SPEEDY) || \
  

•         defined(CONFIG_TARGET_CHROMEBOOK_BOB)
  

Can we use "u-boot,spl-boot-order" in dts instead of hardcode define here?

Yes that sounds better. Will take a look.

Regards, Simon

Hi Kever,

On Sat, 8 Aug 2020 at 06:40, Kever Yang kever.yang@rock-chips.com wrote:

Hi Simon,

On 2020/7/20 上午3:55, Simon Glass wrote:

...

Add a simple binman config and enable CONFIG_HAS_ROM so that U-Boot produces a ROM for bob.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v4:

• Use CONFIG_ROCKCHIP_SPI_IMAGE to select the image

  arch/arm/dts/rk3399-gru-u-boot.dtsi | 4 ++++ arch/arm/dts/rk3399-gru.dtsi | 2 +- arch/arm/dts/rk3399-u-boot.dtsi | 27 +++++++++++++++++++++++++++ arch/arm/mach-rockchip/rk3399/Kconfig | 2 ++ 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi index 7bddc3acdb..390ac2bb5a 100644 --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi @@ -4,3 +4,7 @@ */

#include "rk3399-u-boot.dtsi"

+&spi_flash {

• u-boot,dm-pre-reloc;
  

+}; diff --git a/arch/arm/dts/rk3399-gru.dtsi b/arch/arm/dts/rk3399-gru.dtsi index 7ac88392f2..f9c5bb607b 100644 --- a/arch/arm/dts/rk3399-gru.dtsi +++ b/arch/arm/dts/rk3399-gru.dtsi @@ -537,7 +537,7 @@ ap_i2c_audio: &i2c8 { pinctrl-names = "default", "sleep"; pinctrl-1 = <&spi1_sleep>;

• spiflash@0 {
  

• spi_flash: spiflash@0 {
          compatible = "jedec,spi-nor";
          reg = <0>;
  
  

diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi index 8237782408..ecd230c720 100644 --- a/arch/arm/dts/rk3399-u-boot.dtsi +++ b/arch/arm/dts/rk3399-u-boot.dtsi @@ -4,11 +4,14 @@ */ #define USB_CLASS_HUB 9

+#include "rockchip-u-boot.dtsi"

• / { aliases { mmc0 = &sdhci; mmc1 = &sdmmc; pci0 = &pcie0;

•         spi1 = &spi1;
  };
  
  cic: syscon@ff620000 {
  

@@ -57,6 +60,30 @@

};

+#ifdef CONFIG_ROCKCHIP_SPI_IMAGE +&binman {

• rom {
  

•         filename = "u-boot.rom";
  

•         size = <0x400000>;
  

•         pad-byte = <0xff>;
  

•         mkimage {
  

•                 args = "-n rk3399 -T rkspi";
  

•                 u-boot-spl {
  

•                 };
  

•         };
  

•         u-boot-img {
  

•                 offset = <0x40000>;
  

•         };
  

•         u-boot {
  

•                 offset = <0x300000>;
  

•         };
  

•         fdtmap {
  

•         };
  

There is a output name 'u-boot.rom' including:

• u-boot-spl output with mkimage cmd at offset 0?

Yes

• u-boot-img at offset 0x40000, how this is generated and what is

inside? does binary in bl31.elf included?

It is just the u-boot.img file created by the Makefile. There is no BL31.

• u-boot at offset 0x300000, what is this binary and where is it from?

This is u-boot.bin as created by the Makefile.

Regards, Simon

Hello,

On 11 Oct 2020, at 16:39, Emmanuel Vadot manu@bidouilliste.com wrote:

Hi Simon,

On Sun, 19 Jul 2020 13:55:58 -0600 Simon Glass sjg@chromium.org wrote:

...

Add a simple binman config and enable CONFIG_HAS_ROM so that U-Boot produces a ROM for bob.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v4:

• Use CONFIG_ROCKCHIP_SPI_IMAGE to select the image

arch/arm/dts/rk3399-gru-u-boot.dtsi | 4 ++++ arch/arm/dts/rk3399-gru.dtsi | 2 +- arch/arm/dts/rk3399-u-boot.dtsi | 27 +++++++++++++++++++++++++++ arch/arm/mach-rockchip/rk3399/Kconfig | 2 ++ 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi index 7bddc3acdb..390ac2bb5a 100644 --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi @@ -4,3 +4,7 @@ */

#include "rk3399-u-boot.dtsi"

+&spi_flash {

• u-boot,dm-pre-reloc;

+}; diff --git a/arch/arm/dts/rk3399-gru.dtsi b/arch/arm/dts/rk3399-gru.dtsi index 7ac88392f2..f9c5bb607b 100644 --- a/arch/arm/dts/rk3399-gru.dtsi +++ b/arch/arm/dts/rk3399-gru.dtsi @@ -537,7 +537,7 @@ ap_i2c_audio: &i2c8 { pinctrl-names = "default", "sleep"; pinctrl-1 = <&spi1_sleep>;

• spiflash@0 {

• spi_flash: spiflash@0 { compatible = "jedec,spi-nor"; reg = <0>;

diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi index 8237782408..ecd230c720 100644 --- a/arch/arm/dts/rk3399-u-boot.dtsi +++ b/arch/arm/dts/rk3399-u-boot.dtsi @@ -4,11 +4,14 @@ */ #define USB_CLASS_HUB 9

+#include "rockchip-u-boot.dtsi"

/ { aliases { mmc0 = &sdhci; mmc1 = &sdmmc; pci0 = &pcie0;

• spi1 = &spi1;
  

I don't really understand why but this added alias break spi flash detection on rockpro64. Removing it make it work again.

I've noticed the same problem - rockpro64 already had an alias

spi0 = &spi1;

so after this change there are spi0 and spi1 aliases pointing to &spi1. In U-Boot proper, that seems to cause the SPI flash to appear on bus 1 rather than the former bus 0, e.g. sf probe 1:0 works but sf probe 0:0 doesn't.

So I tried removing the spi0 alias and setting CONFIG_SF_DEFAULT_BUS=1 which fixed the flash detection in U-Boot proper, but now the SPL can't load U-Boot from SPI flash - it fails with an "Invalid bus 1 (err=-19)" It seems like SPL doesn't pay attention to the spi1 alias in the same way that U-boot proper does, I haven't yet figured out why.

Cheers,

...

};

cic: syscon@ff620000 { @@ -57,6 +60,30 @@

};

+#ifdef CONFIG_ROCKCHIP_SPI_IMAGE +&binman {

• rom {

• filename = "u-boot.rom";
  

• size = <0x400000>;
  

• pad-byte = <0xff>;
  

• mkimage {
  

• 	args = "-n rk3399 -T rkspi";
  

• 	u-boot-spl {
  

• 	};
  

• };
  

• u-boot-img {
  

• 	offset = <0x40000>;
  

• };
  

• u-boot {
  

• 	offset = <0x300000>;
  

• };
  

• fdtmap {
  

• };
  

• };

+}; +#endif

&cru { u-boot,dm-pre-reloc; }; diff --git a/arch/arm/mach-rockchip/rk3399/Kconfig b/arch/arm/mach-rockchip/rk3399/Kconfig index 254b9c5b4d..17628f9171 100644 --- a/arch/arm/mach-rockchip/rk3399/Kconfig +++ b/arch/arm/mach-rockchip/rk3399/Kconfig @@ -5,6 +5,8 @@ choice

config TARGET_CHROMEBOOK_BOB bool "Asus Flip C101PA Chromebook (RK3399)"

• select HAS_ROM
• select ROCKCHIP_SPI_IMAGE help Bob is a small RK3299-based device similar in apperance to Minnie. It has two USB 3.0 type-C ports, 4GB of SDRAM, WiFi and a 10.1",

-- 2.28.0.rc0.105.gf9edc3c819-goog

-- Emmanuel Vadot manu@bidouilliste.com manu@freebsd.org

Hi Simon,

On Tue, 3 Nov 2020 08:11:59 -0700 Simon Glass sjg@chromium.org wrote:

Hi Hugh,

On Wed, 14 Oct 2020 at 12:37, Hugh Cole-Baker sigmaris@gmail.com wrote:

...

Hello,

...

On 11 Oct 2020, at 16:39, Emmanuel Vadot manu@bidouilliste.com wrote:

Hi Simon,

On Sun, 19 Jul 2020 13:55:58 -0600 Simon Glass sjg@chromium.org wrote:

...

Add a simple binman config and enable CONFIG_HAS_ROM so that U-Boot produces a ROM for bob.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v4:

• Use CONFIG_ROCKCHIP_SPI_IMAGE to select the image

arch/arm/dts/rk3399-gru-u-boot.dtsi | 4 ++++ arch/arm/dts/rk3399-gru.dtsi | 2 +- arch/arm/dts/rk3399-u-boot.dtsi | 27 +++++++++++++++++++++++++++ arch/arm/mach-rockchip/rk3399/Kconfig | 2 ++ 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi index 7bddc3acdb..390ac2bb5a 100644 --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi @@ -4,3 +4,7 @@ */

#include "rk3399-u-boot.dtsi"

+&spi_flash {

• u-boot,dm-pre-reloc;

+}; diff --git a/arch/arm/dts/rk3399-gru.dtsi b/arch/arm/dts/rk3399-gru.dtsi index 7ac88392f2..f9c5bb607b 100644 --- a/arch/arm/dts/rk3399-gru.dtsi +++ b/arch/arm/dts/rk3399-gru.dtsi @@ -537,7 +537,7 @@ ap_i2c_audio: &i2c8 { pinctrl-names = "default", "sleep"; pinctrl-1 = <&spi1_sleep>;

• spiflash@0 {

• spi_flash: spiflash@0 { compatible = "jedec,spi-nor"; reg = <0>;

diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi index 8237782408..ecd230c720 100644 --- a/arch/arm/dts/rk3399-u-boot.dtsi +++ b/arch/arm/dts/rk3399-u-boot.dtsi @@ -4,11 +4,14 @@ */ #define USB_CLASS_HUB 9

+#include "rockchip-u-boot.dtsi"

/ { aliases { mmc0 = &sdhci; mmc1 = &sdmmc; pci0 = &pcie0;

•        spi1 = &spi1;
  

I don't really understand why but this added alias break spi flash detection on rockpro64. Removing it make it work again.

I've noticed the same problem - rockpro64 already had an alias

spi0 = &spi1;

so after this change there are spi0 and spi1 aliases pointing to &spi1. In U-Boot proper, that seems to cause the SPI flash to appear on bus 1 rather than the former bus 0, e.g. sf probe 1:0 works but sf probe 0:0 doesn't.

So I tried removing the spi0 alias and setting CONFIG_SF_DEFAULT_BUS=1 which fixed the flash detection in U-Boot proper, but now the SPL can't load U-Boot from SPI flash - it fails with an "Invalid bus 1 (err=-19)" It seems like SPL doesn't pay attention to the spi1 alias in the same way that U-boot proper does, I haven't yet figured out why.

Could this be CONFIG_SPL_DM_SEQ_ALIAS ?

Regards, Simon

I don't know if you suggested to set CONFIG_SPL_DM_SEQ_ALIAS=y in configs/rockpro64-rk3399_defconfig but this doesn't help.

Cheers,

Hi Simon,

ne 19. 7. 2020 v 22:06 odesílatel Simon Glass sjg@chromium.org napsal:

This option is used to run arch-specific shell scripts which produce .its files which are used to produce FIT images. We already have binman which is designed to produce firmware images. It is more powerful and has tests.

So this option should be deprecated and not used. Existing uses should be migrated.

Mentions of this in code reviews over the last year or so do not seem to have resulted in action, and things are getting worse.

So let's add a warning.

Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Bin Meng bmeng.cn@gmail.com

---

(no changes since v1)

Makefile | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/Makefile b/Makefile index f1b5be1882..d73c10a973 100644 --- a/Makefile +++ b/Makefile @@ -1148,6 +1148,13 @@ ifneq ($(CONFIG_DM_ETH),y) @echo >&2 "See doc/driver-model/migration.rst for more info." @echo >&2 "====================================================" endif +endif +ifneq ($(CONFIG_SPL_FIT_GENERATOR),)

•   @echo >&2 "===================== WARNING ======================"
  

•   @echo >&2 "This board uses CONFIG_SPL_FIT_GENERATOR. Please migrate"
  

•   @echo >&2 "to binman instead, to avoid the proliferation of"
  

•   @echo >&2 "arch-specific scripts with no tests."
  

•   @echo >&2 "===================================================="
  

endif @# Check that this build does not use CONFIG options that we do not @# know about unless they are in Kconfig. All the existing CONFIG @@ -1345,6 +1352,8 @@ endif

# Boards with more complex image requirements can provide an .its source file # or a generator script +# NOTE: Please do not use this. We are migrating away from Makefile rules to use +# binman instead. ifneq ($(CONFIG_SPL_FIT_SOURCE),"") U_BOOT_ITS := u-boot.its $(U_BOOT_ITS): $(subst ",,$(CONFIG_SPL_FIT_SOURCE)) -- 2.28.0.rc0.105.gf9edc3c819-goog

I just got to this conversion and I am curious how that transition should look like. I found how FIT image is created which is fine but I didn't find any reference on how to generate images based on CONFIG_OF_LIST. If you look at arch/arm/mach-zynqmp/mkimage_fit_atf.sh you will see that I loop over this entry and create multiple DT nodes and the same amount of configurations to cover it. Is this supported by binman? If yes, what's the syntax for it?

I tried several configurations and we can use that for generating qspi images and also images with different configurations to have them ready but first I need to be able to handle the case above.

Thanks, Michal

Hi Simon,

On 16. 08. 20 5:39, Simon Glass wrote:

Hi Michal,

On Fri, 14 Aug 2020 at 07:28, Michal Simek monstr@monstr.eu wrote:

...

Hi Simon,

ne 19. 7. 2020 v 22:06 odesílatel Simon Glass sjg@chromium.org napsal:

...

This option is used to run arch-specific shell scripts which produce .its files which are used to produce FIT images. We already have binman which is designed to produce firmware images. It is more powerful and has tests.

So this option should be deprecated and not used. Existing uses should be migrated.

Mentions of this in code reviews over the last year or so do not seem to have resulted in action, and things are getting worse.

So let's add a warning.

Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Bin Meng bmeng.cn@gmail.com

---

(no changes since v1)

Makefile | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/Makefile b/Makefile index f1b5be1882..d73c10a973 100644 --- a/Makefile +++ b/Makefile @@ -1148,6 +1148,13 @@ ifneq ($(CONFIG_DM_ETH),y) @echo >&2 "See doc/driver-model/migration.rst for more info." @echo >&2 "====================================================" endif +endif +ifneq ($(CONFIG_SPL_FIT_GENERATOR),)

•   @echo >&2 "===================== WARNING ======================"
  

•   @echo >&2 "This board uses CONFIG_SPL_FIT_GENERATOR. Please migrate"
  

•   @echo >&2 "to binman instead, to avoid the proliferation of"
  

•   @echo >&2 "arch-specific scripts with no tests."
  

•   @echo >&2 "===================================================="
  

endif @# Check that this build does not use CONFIG options that we do not @# know about unless they are in Kconfig. All the existing CONFIG @@ -1345,6 +1352,8 @@ endif

# Boards with more complex image requirements can provide an .its source file # or a generator script +# NOTE: Please do not use this. We are migrating away from Makefile rules to use +# binman instead. ifneq ($(CONFIG_SPL_FIT_SOURCE),"") U_BOOT_ITS := u-boot.its $(U_BOOT_ITS): $(subst ",,$(CONFIG_SPL_FIT_SOURCE)) -- 2.28.0.rc0.105.gf9edc3c819-goog

I just got to this conversion and I am curious how that transition should look like. I found how FIT image is created which is fine but I didn't find any reference on how to generate images based on CONFIG_OF_LIST. If you look at arch/arm/mach-zynqmp/mkimage_fit_atf.sh you will see that I loop over this entry and create multiple DT nodes and the same amount of configurations to cover it. Is this supported by binman? If yes, what's the syntax for it?

The easiest way is probably to create a new entry type, like zynq-fit. Then you can generate the DT using the sequence writer functions. See _ReadSubNodes() in fit.py for an example.

You can perhaps have a template subnode and use that in a for loop to generate the nodes.

...

I tried several configurations and we can use that for generating qspi images and also images with different configurations to have them ready but first I need to be able to handle the case above.

I was thinking of converting sunxi which has the same need, but it sounds like you are on the case. Let me know if you need help.

Nope. I just saw that message and started to play with it to find out what needs to be done and how this fits to bigger picture. If this doesn't work directly then the work needs to be planned which will take time especially when this utility is new for us and we could have issues with writing code in python. Would be good if you can do the first shot because you know this utility and I am more than happy to test it, try and adopt if needed for our case.

Sunxi is very similar case as is zynqmp. Difference is they hardcode default configuration to config_1. ZynqMP is setting up default based on default DT configured at that time.

In connection to binman I see that there would be a need to generate images with ATF and without ATF in configuration node and with different default configuration. There could be also a need to add additional loadable entry such as bitstreams.

Back to zynq-fit new entry type. I don't think it should be zynq/zynqmp type because as was state in commit message u-boot.itb generation is very similar for all these boards that's why name for this new entry should be generic.

Thanks, Michal

Hi Simon,

On 22. 08. 20 17:08, Simon Glass wrote:

Hi Michal,

On Mon, 17 Aug 2020 at 00:49, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 16. 08. 20 5:39, Simon Glass wrote:

...

Hi Michal,

On Fri, 14 Aug 2020 at 07:28, Michal Simek monstr@monstr.eu wrote:

...

Hi Simon,

ne 19. 7. 2020 v 22:06 odesílatel Simon Glass sjg@chromium.org napsal:

...

This option is used to run arch-specific shell scripts which produce .its files which are used to produce FIT images. We already have binman which is designed to produce firmware images. It is more powerful and has tests.

So this option should be deprecated and not used. Existing uses should be migrated.

Mentions of this in code reviews over the last year or so do not seem to have resulted in action, and things are getting worse.

So let's add a warning.

Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Bin Meng bmeng.cn@gmail.com

---

(no changes since v1)

Makefile | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/Makefile b/Makefile index f1b5be1882..d73c10a973 100644 --- a/Makefile +++ b/Makefile @@ -1148,6 +1148,13 @@ ifneq ($(CONFIG_DM_ETH),y) @echo >&2 "See doc/driver-model/migration.rst for more info." @echo >&2 "====================================================" endif +endif +ifneq ($(CONFIG_SPL_FIT_GENERATOR),)

•   @echo >&2 "===================== WARNING ======================"
  

•   @echo >&2 "This board uses CONFIG_SPL_FIT_GENERATOR. Please migrate"
  

•   @echo >&2 "to binman instead, to avoid the proliferation of"
  

•   @echo >&2 "arch-specific scripts with no tests."
  

•   @echo >&2 "===================================================="
  

endif @# Check that this build does not use CONFIG options that we do not @# know about unless they are in Kconfig. All the existing CONFIG @@ -1345,6 +1352,8 @@ endif

# Boards with more complex image requirements can provide an .its source file # or a generator script +# NOTE: Please do not use this. We are migrating away from Makefile rules to use +# binman instead. ifneq ($(CONFIG_SPL_FIT_SOURCE),"") U_BOOT_ITS := u-boot.its $(U_BOOT_ITS): $(subst ",,$(CONFIG_SPL_FIT_SOURCE)) -- 2.28.0.rc0.105.gf9edc3c819-goog

I just got to this conversion and I am curious how that transition should look like. I found how FIT image is created which is fine but I didn't find any reference on how to generate images based on CONFIG_OF_LIST. If you look at arch/arm/mach-zynqmp/mkimage_fit_atf.sh you will see that I loop over this entry and create multiple DT nodes and the same amount of configurations to cover it. Is this supported by binman? If yes, what's the syntax for it?

The easiest way is probably to create a new entry type, like zynq-fit. Then you can generate the DT using the sequence writer functions. See _ReadSubNodes() in fit.py for an example.

You can perhaps have a template subnode and use that in a for loop to generate the nodes.

...

I tried several configurations and we can use that for generating qspi images and also images with different configurations to have them ready but first I need to be able to handle the case above.

I was thinking of converting sunxi which has the same need, but it sounds like you are on the case. Let me know if you need help.

Nope. I just saw that message and started to play with it to find out what needs to be done and how this fits to bigger picture. If this doesn't work directly then the work needs to be planned which will take time especially when this utility is new for us and we could have issues with writing code in python. Would be good if you can do the first shot because you know this utility and I am more than happy to test it, try and adopt if needed for our case.

Sunxi is very similar case as is zynqmp. Difference is they hardcode default configuration to config_1. ZynqMP is setting up default based on default DT configured at that time.

In connection to binman I see that there would be a need to generate images with ATF and without ATF in configuration node and with different default configuration. There could be also a need to add additional loadable entry such as bitstreams.

Back to zynq-fit new entry type. I don't think it should be zynq/zynqmp type because as was state in commit message u-boot.itb generation is very similar for all these boards that's why name for this new entry should be generic.

I sent an initial series to add this to binman. I've since found a few problems so will send a v2 at some point. You can try it out at u-boot-dm/binman-working

I looked at this branch and add my changes on the top.

The first thing what I see is that I miss fit,fdt-list = "of-list"; in sunxi dt file. I had to add it to work for me.

With BINMAN_FDT enabled I am getting error that there is no valid "binman node" in DT. I didn't study that code yet but that's the point of keeping this DT node out there?

This is my binman configuration.

diff --git a/arch/arm/dts/zynqmp-u-boot.dtsi b/arch/arm/dts/zynqmp-u-boot.dtsi new file mode 100644 index 000000000000..b3364d3e2df8 --- /dev/null +++ b/arch/arm/dts/zynqmp-u-boot.dtsi @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 Xilinx, Inc. + */ + +#include <config.h> + +/ { + binman: binman { + multiple-images; + }; +}; + +&binman { + u-boot-itb { + filename = "u-boot.itb"; + fit { + fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>; + description = "FIT image with ATF support"; + fit,fdt-list = "of-list"; + #address-cells = <1>; + + images { + uboot { + description = "U-Boot (64-bit)"; + type = "firmware"; + os = "u-boot"; + arch = "arm64"; + compression = "none"; + load = <CONFIG_SYS_TEXT_BASE>; + entry = <CONFIG_SYS_TEXT_BASE>; + + u-boot-nodtb { + }; + }; + atf { + description = "ARM Trusted Firmware"; + type = "firmware"; + os = "arm-trusted-firmware"; + arch = "arm64"; + compression = "none"; + load = <0xfffea000>; /* FIXME */ + entry = <0xfffea000>; + + blob-ext { + filename = "bl31.bin"; + }; + }; + @fdt-SEQ { + description = "NAME"; + type = "flat_dt"; + arch = "arm64"; + compression = "none"; + }; + }; + + configurations { + default = "config-1"; + @config-SEQ { + description = "NAME"; + firmware = "atf"; + loadables = "uboot"; + fdt = "fdt-SEQ"; + }; + }; + }; + fdtmap{}; + }; + +};

Anyway compare to current script default option is hardcoded to config-1. Current arch/arm/mach-zynqmp/mkimage_fit_atf.sh is also setting up default option based on selected default DT (I can fix this by implementing board_fit_config_name_match() but IIRC it is looping over all configurations and slowing down boot).

I will play with it a little bit more to get more experience with it

Thanks, Michal

Hi Simon,

On 25. 08. 20 17:04, Simon Glass wrote:

Hi Michal,

On Mon, 24 Aug 2020 at 08:12, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 22. 08. 20 17:08, Simon Glass wrote:

...

Hi Michal,

On Mon, 17 Aug 2020 at 00:49, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 16. 08. 20 5:39, Simon Glass wrote:

...

Hi Michal,

On Fri, 14 Aug 2020 at 07:28, Michal Simek monstr@monstr.eu wrote:

...

Hi Simon,

ne 19. 7. 2020 v 22:06 odesílatel Simon Glass sjg@chromium.org napsal: > > This option is used to run arch-specific shell scripts which produce .its > files which are used to produce FIT images. We already have binman which > is designed to produce firmware images. It is more powerful and has tests. > > So this option should be deprecated and not used. Existing uses should be > migrated. > > Mentions of this in code reviews over the last year or so do not seem to > have resulted in action, and things are getting worse. > > So let's add a warning. > > Signed-off-by: Simon Glass sjg@chromium.org > Reviewed-by: Bin Meng bmeng.cn@gmail.com > --- > > (no changes since v1) > > Makefile | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Makefile b/Makefile > index f1b5be1882..d73c10a973 100644 > --- a/Makefile > +++ b/Makefile > @@ -1148,6 +1148,13 @@ ifneq ($(CONFIG_DM_ETH),y) > @echo >&2 "See doc/driver-model/migration.rst for more info." > @echo >&2 "====================================================" > endif > +endif > +ifneq ($(CONFIG_SPL_FIT_GENERATOR),) > + @echo >&2 "===================== WARNING ======================" > + @echo >&2 "This board uses CONFIG_SPL_FIT_GENERATOR. Please migrate" > + @echo >&2 "to binman instead, to avoid the proliferation of" > + @echo >&2 "arch-specific scripts with no tests." > + @echo >&2 "====================================================" > endif > @# Check that this build does not use CONFIG options that we do not > @# know about unless they are in Kconfig. All the existing CONFIG > @@ -1345,6 +1352,8 @@ endif > > # Boards with more complex image requirements can provide an .its source file > # or a generator script > +# NOTE: Please do not use this. We are migrating away from Makefile rules to use > +# binman instead. > ifneq ($(CONFIG_SPL_FIT_SOURCE),"") > U_BOOT_ITS := u-boot.its > $(U_BOOT_ITS): $(subst ",,$(CONFIG_SPL_FIT_SOURCE)) > -- > 2.28.0.rc0.105.gf9edc3c819-goog >

I just got to this conversion and I am curious how that transition should look like. I found how FIT image is created which is fine but I didn't find any reference on how to generate images based on CONFIG_OF_LIST. If you look at arch/arm/mach-zynqmp/mkimage_fit_atf.sh you will see that I loop over this entry and create multiple DT nodes and the same amount of configurations to cover it. Is this supported by binman? If yes, what's the syntax for it?

The easiest way is probably to create a new entry type, like zynq-fit. Then you can generate the DT using the sequence writer functions. See _ReadSubNodes() in fit.py for an example.

You can perhaps have a template subnode and use that in a for loop to generate the nodes.

...

I tried several configurations and we can use that for generating qspi images and also images with different configurations to have them ready but first I need to be able to handle the case above.

I was thinking of converting sunxi which has the same need, but it sounds like you are on the case. Let me know if you need help.

Nope. I just saw that message and started to play with it to find out what needs to be done and how this fits to bigger picture. If this doesn't work directly then the work needs to be planned which will take time especially when this utility is new for us and we could have issues with writing code in python. Would be good if you can do the first shot because you know this utility and I am more than happy to test it, try and adopt if needed for our case.

Sunxi is very similar case as is zynqmp. Difference is they hardcode default configuration to config_1. ZynqMP is setting up default based on default DT configured at that time.

In connection to binman I see that there would be a need to generate images with ATF and without ATF in configuration node and with different default configuration. There could be also a need to add additional loadable entry such as bitstreams.

Back to zynq-fit new entry type. I don't think it should be zynq/zynqmp type because as was state in commit message u-boot.itb generation is very similar for all these boards that's why name for this new entry should be generic.

I sent an initial series to add this to binman. I've since found a few problems so will send a v2 at some point. You can try it out at u-boot-dm/binman-working

I looked at this branch and add my changes on the top.

The first thing what I see is that I miss fit,fdt-list = "of-list"; in sunxi dt file. I had to add it to work for me.

Ah yes, I decided to add this at the last minute so it is not relying on a convention.

...

With BINMAN_FDT enabled I am getting error that there is no valid "binman node" in DT. I didn't study that code yet but that's the point of keeping this DT node out there?

Is this in SPL? Perhaps something is filtering out the node.

Nope in full U-Boot but in SPL flow. SPL->ATF->FULL U-Boot.

...

This is my binman configuration.

diff --git a/arch/arm/dts/zynqmp-u-boot.dtsi b/arch/arm/dts/zynqmp-u-boot.dtsi new file mode 100644 index 000000000000..b3364d3e2df8 --- /dev/null +++ b/arch/arm/dts/zynqmp-u-boot.dtsi @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2020 Xilinx, Inc.
• */

+#include <config.h>

+/ {

•   binman: binman {
  

•           multiple-images;
  

•   };
  

+};

+&binman {

•   u-boot-itb {
  

•           filename = "u-boot.itb";
  

•           fit {
  

•                   fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
  

•                   description = "FIT image with ATF support";
  

•                   fit,fdt-list = "of-list";
  

•                   #address-cells = <1>;
  

•                   images {
  

•                           uboot {
  

•                                   description = "U-Boot (64-bit)";
  

•                                   type = "firmware";
  

•                                   os = "u-boot";
  

•                                   arch = "arm64";
  

•                                   compression = "none";
  

•                                   load = <CONFIG_SYS_TEXT_BASE>;
  

•                                   entry = <CONFIG_SYS_TEXT_BASE>;
  

•                                   u-boot-nodtb {
  

•                                   };
  

•                           };
  

•                           atf {
  

•                                   description = "ARM Trusted Firmware";
  

•                                   type = "firmware";
  

•                                   os = "arm-trusted-firmware";
  

•                                   arch = "arm64";
  

•                                   compression = "none";
  

•                                   load = <0xfffea000>; /* FIXME */
  

•                                   entry = <0xfffea000>;
  

•                                   blob-ext {
  

•                                           filename = "bl31.bin";
  

•                                   };
  

•                           };
  

•                           @fdt-SEQ {
  

•                                   description = "NAME";
  

•                                   type = "flat_dt";
  

•                                   arch = "arm64";
  

•                                   compression = "none";
  

•                           };
  

•                   };
  

•                   configurations {
  

•                           default = "config-1";
  

•                           @config-SEQ {
  

•                                   description = "NAME";
  

•                                   firmware = "atf";
  

•                                   loadables = "uboot";
  

•                                   fdt = "fdt-SEQ";
  

•                           };
  

•                   };
  

•           };
  

•           fdtmap{};
  

•   };
  

+};

Anyway compare to current script default option is hardcoded to config-1.

...

Current arch/arm/mach-zynqmp/mkimage_fit_atf.sh is also setting up default option based on selected default DT (I can fix this by implementing board_fit_config_name_match() but IIRC it is looping over all configurations and slowing down boot).

Is this using an environment variable to select the default? Would it be OK to put this in the DT for each individual board?

I have added this code to board_fit_config_name_match() to select proper configuration from SPL

+ if (!strcmp(name, DEVICE_TREE)) + return 0;

DEVICE_TREE is setup in generated/dt.h.

I am not quite sure what you mean by put this to each individual board. Like a property for SPL which DT should be select?

Thanks, Michal

Hi,

On 25. 08. 20 18:57, Simon Glass wrote:

Hi Michal,

On Tue, 25 Aug 2020 at 09:13, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 25. 08. 20 17:04, Simon Glass wrote:

...

Hi Michal,

On Mon, 24 Aug 2020 at 08:12, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 22. 08. 20 17:08, Simon Glass wrote:

...

Hi Michal,

On Mon, 17 Aug 2020 at 00:49, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 16. 08. 20 5:39, Simon Glass wrote: > Hi Michal, > > On Fri, 14 Aug 2020 at 07:28, Michal Simek monstr@monstr.eu wrote: >> >> Hi Simon, >> >> ne 19. 7. 2020 v 22:06 odesílatel Simon Glass sjg@chromium.org napsal: >>> >>> This option is used to run arch-specific shell scripts which produce .its >>> files which are used to produce FIT images. We already have binman which >>> is designed to produce firmware images. It is more powerful and has tests. >>> >>> So this option should be deprecated and not used. Existing uses should be >>> migrated. >>> >>> Mentions of this in code reviews over the last year or so do not seem to >>> have resulted in action, and things are getting worse. >>> >>> So let's add a warning. >>> >>> Signed-off-by: Simon Glass sjg@chromium.org >>> Reviewed-by: Bin Meng bmeng.cn@gmail.com >>> --- >>> >>> (no changes since v1) >>> >>> Makefile | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/Makefile b/Makefile >>> index f1b5be1882..d73c10a973 100644 >>> --- a/Makefile >>> +++ b/Makefile >>> @@ -1148,6 +1148,13 @@ ifneq ($(CONFIG_DM_ETH),y) >>> @echo >&2 "See doc/driver-model/migration.rst for more info." >>> @echo >&2 "====================================================" >>> endif >>> +endif >>> +ifneq ($(CONFIG_SPL_FIT_GENERATOR),) >>> + @echo >&2 "===================== WARNING ======================" >>> + @echo >&2 "This board uses CONFIG_SPL_FIT_GENERATOR. Please migrate" >>> + @echo >&2 "to binman instead, to avoid the proliferation of" >>> + @echo >&2 "arch-specific scripts with no tests." >>> + @echo >&2 "====================================================" >>> endif >>> @# Check that this build does not use CONFIG options that we do not >>> @# know about unless they are in Kconfig. All the existing CONFIG >>> @@ -1345,6 +1352,8 @@ endif >>> >>> # Boards with more complex image requirements can provide an .its source file >>> # or a generator script >>> +# NOTE: Please do not use this. We are migrating away from Makefile rules to use >>> +# binman instead. >>> ifneq ($(CONFIG_SPL_FIT_SOURCE),"") >>> U_BOOT_ITS := u-boot.its >>> $(U_BOOT_ITS): $(subst ",,$(CONFIG_SPL_FIT_SOURCE)) >>> -- >>> 2.28.0.rc0.105.gf9edc3c819-goog >>> >> >> I just got to this conversion and I am curious how that transition >> should look like. >> I found how FIT image is created which is fine but I didn't find any >> reference on how to generate images based on CONFIG_OF_LIST. >> If you look at arch/arm/mach-zynqmp/mkimage_fit_atf.sh you will see >> that I loop over this entry and create multiple DT nodes and the same >> amount of configurations to cover it. Is this supported by binman? >> If yes, what's the syntax for it? > > The easiest way is probably to create a new entry type, like zynq-fit. > Then you can generate the DT using the sequence writer functions. See > _ReadSubNodes() in fit.py for an example. > > You can perhaps have a template subnode and use that in a for loop to > generate the nodes. > >> >> I tried several configurations and we can use that for generating qspi >> images and also images with different configurations to have them >> ready >> but first I need to be able to handle the case above. > > I was thinking of converting sunxi which has the same need, but it > sounds like you are on the case. Let me know if you need help.

Nope. I just saw that message and started to play with it to find out what needs to be done and how this fits to bigger picture. If this doesn't work directly then the work needs to be planned which will take time especially when this utility is new for us and we could have issues with writing code in python. Would be good if you can do the first shot because you know this utility and I am more than happy to test it, try and adopt if needed for our case.

Sunxi is very similar case as is zynqmp. Difference is they hardcode default configuration to config_1. ZynqMP is setting up default based on default DT configured at that time.

In connection to binman I see that there would be a need to generate images with ATF and without ATF in configuration node and with different default configuration. There could be also a need to add additional loadable entry such as bitstreams.

Back to zynq-fit new entry type. I don't think it should be zynq/zynqmp type because as was state in commit message u-boot.itb generation is very similar for all these boards that's why name for this new entry should be generic.

I sent an initial series to add this to binman. I've since found a few problems so will send a v2 at some point. You can try it out at u-boot-dm/binman-working

I looked at this branch and add my changes on the top.

The first thing what I see is that I miss fit,fdt-list = "of-list"; in sunxi dt file. I had to add it to work for me.

Ah yes, I decided to add this at the last minute so it is not relying on a convention.

...

With BINMAN_FDT enabled I am getting error that there is no valid "binman node" in DT. I didn't study that code yet but that's the point of keeping this DT node out there?

Is this in SPL? Perhaps something is filtering out the node.

Nope in full U-Boot but in SPL flow. SPL->ATF->FULL U-Boot.

That needs debugging. I can't understand how the /binman node can be missing in U-Boot. The C library is very simple and doesn't handle finding nodes in multiple images...perhaps that is the problem?

I found the reason for this behavior. On our platforms we are checking specific address where DTB can be placed. And because I have played with it also with previous image. It was pick up automatically.

But still missing why BINMAN_FDT should be enabled by default on platforms which don't call any binman functions. I see that you are calling that functions from x86 platforms to try to map and find out some image offsets/positions and it looks like that you are loading them. I can imagine that this could be use for example for better space utilization that my boot.bin with SPL can be followed immediately by u-boot.itb in qspi. But for supporting this I expect spl_spi.c needs to be aligned. And adding support in a generic way there needs to be an agreement on node name which should be loaded.

...

...

...

This is my binman configuration.

diff --git a/arch/arm/dts/zynqmp-u-boot.dtsi b/arch/arm/dts/zynqmp-u-boot.dtsi new file mode 100644 index 000000000000..b3364d3e2df8 --- /dev/null +++ b/arch/arm/dts/zynqmp-u-boot.dtsi @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2020 Xilinx, Inc.
• */

+#include <config.h>

+/ {

•   binman: binman {
  

•           multiple-images;
  

•   };
  

+};

+&binman {

•   u-boot-itb {
  

•           filename = "u-boot.itb";
  

•           fit {
  

•                   fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
  

•                   description = "FIT image with ATF support";
  

•                   fit,fdt-list = "of-list";
  

•                   #address-cells = <1>;
  

•                   images {
  

•                           uboot {
  

•                                   description = "U-Boot (64-bit)";
  

•                                   type = "firmware";
  

•                                   os = "u-boot";
  

•                                   arch = "arm64";
  

•                                   compression = "none";
  

•                                   load = <CONFIG_SYS_TEXT_BASE>;
  

•                                   entry = <CONFIG_SYS_TEXT_BASE>;
  

•                                   u-boot-nodtb {
  

•                                   };
  

•                           };
  

•                           atf {
  

•                                   description = "ARM Trusted Firmware";
  

•                                   type = "firmware";
  

•                                   os = "arm-trusted-firmware";
  

•                                   arch = "arm64";
  

•                                   compression = "none";
  

•                                   load = <0xfffea000>; /* FIXME */
  

•                                   entry = <0xfffea000>;
  

•                                   blob-ext {
  

•                                           filename = "bl31.bin";
  

•                                   };
  

•                           };
  

•                           @fdt-SEQ {
  

•                                   description = "NAME";
  

•                                   type = "flat_dt";
  

•                                   arch = "arm64";
  

•                                   compression = "none";
  

•                           };
  

•                   };
  

•                   configurations {
  

•                           default = "config-1";
  

•                           @config-SEQ {
  

•                                   description = "NAME";
  

•                                   firmware = "atf";
  

•                                   loadables = "uboot";
  

•                                   fdt = "fdt-SEQ";
  

•                           };
  

•                   };
  

•           };
  

•           fdtmap{};
  

•   };
  

+};

Anyway compare to current script default option is hardcoded to config-1.

...

Current arch/arm/mach-zynqmp/mkimage_fit_atf.sh is also setting up default option based on selected default DT (I can fix this by implementing board_fit_config_name_match() but IIRC it is looping over all configurations and slowing down boot).

Is this using an environment variable to select the default? Would it be OK to put this in the DT for each individual board?

I have added this code to board_fit_config_name_match() to select proper configuration from SPL

•   if (!strcmp(name, DEVICE_TREE))
  

•           return 0;
  
  

DEVICE_TREE is setup in generated/dt.h.

I am not quite sure what you mean by put this to each individual board. Like a property for SPL which DT should be select?

OK I see. In that case I think we need another entry argument to pass ${DEVICE_TREE} to the fit entry, and pass it in to binman from the Makefile with another -a parameter.

Can you please include this option?

Also what's the easiest way to compose multiple images though binman and share images among others configurations?

I will generate u-boot.itb through binman and then I want to use this file for composing qspi image. Should I just point to u-boot.itb as blob with filename? Layout for qspi is quite simply spl/boot.bin which is generated as ./tools/mkimage -T zynqmpimage -R ./"" -n "/home/monstr/data/disk/u-boot-bins/zynqmp/pmu.bin" -d spl/u-boot-spl-align.bin spl/boot.bin >/dev/null && cat /dev/null

And then u-boot.itb placed at CONFIG_SYS_SPI_KERNEL_OFFS offset.

Thanks, Michal

Hi Simon,

On 30. 08. 20 22:37, Simon Glass wrote:

Hi Michal,

On Wed, 26 Aug 2020 at 08:12, Michal Simek michal.simek@xilinx.com wrote:

...

Hi,

On 25. 08. 20 18:57, Simon Glass wrote:

...

Hi Michal,

On Tue, 25 Aug 2020 at 09:13, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 25. 08. 20 17:04, Simon Glass wrote:

...

Hi Michal,

On Mon, 24 Aug 2020 at 08:12, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 22. 08. 20 17:08, Simon Glass wrote: > Hi Michal, > > On Mon, 17 Aug 2020 at 00:49, Michal Simek michal.simek@xilinx.com wrote: >> >> Hi Simon, >> >> On 16. 08. 20 5:39, Simon Glass wrote: >>> Hi Michal, >>> >>> On Fri, 14 Aug 2020 at 07:28, Michal Simek monstr@monstr.eu wrote: >>>> >>>> Hi Simon, >>>> >>>> ne 19. 7. 2020 v 22:06 odesílatel Simon Glass sjg@chromium.org napsal: >>>>> >>>>> This option is used to run arch-specific shell scripts which produce .its >>>>> files which are used to produce FIT images. We already have binman which >>>>> is designed to produce firmware images. It is more powerful and has tests. >>>>> >>>>> So this option should be deprecated and not used. Existing uses should be >>>>> migrated. >>>>> >>>>> Mentions of this in code reviews over the last year or so do not seem to >>>>> have resulted in action, and things are getting worse. >>>>> >>>>> So let's add a warning. >>>>> >>>>> Signed-off-by: Simon Glass sjg@chromium.org >>>>> Reviewed-by: Bin Meng bmeng.cn@gmail.com >>>>> --- >>>>> >>>>> (no changes since v1) >>>>> >>>>> Makefile | 9 +++++++++ >>>>> 1 file changed, 9 insertions(+) >>>>> >>>>> diff --git a/Makefile b/Makefile >>>>> index f1b5be1882..d73c10a973 100644 >>>>> --- a/Makefile >>>>> +++ b/Makefile >>>>> @@ -1148,6 +1148,13 @@ ifneq ($(CONFIG_DM_ETH),y) >>>>> @echo >&2 "See doc/driver-model/migration.rst for more info." >>>>> @echo >&2 "====================================================" >>>>> endif >>>>> +endif >>>>> +ifneq ($(CONFIG_SPL_FIT_GENERATOR),) >>>>> + @echo >&2 "===================== WARNING ======================" >>>>> + @echo >&2 "This board uses CONFIG_SPL_FIT_GENERATOR. Please migrate" >>>>> + @echo >&2 "to binman instead, to avoid the proliferation of" >>>>> + @echo >&2 "arch-specific scripts with no tests." >>>>> + @echo >&2 "====================================================" >>>>> endif >>>>> @# Check that this build does not use CONFIG options that we do not >>>>> @# know about unless they are in Kconfig. All the existing CONFIG >>>>> @@ -1345,6 +1352,8 @@ endif >>>>> >>>>> # Boards with more complex image requirements can provide an .its source file >>>>> # or a generator script >>>>> +# NOTE: Please do not use this. We are migrating away from Makefile rules to use >>>>> +# binman instead. >>>>> ifneq ($(CONFIG_SPL_FIT_SOURCE),"") >>>>> U_BOOT_ITS := u-boot.its >>>>> $(U_BOOT_ITS): $(subst ",,$(CONFIG_SPL_FIT_SOURCE)) >>>>> -- >>>>> 2.28.0.rc0.105.gf9edc3c819-goog >>>>> >>>> >>>> I just got to this conversion and I am curious how that transition >>>> should look like. >>>> I found how FIT image is created which is fine but I didn't find any >>>> reference on how to generate images based on CONFIG_OF_LIST. >>>> If you look at arch/arm/mach-zynqmp/mkimage_fit_atf.sh you will see >>>> that I loop over this entry and create multiple DT nodes and the same >>>> amount of configurations to cover it. Is this supported by binman? >>>> If yes, what's the syntax for it? >>> >>> The easiest way is probably to create a new entry type, like zynq-fit. >>> Then you can generate the DT using the sequence writer functions. See >>> _ReadSubNodes() in fit.py for an example. >>> >>> You can perhaps have a template subnode and use that in a for loop to >>> generate the nodes. >>> >>>> >>>> I tried several configurations and we can use that for generating qspi >>>> images and also images with different configurations to have them >>>> ready >>>> but first I need to be able to handle the case above. >>> >>> I was thinking of converting sunxi which has the same need, but it >>> sounds like you are on the case. Let me know if you need help. >> >> Nope. I just saw that message and started to play with it to find out >> what needs to be done and how this fits to bigger picture. If this >> doesn't work directly then the work needs to be planned which will take >> time especially when this utility is new for us and we could have issues >> with writing code in python. Would be good if you can do the first shot >> because you know this utility and I am more than happy to test it, try >> and adopt if needed for our case. >> >> Sunxi is very similar case as is zynqmp. Difference is they hardcode >> default configuration to config_1. ZynqMP is setting up default based on >> default DT configured at that time. >> >> In connection to binman I see that there would be a need to generate >> images with ATF and without ATF in configuration node and with different >> default configuration. There could be also a need to add additional >> loadable entry such as bitstreams. >> >> Back to zynq-fit new entry type. I don't think it should be zynq/zynqmp >> type because as was state in commit message u-boot.itb generation is >> very similar for all these boards that's why name for this new entry >> should be generic. >> > > I sent an initial series to add this to binman. I've since found a few > problems so will send a v2 at some point. You can try it out at > u-boot-dm/binman-working

I looked at this branch and add my changes on the top.

The first thing what I see is that I miss fit,fdt-list = "of-list"; in sunxi dt file. I had to add it to work for me.

Ah yes, I decided to add this at the last minute so it is not relying on a convention.

...

With BINMAN_FDT enabled I am getting error that there is no valid "binman node" in DT. I didn't study that code yet but that's the point of keeping this DT node out there?

Is this in SPL? Perhaps something is filtering out the node.

Nope in full U-Boot but in SPL flow. SPL->ATF->FULL U-Boot.

That needs debugging. I can't understand how the /binman node can be missing in U-Boot. The C library is very simple and doesn't handle finding nodes in multiple images...perhaps that is the problem?

I found the reason for this behavior. On our platforms we are checking specific address where DTB can be placed. And because I have played with it also with previous image. It was pick up automatically.

But still missing why BINMAN_FDT should be enabled by default on platforms which don't call any binman functions. I see that you are calling that functions from x86 platforms to try to map and find out some image offsets/positions and it looks like that you are loading them. I can imagine that this could be use for example for better space utilization that my boot.bin with SPL can be followed immediately by u-boot.itb in qspi. But for supporting this I expect spl_spi.c needs to be aligned. And adding support in a generic way there needs to be an agreement on node name which should be loaded.

If you don't need CONFIG_BINMAN_FDT then it is fine to disable it. You could do that by updating the default condition there, or selecting a different value for your arch.

...

...

...

...

...

This is my binman configuration.

diff --git a/arch/arm/dts/zynqmp-u-boot.dtsi b/arch/arm/dts/zynqmp-u-boot.dtsi new file mode 100644 index 000000000000..b3364d3e2df8 --- /dev/null +++ b/arch/arm/dts/zynqmp-u-boot.dtsi @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2020 Xilinx, Inc.
• */

+#include <config.h>

+/ {

•   binman: binman {
  

•           multiple-images;
  

•   };
  

+};

+&binman {

•   u-boot-itb {
  

•           filename = "u-boot.itb";
  

•           fit {
  

•                   fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
  

•                   description = "FIT image with ATF support";
  

•                   fit,fdt-list = "of-list";
  

•                   #address-cells = <1>;
  

•                   images {
  

•                           uboot {
  

•                                   description = "U-Boot (64-bit)";
  

•                                   type = "firmware";
  

•                                   os = "u-boot";
  

•                                   arch = "arm64";
  

•                                   compression = "none";
  

•                                   load = <CONFIG_SYS_TEXT_BASE>;
  

•                                   entry = <CONFIG_SYS_TEXT_BASE>;
  

•                                   u-boot-nodtb {
  

•                                   };
  

•                           };
  

•                           atf {
  

•                                   description = "ARM Trusted Firmware";
  

•                                   type = "firmware";
  

•                                   os = "arm-trusted-firmware";
  

•                                   arch = "arm64";
  

•                                   compression = "none";
  

•                                   load = <0xfffea000>; /* FIXME */
  

•                                   entry = <0xfffea000>;
  

•                                   blob-ext {
  

•                                           filename = "bl31.bin";
  

•                                   };
  

•                           };
  

•                           @fdt-SEQ {
  

•                                   description = "NAME";
  

•                                   type = "flat_dt";
  

•                                   arch = "arm64";
  

•                                   compression = "none";
  

•                           };
  

•                   };
  

•                   configurations {
  

•                           default = "config-1";
  

•                           @config-SEQ {
  

•                                   description = "NAME";
  

•                                   firmware = "atf";
  

•                                   loadables = "uboot";
  

•                                   fdt = "fdt-SEQ";
  

•                           };
  

•                   };
  

•           };
  

•           fdtmap{};
  

•   };
  

+};

Anyway compare to current script default option is hardcoded to config-1.

...

Current arch/arm/mach-zynqmp/mkimage_fit_atf.sh is also setting up default option based on selected default DT (I can fix this by implementing board_fit_config_name_match() but IIRC it is looping over all configurations and slowing down boot).

Is this using an environment variable to select the default? Would it be OK to put this in the DT for each individual board?

I have added this code to board_fit_config_name_match() to select proper configuration from SPL

•   if (!strcmp(name, DEVICE_TREE))
  

•           return 0;
  
  

DEVICE_TREE is setup in generated/dt.h.

I am not quite sure what you mean by put this to each individual board. Like a property for SPL which DT should be select?

OK I see. In that case I think we need another entry argument to pass ${DEVICE_TREE} to the fit entry, and pass it in to binman from the Makefile with another -a parameter.

Can you please include this option?

OK I'll add something in the v2 series.

Thanks.

...

Also what's the easiest way to compose multiple images though binman and share images among others configurations?

I will generate u-boot.itb through binman and then I want to use this file for composing qspi image. Should I just point to u-boot.itb as blob with filename?

At present binman doesn't support including one image in another, although since generation of images is ordered, yes it should be possible to do it that way.

ok.

...

Layout for qspi is quite simply spl/boot.bin which is generated as ./tools/mkimage -T zynqmpimage -R ./"" -n "/home/monstr/data/disk/u-boot-bins/zynqmp/pmu.bin" -d spl/u-boot-spl-align.bin spl/boot.bin >/dev/null && cat /dev/null

OK you should be able to use the mkimage entry-type for that.

Normally it should be enough mkimage { args = "-T zynqmpimage -R $CONFIG_BOOT_INIT_FILE"; u-boot-spl-align { }; };

It means I would have to create new type for u-boot-spl-align file. And second in argument there is $CONFIG_BOOT_INIT_FILE.

Anyway I can simply take spl/boot.bin which is generated already as the part of build and don't need to call mkimage from here.

...

And then u-boot.itb placed at CONFIG_SYS_SPI_KERNEL_OFFS offset.

OK, you can access CONFIG options in the .dtsi

Can you please point me to the docs for zynqmp in the U-Boot tree? I see stuff about zynq inthe tree but it is quite old. I think there needs to be a link from doc/board/xilinx

We haven't really spent any time on writing proper doc for zynqmp. But I normally just do this.

setup arm64 toochain make xilinx_zynqmp_virt_defconfig Fill CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE - which points to pmu configuration object. and CONFIG_PMUFW_INIT_FILE - which points to PMU binary file.

export DEVICE_TREE=zynqmp-zcu104-revC make -j8

you get u-boot.itb and spl/boot.bin. You copy them to SD card and boot from it. This will get you to u-boot running from EL3.

if you add bl31.bin to your source tree or point to it via BL31 variable itb is created with ATF inside and you will get to to SPL->ATF->U-Boot in EL2.

Also is there a low-cost zynqmp board about? I'm thinking about adding it to my lab.

The cheapest is likely ultra96/zcu100. But v1 is sold out and v2 is made by Avnet and they changed a lot of components and never invested time to really support this board upstream. But let me ask around if we have any v1 which we can share.

Finally, these boards seem to use CONFIG_OF_EMBED which is not allowed. Can you fix that sometime?

Only that mini configurations which is used only for memory tests or flash programming. Conversion to OF_SEPARATE should be easy but we need to retest all configurations again.

Thanks, Michal

Hi Simon,

On 01. 09. 20 17:26, Simon Glass wrote:

Hi Michal,

On Tue, 1 Sep 2020 at 09:15, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 30. 08. 20 22:37, Simon Glass wrote:

...

Hi Michal,

On Wed, 26 Aug 2020 at 08:12, Michal Simek michal.simek@xilinx.com wrote:

...

Hi,

On 25. 08. 20 18:57, Simon Glass wrote:

...

Hi Michal,

On Tue, 25 Aug 2020 at 09:13, Michal Simek michal.simek@xilinx.com wrote:

...

Hi Simon,

On 25. 08. 20 17:04, Simon Glass wrote: > Hi Michal, > > On Mon, 24 Aug 2020 at 08:12, Michal Simek michal.simek@xilinx.com wrote: >> >> Hi Simon, >> >> On 22. 08. 20 17:08, Simon Glass wrote: >>> Hi Michal, >>> >>> On Mon, 17 Aug 2020 at 00:49, Michal Simek michal.simek@xilinx.com wrote: >>>> >>>> Hi Simon, >>>> >>>> On 16. 08. 20 5:39, Simon Glass wrote: >>>>> Hi Michal, >>>>> >>>>> On Fri, 14 Aug 2020 at 07:28, Michal Simek monstr@monstr.eu wrote: >>>>>> >>>>>> Hi Simon, >>>>>> >>>>>> ne 19. 7. 2020 v 22:06 odesílatel Simon Glass sjg@chromium.org napsal: >>>>>>> >>>>>>> This option is used to run arch-specific shell scripts which produce .its >>>>>>> files which are used to produce FIT images. We already have binman which >>>>>>> is designed to produce firmware images. It is more powerful and has tests. >>>>>>> >>>>>>> So this option should be deprecated and not used. Existing uses should be >>>>>>> migrated. >>>>>>> >>>>>>> Mentions of this in code reviews over the last year or so do not seem to >>>>>>> have resulted in action, and things are getting worse. >>>>>>> >>>>>>> So let's add a warning. >>>>>>> >>>>>>> Signed-off-by: Simon Glass sjg@chromium.org >>>>>>> Reviewed-by: Bin Meng bmeng.cn@gmail.com >>>>>>> --- >>>>>>> >>>>>>> (no changes since v1) >>>>>>> >>>>>>> Makefile | 9 +++++++++ >>>>>>> 1 file changed, 9 insertions(+) >>>>>>> >>>>>>> diff --git a/Makefile b/Makefile >>>>>>> index f1b5be1882..d73c10a973 100644 >>>>>>> --- a/Makefile >>>>>>> +++ b/Makefile >>>>>>> @@ -1148,6 +1148,13 @@ ifneq ($(CONFIG_DM_ETH),y) >>>>>>> @echo >&2 "See doc/driver-model/migration.rst for more info." >>>>>>> @echo >&2 "====================================================" >>>>>>> endif >>>>>>> +endif >>>>>>> +ifneq ($(CONFIG_SPL_FIT_GENERATOR),) >>>>>>> + @echo >&2 "===================== WARNING ======================" >>>>>>> + @echo >&2 "This board uses CONFIG_SPL_FIT_GENERATOR. Please migrate" >>>>>>> + @echo >&2 "to binman instead, to avoid the proliferation of" >>>>>>> + @echo >&2 "arch-specific scripts with no tests." >>>>>>> + @echo >&2 "====================================================" >>>>>>> endif >>>>>>> @# Check that this build does not use CONFIG options that we do not >>>>>>> @# know about unless they are in Kconfig. All the existing CONFIG >>>>>>> @@ -1345,6 +1352,8 @@ endif >>>>>>> >>>>>>> # Boards with more complex image requirements can provide an .its source file >>>>>>> # or a generator script >>>>>>> +# NOTE: Please do not use this. We are migrating away from Makefile rules to use >>>>>>> +# binman instead. >>>>>>> ifneq ($(CONFIG_SPL_FIT_SOURCE),"") >>>>>>> U_BOOT_ITS := u-boot.its >>>>>>> $(U_BOOT_ITS): $(subst ",,$(CONFIG_SPL_FIT_SOURCE)) >>>>>>> -- >>>>>>> 2.28.0.rc0.105.gf9edc3c819-goog >>>>>>> >>>>>> >>>>>> I just got to this conversion and I am curious how that transition >>>>>> should look like. >>>>>> I found how FIT image is created which is fine but I didn't find any >>>>>> reference on how to generate images based on CONFIG_OF_LIST. >>>>>> If you look at arch/arm/mach-zynqmp/mkimage_fit_atf.sh you will see >>>>>> that I loop over this entry and create multiple DT nodes and the same >>>>>> amount of configurations to cover it. Is this supported by binman? >>>>>> If yes, what's the syntax for it? >>>>> >>>>> The easiest way is probably to create a new entry type, like zynq-fit. >>>>> Then you can generate the DT using the sequence writer functions. See >>>>> _ReadSubNodes() in fit.py for an example. >>>>> >>>>> You can perhaps have a template subnode and use that in a for loop to >>>>> generate the nodes. >>>>> >>>>>> >>>>>> I tried several configurations and we can use that for generating qspi >>>>>> images and also images with different configurations to have them >>>>>> ready >>>>>> but first I need to be able to handle the case above. >>>>> >>>>> I was thinking of converting sunxi which has the same need, but it >>>>> sounds like you are on the case. Let me know if you need help. >>>> >>>> Nope. I just saw that message and started to play with it to find out >>>> what needs to be done and how this fits to bigger picture. If this >>>> doesn't work directly then the work needs to be planned which will take >>>> time especially when this utility is new for us and we could have issues >>>> with writing code in python. Would be good if you can do the first shot >>>> because you know this utility and I am more than happy to test it, try >>>> and adopt if needed for our case. >>>> >>>> Sunxi is very similar case as is zynqmp. Difference is they hardcode >>>> default configuration to config_1. ZynqMP is setting up default based on >>>> default DT configured at that time. >>>> >>>> In connection to binman I see that there would be a need to generate >>>> images with ATF and without ATF in configuration node and with different >>>> default configuration. There could be also a need to add additional >>>> loadable entry such as bitstreams. >>>> >>>> Back to zynq-fit new entry type. I don't think it should be zynq/zynqmp >>>> type because as was state in commit message u-boot.itb generation is >>>> very similar for all these boards that's why name for this new entry >>>> should be generic. >>>> >>> >>> I sent an initial series to add this to binman. I've since found a few >>> problems so will send a v2 at some point. You can try it out at >>> u-boot-dm/binman-working >> >> I looked at this branch and add my changes on the top. >> >> The first thing what I see is that I miss fit,fdt-list = "of-list"; in >> sunxi dt file. I had to add it to work for me. > > Ah yes, I decided to add this at the last minute so it is not relying > on a convention. > >> >> With BINMAN_FDT enabled I am getting error that there is no valid >> "binman node" in DT. I didn't study that code yet but that's the point >> of keeping this DT node out there? > > Is this in SPL? Perhaps something is filtering out the node.

Nope in full U-Boot but in SPL flow. SPL->ATF->FULL U-Boot.

That needs debugging. I can't understand how the /binman node can be missing in U-Boot. The C library is very simple and doesn't handle finding nodes in multiple images...perhaps that is the problem?

I found the reason for this behavior. On our platforms we are checking specific address where DTB can be placed. And because I have played with it also with previous image. It was pick up automatically.

But still missing why BINMAN_FDT should be enabled by default on platforms which don't call any binman functions. I see that you are calling that functions from x86 platforms to try to map and find out some image offsets/positions and it looks like that you are loading them. I can imagine that this could be use for example for better space utilization that my boot.bin with SPL can be followed immediately by u-boot.itb in qspi. But for supporting this I expect spl_spi.c needs to be aligned. And adding support in a generic way there needs to be an agreement on node name which should be loaded.

If you don't need CONFIG_BINMAN_FDT then it is fine to disable it. You could do that by updating the default condition there, or selecting a different value for your arch.

...

...

...

> >> >> This is my binman configuration. >> >> diff --git a/arch/arm/dts/zynqmp-u-boot.dtsi >> b/arch/arm/dts/zynqmp-u-boot.dtsi >> new file mode 100644 >> index 000000000000..b3364d3e2df8 >> --- /dev/null >> +++ b/arch/arm/dts/zynqmp-u-boot.dtsi >> @@ -0,0 +1,72 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2020 Xilinx, Inc. >> + */ >> + >> +#include <config.h> >> + >> +/ { >> + binman: binman { >> + multiple-images; >> + }; >> +}; >> + >> +&binman { >> + u-boot-itb { >> + filename = "u-boot.itb"; >> + fit { >> + fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>; >> + description = "FIT image with ATF support"; >> + fit,fdt-list = "of-list"; >> + #address-cells = <1>; >> + >> + images { >> + uboot { >> + description = "U-Boot (64-bit)"; >> + type = "firmware"; >> + os = "u-boot"; >> + arch = "arm64"; >> + compression = "none"; >> + load = <CONFIG_SYS_TEXT_BASE>; >> + entry = <CONFIG_SYS_TEXT_BASE>; >> + >> + u-boot-nodtb { >> + }; >> + }; >> + atf { >> + description = "ARM Trusted Firmware"; >> + type = "firmware"; >> + os = "arm-trusted-firmware"; >> + arch = "arm64"; >> + compression = "none"; >> + load = <0xfffea000>; /* FIXME */ >> + entry = <0xfffea000>; >> + >> + blob-ext { >> + filename = "bl31.bin"; >> + }; >> + }; >> + @fdt-SEQ { >> + description = "NAME"; >> + type = "flat_dt"; >> + arch = "arm64"; >> + compression = "none"; >> + }; >> + }; >> + >> + configurations { >> + default = "config-1"; >> + @config-SEQ { >> + description = "NAME"; >> + firmware = "atf"; >> + loadables = "uboot"; >> + fdt = "fdt-SEQ"; >> + }; >> + }; >> + }; >> + fdtmap{}; >> + }; >> + >> +}; >> >> Anyway compare to current script default option is hardcoded to >> config-1. > > >> Current arch/arm/mach-zynqmp/mkimage_fit_atf.sh is also >> setting up default option based on selected default DT (I can fix this >> by implementing board_fit_config_name_match() but IIRC it is looping >> over all configurations and slowing down boot). > > Is this using an environment variable to select the default? Would it > be OK to put this in the DT for each individual board?

I have added this code to board_fit_config_name_match() to select proper configuration from SPL

•   if (!strcmp(name, DEVICE_TREE))
  

•           return 0;
  
  

DEVICE_TREE is setup in generated/dt.h.

I am not quite sure what you mean by put this to each individual board. Like a property for SPL which DT should be select?

OK I see. In that case I think we need another entry argument to pass ${DEVICE_TREE} to the fit entry, and pass it in to binman from the Makefile with another -a parameter.

Can you please include this option?

OK I'll add something in the v2 series.

Thanks.

...

...

Also what's the easiest way to compose multiple images though binman and share images among others configurations?

I will generate u-boot.itb through binman and then I want to use this file for composing qspi image. Should I just point to u-boot.itb as blob with filename?

At present binman doesn't support including one image in another, although since generation of images is ordered, yes it should be possible to do it that way.

ok.

...

...

Layout for qspi is quite simply spl/boot.bin which is generated as ./tools/mkimage -T zynqmpimage -R ./"" -n "/home/monstr/data/disk/u-boot-bins/zynqmp/pmu.bin" -d spl/u-boot-spl-align.bin spl/boot.bin >/dev/null && cat /dev/null

OK you should be able to use the mkimage entry-type for that.

Normally it should be enough mkimage { args = "-T zynqmpimage -R $CONFIG_BOOT_INIT_FILE"; u-boot-spl-align { }; };

It means I would have to create new type for u-boot-spl-align file. And second in argument there is $CONFIG_BOOT_INIT_FILE.

One option would be to allow args to be a list of strings, i.e. one string per argument. Then you could do:

args = "-T", "zynqmpimage", "-R", CONFIG_BOOT_INIT_FILE;

...

Anyway I can simply take spl/boot.bin which is generated already as the part of build and don't need to call mkimage from here.

The goal here is to get rid of arch-specific stuff in Makefile.

...

...

...

And then u-boot.itb placed at CONFIG_SYS_SPI_KERNEL_OFFS offset.

OK, you can access CONFIG options in the .dtsi

Can you please point me to the docs for zynqmp in the U-Boot tree? I see stuff about zynq inthe tree but it is quite old. I think there needs to be a link from doc/board/xilinx

We haven't really spent any time on writing proper doc for zynqmp. But I normally just do this.

setup arm64 toochain make xilinx_zynqmp_virt_defconfig Fill CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE - which points to pmu configuration object. and CONFIG_PMUFW_INIT_FILE - which points to PMU binary file.

export DEVICE_TREE=zynqmp-zcu104-revC make -j8

you get u-boot.itb and spl/boot.bin. You copy them to SD card and boot from it. This will get you to u-boot running from EL3.

if you add bl31.bin to your source tree or point to it via BL31 variable itb is created with ATF inside and you will get to to SPL->ATF->U-Boot in EL2.

Would you mind putting the above in a patch?

I have sent patches which updates documentation.

Thanks, Michal