Re: [PATCH 6/8] [POWERPC] sysdev,qe_lib: implement FSL GTM support

[Anton Vorontsov <avorontsov@ru.mvista.com>]

On Tue, Apr 08, 2008 at 11:01:53AM +0200, Laurent Pinchart wrote:
> On Tuesday 11 March 2008 18:24, Anton Vorontsov wrote:
> > GTM stands for General-purpose Timers Module and able to generate
> > timer{1,2,3,4} interrupts.
> > 
> > There are several limitations in this support:
> > 1. Cascaded (32 bit) timers unimplemented (1-2, 3-4).
> >    This is straightforward to implement when needed, two timers should
> >    be marked as "requested" and configured as appropriate.
> > 2. Super-cascaded (64 bit) timers unimplemented (1-2-3-4).
> >    This is also straightforward to implement when needed, all timers
> >    should be marked as "requested" and configured as appropriate.
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> 
> [snip]
> 
> > +void gtm_stop_timer_16(struct gtm_timer *tmr)
> > +{
> > +	struct gtm *gtm = tmr->gtm;
> > +	int num = tmr - &gtm->timers[0];
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&gtm->lock, flags);
> > +
> > +	setbits8(tmr->gtcfr, GTCFR_STP(num));
> 
> Shouldn't we clear the timer events with
> 
> out_be16(tmr->gtevr, 0xFFFF);

Yeah.

> here ? Otherwise the timer interrupt could still fire after the timer is 
> stopped. This introduces a race condition in drivers that blindly re-arm the 
> timer in the interrupt handler. I've been bitten by this while porting your 
> FHCI USB driver to a CPM2 platform.

Thanks, will fix.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2