On 09/06/2022 23:49, Heiko Thiery wrote: > To have only one place to describe the binman images us the > imx8mn-u-boot.dtsi. To have support for different DDR firmwares this > nodes are included dependent on the used DDR config option. > > Signed-off-by: Heiko Thiery <heiko.thi...@gmail.com> > Reviewed-by: Fabio Estevam <feste...@gmail.com> > --- > v2: sync with current master and fix merge conflict > > arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi | 125 +----------------- > arch/arm/dts/imx8mn-evk-u-boot.dtsi | 120 +---------------- > arch/arm/dts/imx8mn-u-boot.dtsi | 156 +++++++++++++++++++++++
I see most of this is moving existing definitions, but I'm adding comments in a general sense, partially for future reference. > 3 files changed, 159 insertions(+), 242 deletions(-) > create mode 100644 arch/arm/dts/imx8mn-u-boot.dtsi > > diff --git a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi > b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi > index 4d0ecb07d4..3d0e817313 100644 > --- a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi > +++ b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi > @@ -3,11 +3,9 @@ > * Copyright 2019, 2021 NXP > */ > > -/ { > - binman: binman { > - multiple-images; > - }; > +#include "imx8mn-u-boot.dtsi" > > +/ { > wdt-reboot { > compatible = "wdt-reboot"; > wdt = <&wdog1>; > @@ -143,122 +141,3 @@ > [...] > diff --git a/arch/arm/dts/imx8mn-evk-u-boot.dtsi > b/arch/arm/dts/imx8mn-evk-u-boot.dtsi > index d1427941eb..339c3dd681 100644 > --- a/arch/arm/dts/imx8mn-evk-u-boot.dtsi > +++ b/arch/arm/dts/imx8mn-evk-u-boot.dtsi > @@ -3,6 +3,7 @@ > * Copyright 2019 NXP > */ > > +#include "imx8mn-u-boot.dtsi" > #include "imx8mn-ddr4-evk-u-boot.dtsi" "imx8mn-ddr4-evk-u-boot.dtsi" already includes "imx8mn-u-boot.dtsi". I think it should only be included once. I don't know why these are structured this way, but I think common parts of the two boards should've been in a "imx8mn-evk-common-u-boot.dtsi" or something which the board -u-boot.dtsi files would include. > > &i2c1 { > @@ -24,122 +25,3 @@ > [...] > diff --git a/arch/arm/dts/imx8mn-u-boot.dtsi b/arch/arm/dts/imx8mn-u-boot.dtsi > new file mode 100644 > index 0000000000..7b591085a0 > --- /dev/null > +++ b/arch/arm/dts/imx8mn-u-boot.dtsi > @@ -0,0 +1,156 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2019 NXP > + */ > + > +/ { > + binman: binman { > + multiple-images; > + }; > +}; > + > +&binman { > + u_boot_spl_ddr: u-boot-spl-ddr { > + filename = "u-boot-spl-ddr.bin"; > + pad-byte = <0xff>; > + align-size = <4>; > + align = <4>; > + > + u-boot-spl { > + align-end = <4>; > + }; Maybe it's better to add an empty 'ddr-fw' section here, and populate that below. > + }; > + > + spl { > + filename = "spl.bin"; > + > + mkimage { > + args = "-n spl/u-boot-spl.cfgout -T imx8mimage -e > 0x912000"; > + > + blob { > + filename = "u-boot-spl-ddr.bin"; > + }; > + }; > + }; > + > + itb { > + filename = "u-boot.itb"; > + > + fit { > + description = "Configuration to load ATF before U-Boot"; > + #address-cells = <1>; > + fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>; > + > + images { > + uboot { > + description = "U-Boot (64-bit)"; > + type = "standalone"; > + arch = "arm64"; > + compression = "none"; > + load = <CONFIG_SYS_TEXT_BASE>; > + > + uboot_blob: blob-ext { > + filename = "u-boot-nodtb.bin"; > + }; > + }; > + > + atf { > + description = "ARM Trusted Firmware"; > + type = "firmware"; > + arch = "arm64"; > + compression = "none"; > + load = <0x960000>; > + entry = <0x960000>; > + > + atf_blob: blob-ext { > + filename = "bl31.bin"; > + }; > + }; > + > + fdt { > + description = "NAME"; > + type = "flat_dt"; > + compression = "none"; > + > + uboot_fdt_blob: blob-ext { > + filename = "u-boot.dtb"; > + }; > + }; These three 'blob-ext's would normally be 'u-boot-nodtb', 'atf-bl31', and 'u-boot-dtb' entries respectively. > + }; > + > + configurations { > + default = "conf"; > + > + conf { > + description = "NAME"; This 'NAME' (and the fdt one) shows up in `mkimage -l u-boot.itb` output. Perhaps they were originally copied from a @fdt-SEQ version (which replaces it with the actual dtb filename). > + firmware = "uboot"; > + loadables = "atf"; > + fdt = "fdt"; > + }; > + }; > + }; > + }; > + > + imx-boot { > + filename = "flash.bin"; > + pad-byte = <0x00>; > + > + spl: blob-ext@1 { > + offset = <0x0>; > + filename = "spl.bin"; > + }; > + > + uboot: blob-ext@2 { > + offset = <0x58000>; > + filename = "u-boot.itb"; > + }; > + }; I dislike having 'blob-ext's as names, they should be descriptive. 'imx8mm-u-boot.dtsi' has these labels as node names which is slightly better. > +}; > + > +#ifdef CONFIG_IMX8M_DDR4 > +&u_boot_spl_ddr { > + blob_1: blob-ext@1 { > + filename = "ddr4_imem_1d_201810.bin"; > + size = <0x8000>; > + }; > + > + blob_2: blob-ext@2 { > + filename = "ddr4_dmem_1d_201810.bin"; > + size = <0x4000>; > + }; > + > + blob_3: blob-ext@3 { > + filename = "ddr4_imem_2d_201810.bin"; > + size = <0x8000>; > + }; > + > + blob_4: blob-ext@4 { > + filename = "ddr4_dmem_2d_201810.bin"; > + size = <0x4000>; > + }; > +}; > +#elif CONFIG_IMX8M_LPDDR4 > +&u_boot_spl_ddr { > + blob_1: blob-ext@1 { > + filename = "lpddr4_pmu_train_1d_imem.bin"; > + size = <0x8000>; > + }; > + > + blob_2: blob-ext@2 { > + filename = "lpddr4_pmu_train_1d_dmem.bin"; > + size = <0x4000>; > + }; > + > + blob_3: blob-ext@3 { > + filename = "lpddr4_pmu_train_2d_imem.bin"; > + size = <0x8000>; > + }; > + > + blob_4: blob-ext@4 { > + filename = "lpddr4_pmu_train_2d_dmem.bin"; > + size = <0x4000>; > + }; > +}; These conflict a bit with one of Peng's series which changes the names and removes the size for these blobs, see [1] [2]. [1] arm: dts: imx8m: update binman ddr firmware node name https://lore.kernel.org/u-boot/20220603071715.15212-4-peng....@oss.nxp.com/ [2] arm: dts: imx8m: shrink ddr firmware size to actual file size https://lore.kernel.org/u-boot/20220603071715.15212-7-peng....@oss.nxp.com/ > +#else > + #error "no valid ddr config selected" There's also IMX8M_DDR3L, see imx8mn-bsh-smm-s2-u-boot-common.dtsi > +#endif