[U-Boot] [PATCH] [v4] powerpc/85xx: fix compatible property for the L2 cache node

[U-Boot] [PATCH] [v4] powerpc/85xx: fix compatible property for the L2 cache node

[Timur Tabi]

The compatible property for the L2 cache node (on 85xx systems that don't
have a CPC) was using a value for the property length that did not match
the actual length of the property.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 arch/powerpc/cpu/mpc85xx/fdt.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c
index 642f6c5..121db5f 100644
--- a/arch/powerpc/cpu/mpc85xx/fdt.c
+++ b/arch/powerpc/cpu/mpc85xx/fdt.c
@@ -165,7 +165,6 @@ static inline void ft_fixup_l2cache(void *blob)
 	int len, off;
 	u32 *ph;
 	struct cpu_type *cpu = identify_cpu(SVR_SOC_VER(get_svr()));
-	char compat_buf[38];
 
 	const u32 line_size = 32;
 	const u32 num_ways = 8;
@@ -192,22 +191,27 @@ static inline void ft_fixup_l2cache(void *blob)
 	}
 
 	if (cpu) {
+		char buf[40];
+
 		if (isdigit(cpu->name[0]))
-			len = sprintf(compat_buf,
-				"fsl,mpc%s-l2-cache-controller", cpu->name);
+			/* MPCxxxx, where xxxx == 4-digit number */
+			len = sprintf(buf, "fsl,mpc%s-l2-cache-controller",
+				cpu->name) + 1;
 		else
-			len = sprintf(compat_buf,
-				"fsl,%c%s-l2-cache-controller",
-				tolower(cpu->name[0]), cpu->name + 1);
+			/* Pxxxx or Txxxx, where xxxx == 4-digit number */
+			len = sprintf(buf, "fsl,%c%s-l2-cache-controller",
+				tolower(cpu->name[0]), cpu->name + 1) + 1;
+
+		/* append "cache" to the string */
+		len += sprintf(buf + len, "cache") + 1;
 
-		sprintf(&compat_buf[len + 1], "cache");
+		fdt_setprop(blob, off, "compatible", buf, len);
 	}
 	fdt_setprop(blob, off, "cache-unified", NULL, 0);
 	fdt_setprop_cell(blob, off, "cache-block-size", line_size);
 	fdt_setprop_cell(blob, off, "cache-size", size);
 	fdt_setprop_cell(blob, off, "cache-sets", num_sets);
 	fdt_setprop_cell(blob, off, "cache-level", 2);
-	fdt_setprop(blob, off, "compatible", compat_buf, sizeof(compat_buf));
 
 	/* we dont bother w/L3 since no platform of this type has one */
 }
-- 
1.7.3.4

[U-Boot] [PATCH] [v4] powerpc/85xx: fix compatible property for the L2 cache node

[Wolfgang Denk]

Dear Timur Tabi,

In message <1304113875-6749-1-git-send-email-timur@freescale.com> you wrote:
> The compatible property for the L2 cache node (on 85xx systems that don't
> have a CPC) was using a value for the property length that did not match
> the actual length of the property.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  arch/powerpc/cpu/mpc85xx/fdt.c |   20 ++++++++++++--------
>  1 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c
> index 642f6c5..121db5f 100644
> --- a/arch/powerpc/cpu/mpc85xx/fdt.c
> +++ b/arch/powerpc/cpu/mpc85xx/fdt.c
> @@ -165,7 +165,6 @@ static inline void ft_fixup_l2cache(void *blob)
>  	int len, off;
>  	u32 *ph;
>  	struct cpu_type *cpu = identify_cpu(SVR_SOC_VER(get_svr()));
> -	char compat_buf[38];
>  
>  	const u32 line_size = 32;
>  	const u32 num_ways = 8;
> @@ -192,22 +191,27 @@ static inline void ft_fixup_l2cache(void *blob)
>  	}
>  
>  	if (cpu) {
> +		char buf[40];
> +
>  		if (isdigit(cpu->name[0]))
> -			len = sprintf(compat_buf,
> -				"fsl,mpc%s-l2-cache-controller", cpu->name);
> +			/* MPCxxxx, where xxxx == 4-digit number */
> +			len = sprintf(buf, "fsl,mpc%s-l2-cache-controller",
> +				cpu->name) + 1;
>  		else
> -			len = sprintf(compat_buf,
> -				"fsl,%c%s-l2-cache-controller",
> -				tolower(cpu->name[0]), cpu->name + 1);
> +			/* Pxxxx or Txxxx, where xxxx == 4-digit number */
> +			len = sprintf(buf, "fsl,%c%s-l2-cache-controller",
> +				tolower(cpu->name[0]), cpu->name + 1) + 1;

Why do we need this "if" at all? tolower() on a digit is a nop, so you
can omit the first branch.

> +		/* append "cache" to the string */
> +		len += sprintf(buf + len, "cache") + 1;

This is wrong and misleading.  This is not an operation on a C string.
You do not "append" (or concatenate) the string cache.  You build a
specifically structured data set, which is not a C string.  So please
don't call it a string.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Speculation is always more interesting than facts.
                                    - Terry Pratchett, _Making_Money_

[U-Boot] [PATCH] [v4] powerpc/85xx: fix compatible property for the L2 cache node

[Tabi Timur-B04825]

Wolfgang Denk wrote:
> Why do we need this "if" at all? tolower() on a digit is a nop, so you
> can omit the first branch.

Because cpu->name looks like one of two ways:

	8578

or
	
	P4080

In the case of "8578", we want to convert that to "mpc8578".  In the case of "P4080", we want to convert that to "p4080".

The "if" is need to determine whether to prepend the "mpc".

>> >  +		/* append "cache" to the string */
>> >  +		len += sprintf(buf + len, "cache") + 1;
> This is wrong and misleading.  This is not an operation on a C string.
> You do not "append" (or concatenate) the string cache.  You build a
> specifically structured data set, which is not a C string.  So please
> don't call it a string.

Ok.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] [v4] powerpc/85xx: fix compatible property for the L2 cache node

[Wolfgang Denk]

Dear Tabi Timur-B04825,

In message <4DBB3E01.7000704@freescale.com> you wrote:
> Wolfgang Denk wrote:
> > Why do we need this "if" at all? tolower() on a digit is a nop, so you
> > can omit the first branch.
> 
> Because cpu->name looks like one of two ways:
> 
> 	8578
> 
> or
> 	
> 	P4080
> 
> In the case of "8578", we want to convert that to "mpc8578".  In the case o
> f "P4080", we want to convert that to "p4080".
> 
> The "if" is need to determine whether to prepend the "mpc".

This gets lost in the code.

If you keep the if / else, you have to add braces for the multiline
statements.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Always try to do things in chronological order; it's  less  confusing
that way.

[U-Boot] [PATCH] [v4] powerpc/85xx: fix compatible property for the L2 cache node

[Tabi Timur-B04825]

Wolfgang Denk wrote:
> This gets lost in the code.

That's why I added the comments:

/* MPCxxxx, where xxxx == 4-digit number */

> If you keep the if / else, you have to add braces for the multiline
> statements.

Ok.

-- 
Timur Tabi
Linux kernel developer at Freescale