[RFC] Clean up sparc64 timecounters

[Marius Strobl]

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. 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'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.
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).

> 
> @@ -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