[PATCH,RFC] mmp clockevent handling race

[Haojian Zhuang]

On Tue, 2011-06-07 at 20:05 -0700, Eric Miao wrote:
> On Wed, Jun 8, 2011 at 10:25 AM, Haojian Zhuang
> <haojian.zhuang at marvell.com> wrote:
> > On Tue, 2011-06-07 at 11:29 -0700, Nicolas Pitre wrote:
> >> On Tue, 7 Jun 2011, Lennert Buytenhek wrote:
> >>
> >> > Hi!
> >> >
> >> > The patch below has been sitting around in the OLPC XO-1.75 (MMP2-based
> >> > XO) bringup kernel tree for some time now, and upon recent rebasing of
> >> > this tree to 3.0, it was discovered that something like this patch is
> >> > still needed.
> >> >
> >> > Symptoms: e.g. as described here: http://dev.laptop.org/ticket/10945
> >> > -- applications hang for a couple of minutes at a time, and sometimes
> >> > there are several-minute hangs during the boot process.
> >> >
> >> > >From the ticket:
> >> >
> >> >     The problem in the current upstream mmp timer handling code
> >> >     that appears to be triggered here is that it handles clockevents
> >> >     by setting up a comparator on the free-running clocksource timer
> >> >     to match and trigger an interrupt at 'current_time + delta',
> >> >     which if delta is small enough can lead to 'current_time + delta'
> >> >     already being in the past when comparator setup has finished,
> >> >     and the requested event will not trigger.
> >>
> >> This is a classical issue that was solved on the SA1100 more than 10
> >> years ago.
> >>
> >> > What this patch does is to rewrite the timer handling code to use
> >> > two timers, one for the free-running clocksource, and one to trigger
> >> > clockevents with, which is more or less the standard approach to this.
> >> > It's kind of invasive, though (certainly more invasive than it strictly
> >> > needs to be, as it reorganises time.c somewhat at the same time), and
> >> > something like this is probably too invasive for 3.0 at this point.
> >> >
> >> > A safer "fix" for 3.0 may be to disallow NO_HZ/HIGH_RES_TIMERS on mmp.
> >> >
> >> > Any thoughts?
> >>
> >> What about simply this:
> >>
> >> diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c
> >> index 99833b9..8b8f99a 100644
> >> --- a/arch/arm/mach-mmp/time.c
> >> +++ b/arch/arm/mach-mmp/time.c
> >> @@ -39,7 +39,7 @@
> >>
> >>  #define TIMERS_VIRT_BASE     TIMERS1_VIRT_BASE
> >>
> >> -#define MAX_DELTA            (0xfffffffe)
> >> +#define MAX_DELTA            (0xfffffffe - 16)
> >>  #define MIN_DELTA            (16)
> >>
> >>  static DEFINE_CLOCK_DATA(cd);
> >> @@ -85,7 +85,7 @@ static irqreturn_t timer_interrupt(int irq, void *dev_id)
> >>  static int timer_set_next_event(unsigned long delta,
> >>                               struct clock_event_device *dev)
> >>  {
> >> -     unsigned long flags, next;
> >> +     unsigned long flags, next, now;
> >>
> >>       local_irq_save(flags);
> >>
> >> @@ -95,9 +95,10 @@ static int timer_set_next_event(unsigned long delta,
> >>
> >>       next = timer_read() + delta;
> >>       __raw_writel(next, TIMERS_VIRT_BASE + TMR_TN_MM(0, 0));
> >> +     now = timer_read();
> >>
> >>       local_irq_restore(flags);
> >> -     return 0;
> >> +     return (signed)(next - now) <= MIN_DELTA ? -ETIME : 0;
> >>  }
> >>
> >>  static void timer_set_mode(enum clock_event_mode mode,
> >>
> >>
> >> Nicolas
> > It seems good. But we still need to use two different timer. Because
> > writing match register needs to stop counter register first. This is a
> > limitation in the silicon.
> 
> Well, the first thing is Lennert's RFC patch needs a bit cleanup :-) I tried
> very hard to figure out the actual changes.
> 
> Using two timers isn't a bad idea. Yet if it has to disable timer0 when
> modifying timer1's next triggering event, it doesn't seem to solve the
> real problem with two timers.
Since timer registers is defined as group mode. Counter register and
match register exist in each group. We can use timer 0 as clock source,
only read timer counter. We can use timer 1 as clock event. While we're
writing match register, we just need to disable counter register of
group 1, not group 0. Since it's silicon limitation.
> 
> Haojian,
> 
> Do we have any other timer that could be used as an independent
> clock source?
So both timer 0 and timer 1 are necessary. Timer0 can be used as clock
source. Timer 1 can be used as clock event. And we needn't other timer
to be used as clock source.