Sequoia kernel crash workaround.

Sequoia kernel crash workaround.

[Valentine Barshak]

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?

Thanks,
Valentine.

Re: Sequoia kernel crash workaround.

[Josh Boyer]

On Wed, 19 Sep 2007 22:39:05 +0400
Valentine Barshak <vbarshak@ru.mvista.com> wrote:

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

Ew.  That sounds nasty.

> 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?

Hm, do you happen to know if AMCC is going to release a newer revision
of 440EPx with that fixed?  If so, we should probably do it
conditionally based off of PVR.

As for where to do it, I'd rather it be in sequoia_setup_arch() than
the bootwrapper.  Eventually bootloaders like U-Boot will take a raw
vmlinux so the wrapper won't even be there anymore.

I don't see any reason to use the dcr_map stuff either.  Just check the
PVR, check if the PLB is already set up correctly, and if not set it
with the regular mfdcr/mtdcr functions.  And a comment about why that's
being done would be good.

That's my $0.02 anyway.

josh

Re: Sequoia kernel crash workaround.

[Stefan Roese]

Hi Valentine,

On Wednesday 19 September 2007, Valentine Barshak wrote:
> 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.

Best regards,
Stefan

Re: Sequoia kernel crash workaround.

[Olof Johansson]

On Wed, Sep 19, 2007 at 09:19:47PM +0200, Stefan Roese wrote:
> Hi Valentine,
> 
> On Wednesday 19 September 2007, Valentine Barshak wrote:
> > 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.

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


-Olof

Re: Sequoia kernel crash workaround.

[Josh Boyer]

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

> On Wed, Sep 19, 2007 at 09:19:47PM +0200, Stefan Roese wrote:
> > Hi Valentine,
> > 
> > On Wednesday 19 September 2007, Valentine Barshak wrote:
> > > 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.
> 
> 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

Re: Sequoia kernel crash workaround.

[Valentine Barshak]

Josh Boyer wrote:
> On Wed, 19 Sep 2007 14:30:24 -0500
> Olof Johansson <olof@lixom.net> wrote:
> 
>> On Wed, Sep 19, 2007 at 09:19:47PM +0200, Stefan Roese wrote:
>>> Hi Valentine,
>>>
>>> On Wednesday 19 September 2007, Valentine Barshak wrote:
>>>> 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.
>> 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?

Thanks,
Valentine.

> 
> josh
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: Sequoia kernel crash workaround.

[Olof Johansson]

On Thu, Sep 20, 2007 at 08:56:32PM +0400, Valentine Barshak wrote:
>
> 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.


-Olof

Re: Sequoia kernel crash workaround.

[Josh Boyer]

On Thu, 20 Sep 2007 12:25:06 -0500
Olof Johansson <olof@lixom.net> wrote:

> On Thu, Sep 20, 2007 at 08:56:32PM +0400, Valentine Barshak wrote:
> >
> > 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

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

josh

Re: Sequoia kernel crash workaround.

[Milton Miller]

On Fri Sep 21 02:56:32 EST 2007, Valentine Barshak wrote:
> Josh Boyer wrote:
>> On Wed, 19 Sep 2007 14:30:24 -0500
>> Olof Johansson <olof at lixom.net> wrote:
>> 
>>> On Wed, Sep 19, 2007 at 09:19:47PM +0200, Stefan Roese wrote:
>>>> Hi Valentine,
>>>>
>>>> On Wednesday 19 September 2007, Valentine Barshak wrote:
>>>>> 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.
>>> 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.

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

milton

[PATCH] PowerPC: add setup_cpu for 44x for processor-specific init

[Valentine Barshak]

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.

Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
---
 arch/powerpc/kernel/Makefile        |    1 
 arch/powerpc/kernel/cpu_setup_44x.S |   54 ++++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/cputable.c      |   25 ++++++++++------
 arch/powerpc/kernel/head_44x.S      |   10 ------
 4 files changed, 70 insertions(+), 20 deletions(-)

