[PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt device tree

[PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

U-Boot device tree is padded just after U-Boot proper.
After the whole stuff loaded to DRAM space, the device tree
area is conflict with BSS region before U-Boot relocation.

So any write to BSS area before reloc_fdt will corrupt the
device tree. Without the fix, there is issue that “binman_init
failed:-2” on i.MX8MP-EVK board.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 move the rev to malloc area in cpu_imx_plat
 tested on i.MX8MP EVK

 drivers/cpu/imx8_cpu.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c
index 6c0a8c0cbe4..b11f8dc0c91 100644
--- a/drivers/cpu/imx8_cpu.c
+++ b/drivers/cpu/imx8_cpu.c
@@ -20,10 +20,11 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+#define IMX_REV_LEN	4
 struct cpu_imx_plat {
 	const char *name;
-	const char *rev;
 	const char *type;
+	char rev[IMX_REV_LEN];
 	u32 cpu_rsrc;
 	u32 cpurev;
 	u32 freq_mhz;
@@ -69,28 +70,32 @@ static const char *get_imx_type_str(u32 imxtype)
 	}
 }
 
-static const char *get_imx_rev_str(u32 rev)
+static void get_imx_rev_str(struct cpu_imx_plat *plat, u32 rev)
 {
-	static char revision[4];
-
 	if (IS_ENABLED(CONFIG_IMX8)) {
+		char rev;
+
 		switch (rev) {
 		case CHIP_REV_A:
-			return "A";
+			rev = 'A';
+			break;
 		case CHIP_REV_B:
-			return "B";
+			rev = 'B';
+			break;
 		case CHIP_REV_C:
-			return "C";
+			rev = 'C';
+			break;
 		default:
-			return "?";
+			rev = '?';
+			break;
 		}
+		plat->rev[0] = rev;
+		plat->rev[1] = '\0';
 	} else {
-		revision[0] = '1' + (((rev & 0xf0) - CHIP_REV_1_0) >> 4);
-		revision[1] = '.';
-		revision[2] = '0' + (rev & 0xf);
-		revision[3] = '\0';
-
-		return revision;
+		plat->rev[0] = '1' + (((rev & 0xf0) - CHIP_REV_1_0) >> 4);
+		plat->rev[1] = '.';
+		plat->rev[2] = '0' + (rev & 0xf);
+		plat->rev[3] = '\0';
 	}
 }
 
@@ -318,7 +323,7 @@ static int imx_cpu_probe(struct udevice *dev)
 	set_core_data(dev);
 	cpurev = get_cpu_rev();
 	plat->cpurev = cpurev;
-	plat->rev = get_imx_rev_str(cpurev & 0xFFF);
+	get_imx_rev_str(plat, cpurev & 0xFFF);
 	plat->type = get_imx_type_str((cpurev & 0x1FF000) >> 12);
 	plat->freq_mhz = imx_get_cpu_rate(dev) / 1000000;
 	plat->mpidr = dev_read_addr(dev);
-- 
2.35.3

Re: [PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Lothar Waßmann]

Hi,

On Thu, 17 Oct 2024 16:12:36 +0800 Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> U-Boot device tree is padded just after U-Boot proper.
> After the whole stuff loaded to DRAM space, the device tree
> area is conflict with BSS region before U-Boot relocation.
> 
> So any write to BSS area before reloc_fdt will corrupt the
> device tree. Without the fix, there is issue that “binman_init
> failed:-2” on i.MX8MP-EVK board.
> 
Placing the 'revision' variable into the 'data' section would achieve
the same goal without any further code change:
static char revision[4] __section(".data");

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>  move the rev to malloc area in cpu_imx_plat
>  tested on i.MX8MP EVK
> 
>  drivers/cpu/imx8_cpu.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c
> index 6c0a8c0cbe4..b11f8dc0c91 100644
> --- a/drivers/cpu/imx8_cpu.c
> +++ b/drivers/cpu/imx8_cpu.c
> @@ -20,10 +20,11 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +#define IMX_REV_LEN	4
>  struct cpu_imx_plat {
>  	const char *name;
> -	const char *rev;
>  	const char *type;
> +	char rev[IMX_REV_LEN];
>  	u32 cpu_rsrc;
>  	u32 cpurev;
>  	u32 freq_mhz;
> @@ -69,28 +70,32 @@ static const char *get_imx_type_str(u32 imxtype)
>  	}
>  }
>  
> -static const char *get_imx_rev_str(u32 rev)
> +static void get_imx_rev_str(struct cpu_imx_plat *plat, u32 rev)
>  {
> -	static char revision[4];
> -
>  	if (IS_ENABLED(CONFIG_IMX8)) {
> +		char rev;
> +
>  		switch (rev) {
>  		case CHIP_REV_A:
> -			return "A";
> +			rev = 'A';
> +			break;
>  		case CHIP_REV_B:
> -			return "B";
> +			rev = 'B';
> +			break;
>  		case CHIP_REV_C:
> -			return "C";
> +			rev = 'C';
> +			break;
>  		default:
> -			return "?";
> +			rev = '?';
> +			break;
>  		}
> +		plat->rev[0] = rev;
> +		plat->rev[1] = '\0';
>  	} else {
> -		revision[0] = '1' + (((rev & 0xf0) - CHIP_REV_1_0) >> 4);
> -		revision[1] = '.';
> -		revision[2] = '0' + (rev & 0xf);
> -		revision[3] = '\0';
> -
> -		return revision;
> +		plat->rev[0] = '1' + (((rev & 0xf0) - CHIP_REV_1_0) >> 4);
> +		plat->rev[1] = '.';
> +		plat->rev[2] = '0' + (rev & 0xf);
> +		plat->rev[3] = '\0';
>  	}
>  }
>  
> @@ -318,7 +323,7 @@ static int imx_cpu_probe(struct udevice *dev)
>  	set_core_data(dev);
>  	cpurev = get_cpu_rev();
>  	plat->cpurev = cpurev;
> -	plat->rev = get_imx_rev_str(cpurev & 0xFFF);
> +	get_imx_rev_str(plat, cpurev & 0xFFF);
>  	plat->type = get_imx_type_str((cpurev & 0x1FF000) >> 12);
>  	plat->freq_mhz = imx_get_cpu_rate(dev) / 1000000;
>  	plat->mpidr = dev_read_addr(dev);


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

