Re: [PATCH V4 2/8] arm: dts: imx8m: update binman ddr firmware node name

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> We are migrating to use BINMAN SYMBOLS, the current name is not
> a valid binman type, so update to use blob-ext@[1,2,3,4].
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/dts/imx8mm-u-boot.dtsi                   | 8 ++++----
>  arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi | 4 ++--
>  arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi  | 8 ++++----
>  arch/arm/dts/imx8mn-venice-u-boot.dtsi            | 8 ++++----
>  arch/arm/dts/imx8mq-u-boot.dtsi                   | 8 ++++----
>  5 files changed, 18 insertions(+), 18 deletions(-)

I think descriptive entry names are better in general, so I prefer the
old names to a bunch of 'blob-ext's. Symbol resolution is done using the
entry names anyway, I don't see why this change would be necessary.

In any case, the names and labels should be consistent across the
different dts files, assuming the blobs have the same purpose / meaning.
I see that labels starting with digits cause a dtc parse error, so the
entries could be `imem_1d: imem-1d { ... };` and so on.

Also, the blobs essentially look the same to me aside from their
filenames, just duplicated over different imx8m* variants and boards.
Maybe binman parts could be unified into a single dtsi file later on?
(Filename differences can be handled by new 'blob_named_by_arg's btw.)

> diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
> index 9f66cdb65a9..5de55a2d80b 100644
> --- a/arch/arm/dts/imx8mm-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mm-u-boot.dtsi
> @@ -39,25 +39,25 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		imem_1d: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d-dmem {
> +		dmem_1d: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d-imem {
> +		imem_2d: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d-dmem {
> +		dmem_2d: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
> diff --git a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> index 46a9d7fd78b..5a52b73d7e9 100644
> --- a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> +++ b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> @@ -111,13 +111,13 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		imem_1d: blob-ext@1 {
>  			filename = "ddr3_imem_1d.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d_dmem {
> +		dmem_1d: blob-ext@2 {
>  			filename = "ddr3_dmem_1d.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
> diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> index 6e37622cca7..001e725f568 100644
> --- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> @@ -130,25 +130,25 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		blob_1: blob-ext@1 {
>  			filename = "ddr4_imem_1d.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d_dmem {
> +		blob_2: blob-ext@2 {
>  			filename = "ddr4_dmem_1d.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d_imem {
> +		blob_3: blob-ext@3 {
>  			filename = "ddr4_imem_2d.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d_dmem {
> +		blob_4: blob-ext@4 {
>  			filename = "ddr4_dmem_2d.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
> diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> index 35819553879..67922146963 100644
> --- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> @@ -126,25 +126,25 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		imem_1d: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d_dmem {
> +		dmem_1d: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d_imem {
> +		imem_2d: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d_dmem {
> +		dmem_2d: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
> diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi b/arch/arm/dts/imx8mq-u-boot.dtsi
> index 912a3d4a356..389414ad26f 100644
> --- a/arch/arm/dts/imx8mq-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mq-u-boot.dtsi
> @@ -46,25 +46,25 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		imem_1d: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d-dmem {
> +		dmem_1d: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d-imem {
> +		imem_2d: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d-dmem {
> +		dmem_2d: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";