diff -ruN linux-2.6.orig/arch/powerpc/kernel/cpu_setup_44x.S linux-2.6/arch/powerpc/kernel/cpu_setup_44x.S
--- linux-2.6.orig/arch/powerpc/kernel/cpu_setup_44x.S	1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/arch/powerpc/kernel/cpu_setup_44x.S	2007-09-20 21:05:44.000000000 +0400
@@ -0,0 +1,54 @@
+/*
+ * This file contains low level CPU setup functions.
+ * Valentine Barshak <vbarshak@ru.mvista.com>
+ * MontaVista Software, Inc (c) 2007
+ *
+ * Based on cpu_setup_6xx code by 
+ * Benjamin Herrenschmidt <benh@kernel.crashing.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ */
+
+#include <asm/processor.h>
+#include <asm/cputable.h>
+#include <asm/ppc_asm.h>
+
+_GLOBAL(__setup_cpu_440ep)
+	b	__init_fpu_44x
+_GLOBAL(__setup_cpu_440epx)
+	mflr	r4
+	bl	__init_fpu_44x
+_GLOBAL(__setup_cpu_440grx)
+	bl	__plb_disable_wrp
+	mtlr	r4
+	blr
+
+/* enable APU between CPU and FPU */
+_GLOBAL(__init_fpu_44x)
+	mfspr	r3,SPRN_CCR0
+	/* Clear DAPUIB flag in CCR0 */
+	rlwinm	r3,r3,0,12,10
+	mtspr	SPRN_CCR0,r3
+	isync
+	blr
+
+/*
+ * Workaround for the incorrect write to DDR SDRAM errata.
+ * The write address can be corrupted during writes to
+ * DDR SDRAM when write pipelining is enabled on PLB0.
+ * Disable write pipelining here.
+ */
+#define DCRN_PLB4A0_ACR	0x81
+
+_GLOBAL(__plb_disable_wrp)
+	mfdcr	r3,DCRN_PLB4A0_ACR
+	/* clear WRP bit in PLB4A0_ACR */
+	rlwinm	r3,r3,0,8,6
+	mtdcr	DCRN_PLB4A0_ACR,r3
+	isync
+	blr
+
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
@@ -31,6 +31,9 @@
  * and ppc64
  */
 #ifdef CONFIG_PPC32
+extern void __setup_cpu_440ep(unsigned long offset, struct cpu_spec* spec);
+extern void __setup_cpu_440epx(unsigned long offset, struct cpu_spec* spec);
+extern void __setup_cpu_440grx(unsigned long offset, struct cpu_spec* spec);
 extern void __setup_cpu_603(unsigned long offset, struct cpu_spec* spec);
 extern void __setup_cpu_604(unsigned long offset, struct cpu_spec* spec);
 extern void __setup_cpu_750(unsigned long offset, struct cpu_spec* spec);
@@ -1111,6 +1114,7 @@
 		.cpu_user_features	= COMMON_USER_BOOKE | PPC_FEATURE_HAS_FPU,
 		.icache_bsize		= 32,
 		.dcache_bsize		= 32,
+		.cpu_setup		= __setup_cpu_440ep,
 		.platform		= "ppc440",
 	},
 	{
@@ -1121,6 +1125,7 @@
 		.cpu_user_features	= COMMON_USER_BOOKE | PPC_FEATURE_HAS_FPU,
 		.icache_bsize		= 32,
 		.dcache_bsize		= 32,
+		.cpu_setup		= __setup_cpu_440ep,
 		.platform		= "ppc440",
 	},
 	{ /* 440EPX */
@@ -1131,6 +1136,8 @@
 		.cpu_user_features	= COMMON_USER_BOOKE | PPC_FEATURE_HAS_FPU,
 		.icache_bsize		= 32,
 		.dcache_bsize		= 32,
+		.cpu_setup		= __setup_cpu_440epx,
+		.platform		= "ppc440",
 	},
 	{ /* 440GRX */
 		.pvr_mask		= 0xf0000ffb,
@@ -1140,6 +1147,8 @@
 		.cpu_user_features	= COMMON_USER_BOOKE,
 		.icache_bsize		= 32,
 		.dcache_bsize		= 32,
+		.cpu_setup		= __setup_cpu_440grx,
+		.platform		= "ppc440",
 	},
 	{	/* 440GP Rev. B */
 		.pvr_mask		= 0xf0000fff,
@@ -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
+			 * for that processor.
 			 */
-			if (s->cpu_setup) {
-				s->cpu_setup(offset, s);
-			}
-#endif /* CONFIG_PPC64 */
+			if (setup_func)
+				setup_func(offset, s);
 			return s;
 		}
 	BUG();
