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

[Andrew Jeffery]

On Fri, 2016-02-26 at 10:20 +0000, Peter Maydell wrote:
> 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.

No worries, I'll add notes in the header comments for each of the new
files.

> 
> > 
> > > 
> > > 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" ?)

It doesn't appear so - overflow interrupts can be enabled or disabled,
but it doesn't appear that there's a way to poll whether the timer has
expired. It doesn't look like it can be inferred either as the counter
status register doesn't stick at zero, rather resets back to the reload
register value (and my interpretation is it continues to count down if
the enabled bit remains set). Further, the interrupt on overflow bit
doesn't appear to be set by the kernel driver, but the two match
registers are initialised to zero which ensures an interrupt will be
fired. Non-zero match registers aren't currently supported by the
device model. I'll try to make this all clearer in the code.

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

Yeah, no worries, v3 of the series will have VMState structs for all
devices.

Cheers,

Andrew

> 
> thanks
> -- PMM

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