Sequoia kernel crash workaround.

I've observed kernel crashes on Sequoia board several times. Kernel often crashes with high network/memory loads. This is due to 440EPx h/w issue.

According to the errata:
CHIP_8: Incorrect Write to DDR SDRAM.
Category: 3
Overview:
The write address can be corrupted during writes to DDR SDRAM when write pipelining is enabled on PLB0. The probability of encountering the problem increases with the amount of back-pressure applied by the SDRAM.
Impact: Data can be written to the wrong address in SDRAM. Workaround:
Disable write pipelining to DDR SDRAM by setting PLB4A0_ACR[WRP] = 0. Only primary write transfers are broadcast and the write address corruption is avoided.

Disabling write pipelining really helps.
Josh, David, what is the right place to put this workaround to?

Is it OK to do mtdcr(PLB4A0_ACR, mfdcr(PLB4A0_ACR) & ~PLB4_WRP); in arch/powerpc/boot/cuboot-sequoia.c:sequoia_fixups()?
or
should this be done in arch/powerpc/platforms/44x/sequoia.c:sequoia_setup_arch() with dcr_map, dcr_read/write stuff?

Disabling write pipelining really helps.
Josh, David, what is the right place to put this workaround to?

Is it OK to do mtdcr(PLB4A0_ACR, mfdcr(PLB4A0_ACR) & ~PLB4_WRP); in arch/powerpc/boot/cuboot-sequoia.c:sequoia_fixups()?
or
should this be done in arch/powerpc/platforms/44x/sequoia.c:sequoia_setup_arch() with dcr_map, dcr_read/write stuff?

Hi Valentine,

Quoted 9 lines · Valentine Barshak

Disabling write pipelining really helps.
Josh, David, what is the right place to put this workaround to?

Is it OK to do mtdcr(PLB4A0_ACR, mfdcr(PLB4A0_ACR) & ~PLB4_WRP); in arch/powerpc/boot/cuboot-sequoia.c:sequoia_fixups()?
or
should this be done in arch/powerpc/platforms/44x/sequoia.c:sequoia_setup_arch() with dcr_map, dcr_read/write stuff?

I vote for putting it into sequoia.c, since it's very likely to happen that Sequoia will at one point be booted without the bootwrapper. Or perhaps it should go into some common code checking the PVR and disabling it when this 440EPx/GRx is detected, since all those boards are affected.

Quoted 13 lines · Stefan Roese

Hi Valentine,

Quoted 9 lines · Valentine Barshak

Disabling write pipelining really helps.
Josh, David, what is the right place to put this workaround to?

Is it OK to do mtdcr(PLB4A0_ACR, mfdcr(PLB4A0_ACR) & ~PLB4_WRP); in arch/powerpc/boot/cuboot-sequoia.c:sequoia_fixups()?
or
should this be done in arch/powerpc/platforms/44x/sequoia.c:sequoia_setup_arch() with dcr_map, dcr_read/write stuff?

Quoted 4 lines

I vote for putting it into sequoia.c, since it's very likely to happen that Sequoia will at one point be booted without the bootwrapper. Or perhaps it should go into some common code checking the PVR and disabling it when this 440EPx/GRx is detected, since all those boards are affected.

This is what we have setup_cpu functions in the cpu table for. Please put it there instead of in board code.

Quoted 13 lines · On Wed, 19 Sep 2007 14:30:24 -0500 Olof Johansson <[email protected]> wrote:

Quoted 12 lines · Stefan Roese

Hi Valentine,

Quoted 9 lines · Valentine Barshak

Disabling write pipelining really helps.
Josh, David, what is the right place to put this workaround to?

Is it OK to do mtdcr(PLB4A0_ACR, mfdcr(PLB4A0_ACR) & ~PLB4_WRP); in arch/powerpc/boot/cuboot-sequoia.c:sequoia_fixups()?
or
should this be done in arch/powerpc/platforms/44x/sequoia.c:sequoia_setup_arch() with dcr_map, dcr_read/write stuff?

Quoted 3 lines

Quoted 1 line

I vote for putting it into sequoia.c, since it's very likely to happen that Sequoia will at one point be booted without the bootwrapper. Or perhaps it should go into some common code checking the PVR and disabling it when this 440EPx/GRx is detected, since all those boards are affected.

This is what we have setup_cpu functions in the cpu table for. Please put it there instead of in board code.

Yes, agreed.

josh
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

