[PATCH] sparc: cleaned up FPU version probing

[PATCH] sparc: cleaned up FPU version probing

[Daniel Hellstrom]

Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>
---
 arch/sparc/include/asm/psr.h |   51 ++++++++++++++++++++++++++++++++++++++++++
 arch/sparc/kernel/cpu.c      |   11 +++++---
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/arch/sparc/include/asm/psr.h b/arch/sparc/include/asm/psr.h
index b8c0e5f..3b34c5a 100644
--- a/arch/sparc/include/asm/psr.h
+++ b/arch/sparc/include/asm/psr.h
@@ -6,11 +6,14 @@
  *        PSTATE.PRIV for the current CPU privilege level.
  *
  * Copyright (C) 1994 David S. Miller (davem@caip.rutgers.edu)
+ * Copyright (C) 2011 Daniel Hellstrom (daniel@gaisler.com)
  */
 
 #ifndef __LINUX_SPARC_PSR_H
 #define __LINUX_SPARC_PSR_H
 
+#include <linux/const.h>
+
 /* The Sparc PSR fields are laid out as the following:
  *
  *  ------------------------------------------------------------------------
@@ -34,6 +37,54 @@
 #define PSR_N       0x00800000         /* negative bit               */
 #define PSR_VERS    0x0f000000         /* cpu-version field          */
 #define PSR_IMPL    0xf0000000         /* cpu-implementation field   */
+#define PSR_CWP_SHIFT     0
+#define PSR_ET_SHIFT      5
+#define PSR_PS_SHIFT      6
+#define PSR_S_SHIFT       7
+#define PSR_PIL_SHIFT     8
+#define PSR_EF_SHIFT      12
+#define PSR_EC_SHIFT      13
+#define PSR_SYSCALL_SHIFT 14
+#define PSR_LE_SHIFT      15
+#define PSR_ICC_SHIFT     20
+#define PSR_C_SHIFT       20
+#define PSR_V_SHIFT       21
+#define PSR_Z_SHIFT       22
+#define PSR_N_SHIFT       23
+#define PSR_VERS_SHIFT    24
+#define PSR_IMPL_SHIFT    28
+
+/*
+ * The SPARC v7/v8/v9 FPU FSR Register bits. fcc on v7/v8 is equal to
+ * fcc0 on v9.
+ */
+#define FSR_CEXC    0x0000001f             /* current exception      */
+#define FSR_AEXC    0x000003e0             /* accured exception      */
+#define FSR_FCC0    0x00000c00             /* FP condition code 0    */
+#define FSR_QNE     0x00002000             /* FQ not empty           */
+#define FSR_FTT     0x0001c000             /* FP trap type           */
+#define FSR_VER     0x000e0000             /* FPU version            */
+#define FSR_NS      0x00400000             /* Non standard FP        */
+#define FSR_TEM     0x0f800000             /* FPU trap enable mask   */
+#define FSR_RD      0xc0000000             /* Rounding direction     */
+#define FSR_FCC1    _AC(0x00300000000, UL) /* v9 FP condition code 1 */
+#define FSR_FCC2    _AC(0x0c00000000, UL)  /* v9 FP condition code 2 */
+#define FSR_FCC3    _AC(0x3000000000, UL)  /* v9 FP condition code 3 */
+#define FSR_CEXC_SHIFT  0
+#define FSR_AEXC_SHIFT  5
+#define FSR_FCC0_SHIFT  10
+#define FSR_QNE_SHIFT   13
+#define FSR_FTT_SHIFT   14
+#define FSR_VER_SHIFT   17
+#define FSR_NS_SHIFT    22
+#define FSR_TEM_SHIFT   23
+#define FSR_RD_SHIFT    30
+#define FSR_FCC1_SHIFT  32
+#define FSR_FCC2_SHIFT  34
+#define FSR_FCC3_SHIFT  36
+
+/* FSR.ver=7 when no FPU available */
+#define FSR_VER_NOFPU 7
 
 #ifdef __KERNEL__
 
diff --git a/arch/sparc/kernel/cpu.c b/arch/sparc/kernel/cpu.c
index 7925c54..bc8d5ef 100644
--- a/arch/sparc/kernel/cpu.c
+++ b/arch/sparc/kernel/cpu.c
@@ -318,15 +318,18 @@ void __cpuinit cpu_probe(void)
 	int psr_impl, psr_vers, fpu_vers;
 	int psr;
 
-	psr_impl = ((get_psr() >> 28) & 0xf);
-	psr_vers = ((get_psr() >> 24) & 0xf);
+	psr_impl = ((get_psr() & PSR_IMPL) >> PSR_IMPL_SHIFT);
+	psr_vers = ((get_psr() & PSR_VERS) >> PSR_VERS_SHIFT);
 
 	psr = get_psr();
 	put_psr(psr | PSR_EF);
 #ifdef CONFIG_SPARC_LEON
