qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH v3 1/3] hw/timer: Add ASPEED timer device model


From: Andrew Jeffery
Subject: Re: [Qemu-arm] [PATCH v3 1/3] hw/timer: Add ASPEED timer device model
Date: Sat, 12 Mar 2016 13:36:39 +1030

On Fri, 2016-03-11 at 15:56 +0700, Peter Maydell wrote:
> On 5 March 2016 at 11:29, Andrew Jeffery <address@hidden> 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 <address@hidden>
> > ---
> > +/**
> > + * 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 <address@hidden>

Thanks!

Andrew

> 
> thanks
> -- PMM

Attachment: signature.asc
Description: This is a digitally signed message part


reply via email to

[Prev in Thread] Current Thread [Next in Thread]