[RESEND] ixp4xx: clockevent set_next_event fix

Submitted by Michał Wróbel on Feb. 14, 2012, 9:55 a.m.

Details

Message ID 4F3A2F79.6050806@flytronic.pl
State New
Headers show

Commit Message

Michał Wróbel Feb. 14, 2012, 9:55 a.m.
IXP43x Developer's Manual [17.4.3] and IXP4[56]x Developer's Manual
[18.4.3] say that for predictable operation the timer needs to be
stopped before writing a new value into the reload register. Indeed,
tests on IXP435 show that writing a new value into the reload register
without stopping the timer first has no immediate effect on the timer.
This makes hrtimers started through hrtimer_start() to be delayed until
the currently earliest hrtimer expires.

IXP42x Developer's Manual [14.3] says that the timer will be reloaded
immediately on setting the timer reload register, so the bug probably
doesn't occur on those CPUs. However, stopping the timer shouldn't have
any negative side effects, so it should be safe to apply it
machine-wide.

Signed-off-by: Michał Wróbel <michal.wrobel@flytronic.pl>
---
 arch/arm/mach-ixp4xx/common.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

The previous patch had a mistake in the description - wrong reference to
"General-Purpose Timers" section in IXP42x Developer's Manual.

Comments

Richard Cochran Feb. 14, 2012, 12:01 p.m.
On Tue, Feb 14, 2012 at 10:55:05AM +0100, Micha?? Wr??bel wrote:
> IXP43x Developer's Manual [17.4.3] and IXP4[56]x Developer's Manual
> [18.4.3] say that for predictable operation the timer needs to be
> stopped before writing a new value into the reload register. Indeed,
> tests on IXP435 show that writing a new value into the reload register
> without stopping the timer first has no immediate effect on the timer.
> This makes hrtimers started through hrtimer_start() to be delayed until
> the currently earliest hrtimer expires.
> 
> IXP42x Developer's Manual [14.3] says that the timer will be reloaded
> immediately on setting the timer reload register, so the bug probably
> doesn't occur on those CPUs. However, stopping the timer shouldn't have
> any negative side effects, so it should be safe to apply it
> machine-wide.

Unless you test this out and confirm that it works for all IXP4xx, I
would prefer to see a specific timer function for the 43x instead.

Thanks,
Richard