RE: [PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Peng Fan]

> Subject: Re: [PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt
> device tree
> 
> Hi,
> 
> On Thu, 17 Oct 2024 16:12:36 +0800 Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > U-Boot device tree is padded just after U-Boot proper.
> > After the whole stuff loaded to DRAM space, the device tree area is
> > conflict with BSS region before U-Boot relocation.
> >
> > So any write to BSS area before reloc_fdt will corrupt the device
> > tree. Without the fix, there is issue that "binman_init failed:-2" on
> > i.MX8MP-EVK board.
> >
> Placing the 'revision' variable into the 'data' section would achieve the
> same goal without any further code change:
> static char revision[4] __section(".data");

This was not welcomed,
https://lore.kernel.org/all/20241017034507.GJ4959@bill-the-cat/

Regards,
Peng.
> 
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V2:
> >  move the rev to malloc area in cpu_imx_plat  tested on i.MX8MP
> EVK
> >
> >  drivers/cpu/imx8_cpu.c | 35 ++++++++++++++++++++---------------
> >  1 file changed, 20 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c index
> > 6c0a8c0cbe4..b11f8dc0c91 100644
> > --- a/drivers/cpu/imx8_cpu.c
> > +++ b/drivers/cpu/imx8_cpu.c
> > @@ -20,10 +20,11 @@
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > +#define IMX_REV_LEN	4
> >  struct cpu_imx_plat {
> >  	const char *name;
> > -	const char *rev;
> >  	const char *type;
> > +	char rev[IMX_REV_LEN];
> >  	u32 cpu_rsrc;
> >  	u32 cpurev;
> >  	u32 freq_mhz;
> > @@ -69,28 +70,32 @@ static const char *get_imx_type_str(u32
> imxtype)
> >  	}
> >  }
> >
> > -static const char *get_imx_rev_str(u32 rev)
> > +static void get_imx_rev_str(struct cpu_imx_plat *plat, u32 rev)
> >  {
> > -	static char revision[4];
> > -
> >  	if (IS_ENABLED(CONFIG_IMX8)) {
> > +		char rev;
> > +
> >  		switch (rev) {
> >  		case CHIP_REV_A:
> > -			return "A";
> > +			rev = 'A';
> > +			break;
> >  		case CHIP_REV_B:
> > -			return "B";
> > +			rev = 'B';
> > +			break;
> >  		case CHIP_REV_C:
> > -			return "C";
> > +			rev = 'C';
> > +			break;
> >  		default:
> > -			return "?";
> > +			rev = '?';
> > +			break;
> >  		}
> > +		plat->rev[0] = rev;
> > +		plat->rev[1] = '\0';
> >  	} else {
> > -		revision[0] = '1' + (((rev & 0xf0) - CHIP_REV_1_0) >> 4);
> > -		revision[1] = '.';
> > -		revision[2] = '0' + (rev & 0xf);
> > -		revision[3] = '\0';
> > -
> > -		return revision;
> > +		plat->rev[0] = '1' + (((rev & 0xf0) - CHIP_REV_1_0) >>
> 4);
> > +		plat->rev[1] = '.';
> > +		plat->rev[2] = '0' + (rev & 0xf);
> > +		plat->rev[3] = '\0';
> >  	}
> >  }
> >
> > @@ -318,7 +323,7 @@ static int imx_cpu_probe(struct udevice *dev)
> >  	set_core_data(dev);
> >  	cpurev = get_cpu_rev();
> >  	plat->cpurev = cpurev;
> > -	plat->rev = get_imx_rev_str(cpurev & 0xFFF);
> > +	get_imx_rev_str(plat, cpurev & 0xFFF);
> >  	plat->type = get_imx_type_str((cpurev & 0x1FF000) >> 12);
> >  	plat->freq_mhz = imx_get_cpu_rate(dev) / 1000000;
> >  	plat->mpidr = dev_read_addr(dev);
> 
> 
> Lothar Waßmann
> --
> ___________________________________________________________
> 
> Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
> Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
> Geschäftsführer: Matthias Kaussen
> Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
> 
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.karo-
> electronics.de%2F&data=05%7C02%7Cpeng.fan%40nxp.com%7Ce7321
> 3a6bf874b3f204f08dcee8cd46a%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C638647535847647409%7CUnknown%7CTWFpbG
> Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ
> XVCI6Mn0%3D%7C0%7C%7C%7C&sdata=jsnPkChbN7l4UlMJonwJhkn
> NBNwIPJD7c3t1j%2Bu%2FDiA%3D&reserved=0 | info@karo-
> electronics.de
> ___________________________________________________________