-	fpu_vers = get_psr() & PSR_EF ? ((get_fsr() >> 17) & 0x7) : 7;
+	if (get_psr() & PSR_EF)
+		fpu_vers =  (get_fsr() & FSR_VER) >> FSR_VER_SHIFT;
+	else
+		fpu_vers = FSR_VER_NOFPU;
 #else
-	fpu_vers = ((get_fsr() >> 17) & 0x7);
+	fpu_vers = ((get_fsr() & FSR_VER) >> FSR_VER_SHIFT);
 #endif
 
 	put_psr(psr);
-- 
1.5.4

Re: [PATCH] sparc: cleaned up FPU version probing

[Richard Mortimer]

On 27/01/2011 11:06, Daniel Hellstrom wrote:
> Signed-off-by: Daniel Hellstrom<daniel@gaisler.com>
> ---
>   arch/sparc/include/asm/psr.h |   51 ++++++++++++++++++++++++++++++++++++++++++
>   arch/sparc/kernel/cpu.c      |   11 +++++---
>   2 files changed, 58 insertions(+), 4 deletions(-)
>
...

> diff --git a/arch/sparc/kernel/cpu.c b/arch/sparc/kernel/cpu.c
> index 7925c54..bc8d5ef 100644
> --- a/arch/sparc/kernel/cpu.c
> +++ b/arch/sparc/kernel/cpu.c
> @@ -318,15 +318,18 @@ void __cpuinit cpu_probe(void)
>   	int psr_impl, psr_vers, fpu_vers;
>   	int psr;
>
> -	psr_impl = ((get_psr()>>  28)&  0xf);
> -	psr_vers = ((get_psr()>>  24)&  0xf);
> +	psr_impl = ((get_psr()&  PSR_IMPL)>>  PSR_IMPL_SHIFT);
This is going to break. If the top bit of psr_impl is set it
will get sign extended when the left shift is done.

> +	psr_vers = ((get_psr()&  PSR_VERS)>>  PSR_VERS_SHIFT);
>
>   	psr = get_psr();
>   	put_psr(psr | PSR_EF);
>   #ifdef CONFIG_SPARC_LEON
> -	fpu_vers = get_psr()&  PSR_EF ? ((get_fsr()>>  17)&  0x7) : 7;
> +	if (get_psr()&  PSR_EF)
> +		fpu_vers =  (get_fsr()&  FSR_VER)>>  FSR_VER_SHIFT;
> +	else
> +		fpu_vers = FSR_VER_NOFPU;
>   #else
> -	fpu_vers = ((get_fsr()>>  17)&  0x7);
> +	fpu_vers = ((get_fsr()&  FSR_VER)>>  FSR_VER_SHIFT);
>   #endif
>
>   	put_psr(psr);

Re: [PATCH] sparc: cleaned up FPU version probing

[Richard Mortimer]

On 27/01/2011 11:14, Richard Mortimer wrote:
>
>
> On 27/01/2011 11:06, Daniel Hellstrom wrote:
>> Signed-off-by: Daniel Hellstrom<daniel@gaisler.com>
>> ---
>> arch/sparc/include/asm/psr.h | 51
>> ++++++++++++++++++++++++++++++++++++++++++
>> arch/sparc/kernel/cpu.c | 11 +++++---
>> 2 files changed, 58 insertions(+), 4 deletions(-)
>>
> ...
>
>> diff --git a/arch/sparc/kernel/cpu.c b/arch/sparc/kernel/cpu.c
>> index 7925c54..bc8d5ef 100644
>> --- a/arch/sparc/kernel/cpu.c
>> +++ b/arch/sparc/kernel/cpu.c
>> @@ -318,15 +318,18 @@ void __cpuinit cpu_probe(void)
>> int psr_impl, psr_vers, fpu_vers;
>> int psr;
>>
>> - psr_impl = ((get_psr()>> 28)& 0xf);
>> - psr_vers = ((get_psr()>> 24)& 0xf);
>> + psr_impl = ((get_psr()& PSR_IMPL)>> PSR_IMPL_SHIFT);
> This is going to break. If the top bit of psr_impl is set it
> will get sign extended when the left shift is done.
Of course I meant right shift :-)

