[RFC] Clean up sparc64 timecounters

[Jung-uk Kim]

On Wednesday 29 June 2011 05:31 pm, Super Bisquit wrote:
> On Wed, Jun 29, 2011 at 4:56 PM, Marius Strobl 
<marius at alchemy.franken.de>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. 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
> >
> I'm curious as to how this would improve performance on the overall
> SPARC64 port or is it only for one model?

It is a trivial clean up and it won't really improve performance.

Jung-uk Kim