Re: [PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Lothar Waßmann]

Hi,

On Thu, 17 Oct 2024 09:21:54 +0000 Peng Fan wrote:
> > Subject: Re: [PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt
> > device tree
> > 
> > Hi,
> > 
> > On Thu, 17 Oct 2024 16:12:36 +0800 Peng Fan (OSS) wrote:  
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > U-Boot device tree is padded just after U-Boot proper.
> > > After the whole stuff loaded to DRAM space, the device tree area is
> > > conflict with BSS region before U-Boot relocation.
> > >
> > > So any write to BSS area before reloc_fdt will corrupt the device
> > > tree. Without the fix, there is issue that "binman_init failed:-2" on
> > > i.MX8MP-EVK board.
> > >  
> > Placing the 'revision' variable into the 'data' section would achieve the
> > same goal without any further code change:
> > static char revision[4] __section(".data");  
> 
> This was not welcomed,
> https://lore.kernel.org/all/20241017034507.GJ4959@bill-the-cat/
> 
Thanks, good to know.



Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Fabio Estevam]

On Thu, Oct 17, 2024 at 4:08 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> U-Boot device tree is padded just after U-Boot proper.
> After the whole stuff loaded to DRAM space, the device tree
> area is conflict with BSS region before U-Boot relocation.
>
> So any write to BSS area before reloc_fdt will corrupt the
> device tree. Without the fix, there is issue that “binman_init
> failed:-2” on i.MX8MP-EVK board.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>
> V2:
>  move the rev to malloc area in cpu_imx_plat
>  tested on i.MX8MP EVK

Please adapt the commit log to include this information.

This patch breaks CI. Make sure to run all your patches through CI:

https://source.denx.de/u-boot/custodians/u-boot-imx/-/jobs/920875

