On Fri, 2016-03-11 at 15:56 +0700, Peter Maydell wrote: > On 5 March 2016 at 11:29, Andrew Jeffery <and...@aj.id.au> wrote: > > Implement basic AST2400 timer functionality: Up to 8 timers can > > independently be configured, enabled, reset and disabled. A couple of > > hardware features are not implemented, namely clock value matching and > > pulse generation, but the implementation is enough to boot the Linux > > kernel configured with aspeed_defconfig. > > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au> > > --- > > +/** > > + * Avoid mutual references between AspeedTimerCtrlState and AspeedTimer > > + * structs, as it's a waste of memory and it makes implementing > > + * VMStateDescription a little clunky. > > Not sure what you have in mind with the reference to VMStateDescription > here. The vmstate struct only has to list the fields which contain > actual volatile state -- things like backreference pointers to other > structs aren't volatile state so don't appear.
Good point, looks like I was over-thinking things. I'll remove the part
referencing VMStateDescription.
>
> > The ptimer BH callback needs to know
> > + * whether a specific AspeedTimer is enabled, but this information is held
> > in
> > + * AspeedTimerCtrlState. So, provide a helper to hoist ourselves from an
> > + * arbitrary AspeedTimer to AspeedTimerCtrlState.
> > + */
> > +static inline struct AspeedTimerCtrlState *timer_to_ctrl(AspeedTimer *t)
> > +{
> > + AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
> > + return container_of(timers, AspeedTimerCtrlState, timers);
> > +}
>
> > +static void aspeed_timer_expire(void *opaque)
> > +{
> > + AspeedTimer *t = opaque;
> > +
> > + /* Only support interrupts on match values of zero for the moment -
> > this is
> > + * sufficient to boot an aspeed_defconfig Linux kernel. Non-zero match
> > + * values need some further consideration given the current ptimer API.
> > + * Maybe run multiple ptimers?
> > + */
>
> See hw/timer/a9gtimer.c for an example of a timer with a comparator
> that can fire when the timer hits an arbitrary comparator value
> (it doesn't use ptimers but the principle is the same -- you set
> the timer to fire at the next interesting event, and then in the
> timer-fired handler you reset the timer to fire whenever the next
> event after that is, if any.) In any case this is probably ok for now.
Thanks for the pointer. I'll leave that change to a future patch given
it looks like this is converging on being acceptable, though I'll
expand the comment to cover a9gtimer.
>
> > + bool match = !(t->match[0] && t->match[1]);
> > + bool interrupt = timer_overflow_interrupt(t) || match;
> > + if (timer_enabled(t) && interrupt) {
> > + t->level = !t->level;
> > + qemu_set_irq(t->irq, t->level);
> > + }
> > +}
> > +
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
Thanks!
Andrew
>
> thanks
> -- PMM
signature.asc
Description: This is a digitally signed message part
