[RFC] Clean up sparc64 timecounters

[Jung-uk Kim]

On Wednesday 29 June 2011 04:56 pm, Marius Strobl wrote:
> On Tue, Jun 28, 2011 at 01:37:52PM -0400, Jung-uk Kim wrote:
> > On Tuesday 28 June 2011 01:26 pm, Jung-uk Kim wrote:
> > > Can you please review the attached patch?
> > >
> > > sys/sparc64/pci/fire.c:
> > > - Remove redundant timecounter masking from tc_get_timecount
> > > method. - Remove an unnecessary macro for timecounter mask.
> > > - Remove a redundant NULL assignment.
> > >
> > > sys/sparc64/pci/schizo.c:
> > > - Remove redundant timecounter masking from tc_get_timecount
> > > method. - Correct timecounter mask.  Note this is a no-op
> > > because the STX_CTRL_PERF_CNT_CNT0_SHIFT is actually zero.
> > > - Remove a redundant NULL assignment.
>
> I'm not sure whether you correctly understand how that timer works.
> The hardware actually provides a pair of 32-bit timers which are
> read via a single 64-bit register so the existing tc_counter_mask
> is correct and your change is wrong. For the same reason the
> masking and shifting in schizo_get_timecount() only happens to be
> unnecessary in so far as we currently use the lower 32-bit counter
> and the tc_get_timecount methods return u_int.

Actually, tc_counter_mask is also u_int and the upper 32-bit is always 
ignored.

> If we'd either switch to the upper 32-bit counter or the timecounter
> code would be enhanced to support up to 64-bit counters it wouldn't
> be redundant. 

There is no reason to support 64-bit timecounter as kern_tc.c was 
meant to be MI code.

> There's actually a right-shift missing in schizo_get_timecount()
> though, i.e. it should actually do:
> 	return ((SCHIZO_CTRL_READ_8(sc, STX_CTRL_PERF_CNT) &
> 	    (STX_CTRL_PERF_CNT_MASK << STX_CTRL_PERF_CNT_CNT0_SHIFT) >>
> 	    STX_CTRL_PERF_CNT_CNT0_SHIFT);
> The compiler should be smart enough to boil all that down to a
> single 64-bit to 32-bit conversion when returning though.

tc_get_timecount method should return a raw value.  There is no reason 
to mask it here because kern_tc.c does it whenever necessary.

> For similar reasons I'd prefer to also keep the masking in
> fire_get_timecount(), besides using the macro IMO is cleaner
> than using ~0u(l).

Please see above.  tc_counter_mask is u_int.

Jung-uk Kim

> > @@ -686,8 +684,7 @@ fire_attach(device_t dev)
> >                 if (tc == NULL)
> >                         panic("%s: could not malloc timecounter",
> > __func__); tc->tc_get_timecount = fire_get_timecount; -          
> >     tc->tc_poll_pps = NULL;
> > -               tc->tc_counter_mask = TC_COUNTER_MAX_MASK;
> > +               tc->tc_counter_mask = ~0ul;
> >                                       ^^^^
> >                                       ~0u
> >                 if (OF_getprop(OF_peer(0), "clock-frequency",
> > &prop, sizeof(prop)) == -1)
> >                         panic("%s: could not determine clock
> > frequency",
>
> Well, if you really remove the masking from fire_get_timecount()
> then you should actually also use ~0ul here for consistency as it's
> an 64-bit counter.
>
> Marius