aarch64: + imx8qm_rom7720_a1_4G
321+WARNING 'mx8qm-ahab-container.img' not found, resulting binary is
not-functional
322+drivers/cpu/imx8_cpu.c: In function 'get_imx_rev_str':
323+drivers/cpu/imx8_cpu.c:78:17: error: 'rev' is used uninitialized
[-Werror=uninitialized]
324+ 78 | switch (rev) {
325+ | ^~~~~~
326+drivers/cpu/imx8_cpu.c:76:22: note: 'rev' was declared here
327+ 76 | char rev;
328+ | ^~~
329+cc1: all warnings being treated as errors

I am getting too much extra work to process the patches from NXP folks
and I am not happy with it.

Re: [PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

On Thu, Oct 17, 2024 at 09:50:22AM -0300, Fabio Estevam wrote:
> On Thu, Oct 17, 2024 at 4:08 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > U-Boot device tree is padded just after U-Boot proper.
> > After the whole stuff loaded to DRAM space, the device tree
> > area is conflict with BSS region before U-Boot relocation.
> >
> > So any write to BSS area before reloc_fdt will corrupt the
> > device tree. Without the fix, there is issue that “binman_init
> > failed:-2” on i.MX8MP-EVK board.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V2:
> >  move the rev to malloc area in cpu_imx_plat
> >  tested on i.MX8MP EVK
> 
> Please adapt the commit log to include this information.
> 
> This patch breaks CI. Make sure to run all your patches through CI:
> 
> https://source.denx.de/u-boot/custodians/u-boot-imx/-/jobs/920875
> 
> aarch64: + imx8qm_rom7720_a1_4G
> 321+WARNING 'mx8qm-ahab-container.img' not found, resulting binary is
> not-functional
> 322+drivers/cpu/imx8_cpu.c: In function 'get_imx_rev_str':
> 323+drivers/cpu/imx8_cpu.c:78:17: error: 'rev' is used uninitialized
> [-Werror=uninitialized]
> 324+ 78 | switch (rev) {
> 325+ | ^~~~~~
> 326+drivers/cpu/imx8_cpu.c:76:22: note: 'rev' was declared here
> 327+ 76 | char rev;
> 328+ | ^~~
> 329+cc1: all warnings being treated as errors
> 
> I am getting too much extra work to process the patches from NXP folks
> and I am not happy with it.

Azure can be slow, yes, but I would expect NXP to be able to setup their
own internal GitLab instance and runners, or paid Azure setup, or even
just use your own GitHub account and then own Azure account and not be
using the shared free pool of runners.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Peng Fan]

> Subject: Re: [PATCH V2] cpu: imx8_cpu: Avoid revision to corrupt
> device tree
> 
> On Thu, Oct 17, 2024 at 4:08 AM Peng Fan (OSS)
> <peng.fan@oss.nxp.com> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > U-Boot device tree is padded just after U-Boot proper.
> > After the whole stuff loaded to DRAM space, the device tree area is
> > conflict with BSS region before U-Boot relocation.
> >
> > So any write to BSS area before reloc_fdt will corrupt the device
> > tree. Without the fix, there is issue that “binman_init failed:-2” on
> > i.MX8MP-EVK board.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V2:
> >  move the rev to malloc area in cpu_imx_plat  tested on i.MX8MP
> EVK
> 
> Please adapt the commit log to include this information.
> 
> This patch breaks CI. Make sure to run all your patches through CI:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> source.denx.de%2Fu-boot%2Fcustodians%2Fu-boot-imx%2F-
> %2Fjobs%2F920875&data=05%7C02%7Cpeng.fan%40nxp.com%7C385
> 8971a1966454c3c5c08dceeaa4b47%7C686ea1d3bc2b4c6fa92cd99c5
> c301635%7C0%7C0%7C638647662428479157%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw
> iLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=AcYrq2JFg1BPegpK18%2
> B7KsaD4ZoxIAqGX8ZYlGmUB2o%3D&reserved=0
> 
> aarch64: + imx8qm_rom7720_a1_4G
> 321+WARNING 'mx8qm-ahab-container.img' not found, resulting
> binary is
> not-functional
> 322+drivers/cpu/imx8_cpu.c: In function 'get_imx_rev_str':
> 323+drivers/cpu/imx8_cpu.c:78:17: error: 'rev' is used uninitialized
> [-Werror=uninitialized]
> 324+ 78 | switch (rev) {
> 325+ | ^~~~~~
> 326+drivers/cpu/imx8_cpu.c:76:22: note: 'rev' was declared here
> 327+ 76 | char rev;
> 328+ | ^~~
> 329+cc1: all warnings being treated as errors
> 
> I am getting too much extra work to process the patches from NXP
> folks and I am not happy with it.

Sorry for this. I was too confident with the patch (:
I will take care.

-Peng.