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

[Scott Wood]

On 03/27/2013 09:29:26 PM, Wang Dongsheng-B40534 wrote:
> 
> 
> > -----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;  
> linuxppc-dev at 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 at 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 at lists.ozlabs.org;
> > > > > > Li Yang-R58472
> > > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer  
> 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 at 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  
> 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))"  
> branch,
> > > > > there
> > > > > > > will be no redundant code.
> > > > > >
> > > > > > I'd rather that branch be kept and the more complicated  
> branch
> > > > > deleted,
> > > > > > and priv->timerfreq frequency be adjusted on initialization  
> to
> > > > > account
> > > > > > for the scaler.
> > > > >
> > > > > static void convert_ticks_to_time(struct timer_group_priv  
> *priv,
> > > > >                 const u64 ticks, struct timeval *time) {
> > > > >         u64 tmp_sec;
> > > > >
> > > > >         time->tv_sec = (__kernel_time_t)div_u64(ticks,
> > > > > priv->timerfreq);
> > > > >         tmp_sec = (u64)time->tv_sec * (u64)priv->timerfreq;
> > > > >
> > > > >         time->tv_usec = (__kernel_suseconds_t)
> > > > >                 div_u64((ticks - tmp_sec) * 1000000,
> > > priv->timerfreq);
> > > > >
> > > > >         return;
> > > > > }
> > > > >
> > > > > timer_group_get_freq() {
> > > > > 	...
> > > > > 	if (priv->flags & FSL_GLOBAL_TIMER) {
> > > > > 		div = (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) *  
> 8;
> > > > > 		priv->timerfreq /= div;
> > > > > 	}
> > > > > 	...
> > > > > }
> > > > > Do you want to do that?
> > > >
> > > > 	if (priv->flags & FSL_GLOBAL_TIMER)
> > > > 		priv->timerfreq /= 64;
> > > >
> > > > ...but otherwise yes.
> > > Ok, I would like do this.
> > >
> > > if (priv->flags & FSL_GLOBAL_TIMER) {
> > > 	div = (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > 	priv->timerfreq /= 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  
in one place rather than two -- though I don't know why we'd ever need  
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  
friendly.  The calculation to turn the bit encoding into an actual  
divider value is not particularly friendly.

-Scott