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

[Peter Maydell]

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.

> 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.

> +    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
-- PMM