[PATCH] [POWERPC] ppc32: Fix errata for 603 CPUs

[PATCH] [POWERPC] ppc32: Fix errata for 603 CPUs

[Kumar Gala]

603 CPUs have the same issue that some 750 CPUs have in that they can crash
in funny ways if a store from an FPU register instruction is executed on a
register that has never been initialized since power on.  This patch fixes
it by making sure all FP registers have been properly initialized at kernel
boot.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---

Paul, I've put this into my powerpc-next tree since its a pretty trivial
patch.

- k

 arch/powerpc/kernel/cpu_setup_6xx.S |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/cpu_setup_6xx.S b/arch/powerpc/kernel/cpu_setup_6xx.S
index f1ee0b3..4a1c3cd 100644
--- a/arch/powerpc/kernel/cpu_setup_6xx.S
+++ b/arch/powerpc/kernel/cpu_setup_6xx.S
@@ -17,7 +17,14 @@
 #include <asm/cache.h>

 _GLOBAL(__setup_cpu_603)
-	b	setup_common_caches
+	mflr	r4
+BEGIN_FTR_SECTION
+	bl	__init_fpu_registers
+END_FTR_SECTION_IFCLR(CPU_FTR_FPU_UNAVAILABLE)
+	bl	__init_fpu_registers
+	bl	setup_common_caches
+	mtlr	r4
+	blr
 _GLOBAL(__setup_cpu_604)
 	mflr	r4
 	bl	setup_common_caches
-- 
1.5.4.1

Re: [PATCH] [POWERPC] ppc32: Fix errata for 603 CPUs

[Gabriel Paubert]

On Mon, Apr 21, 2008 at 02:57:47PM -0500, Kumar Gala wrote:
> 603 CPUs have the same issue that some 750 CPUs have in that they can crash
> in funny ways if a store from an FPU register instruction is executed on a
> register that has never been initialized since power on.  This patch fixes
> it by making sure all FP registers have been properly initialized at kernel
> boot.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> 
> Paul, I've put this into my powerpc-next tree since its a pretty trivial
> patch.
> 
> - k
> 
>  arch/powerpc/kernel/cpu_setup_6xx.S |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/cpu_setup_6xx.S b/arch/powerpc/kernel/cpu_setup_6xx.S
> index f1ee0b3..4a1c3cd 100644
> --- a/arch/powerpc/kernel/cpu_setup_6xx.S
> +++ b/arch/powerpc/kernel/cpu_setup_6xx.S
> @@ -17,7 +17,14 @@
>  #include <asm/cache.h>
> 
>  _GLOBAL(__setup_cpu_603)
> -	b	setup_common_caches
> +	mflr	r4
> +BEGIN_FTR_SECTION
> +	bl	__init_fpu_registers
> +END_FTR_SECTION_IFCLR(CPU_FTR_FPU_UNAVAILABLE)
> +	bl	__init_fpu_registers

I don't understand that code. 

What does the second bl __init_fpu_regiters do?

As I understand it, the first one is conditional on the presence 
of an FPU (reasonable). But the secone one makes no sense.

Finally wouldn't it be simpler to initialize the FPU on all CPU
that have it? 

> +	bl	setup_common_caches
> +	mtlr	r4
> +	blr

You can use tail call elimination and save one instruction there :-)


	Regards,
	Gabriel

Re: [PATCH] [POWERPC] ppc32: Fix errata for 603 CPUs

[Kumar Gala]

On Apr 21, 2008, at 3:19 PM, Gabriel Paubert wrote:
> On Mon, Apr 21, 2008 at 02:57:47PM -0500, Kumar Gala wrote:
>> 603 CPUs have the same issue that some 750 CPUs have in that they  
>> can crash
>> in funny ways if a store from an FPU register instruction is  
>> executed on a
>> register that has never been initialized since power on.  This  
>> patch fixes
>> it by making sure all FP registers have been properly initialized  
>> at kernel
>> boot.
>>
>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>> ---
>>
>> Paul, I've put this into my powerpc-next tree since its a pretty  
>> trivial
>> patch.
>>
>> - k
>>
>> arch/powerpc/kernel/cpu_setup_6xx.S |    9 ++++++++-
>> 1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/cpu_setup_6xx.S b/arch/powerpc/ 
>> kernel/cpu_setup_6xx.S
>> index f1ee0b3..4a1c3cd 100644
>> --- a/arch/powerpc/kernel/cpu_setup_6xx.S
>> +++ b/arch/powerpc/kernel/cpu_setup_6xx.S
>> @@ -17,7 +17,14 @@
>> #include <asm/cache.h>
>>
>> _GLOBAL(__setup_cpu_603)
>> -	b	setup_common_caches
>> +	mflr	r4
>> +BEGIN_FTR_SECTION
>> +	bl	__init_fpu_registers
>> +END_FTR_SECTION_IFCLR(CPU_FTR_FPU_UNAVAILABLE)
>> +	bl	__init_fpu_registers
>
> I don't understand that code.
>
> What does the second bl __init_fpu_regiters do?

that was a copy/paste error.  (see v2 of the patch).

> As I understand it, the first one is conditional on the presence
> of an FPU (reasonable). But the secone one makes no sense.
>
> Finally wouldn't it be simpler to initialize the FPU on all CPU
> that have it?

that would require a change to the entry point of all these  
functions.  Its not worth the savings.

