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

[Scott Wood <scottwood@freescale.com>]

On 03/27/2013 09:29:26 PM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, March 28, 2013 1:12 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/26/2013 10:23:38 PM, Wang Dongsheng-B40534 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, March 27, 2013 1:32 AM
> > > > To: Wang Dongsheng-B40534
> > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > linuxppc-dev@lists.ozlabs.org;
> > > > Li Yang-R58472
> > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > > >
> > > > On 03/25/2013 10:29:58 PM, Wang Dongsheng-B40534 wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Saturday, March 23, 2013 6:30 AM
> > > > > > To: Wang Dongsheng-B40534
> > > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > > linuxppc-dev@lists.ozlabs.org;
> > > > > > Li Yang-R58472
> > > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer =20
> support
> > > > > >
> > > > > > On 03/22/2013 01:14:51 AM, Wang Dongsheng-B40534 wrote:
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Wood Scott-B07421
> > > > > > > > Sent: Thursday, March 21, 2013 7:00 AM
> > > > > > > > To: Wang Dongsheng-B40534
> > > > > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > > > > linuxppc-dev@lists.ozlabs.org;
> > > > > > > > Li Yang-R58472
> > > > > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer
> > > support
> > > > > > > >
> > > > > > > > BTW, the input clock frequency has been similarly =20
> scaled,
> > > yet
> > > > > you
> > > > > > > don't
> > > > > > > > try to scrounge up that information to get further
> > > precision...
> > > > > > > >
> > > > > > > Let's go back patch, do you think the code is repeated?
> > > > > > > I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))" =20
> branch,
> > > > > there
> > > > > > > will be no redundant code.
> > > > > >
> > > > > > I'd rather that branch be kept and the more complicated =20
> branch
> > > > > deleted,
> > > > > > and priv->timerfreq frequency be adjusted on initialization =20
> to
> > > > > account
> > > > > > for the scaler.
> > > > >
> > > > > static void convert_ticks_to_time(struct timer_group_priv =20
> *priv,
> > > > >                 const u64 ticks, struct timeval *time) {
> > > > >         u64 tmp_sec;
> > > > >
> > > > >         time->tv_sec =3D (__kernel_time_t)div_u64(ticks,
> > > > > priv->timerfreq);
> > > > >         tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> > > > >
> > > > >         time->tv_usec =3D (__kernel_suseconds_t)
> > > > >                 div_u64((ticks - tmp_sec) * 1000000,
> > > priv->timerfreq);
> > > > >
> > > > >         return;
> > > > > }
> > > > >
> > > > > timer_group_get_freq() {
> > > > > 	...
> > > > > 	if (priv->flags & FSL_GLOBAL_TIMER) {
> > > > > 		div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * =20
> 8;
> > > > > 		priv->timerfreq /=3D div;
> > > > > 	}
> > > > > 	...
> > > > > }
> > > > > Do you want to do that?
> > > >
> > > > 	if (priv->flags & FSL_GLOBAL_TIMER)
> > > > 		priv->timerfreq /=3D 64;
> > > >
> > > > ...but otherwise yes.
> > > Ok, I would like do this.
> > >
> > > if (priv->flags & FSL_GLOBAL_TIMER) {
> > > 	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > 	priv->timerfreq /=3D div;
> >
> > Why?  What do you get out of that obfuscation?
> >
> Change MPIC_TIMER_TCR_CLKDIV_64 to MPIC_TIMER_TCR_CLKDIV

OK, that would at least provide the ability to adjust the clock divider =20
in one place rather than two -- though I don't know why we'd ever need =20
to change the divider.

> Because macro is friendly, and other functions also used the macro.

Using the macro rather than hardcoding register bit encodings is =20
friendly.  The calculation to turn the bit encoding into an actual =20
divider value is not particularly friendly.

-Scott=