I was thinking about it. Looks like it's the best place, but the code that actually calls setup_cpu is under ifdef CONFIG_PPC64, while lots of cpu_setup functions are defined for ppc32 processors.
Is it OK to remove this ifdef, or should I do CONFIG_PPC64 || CONFIG_44x?

Quoted 5 lines · Valentine Barshak

I was thinking about it. Looks like it's the best place, but the code that actually calls setup_cpu is under ifdef CONFIG_PPC64, while lots of cpu_setup functions are defined for ppc32 processors.
Is it OK to remove this ifdef, or should I do CONFIG_PPC64 || CONFIG_44x?

Sounds like something that went wrong at the merge of ppc and ppc64.

Take out the ifdef, even if there's fallout we should deal with it instead of adding more complex ifdefs.

Quoted 23 lines · Josh Boyer

Quoted 19 lines · On Wed, 19 Sep 2007 14:30:24 -0500 Olof Johansson <olof at lixom.net> wrote:

Quoted 12 lines · Stefan Roese

Hi Valentine,

Quoted 9 lines · Valentine Barshak

Disabling write pipelining really helps.
Josh, David, what is the right place to put this workaround to?

Is it OK to do mtdcr(PLB4A0_ACR, mfdcr(PLB4A0_ACR) & ~PLB4_WRP); in arch/powerpc/boot/cuboot-sequoia.c:sequoia_fixups()?
or
should this be done in arch/powerpc/platforms/44x/sequoia.c:sequoia_setup_arch() with dcr_map, dcr_read/write stuff?

Quoted 4 lines

I vote for putting it into sequoia.c, since it's very likely to happen that Sequoia will at one point be booted without the bootwrapper. Or perhaps it should go into some common code checking the PVR and disabling it when this 440EPx/GRx is detected, since all those boards are affected.

This is what we have setup_cpu functions in the cpu table for. Please put it there instead of in board code.

Quoted 1 line

Yes, agreed.

I was thinking about it. Looks like it's the best place, but the code that actually calls setup_cpu is under ifdef CONFIG_PPC64, while lots of cpu_setup functions are defined for ppc32 processors.
Is it OK to remove this ifdef, or should I do CONFIG_PPC64 || CONFIG_44x?

Quoted 5 lines · Valentine Barshak

I was thinking about it. Looks like it's the best place, but the code that actually calls setup_cpu is under ifdef CONFIG_PPC64, while lots of cpu_setup functions are defined for ppc32 processors.
Is it OK to remove this ifdef, or should I do CONFIG_PPC64 || CONFIG_44x?

Sounds like something that went wrong at the merge of ppc and ppc64.

Take out the ifdef, even if there's fallout we should deal with it instead of adding more complex ifdefs.

Quoted 24 lines · Valentine Barshak

Quoted 23 lines · Josh Boyer

Quoted 19 lines · On Wed, 19 Sep 2007 14:30:24 -0500 Olof Johansson <olof at lixom.net> wrote:

Quoted 12 lines · Stefan Roese

Hi Valentine,

Quoted 9 lines · Valentine Barshak

Disabling write pipelining really helps.
Josh, David, what is the right place to put this workaround to?

Is it OK to do mtdcr(PLB4A0_ACR, mfdcr(PLB4A0_ACR) & ~PLB4_WRP); in arch/powerpc/boot/cuboot-sequoia.c:sequoia_fixups()?
or
should this be done in arch/powerpc/platforms/44x/sequoia.c:sequoia_setup_arch() with dcr_map, dcr_read/write stuff?

Quoted 4 lines

I vote for putting it into sequoia.c, since it's very likely to happen that Sequoia will at one point be booted without the bootwrapper. Or perhaps it should go into some common code checking the PVR and disabling it when this 440EPx/GRx is detected, since all those boards are affected.

This is what we have setup_cpu functions in the cpu table for. Please put it there instead of in board code.

Quoted 6 lines

Quoted 1 line

Yes, agreed.

I was thinking about it. Looks like it's the best place, but the code that actually calls setup_cpu is under ifdef CONFIG_PPC64, while lots of cpu_setup functions are defined for ppc32 processors.
Is it OK to remove this ifdef, or should I do CONFIG_PPC64 || CONFIG_44x?

head_32.S calls call_setup_cpu in misc_32.S to call the cpu setup functon.

Note that these functions are called before the kernel is copied down to 0, so on ppc32 you will need the PTRRELOC type stuff. Also the callsite implies that the cpu number is available in r24, which may or may not be true when called from C.

