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

[Peter Maydell]

On 26 February 2016 at 03:14, Andrew Jeffery <address@hidden> wrote:
> Hi Peter,
>
> On Thu, 2016-02-25 at 16:11 +0000, Peter Maydell wrote:
>> On 16 February 2016 at 11:34, Andrew Jeffery <address@hidden> wrote:
>> > Implement basic AST2400 timer functionality: Timers can be configured,
>> > enabled, reset and disabled.
>> >
>> > A number of hardware features are not implemented:
>> >
>> > * Timer Overflow interrupts
>> > * Clock value matching
>> > * Pulse generation
>> >
>> > The implementation is enough to boot the Linux kernel configured with
>> > aspeed_defconfig.
>>
>> Thanks; this mostly looks in reasonable shape; I have some comments below.
>>
>> Do we have a datasheet for this chip ?
>
> Unfortunately I don't know of a publicly available datasheet. What's
> the best way to proceed in this case?

We have devices in the tree that are either based on non-public datasheets
or occasionally reverse engineered from Linux kernel drivers. That's OK,
but it's nice to be clear in a comment at the top what the source is,
so people maintaining it later know how much to trust the current code
and (if possible) where to look for clarification.

>> All source files need to #include "qemu/osdep.h" as their first
>> include. That then means you don't need to include assert.h or
>> stdio.h yourself.
>>
>> What do we need from qemu/main-loop.h?
>
> I'm using it for qemu_bh_new() which is required by ptimer, who registers
> the aspeed_timer_tick() callback into the main loop timer handling.

OK, no problem.

>> > +static void aspeed_timer_irq_update(AspeedTimer *t)
>> > +{
>> > +    qemu_set_irq(t->irq, t->enabled);
>>
>> Surely the timer doesn't assert its IRQ line all
>> the time it's enabled? This doesn't look like the right condition.
>
> So I think this is correct despite how it looks. There's some cruft
> from modelling the implementation off of another timer that's probably
> obscuring things, which should be fixed. aspeed_timer_irq_update()
> is only called from aspeed_timer_tick(), so I'll just merge the two.
> Then by renaming aspeed_timer_tick() to aspeed_timer_expire() as
> mentioned above, this won't look so strange? I've read through the
> timer handling code (the processing loop in timerlist_run_timers())
> and my understanding is it has the behaviour we want - callback on
> expiry, not on ticks - which is not how it reads as above.

Usually functions in QEMU called thingy_irq_update() are the ones
that do "look at current state of device and assert IRQ as
necessary"; often this is "mask irq raw state against some
irq-masking bit". Merging this into the timer expire function will
probably help. (Is there no register bit that the guest can query
that indicates "timer expired" or "raw interrupt state" ?)

>> You should implement a VMState struct for device migration,
>> and wire it up here via dc->vmsd.
>
> I'll look into it. I started experimenting with a VMState struct
> early on in the implementation but threw it away as it wasn't my
> primary focus at the time.

We insist on vmstate structs for all new devices, because
they're fairly easy to implement, and if the original
submitter doesn't implement one then the device becomes
a landmine for any user trying migration or vmstate snapshots,
because it will silently misbehave.

thanks
-- PMM