diff -ruN linux-2.6.orig/arch/powerpc/kernel/head_44x.S linux-2.6/arch/powerpc/kernel/head_44x.S
--- linux-2.6.orig/arch/powerpc/kernel/head_44x.S	2007-09-20 19:30:47.000000000 +0400
+++ linux-2.6/arch/powerpc/kernel/head_44x.S	2007-09-20 20:04:39.000000000 +0400
@@ -217,16 +217,6 @@
 	lis	r4,interrupt_base@h	/* IVPR only uses the high 16-bits */
 	mtspr	SPRN_IVPR,r4
 
-#if defined(CONFIG_440EP) || defined(CONFIG_440EPX)
-	/* Clear DAPUIB flag in CCR0 (enable APU between CPU and FPU) */
-	mfspr	r2,SPRN_CCR0
-	lis	r3,0xffef
-	ori	r3,r3,0xffff
-	and	r2,r2,r3
-	mtspr	SPRN_CCR0,r2
-	isync
-#endif
-
 	/*
 	 * This is where the main kernel code starts.
 	 */
diff -ruN linux-2.6.orig/arch/powerpc/kernel/Makefile linux-2.6/arch/powerpc/kernel/Makefile
--- linux-2.6.orig/arch/powerpc/kernel/Makefile	2007-09-20 19:30:47.000000000 +0400
+++ linux-2.6/arch/powerpc/kernel/Makefile	2007-09-20 19:52:21.000000000 +0400
@@ -56,6 +56,7 @@
 				   udbg.o misc.o io.o
 obj-$(CONFIG_PPC32)		+= entry_32.o setup_32.o misc_32.o
 obj-$(CONFIG_PPC64)		+= misc_64.o dma_64.o iommu.o
+obj-$(CONFIG_44x)		+= cpu_setup_44x.o
 obj-$(CONFIG_PPC_MULTIPLATFORM)	+= prom_init.o
 obj-$(CONFIG_MODULES)		+= ppc_ksyms.o
 obj-$(CONFIG_BOOTX_TEXT)	+= btext.o

Re: Sequoia kernel crash workaround.

[Josh Boyer]

On Thu, 20 Sep 2007 12:32:56 -0500 (CDT)
Milton Miller <miltonm@bga.com> wrote:

> On Fri Sep 21 02:56:32 EST 2007, Valentine Barshak wrote:
> > Josh Boyer wrote:
> >> On Wed, 19 Sep 2007 14:30:24 -0500
> >> Olof Johansson <olof at lixom.net> wrote:
> >> 
> >>> On Wed, Sep 19, 2007 at 09:19:47PM +0200, Stefan Roese wrote:
> >>>> Hi Valentine,
> >>>>
> >>>> On Wednesday 19 September 2007, Valentine Barshak wrote:
> >>>>> 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.
> >>> 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.
> 
> 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

Re: Sequoia kernel crash workaround.

[Olof Johansson]

On Thu, Sep 20, 2007 at 12:25:06PM -0500, Olof Johansson wrote:
> On Thu, Sep 20, 2007 at 08:56:32PM +0400, Valentine Barshak wrote:
> >
> > 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.

See comment from Milton, my bad. It's just called from head_32.S right now.
It'll need to be added to the 4xx head as well, or an ifdef added. One or
the other, I'll leave it to Josh to pick.


-Olof

Re: Sequoia kernel crash workaround.

[Valentine Barshak]

