Re: [PATCH 2/3] powerpc/mpic: add global timer support

[Scott Wood <scottwood@freescale.com>]

On 03/20/2013 01:45:03 AM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, March 20, 2013 6:59 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Li Yang-R58472
> > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> >
> > On 03/19/2013 02:55:58 AM, Wang Dongsheng-B40534 wrote:
> > > > > +static void convert_ticks_to_time(struct timer_group_priv =20
> *priv,
> > > > > +		const u64 ticks, struct timeval *time) {
> > > > > +	u64 tmp_sec;
> > > > > +	u32 rem_us;
> > > > > +	u32 div;
> > > > > +
> > > > > +	if (!(priv->flags & FSL_GLOBAL_TIMER)) {
> > > > > +		time->tv_sec =3D (__kernel_time_t)
> > > > > +			div_u64_rem(ticks, priv->timerfreq, =20
> &rem_us);
> > > > > +		tmp_sec =3D (u64)time->tv_sec * =20
> (u64)priv->timerfreq;
> > > > > +		time->tv_usec =3D (__kernel_suseconds_t)
> > > > > +			div_u64((ticks - tmp_sec) * 1000000,
> > > > > priv->timerfreq);
> > > > > +
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > > > +
> > > > > +	time->tv_sec =3D (__kernel_time_t)div_u64(ticks, priv-
> > >timerfreq
> > > > > / div);
> > > > > +	tmp_sec =3D div_u64((u64)time->tv_sec * =20
> (u64)priv->timerfreq,
> > > > > div);
> > > > > +
> > > > > +	time->tv_usec =3D (__kernel_suseconds_t)
> > > > > +		div_u64((ticks - tmp_sec) * 1000000, =20
> priv->timerfreq /
> > > > > div);
> > > > > +
> > > > > +	return;
> > > >
> > > > Why don't you just adjust the clock frequency up front for
> > > CLKDIV_64,
> > > > rather than introduce alternate (and untested!) code paths
> > > throughout the
> > > > driver?
> > > >
> > > No, It cannot be integrated. The div cannot be removed.
> > > Because if do priv->timerfreq /=3D div, that will affect the =20
> accuracy.
> > >
> > > Like:
> > > 3 * 5 / 2 =3D 7;
> > > 3 / 2 * 5 =3D 5;
> >
> > I don't follow -- a change in the clock speed is a change in the =20
> clock
> > speed, no matter how you accomplish it.
> >
> This is not change hardware clock frequency.

Citation needed.  It looks like a change in the timer frequency to me:

   Clock ratio. Specifies the ratio of the timer frequency to the MPIC =20
input clock (platform clock/2) . The
   following clock ratios are supported:
   00
   01
   10
   11
   Default. Divide by 8
   Divide by 16
   Divide by 32
   Divide by 64

The end result is that the counter in the timer register changes only
1/64 as often as the input clock.  There's nothing special about that,
compared to having an input clock that is 1/64 the speed.

> The mpic timer hardware clock is not be changed after initialization. =20
> This is just conversion ticks.
> These calculated ticks will be set to the hardware.
>=20
> > How you round is a different question.  You should probably be =20
> rounding
> > up always, based on the final clock frequency -- though it's =20
> unlikely to
> > matter much given the high precision of the timer relative to the =20
> input
> > granularity.
> >
> Each ticks are based on the mpic timer hardware clock frequency.
> The conversion and calculation are in order to make the tick value is =20
> more
> accurate, more close to real time.
> If echo 40 seconds may be difference is not obvious. But echo =20
> 315360000(10 years)
> difference is obvious.

So basically you're taking advantage of the fact that you have what
appears to be a more precise value of the frequency than is expressible
in integer Hz -- but I think that's false precision; odds are the
frequency is not accurate to 1 Hz to begin with.  Even if it is, I doubt
it's worth worrying about.  The error as a percentage will still be very
small with an input frequency of many MHz.  Does an error of a few
minutes really matter if you're delaying for 10 years?  That's =20
acceptable
clock drift for something not synced to network time.  The main thing is
to ensure that you round up, not down, so that software doesn't see an
early wakeup as measured by its own timers.

BTW, the input clock frequency has been similarly scaled, yet you don't
try to scrounge up that information to get further precision...

-Scott=