* Re: [PATCH] sparc: cleaned up FPU version probing
2011-01-27 11:06 [PATCH] sparc: cleaned up FPU version probing Daniel Hellstrom
@ 2011-01-27 11:14 ` Richard Mortimer
2011-01-27 11:21 ` Richard Mortimer
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Richard Mortimer @ 2011-01-27 11:14 UTC (permalink / raw)
To: sparclinux
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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sparc: cleaned up FPU version probing
2011-01-27 11:06 [PATCH] sparc: cleaned up FPU version probing Daniel Hellstrom
2011-01-27 11:14 ` Richard Mortimer
@ 2011-01-27 11:21 ` Richard Mortimer
2011-01-27 17:34 ` Sam Ravnborg
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Richard Mortimer @ 2011-01-27 11:21 UTC (permalink / raw)
To: sparclinux
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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sparc: cleaned up FPU version probing
2011-01-27 11:06 [PATCH] sparc: cleaned up FPU version probing Daniel Hellstrom
2011-01-27 11:14 ` Richard Mortimer
2011-01-27 11:21 ` Richard Mortimer
@ 2011-01-27 17:34 ` Sam Ravnborg
2011-01-27 17:36 ` Sam Ravnborg
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Sam Ravnborg @ 2011-01-27 17:34 UTC (permalink / raw)
To: sparclinux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sparc: cleaned up FPU version probing
2011-01-27 11:06 [PATCH] sparc: cleaned up FPU version probing Daniel Hellstrom
` (2 preceding siblings ...)
2011-01-27 17:34 ` Sam Ravnborg
@ 2011-01-27 17:36 ` Sam Ravnborg
2011-01-27 18:39 ` Richard Mortimer
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Sam Ravnborg @ 2011-01-27 17:36 UTC (permalink / raw)
To: sparclinux
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sparc: cleaned up FPU version probing
2011-01-27 11:06 [PATCH] sparc: cleaned up FPU version probing Daniel Hellstrom
` (3 preceding siblings ...)
2011-01-27 17:36 ` Sam Ravnborg
@ 2011-01-27 18:39 ` Richard Mortimer
2011-01-27 19:36 ` Sam Ravnborg
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Richard Mortimer @ 2011-01-27 18:39 UTC (permalink / raw)
To: sparclinux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sparc: cleaned up FPU version probing
2011-01-27 11:06 [PATCH] sparc: cleaned up FPU version probing Daniel Hellstrom
` (4 preceding siblings ...)
2011-01-27 18:39 ` Richard Mortimer
@ 2011-01-27 19:36 ` Sam Ravnborg
2011-01-28 23:09 ` David Miller
2011-01-31 17:00 ` Daniel Hellstrom
7 siblings, 0 replies; 9+ messages in thread
From: Sam Ravnborg @ 2011-01-27 19:36 UTC (permalink / raw)
To: sparclinux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sparc: cleaned up FPU version probing
2011-01-27 11:06 [PATCH] sparc: cleaned up FPU version probing Daniel Hellstrom
` (5 preceding siblings ...)
2011-01-27 19:36 ` Sam Ravnborg
@ 2011-01-28 23:09 ` David Miller
2011-01-31 17:00 ` Daniel Hellstrom
7 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2011-01-28 23:09 UTC (permalink / raw)
To: sparclinux
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sparc: cleaned up FPU version probing
2011-01-27 11:06 [PATCH] sparc: cleaned up FPU version probing Daniel Hellstrom
` (6 preceding siblings ...)
2011-01-28 23:09 ` David Miller
@ 2011-01-31 17:00 ` Daniel Hellstrom
7 siblings, 0 replies; 9+ messages in thread
From: Daniel Hellstrom @ 2011-01-31 17:00 UTC (permalink / raw)
To: sparclinux
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.
^ permalink raw reply [flat|nested] 9+ messages in thread