On Fri, 2016-06-10 at 11:42 +0100, Peter Maydell wrote: > On 10 June 2016 at 01:59, Andrew Jeffery <and...@aj.id.au> wrote: > > > > On Thu, 2016-06-09 at 19:15 +0100, Peter Maydell wrote: > > > > > > On 27 May 2016 at 06:08, Andrew Jeffery <and...@aj.id.au> wrote: > > > > > > > > > > > > Value matching allows Linux to boot with CONFIG_NO_HZ_IDLE=y on the > > > > palmetto-bmc machine. Two match registers are provided for each timer. > > > > > > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au> > > > > --- > > > > > > > > The change pulls out ptimer in favour of the regular timer > > > > infrastructure. As a > > > > consequence it implements the conversions between ticks and time which > > > > feels a > > > > little tedious. Any comments there would be appreciated. > > > Couple of minor comments below. > > > > > > > > > > > > > > > hw/timer/aspeed_timer.c | 135 > > > > ++++++++++++++++++++++++++++++---------- > > > > include/hw/timer/aspeed_timer.h | 6 +- > > > > 2 files changed, 105 insertions(+), 36 deletions(-) > > > > > > > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > > > > index 4b94808821b4..905de0f788b2 100644 > > > > --- a/hw/timer/aspeed_timer.c > > > > +++ b/hw/timer/aspeed_timer.c > > > > @@ -9,13 +9,12 @@ > > > > * the COPYING file in the top-level directory. > > > > */ > > > > > > > > +#include > > > > #include "qemu/osdep.h" > > > osdep.h must always be the first include. > > Thanks for picking that up. > If you like you can run scripts/clean-includes file ... > and it will automatically make any necessary changes like this to > your files.
Thanks, I'll add that to my submission checklist
>
>
> >
> > >
> > > >
> > > > +static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t
> > > > now_ns)
> > > > +{
> > > > + uint64_t delta_ns = now_ns - MIN(now_ns, t->start);
> > > > + uint32_t ticks = (uint32_t) floor(delta_ns / calculate_period(t));
> > > Do we really need to do this with floating point ?
> > I went with floating point to avoid accumulating errors from truncation
> > by integer division. At eg 24MHz truncation increases the tick rate by
> > approximately 1 in 63. We're using floor() here, but that only
> > truncates at the end of the calculation, so the fractional current
> > tick.
> Right, but you only have a risk of truncation because of the way
> you've structured the calculation. You have
>
> floor(delta_ns / calculate_period())
> == floor(delta_ns / (calculate_rate() / NS_PER_SEC))
> == floor((delta_ns * NS_PER_SEC) / calculate_rate())
>
> and calculate_rate() returns either 1000000 or 24000000.
>
> So you have a 64 bit delta_ns, a 32 bit NANOSECONDS_PER_SECOND
> and a 32 bit frequency. We have a function for doing this
> accurately with integer arithmetic: muldiv64() (which takes
> a 64 bit a, 32 bit b and 32 bit c and returns (a*b)/c using
> an intermediate 96 bit precision to avoid overflow).
>
> Doing ticks-to-ns and ns-to-ticks with muldiv64() is pretty
> standard (grep the codebase and you'll see a lot of it).
Right! As I didn't see a concern with floating point prior to sending
the patch I didn't try to rearrange the calculation. I'll rework to use
muldiv64().
>
> >
> > Having said that, grep'ing around under {,include/}hw/ doesn't show a
> > lot of use of floating point. It looks like we are explicitly avoiding
> > it, however with a quick search I didn't find it mentioned in any of
> > the docs. What's the reasoning? Should I use a fixed-point approach
> > like ptimer?
> My view is that hardware doesn't generally use floating point
> so it's odd to see it in a software model of hardware.
Fair enough.
> Double
> doesn't actually get you any more bits than uint64_t anyway...
>
> >
> > >
> > > >
> > > > + return calculate_next(t);
> >
> > >
> > > Why is this recursing ?
> > The common case is that we return during iterating over seq array i.e.
> > we're anticipating another match event (either from the match values or
> > the timer reaching zero). If not then we've overrun, so we reinitialise
> > the timer by resetting the 'start' reference point then searching again
> > for the next event (iterating over seq). As the search for the next
> > event is encoded in the function, I've recursed to reuse it.
> >
> > Would you prefer a loop here?
> >
> > Considering the two approaches (recursion vs loop), if TCO doesn't
> > apply we could blow the stack, and with loops or TCO it's possible we
> > could spin here indefinitely if the timer period is shorter than the
> > time it takes to recalculate. Arguably, not crashing is better so I'm
> > happy to rework it.
> Yes, I'd prefer a loop -- TCO is an optimization, not a guarantee
> by the compiler.
Yes, on reflection I agree. I'll rework it.
>
> >
> > As an aside, the approach for reinitialising start skips countdown
> > periods that were completely missed through high service latency, and
> > there will be no interrupts issued for the missing events. Is that
> > reasonable?
> If the countdown period is so short that we can't service it
> in time then the system is probably not going to be functional
> anyway, so I don't think it matters very much. (In ptimer we
> impose an artificial limit on the timeout rate for a periodic timer
> and just refuse to fire any faster than that: see ptimer_reload().)
I'll leave it as is for v2.
Thanks for the feedback!
Andrew
signature.asc
Description: This is a digitally signed message part
