From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56967) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCgiS-0004pZ-Dt for qemu-devel@nongnu.org; Tue, 14 Jun 2016 01:16:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCgiQ-0005LF-16 for qemu-devel@nongnu.org; Tue, 14 Jun 2016 01:16:43 -0400 Message-ID: <1465881389.2644.14.camel@aj.id.au> From: Andrew Jeffery Reply-To: andrew@aj.id.au Date: Tue, 14 Jun 2016 15:16:29 +1000 In-Reply-To: References: <1464325706-11221-1-git-send-email-andrew@aj.id.au> <1465520347.16048.180.camel@aj.id.au> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-Mfb4wUvIwWOxB0HA5QvL" Mime-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] hw/timer: Add value matching support to aspeed_timer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: =?ISO-8859-1?Q?C=E9dric?= Le Goater , Joel Stanley , QEMU Developers , qemu-arm --=-Mfb4wUvIwWOxB0HA5QvL Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2016-06-10 at 11:42 +0100, Peter Maydell wrote: > On 10 June 2016 at 01:59, Andrew Jeffery wrote: > >=20 > > On Thu, 2016-06-09 at 19:15 +0100, Peter Maydell wrote: > > >=20 > > > On 27 May 2016 at 06:08, Andrew Jeffery wrote: > > > >=20 > > > >=20 > > > > Value matching allows Linux to boot with CONFIG_NO_HZ_IDLE=3Dy on t= he > > > > palmetto-bmc machine. Two match registers are provided for each tim= er. > > > >=20 > > > > Signed-off-by: Andrew Jeffery > > > > --- > > > >=20 > > > > The change pulls out ptimer in favour of the regular timer infrastr= ucture. As a > > > > consequence it implements the conversions between ticks and time wh= ich feels a > > > > little tedious. Any comments there would be appreciated. > > > Couple of minor comments below. > > >=20 > > > >=20 > > > >=20 > > > > =C2=A0hw/timer/aspeed_timer.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0| 135 ++++++++++++++++++++++++++++++---------- > > > > =C2=A0include/hw/timer/aspeed_timer.h |=C2=A0=C2=A0=C2=A06 +- > > > > =C2=A02 files changed, 105 insertions(+), 36 deletions(-) > > > >=20 > > > > 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 @@ > > > > =C2=A0 * the COPYING file in the top-level directory. > > > > =C2=A0 */ > > > >=20 > > > > +#include > > > > =C2=A0#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 >=20 >=20 > >=20 > > >=20 > > > >=20 > > > > +static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint= 64_t now_ns) > > > > +{ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0uint64_t delta_ns =3D now_ns - MIN(now_ns,= t->start); > > > > +=C2=A0=C2=A0=C2=A0=C2=A0uint32_t ticks =3D (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 >=20 > floor(delta_ns / calculate_period()) > =3D=3D floor(delta_ns / (calculate_rate() / NS_PER_SEC)) > =3D=3D floor((delta_ns * NS_PER_SEC) / calculate_rate()) >=20 > and calculate_rate() returns either 1000000 or 24000000. >=20 > 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). >=20 > 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(). >=20 > >=20 > > 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... >=20 > >=20 > > >=20 > > > >=20 > > > > +=C2=A0=C2=A0=C2=A0=C2=A0return calculate_next(t); > >=20 > > >=20 > > > 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. > >=20 > > Would you prefer a loop here? > >=20 > > 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. >=20 > >=20 > > 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 --=-Mfb4wUvIwWOxB0HA5QvL Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCgAGBQJXX5MtAAoJEJ0dnzgO5LT56F4P/3C0pmIsfwsr0RaZUc7CQdiN 8j488ThZV1WJlj7pzG8cv7NvBwWOOokJ9ezmJ9S907CQ7oIANoQ552FxsFyruiNi hkvmgBvwBp+HRwa+FHU6/vFrAYI6kFiJYAD8hY0dZHWRZGao/PZzTh/fq4IvKY/r tUY69GLMXjWfWb54vksY33PoOWBBmgHlKpb5naV+qPIrtnhjZ4PSDknyE91u1T5g pDNCuxNAa+UKpzXXmAnLAxm0U0OQEPnz+RyAdMjqYNLjuW/NSJy/1Y9taIT5VdwC wMcUmeoDNY5AeYlKI+eRGbyuYYBL93v9W516qeTsSXFHPAwW4gF+l8jP/yU41Tx9 TU2TYCRhhncbcjL++M4zw5JUQl/IEoTmnsPLK1kuLtyuN/KpG1ZsrL8Dl7zLUaRQ TR6gBdqpWsw1PYcUXgjLOnfmknQ+CIcqTr8kkFdCUxbi/ZwYmO+bT7frFIGSNUIR 1UKYb50FYsjxVf0hZsfJYarCH1DUTEpefnDaHmaMYPnh4veGLu/Qdp0sIWkUp9Jg 6t1rB0NPPqSXv2d8L5W47IB5gXEiC0aZD9n/qqntHhILtc9tFDcm0zGGR2z1+DV2 /z6MPKs35BPe2EJvKU5tkFDc74uAbOr8WogrvVSZi4kX8g4sg4EzTQc42BwsOt+D hiXd21d9cO28VPeI+i9Q =7NWu -----END PGP SIGNATURE----- --=-Mfb4wUvIwWOxB0HA5QvL--