Re: [PATCH v2 2/2] ARM: imx: imx8mn-evk: use one common u-boot.dtsi for the evk boards
Hi Alper. Am Mi., 22. Juni 2022 um 20:18 Uhr schrieb Alper Nebi Yasak : > > 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 > > Reviewed-by: Fabio Estevam > > --- > > 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. I have implemented your comments to prepare the dtb files for uniform use and will send an updated version. [SNIP] Thanks -- Heiko
Re: [PATCH v2 2/2] ARM: imx: imx8mn-evk: use one common u-boot.dtsi for the evk boards
On 10/06/2022 00:38, Heiko Thiery wrote:
> Hi Tim, Hi Simon,
>
> [SNIP]
>
>>
>> Heiko,
>>
>> You can add multi-dtb support to this so that it's usable by the other
>> imx8mn boards with the following:
>>
>> [...]
>>
>> I don't mind sending this as a follow-up to your patch here.
>
> Since this patch moves the parts from the 2 imx8mn-evk boards to one
> "common" file it would be better to do more changes on that in a
> separate patch.
>
>> It looks like there are only the following boards in mainline that
>> would benefit from using this shared include:
>> imx8mn-beacon-kit-u-boot.dtsi
>> imx8mn-var-som-symphony-u-boot.dtsi
>> imx8mn-venice-u-boot.dtsi
There's also 'imx8mn-bsh-smm-s2-u-boot-common.dtsi'. Slightly different
because only half of the blobs are there with IMX8M_DDR3L.
>> Have you compared the binman portions of imx8m{m,n,p}-u-boot.dtsi?
>
> No not yet.
I looked a bit and they look very much alike. I suspect it's possible to
eventually unify everything into a shared 'imx8m-u-boot.dtsi', but I
didn't actually try.
>> There are a lot of differences due to different property ordering and
>> label/node naming conventions. I would like to see these normalized
>> but i'm not clear which is the best example to normalize to.
>> Specifically I don't know:
>> 1. what is the convention for property ordering in dt... is it simply
>> alphabetical order?
AFAIK property order doesn't matter for binman. Node order is very
significant though. For names, I think we should:
- Prefer hyphens to underscores
- Prefer lowercase to uppercase
- Prefer meaningful names to things like 'blob-ext@1'
- Avoid first char being a number (because labels can't have that)
>> 2. have we settled on a convention for the blob naming, if so what is
>> the best example?
I had suggested 'ddr-1d-imem-fw' etc. on a series from Peng, see latest
version of it [1].
[1] arm: dts: imx8m: update binman ddr firmware node name
https://lore.kernel.org/u-boot/20220603071715.15212-4-peng@oss.nxp.com/
> I am not aware that there is a conventional here. But maybe simon can
> give some hints here.
>
Re: [PATCH v2 2/2] ARM: imx: imx8mn-evk: use one common u-boot.dtsi for the evk boards
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
> Reviewed-by: Fabio Estevam
> ---
> 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 = <>;
> @@ -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.
>
> {
> @@ -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 00..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;
> + };
> +};
> +
> + {
> + 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 = ;
> +
> + images {
> + uboot {
> + description = "U-Boot (64-bit)";
> + type = "standalone";
> + arch = "arm64";
> + compression = "none";
> + load = ;
> +
> + uboot_blob: blob-ext {
> + filename = "u-boot-nodtb.bin";
> + };
> + };
> +
> + atf {
> + description = "ARM Trusted Firmware";
> + type = "firmware";
> + arch = "arm64";
> + compression = "none";
> + load = <0x96>;
> + entry = <0x96>;
> +
> + atf_blob: blob-ext {
> + filename = "bl31.bin";
> + };
> + };
> +
> + fdt {
> + description = "NAME";
> + type = "flat_dt";
> + compression = "none";
> +
> + uboot_fdt_blob: blob-ext {
> +
Re: [PATCH v2 2/2] ARM: imx: imx8mn-evk: use one common u-boot.dtsi for the evk boards
Hi Tim, Hi Simon,
[SNIP]
>
> Heiko,
>
> You can add multi-dtb support to this so that it's usable by the other
> imx8mn boards with the following:
> diff --git a/arch/arm/dts/imx8mn-u-boot.dtsi b/arch/arm/dts/imx8mn-u-boot.dtsi
> index 7b591085a0be..af6697b1efbc 100644
> --- a/arch/arm/dts/imx8mn-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-u-boot.dtsi
> @@ -38,6 +38,7 @@
>
> fit {
> description = "Configuration to load ATF before
> U-Boot";
> + fit,fdt-list = "of-list";
> #address-cells = <1>;
> fit,external-offset = ;
>
> @@ -67,7 +68,7 @@
> };
> };
>
> - fdt {
> + @fdt-SEQ {
> description = "NAME";
> type = "flat_dt";
> compression = "none";
> @@ -79,13 +80,13 @@
> };
>
> configurations {
> - default = "conf";
> + default = "@config-DEFAULT-SEQ";
>
> - conf {
> + binman_configuration: @config-SEQ {
> description = "NAME";
> firmware = "uboot";
> loadables = "atf";
> - fdt = "fdt";
> + fdt = "fdt-SEQ";
> };
> };
> };
>
> I don't mind sending this as a follow-up to your patch here.
Since this patch moves the parts from the 2 imx8mn-evk boards to one
"common" file it would be better to do more changes on that in a
separate patch.
> It looks like there are only the following boards in mainline that
> would benefit from using this shared include:
> imx8mn-beacon-kit-u-boot.dtsi
> imx8mn-var-som-symphony-u-boot.dtsi
> imx8mn-venice-u-boot.dtsi
>
> Have you compared the binman portions of imx8m{m,n,p}-u-boot.dtsi?
No not yet.
> There are a lot of differences due to different property ordering and
> label/node naming conventions. I would like to see these normalized
> but i'm not clear which is the best example to normalize to.
> Specifically I don't know:
> 1. what is the convention for property ordering in dt... is it simply
> alphabetical order?
> 2. have we settled on a convention for the blob naming, if so what is
> the best example?
I am not aware that there is a conventional here. But maybe simon can
give some hints here.
--
Heiko
--
Heiko
Re: [PATCH v2 2/2] ARM: imx: imx8mn-evk: use one common u-boot.dtsi for the evk boards
On Thu, Jun 9, 2022 at 1:50 PM 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
> Reviewed-by: Fabio Estevam
> ---
> 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 +++
> 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 = <>;
> @@ -143,122 +141,3 @@
> {
> u-boot,dm-spl;
> };
> -
> - {
> -u-boot-spl-ddr {
> - filename = "u-boot-spl-ddr.bin";
> - pad-byte = <0xff>;
> - align-size = <4>;
> - align = <4>;
> -
> - u-boot-spl {
> - align-end = <4>;
> - };
> -
> - 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>;
> - };
> - };
> -
> -
> - 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 = ;
> -
> - images {
> - uboot {
> - description = "U-Boot (64-bit)";
> - type = "standalone";
> - arch = "arm64";
> - compression = "none";
> - load = ;
> -
> - uboot_blob: blob-ext {
> - filename = "u-boot-nodtb.bin";
> - };
> - };
> -
> - atf {
> - description = "ARM Trusted Firmware";
> - type = "firmware";
> - arch = "arm64";
> - compression = "none";
> - load = <0x96>;
> - entry = <0x96>;
> -
> - atf_blob: blob-ext {
> - filename = "bl31.bin";
> - };
> - };
> -
> - fdt {
> - description = "NAME";
> - type = "flat_dt";
> - compression = "none";
> -
> - uboot_fdt_blob: blob-ext {
> - filename = "u-boot.dtb";
> - };
> - };
> - };
> -
> - configurations {
> - default = "conf";
> -
> - conf {
> - description = "NAME";
> - firmware = "uboot";
> - loadables = "atf";
> -
[PATCH v2 2/2] ARM: imx: imx8mn-evk: use one common u-boot.dtsi for the evk boards
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
Reviewed-by: Fabio Estevam
---
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 +++
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 = <>;
@@ -143,122 +141,3 @@
{
u-boot,dm-spl;
};
-
- {
-u-boot-spl-ddr {
- filename = "u-boot-spl-ddr.bin";
- pad-byte = <0xff>;
- align-size = <4>;
- align = <4>;
-
- u-boot-spl {
- align-end = <4>;
- };
-
- 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>;
- };
- };
-
-
- 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 = ;
-
- images {
- uboot {
- description = "U-Boot (64-bit)";
- type = "standalone";
- arch = "arm64";
- compression = "none";
- load = ;
-
- uboot_blob: blob-ext {
- filename = "u-boot-nodtb.bin";
- };
- };
-
- atf {
- description = "ARM Trusted Firmware";
- type = "firmware";
- arch = "arm64";
- compression = "none";
- load = <0x96>;
- entry = <0x96>;
-
- atf_blob: blob-ext {
- filename = "bl31.bin";
- };
- };
-
- fdt {
- description = "NAME";
- type = "flat_dt";
- compression = "none";
-
- uboot_fdt_blob: blob-ext {
- filename = "u-boot.dtb";
- };
- };
- };
-
- configurations {
- default = "conf";
-
- conf {
- description = "NAME";
- firmware = "uboot";
- loadables = "atf";
- fdt = "fdt";
- };
- };
- };
- };
-
- imx-boot {
- filename = "flash.bin";
- pad-byte = <0x00>;
-
- spl: blob-ext@1 {
- offset = <0x0>;
-
