powerpc/85xx: fix compatible property for the L2 cache node

PATCH Superseded Timur Tabi 3 participants 5 messages Apr 29, 2011 PowerPC

Share & tools

Timur Tabi

author

2011-04-29 21:51 UTC #0 lore raw

PATCH v4 1/1 powerpc/85xx: fix compatible property for the L2 cache node

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 <[email protected]>

---
 arch/powerpc/cpu/mpc85xx/fdt.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

arch/powerpc/cpu/mpc85xx/fdt.c +12 -8

index 642f6c5..121db5f 100644
@@ -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 */
 }

Signature · 1 line

1.7.3.4

Wolfgang Denk

2011-04-29 22:21 UTC #1 lore raw

PATCH v4 1/1 powerpc/85xx: fix compatible property for the L2 cache node

Dear Timur Tabi,

Quoted 30 lines · In message <[email protected]> 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 <[email protected]>

---
 arch/powerpc/cpu/mpc85xx/fdt.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

arch/powerpc/cpu/mpc85xx/fdt.c +2 -3

index 642f6c5..121db5f 100644
@@ -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);

arch/powerpc/cpu/mpc85xx/fdt.c static inline void ft_fixup_l2cache(void *blob) · line 203

Quoted 10 lines

arch/powerpc/cpu/mpc85xx/fdt.c +6 -3

+			/* 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.

arch/powerpc/cpu/mpc85xx/fdt.c static inline void ft_fixup_l2cache(void *blob) · line 206

Quoted 2 lines

arch/powerpc/cpu/mpc85xx/fdt.c +2 -0

+		/* 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

Signature · 5 lines

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_

Tabi Timur-B04825

2011-04-29 22:38 UTC #2 lore raw

PATCH v4 1/1 powerpc/85xx: fix compatible property for the L2 cache node

Quoted 2 lines · Wolfgang Denk

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:

	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".

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
+ /* 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.

Signature · 2 lines

Timur Tabi
Linux kernel developer at Freescale

Wolfgang Denk

2011-04-29 22:51 UTC #3 lore raw

PATCH v4 1/1 powerpc/85xx: fix compatible property for the L2 cache node

Dear Tabi Timur-B04825,

Quoted 8 lines · In message <[email protected]> you wrote:

Quoted 2 lines · Wolfgang Denk
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

Quoted 8 lines

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

Signature · 5 lines

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.

Tabi Timur-B04825

2011-04-29 22:58 UTC #4 lore raw

PATCH v4 1/1 powerpc/85xx: fix compatible property for the L2 cache node

Quoted 1 line · Wolfgang Denk

This gets lost in the code.

That's why I added the comments:

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

Quoted 2 lines

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

Ok.

Signature · 2 lines

Timur Tabi
Linux kernel developer at Freescale