Josh Boyer wrote:
> On Thu, 20 Sep 2007 12:32:56 -0500 (CDT)
> Milton Miller <miltonm@bga.com> wrote:
> 
>> On Fri Sep 21 02:56:32 EST 2007, Valentine Barshak wrote:
>>> Josh Boyer wrote:
>>>> On Wed, 19 Sep 2007 14:30:24 -0500
>>>> Olof Johansson <olof at lixom.net> wrote:
>>>>
>>>>> On Wed, Sep 19, 2007 at 09:19:47PM +0200, Stefan Roese wrote:
>>>>>> Hi Valentine,
>>>>>>
>>>>>> On Wednesday 19 September 2007, Valentine Barshak wrote:
>>>>>>> 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.
>>>>> 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.
>>
>> 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.

I started preparing the patch after Olof's "take out the ifdef" :)
I've tested it on 4xx. seems to work fine. FPU works OK and EPX/GRX 
workaround is fine also, but it has to be tested on 8xx and fsl.
Adding call_setup_cpu to head_44x is no problem.

> 
> josh

Re: [PATCH] PowerPC: add setup_cpu for 44x for processor-specific init

[Josh Boyer]

On Thu, 20 Sep 2007 21:55:58 +0400
Valentine Barshak <vbarshak@ru.mvista.com> wrote:

> 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

Re: [PATCH] PowerPC: add setup_cpu for 44x for processor-specific init

[Valentine Barshak]

Josh Boyer wrote:
> On Thu, 20 Sep 2007 21:55:58 +0400
> Valentine Barshak <vbarshak@ru.mvista.com> wrote:
> 
>> 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.

OK

> 
> I still need to look at the code too :)

sure :)

> 
> josh

Thanks,
Valentine.

Re: [PATCH] PowerPC: add setup_cpu for 44x for processor-specific init

[Kumar Gala]

> 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]

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

Re: [PATCH] PowerPC: add setup_cpu for 44x for processor-specific init

[Valentine Barshak]

Kumar Gala wrote:
> 
>> 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]
> 
>> @@ -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
>> +             * 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

OK,
thanks,
Valentine.

> 
> 
> 

Re: [PATCH] PowerPC: add setup_cpu for 44x for processor-specific init

[Paul Mackerras]

Valentine Barshak writes:

> +_GLOBAL(__setup_cpu_440epx)
> +	mflr	r4
> +	bl	__init_fpu_44x
> +_GLOBAL(__setup_cpu_440grx)
> +	bl	__plb_disable_wrp
> +	mtlr	r4
> +	blr

If we enter at __setup_cpu_440grx we'll return to some bogus
address...

Paul.

Re: Sequoia kernel crash workaround.

[Benjamin Herrenschmidt]

On Thu, 2007-09-20 at 12:29 -0500, Josh Boyer wrote:
> On Thu, 20 Sep 2007 12:25:06 -0500
> Olof Johansson <olof@lixom.net> wrote:
> 
> > On Thu, Sep 20, 2007 at 08:56:32PM +0400, Valentine Barshak wrote:
> > >
> > > 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
> 
> 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.

Re: Sequoia kernel crash workaround.

[Valentine Barshak]

Benjamin Herrenschmidt wrote:
> On Thu, 2007-09-20 at 12:29 -0500, Josh Boyer wrote:
>> On Thu, 20 Sep 2007 12:25:06 -0500
>> Olof Johansson <olof@lixom.net> wrote:
>>
>>> On Thu, Sep 20, 2007 at 08:56:32PM +0400, Valentine Barshak wrote:
>>>> 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
>>
>> 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,
Valentine.

> 

Re: Sequoia kernel crash workaround.

[Benjamin Herrenschmidt]

On Mon, 2007-09-24 at 14:35 +0400, Valentine Barshak wrote:
> 
>  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.

Ben.

Re: Sequoia kernel crash workaround.

[Josh Boyer]

On Tue, 25 Sep 2007 06:55:50 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> On Mon, 2007-09-24 at 14:35 +0400, Valentine Barshak wrote:
> > 
> >  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.

Valentine has patches for adding this for BookE on the list.

josh