>
>> + psr_vers = ((get_psr()& PSR_VERS)>> PSR_VERS_SHIFT);
>>
>> psr = get_psr();
>> put_psr(psr | PSR_EF);
>> #ifdef CONFIG_SPARC_LEON
>> - fpu_vers = get_psr()& PSR_EF ? ((get_fsr()>> 17)& 0x7) : 7;
>> + if (get_psr()& PSR_EF)
>> + fpu_vers = (get_fsr()& FSR_VER)>> FSR_VER_SHIFT;
>> + else
>> + fpu_vers = FSR_VER_NOFPU;
>> #else
>> - fpu_vers = ((get_fsr()>> 17)& 0x7);
>> + fpu_vers = ((get_fsr()& FSR_VER)>> FSR_VER_SHIFT);
>> #endif
>>
>> put_psr(psr);

Re: [PATCH] sparc: cleaned up FPU version probing

[Sam Ravnborg]

On Thu, Jan 27, 2011 at 11:14:16AM +0000, Richard Mortimer wrote:
>
>
> On 27/01/2011 11:06, Daniel Hellstrom wrote:
>> Signed-off-by: Daniel Hellstrom<daniel@gaisler.com>
>> ---
>>   arch/sparc/include/asm/psr.h |   51 ++++++++++++++++++++++++++++++++++++++++++
>>   arch/sparc/kernel/cpu.c      |   11 +++++---
>>   2 files changed, 58 insertions(+), 4 deletions(-)
>>
> ...
>
>> diff --git a/arch/sparc/kernel/cpu.c b/arch/sparc/kernel/cpu.c
>> index 7925c54..bc8d5ef 100644
>> --- a/arch/sparc/kernel/cpu.c
>> +++ b/arch/sparc/kernel/cpu.c
>> @@ -318,15 +318,18 @@ void __cpuinit cpu_probe(void)
>>   	int psr_impl, psr_vers, fpu_vers;
>>   	int psr;
>>
>> -	psr_impl = ((get_psr()>>  28)&  0xf);
>> -	psr_vers = ((get_psr()>>  24)&  0xf);
>> +	psr_impl = ((get_psr()&  PSR_IMPL)>>  PSR_IMPL_SHIFT);
> This is going to break. If the top bit of psr_impl is set it
> will get sign extended when the left shift is done.

That would not matter as we use the " & PSR_IMPL" to clear
the unused upper bits.
So IMO the patch is correct.

	Sam

Re: [PATCH] sparc: cleaned up FPU version probing

[Sam Ravnborg]

On Thu, Jan 27, 2011 at 12:06:32PM +0100, Daniel Hellstrom wrote:
> Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>

Hi Daniel.
Thanks for cleaning up this.

Acked-by: Sam Ravnborg <sam@ravnborg.org>

Re: [PATCH] sparc: cleaned up FPU version probing

[Richard Mortimer]

On 27/01/2011 17:34, Sam Ravnborg wrote:
> On Thu, Jan 27, 2011 at 11:14:16AM +0000, Richard Mortimer wrote:
>>
>>
>> On 27/01/2011 11:06, Daniel Hellstrom wrote:
>>> Signed-off-by: Daniel Hellstrom<daniel@gaisler.com>
>>> ---
>>>    arch/sparc/include/asm/psr.h |   51 ++++++++++++++++++++++++++++++++++++++++++
>>>    arch/sparc/kernel/cpu.c      |   11 +++++---
>>>    2 files changed, 58 insertions(+), 4 deletions(-)
>>>
>> ...
>>
>>> diff --git a/arch/sparc/kernel/cpu.c b/arch/sparc/kernel/cpu.c
>>> index 7925c54..bc8d5ef 100644
>>> --- a/arch/sparc/kernel/cpu.c
>>> +++ b/arch/sparc/kernel/cpu.c
>>> @@ -318,15 +318,18 @@ void __cpuinit cpu_probe(void)
>>>    	int psr_impl, psr_vers, fpu_vers;
>>>    	int psr;
>>>
>>> -	psr_impl = ((get_psr()>>   28)&   0xf);
>>> -	psr_vers = ((get_psr()>>   24)&   0xf);
>>> +	psr_impl = ((get_psr()&   PSR_IMPL)>>   PSR_IMPL_SHIFT);
>> This is going to break. If the top bit of psr_impl is set it
>> will get sign extended when the left shift is done.
>
> That would not matter as we use the "&  PSR_IMPL" to clear
> the unused upper bits.
> So IMO the patch is correct.
>
I'm not sure. But I'm willing to be pursuaded.

Daniel changed the order of things. The shift is now done after the 
mask. Previously the mask was using 0xf and now it is using 0xf0000000.
e.g.
originally (0x8??????? >> 28)  gives 0xfffffff8 which is masked by 0xf 
to give 8

but now (0x8??????? & 0xf0000000) gives 0x80000000 which is shifted right 28
to give 0xfffffff8.

