| Message ID | 4F3A2F79.6050806@flytronic.pl |
|---|---|
| State | New |
| Headers | show |
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
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
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
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;
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.