Its probably easier to just call call_setup_cpu in the other 32 bit head_xxx files.

This adds cpu_setup functionality to PowerPC 44x platform. The cpu_setup callback is invoked by head_32 code and the identify_cpu() function at early init and is used to initialize FPU on 440EP(x) processors. The FPU initialization was previously done in head_44x.S. Also a workaround for the incorrect write to DDR SDRAM 440EPx/440GRx errata added. Data can be written to wrong address in SDRAM when write pipelining is enabled on plb0. The setup_cpu function for these processors disables write pipelining.

Quoted 31 lines · On Thu, 20 Sep 2007 12:32:56 -0500 (CDT) Milton Miller <[email protected]> wrote:

Quoted 17 lines · Valentine Barshak

Quoted 16 lines · Josh Boyer

Quoted 13 lines · On Wed, 19 Sep 2007 14:30:24 -0500 Olof Johansson <olof at lixom.net> wrote:

Quoted 12 lines · Stefan Roese

Hi Valentine,

Quoted 9 lines · Valentine Barshak

Disabling write pipelining really helps.
Josh, David, what is the right place to put this workaround to?

Is it OK to do mtdcr(PLB4A0_ACR, mfdcr(PLB4A0_ACR) & ~PLB4_WRP); in arch/powerpc/boot/cuboot-sequoia.c:sequoia_fixups()?
or
should this be done in arch/powerpc/platforms/44x/sequoia.c:sequoia_setup_arch() with dcr_map, dcr_read/write stuff?

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

I vote for putting it into sequoia.c, since it's very likely to happen that Sequoia will at one point be booted without the bootwrapper. Or perhaps it should go into some common code checking the PVR and disabling it when this 440EPx/GRx is detected, since all those boards are affected.

This is what we have setup_cpu functions in the cpu table for. Please put it there instead of in board code.

Yes, agreed.

I was thinking about it. Looks like it's the best place, but the code that actually calls setup_cpu is under ifdef CONFIG_PPC64, while lots of cpu_setup functions are defined for ppc32 processors. Is it OK to remove this ifdef, or should I do CONFIG_PPC64 || CONFIG_44x?

head_32.S calls call_setup_cpu in misc_32.S to call the cpu setup functon.

Note that these functions are called before the kernel is copied down to 0, so on ppc32 you will need the PTRRELOC type stuff. Also the callsite implies that the cpu number is available in r24, which may or may not be true when called from C.

Quoted 1 line

Its probably easier to just call call_setup_cpu in the other 32 bit head_xxx files.

Hm. I'll have to see how well that would work for 4xx. Seems 8xx and FSL BookE are in a similar situation.

josh

Quoted 4 lines · On Thu, 20 Sep 2007 21:55:58 +0400 Valentine Barshak <[email protected]>…

This adds cpu_setup functionality to PowerPC 44x platform.
The cpu_setup callback is invoked by head_32 code and the identify_cpu() function at early init and is used to initialize FPU on 440EP(x) processors. The FPU initialization was previously done in head_44x.S. Also a workaround for the incorrect write to DDR SDRAM 440EPx/440GRx errata added. Data can be written to wrong address in SDRAM when write pipelining is enabled on plb0. The setup_cpu function
for these processors disables write pipelining.

Wow, you're fast ;)

I'd like to see this split up a bit so there's not so much in one patch. Essentially, introduce cpu_setup_44x.S, move the FPU init to use it, and then add the PLB workaround.

I still need to look at the code too :)

josh

diff -ruN linux-2.6.orig/arch/powerpc/kernel/cputable.c linux-2.6/arch/powerpc/kernel/cputable.c
--- linux-2.6.orig/arch/powerpc/kernel/cputable.c 2007-09-20
19:30:47.000000000 +0400
+++ linux-2.6/arch/powerpc/kernel/cputable.c 2007-09-20
21:27:35.000000000 +0400

arch/powerpc/kernel/cputable.c +3 -6