I must admit that I haven't looked at the defition of get_psr() but I 
think it returns an int and even if it returns unsigned I would be wary 
of leaving it as is because it is fragile in that particular case.

Regards

Richard

Re: [PATCH] sparc: cleaned up FPU version probing

[Sam Ravnborg]

On Thu, Jan 27, 2011 at 06:39:47PM +0000, Richard Mortimer wrote:
>
>
> On 27/01/2011 17:34, Sam Ravnborg wrote:
>> On Thu, Jan 27, 2011 at 11:14:16AM +0000, Richard Mortimer wrote:
>>>
>>>
>>> On 27/01/2011 11:06, Daniel Hellstrom wrote:
>>>> Signed-off-by: Daniel Hellstrom<daniel@gaisler.com>
>>>> ---
>>>>    arch/sparc/include/asm/psr.h |   51 ++++++++++++++++++++++++++++++++++++++++++
>>>>    arch/sparc/kernel/cpu.c      |   11 +++++---
>>>>    2 files changed, 58 insertions(+), 4 deletions(-)
>>>>
>>> ...
>>>
>>>> diff --git a/arch/sparc/kernel/cpu.c b/arch/sparc/kernel/cpu.c
>>>> index 7925c54..bc8d5ef 100644
>>>> --- a/arch/sparc/kernel/cpu.c
>>>> +++ b/arch/sparc/kernel/cpu.c
>>>> @@ -318,15 +318,18 @@ void __cpuinit cpu_probe(void)
>>>>    	int psr_impl, psr_vers, fpu_vers;
>>>>    	int psr;
>>>>
>>>> -	psr_impl = ((get_psr()>>   28)&   0xf);
>>>> -	psr_vers = ((get_psr()>>   24)&   0xf);
>>>> +	psr_impl = ((get_psr()&   PSR_IMPL)>>   PSR_IMPL_SHIFT);
>>> This is going to break. If the top bit of psr_impl is set it
>>> will get sign extended when the left shift is done.
>>
>> That would not matter as we use the "&  PSR_IMPL" to clear
>> the unused upper bits.
>> So IMO the patch is correct.
>>
> I'm not sure. But I'm willing to be pursuaded.
>
> Daniel changed the order of things. The shift is now done after the  
> mask. Previously the mask was using 0xf and now it is using 0xf0000000.
> e.g.
> originally (0x8??????? >> 28)  gives 0xfffffff8 which is masked by 0xf  
> to give 8
>
> but now (0x8??????? & 0xf0000000) gives 0x80000000 which is shifted right 28
> to give 0xfffffff8.

You are right. I was only looking at the case where we shifted 17.

But get_psr() return an unsigned int:

   static inline unsigned int get_psr(void)

And same does get_fsr().

I checked a few drivers and it seems to be a common pattern to rely
on use of unisged variables.

If we want to be more explicit we can do:

	u32 psr;

	psr = get_psr();
	psr_impl = (psr & PSR_IMPL) >> PSR_IMPL_SHIFT;

I assume gcc will generate equal code for this.
But this looks like overkill.

The mixed use of unsigned int and int in this file also looks
like something to be cleaned up...

	Sam

Re: [PATCH] sparc: cleaned up FPU version probing

[David Miller]

From: Sam Ravnborg <sam@ravnborg.org>
Date: Thu, 27 Jan 2011 20:36:03 +0100

> If we want to be more explicit we can do:
> 
> 	u32 psr;
> 
> 	psr = get_psr();
> 	psr_impl = (psr & PSR_IMPL) >> PSR_IMPL_SHIFT;
> 
> I assume gcc will generate equal code for this.
> But this looks like overkill.
> 
> The mixed use of unsigned int and int in this file also looks
> like something to be cleaned up...

Daniel, please respin your patch so that we explicitly use unsigned
variables here as Sam suggests, and thus avoid the sign-extension
issues during the right-shift operations.

Thanks.

Re: [PATCH] sparc: cleaned up FPU version probing

[Daniel Hellstrom]

David Miller wrote:

>From: Sam Ravnborg <sam@ravnborg.org>
>Date: Thu, 27 Jan 2011 20:36:03 +0100
>
>  
>
>>If we want to be more explicit we can do:
>>
>>	u32 psr;
>>
>>	psr = get_psr();
>>	psr_impl = (psr & PSR_IMPL) >> PSR_IMPL_SHIFT;
>>
>>I assume gcc will generate equal code for this.
>>But this looks like overkill.
>>
>>The mixed use of unsigned int and int in this file also looks
>>like something to be cleaned up...
>>    
>>
>
>Daniel, please respin your patch so that we explicitly use unsigned
>variables here as Sam suggests, and thus avoid the sign-extension
>issues during the right-shift operations.
>  
>
Thanks, I will do that.