> 
> Signed-off-by: Micha?? Wr??bel <michal.wrobel@flytronic.pl>
> ---
>  arch/arm/mach-ixp4xx/common.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> The previous patch had a mistake in the description - wrong reference to
> "General-Purpose Timers" section in IXP42x Developer's Manual.
> 
> diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c
> index 3841ab4..fd37c83 100644
> --- a/arch/arm/mach-ixp4xx/common.c
> +++ b/arch/arm/mach-ixp4xx/common.c
> @@ -434,6 +434,7 @@ static int ixp4xx_set_next_event(unsigned long evt,
>  {
>  	unsigned long opts = *IXP4XX_OSRT1 & IXP4XX_OST_RELOAD_MASK;
>  
> +	*IXP4XX_OSRT1 = 0;
>  	*IXP4XX_OSRT1 = (evt & ~IXP4XX_OST_RELOAD_MASK) | opts;
>  
>  	return 0;
> -- 
> 1.7.5.4
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Michał Wróbel Feb. 14, 2012, 12:21 p.m.
On 14.02.2012 13:01, Richard Cochran wrote:
> On Tue, Feb 14, 2012 at 10:55:05AM +0100, Michał Wróbel wrote:
>> IXP43x Developer's Manual [17.4.3] and IXP4[56]x Developer's Manual
>> [18.4.3] say that (...)
>>
>> IXP42x Developer's Manual [14.3] says that (...)
> Unless you test this out and confirm that it works for all IXP4xx, I
> would prefer to see a specific timer function for the 43x instead.
>
> Thanks,
> Richard
I think I might have some IXP425-based boards available for testing.
However, I certainly don't have any IXP45x- or IXP46x-based board. Maybe
I'll prepare a small kernel module that will allow to easily test this
issue by other list members who have IXP45x- and IXP46x-based boards?

Regards,
Michał
>> Signed-off-by: Michał Wróbel <michal.wrobel@flytronic.pl>
>> ---
>>  arch/arm/mach-ixp4xx/common.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> The previous patch had a mistake in the description - wrong reference to
>> "General-Purpose Timers" section in IXP42x Developer's Manual.
>>
>> diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c
>> index 3841ab4..fd37c83 100644
>> --- a/arch/arm/mach-ixp4xx/common.c
>> +++ b/arch/arm/mach-ixp4xx/common.c
>> @@ -434,6 +434,7 @@ static int ixp4xx_set_next_event(unsigned long evt,
>>  {
>>  	unsigned long opts = *IXP4XX_OSRT1 & IXP4XX_OST_RELOAD_MASK;
>>  
>> +	*IXP4XX_OSRT1 = 0;
>>  	*IXP4XX_OSRT1 = (evt & ~IXP4XX_OST_RELOAD_MASK) | opts;
>>  
>>  	return 0;
>> -- 
>> 1.7.5.4
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Michał Wróbel Feb. 14, 2012, 2:47 p.m.
On 14.02.2012 13:21, Michał Wróbel wrote:
> On 14.02.2012 13:01, Richard Cochran wrote:
>> On Tue, Feb 14, 2012 at 10:55:05AM +0100, Michał Wróbel wrote:
>>> IXP43x Developer's Manual [17.4.3] and IXP4[56]x Developer's Manual
>>> [18.4.3] say that (...)
>>>
>>> IXP42x Developer's Manual [14.3] says that (...)
>> Unless you test this out and confirm that it works for all IXP4xx, I
>> would prefer to see a specific timer function for the 43x instead.
>>
>> Thanks,
>> Richard
> I think I might have some IXP425-based boards available for testing.
I will have it available later.
> However, I certainly don't have any IXP45x- or IXP46x-based board.
> Maybe I'll prepare a small kernel module that will allow to easily test this
> issue
Done. See the attachments.

Results of my tests performed on IXP435-based board (667 MHz,
CONFIG_HZ=100) follow below.

Without the patch:

starting measurements: 10000 samples * 1000000 ns
measurements finished
min: 2500 ns
avg: 7894228 ns
max: 9970743 ns

With the patch:

starting measurements: 10000 samples * 1000000 ns
measurements finished
min: 1637 ns
avg: 4266 ns
max: 100182 ns

>  by other list members who have IXP45x- and IXP46x-based boards?
Does anyone have any IXP45x- and IXP46x-based board? If so, I would like
to ask to compile ixp4xx_set_next_event_bug_test.c as a module and run
it on such a board. Then, to apply the "ixp4xx: clockevent
set_next_event fix" patch and re-run the tests. Of course, finally
posting the results here.

Note that the kernel used for testing has to be configured with
CONFIG_HIGH_RES_TIMERS=y

Thank you in advance!

Best regards,
Michał

>>> Signed-off-by: Michał Wróbel <michal.wrobel@flytronic.pl>
>>> ---
>>>  arch/arm/mach-ixp4xx/common.c |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>> (...)
>>> diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c
>>> index 3841ab4..fd37c83 100644
>>> --- a/arch/arm/mach-ixp4xx/common.c
>>> +++ b/arch/arm/mach-ixp4xx/common.c
>>> @@ -434,6 +434,7 @@ static int ixp4xx_set_next_event(unsigned long evt,
>>>  {
>>>  	unsigned long opts = *IXP4XX_OSRT1 & IXP4XX_OST_RELOAD_MASK;
>>>  
>>> +	*IXP4XX_OSRT1 = 0;
>>>  	*IXP4XX_OSRT1 = (evt & ~IXP4XX_OST_RELOAD_MASK) | opts;
>>>  
>>>  	return 0;
>>> -- 
>>> 1.7.5.4
obj-m += ixp4xx_set_next_event_bug_test.o

Patch hide | download patch | download mbox

diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c
index 3841ab4..fd37c83 100644
--- a/arch/arm/mach-ixp4xx/common.c
+++ b/arch/arm/mach-ixp4xx/common.c
@@ -434,6 +434,7 @@  static int ixp4xx_set_next_event(unsigned long evt,
 {
 	unsigned long opts = *IXP4XX_OSRT1 & IXP4XX_OST_RELOAD_MASK;
 
+	*IXP4XX_OSRT1 = 0;
 	*IXP4XX_OSRT1 = (evt & ~IXP4XX_OST_RELOAD_MASK) | opts;
 
 	return 0;