>> +	bl	setup_common_caches
>> +	mtlr	r4
>> +	blr
>
> You can use tail call elimination and save one instruction there :-)

I think we can find better places to save code size. :)

- k

Re: [PATCH] [POWERPC] ppc32: Fix errata for 603 CPUs

[Benjamin Herrenschmidt]

> 
>  _GLOBAL(__setup_cpu_603)
> -	b	setup_common_caches
> +	mflr	r4
> +BEGIN_FTR_SECTION
> +	bl	__init_fpu_registers
> +END_FTR_SECTION_IFCLR(CPU_FTR_FPU_UNAVAILABLE)
> +	bl	__init_fpu_registers
> +	bl	setup_common_caches
> +	mtlr	r4
> +	blr
>  _GLOBAL(__setup_cpu_604)
>  	mflr	r4
>  	bl	setup_common_cache

Has the feature fixup been perform yet when __setup_* is called ?

Ben.

Re: [PATCH] [POWERPC] ppc32: Fix errata for 603 CPUs

[Kumar Gala]

On Apr 21, 2008, at 4:22 PM, Benjamin Herrenschmidt wrote:
>
>>
>> _GLOBAL(__setup_cpu_603)
>> -	b	setup_common_caches
>> +	mflr	r4
>> +BEGIN_FTR_SECTION
>> +	bl	__init_fpu_registers
>> +END_FTR_SECTION_IFCLR(CPU_FTR_FPU_UNAVAILABLE)
>> +	bl	__init_fpu_registers
>> +	bl	setup_common_caches
>> +	mtlr	r4
>> +	blr
>> _GLOBAL(__setup_cpu_604)
>> 	mflr	r4
>> 	bl	setup_common_cache
>
> Has the feature fixup been perform yet when __setup_* is called ?

Yes.  We call do_feature_fixups() in early_init and call  
call_setup_cpu() after than on 6xx (in head_32.S).

- k

Re: [PATCH] [POWERPC] ppc32: Fix errata for 603 CPUs

[Benjamin Herrenschmidt]

On Mon, 2008-04-21 at 17:04 -0500, Kumar Gala wrote:
> On Apr 21, 2008, at 4:22 PM, Benjamin Herrenschmidt wrote:
> >
> >>
> >> _GLOBAL(__setup_cpu_603)
> >> -	b	setup_common_caches
> >> +	mflr	r4
> >> +BEGIN_FTR_SECTION
> >> +	bl	__init_fpu_registers
> >> +END_FTR_SECTION_IFCLR(CPU_FTR_FPU_UNAVAILABLE)
> >> +	bl	__init_fpu_registers
> >> +	bl	setup_common_caches
> >> +	mtlr	r4
> >> +	blr
> >> _GLOBAL(__setup_cpu_604)
> >> 	mflr	r4
> >> 	bl	setup_common_cache
> >
> > Has the feature fixup been perform yet when __setup_* is called ?
> 
> Yes.  We call do_feature_fixups() in early_init and call  
> call_setup_cpu() after than on 6xx (in head_32.S).

Hrm. I think that may be the other way around on 64 bits... damn, I need
to consolidate these.

I'd rather have the init code explicitely test for the features.

Ben.

Re: [PATCH] [POWERPC] ppc32: Fix errata for 603 CPUs

[Kumar Gala]

On Apr 21, 2008, at 5:12 PM, Benjamin Herrenschmidt wrote:
>
> On Mon, 2008-04-21 at 17:04 -0500, Kumar Gala wrote:
>> On Apr 21, 2008, at 4:22 PM, Benjamin Herrenschmidt wrote:
>>>
>>>>
>>>> _GLOBAL(__setup_cpu_603)
>>>> -	b	setup_common_caches
>>>> +	mflr	r4
>>>> +BEGIN_FTR_SECTION
>>>> +	bl	__init_fpu_registers
>>>> +END_FTR_SECTION_IFCLR(CPU_FTR_FPU_UNAVAILABLE)
>>>> +	bl	__init_fpu_registers
>>>> +	bl	setup_common_caches
>>>> +	mtlr	r4
>>>> +	blr
>>>> _GLOBAL(__setup_cpu_604)
>>>> 	mflr	r4
>>>> 	bl	setup_common_cache
>>>
>>> Has the feature fixup been perform yet when __setup_* is called ?
>>
>> Yes.  We call do_feature_fixups() in early_init and call
>> call_setup_cpu() after than on 6xx (in head_32.S).
>
> Hrm. I think that may be the other way around on 64 bits... damn, I  
> need
> to consolidate these.
>
> I'd rather have the init code explicitely test for the features.

Ok, but we'd also need to fix setup_750_7400_hid0 and  
setup_745x_specifics to test.

There's a bit more cleanup that needs to be done here.  I'm not going  
to worry about it for this patch since it covers handling a chip  
errata and isn't any more "broken" than other bits of code in this file.

- k 

Re: [PATCH] [POWERPC] ppc32: Fix errata for 603 CPUs

[Benjamin Herrenschmidt]

On Mon, 2008-04-21 at 17:31 -0500, Kumar Gala wrote:
> Ok, but we'd also need to fix setup_750_7400_hid0 and  
> setup_745x_specifics to test.
> 
> There's a bit more cleanup that needs to be done here.  I'm not
> going  
> to worry about it for this patch since it covers handling a chip  
> errata and isn't any more "broken" than other bits of code in this fi

True.

Cheers,
Ben.