@@ -1318,18 +1327,14 @@
 
 	for (i = 0; i < ARRAY_SIZE(cpu_specs); i++,s++)
 		if ((pvr & s->pvr_mask) == s->pvr_value) {
+			cpu_setup_t setup_func  = PTRRELOC(s->cpu_setup);
+
 			*cur = cpu_specs + i;
-#ifdef CONFIG_PPC64
-			/* ppc64 expects identify_cpu to also call setup_cpu
-			 * for that processor. I will consolidate that at a
-			 * later time, for now, just use our friend #ifdef.
-			 * we also don't need to PTRRELOC the function pointer
-			 * on ppc64 as we are running at 0 in real mode.
+			/* ppc expects identify_cpu to also call setup_cpu

Quoted 6 lines

diff -ruN linux-2.6.orig/arch/powerpc/kernel/cputable.c linux-2.6/arch/powerpc/kernel/cputable.c
--- linux-2.6.orig/arch/powerpc/kernel/cputable.c 2007-09-20
19:30:47.000000000 +0400
+++ linux-2.6/arch/powerpc/kernel/cputable.c 2007-09-20
21:27:35.000000000 +0400

[snip]

Quoted 14 lines

arch/powerpc/kernel/cputable.c +3 -6

@@ -1318,18 +1327,14 @@
 
     for (i = 0; i < ARRAY_SIZE(cpu_specs); i++,s++)
         if ((pvr & s->pvr_mask) == s->pvr_value) {
+            cpu_setup_t setup_func  = PTRRELOC(s->cpu_setup);
+
             *cur = cpu_specs + i;
-#ifdef CONFIG_PPC64
-            /* ppc64 expects identify_cpu to also call setup_cpu
-             * for that processor. I will consolidate that at a
-             * later time, for now, just use our friend #ifdef.
-             * we also don't need to PTRRELOC the function pointer
-             * on ppc64 as we are running at 0 in real mode.
+            /* ppc expects identify_cpu to also call setup_cpu

arch/powerpc/kernel/cputable.c line 1339

Quoted 10 lines

arch/powerpc/kernel/cputable.c +3 -4

+             * for that processor.
              */
-            if (s->cpu_setup) {
-                s->cpu_setup(offset, s);
-            }
-#endif /* CONFIG_PPC64 */
+            if (setup_func)
+                setup_func(offset, s);
             return s;
         }

This should just be something like:

#if defined(CONFIG_PPC64) || defined(CONFIG_BOOKE)
    if (s->cpu_setup)

...
#endif

we know reloc_offset is always 0 in book-e, plus fixup the comment.

- k

Quoted 9 lines · On Thu, 20 Sep 2007 12:25:06 -0500 Olof Johansson <[email protected]> wrote:

Quoted 5 lines · Valentine Barshak

I was thinking about it. Looks like it's the best place, but the code that actually calls setup_cpu is under ifdef CONFIG_PPC64, while lots of cpu_setup functions are defined for ppc32 processors.
Is it OK to remove this ifdef, or should I do CONFIG_PPC64 || CONFIG_44x?

Sounds like something that went wrong at the merge of ppc and ppc64.

Quoted 2 lines

Take out the ifdef, even if there's fallout we should deal with it instead of adding more complex ifdefs.

Yeah. Looks like BenH did this in commit:

42c4aaadb737e0e672b3fb86b2c41ff59f0fb8bc

Ben, any reason you ifdef'd it for ppc64?

Quoted 14 lines · Josh Boyer

Quoted 7 lines · On Thu, 20 Sep 2007 12:25:06 -0500 Olof Johansson <[email protected]> wrote:

Quoted 2 lines · Valentine Barshak

I was thinking about it. Looks like it's the best place, but the code that actually calls setup_cpu is under ifdef CONFIG_PPC64, while lots of cpu_setup functions are defined for ppc32 processors. Is it OK to remove this ifdef, or should I do CONFIG_PPC64 || CONFIG_44x?

Sounds like something that went wrong at the merge of ppc and ppc64.

Take out the ifdef, even if there's fallout we should deal with it instead of adding more complex ifdefs.

Yeah. Looks like BenH did this in commit:

42c4aaadb737e0e672b3fb86b2c41ff59f0fb8bc

Quoted 1 line

Ben, any reason you ifdef'd it for ppc64?

I'll have to check on monday what's up there, but isn't setup_cpu called from a different place on 32 bits? There are some subtle difference with the way the cpu feature stuff is initialized /done between 32 and 64 bits that we haven't fully reconciled yet.

Ben.

From what I've seen, setup_cpu is never called for BOOKE.
Currently It's called from cputable.c for PPC64 and from head_32.S for 6xx.
Thanks,

Quoted 5 lines · Valentine Barshak

From what I've seen, setup_cpu is never called for BOOKE.
Currently It's called from cputable.c for PPC64 and from head_32.S for 6xx.
Thanks,

Right. We need to fix/converge some of these but it's not totally trivial.