Date: Thu, 30 Jun 2011 01:02:06 +0200 From: Marius Strobl <marius@alchemy.franken.de> To: Jung-uk Kim <jkim@FreeBSD.org> Cc: freebsd-sparc64@FreeBSD.org Subject: Re: [RFC] Clean up sparc64 timecounters Message-ID: <20110629230206.GM14797@alchemy.franken.de> In-Reply-To: <201106291832.13735.jkim@FreeBSD.org> References: <201106281327.02537.jkim@FreeBSD.org> <201106281337.54901.jkim@FreeBSD.org> <20110629205646.GK14797@alchemy.franken.de> <201106291832.13735.jkim@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jun 29, 2011 at 06:32:11PM -0400, Jung-uk Kim wrote:
> 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.
I'm aware of that. The point of the existing code is to not make
assumptions about what 32-bit half of the register is used as
shifting down has to be done in the tc_get_timecount method as long
as the MI code only supports up to 32-bit counters. I've also just
checked that
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);
and
return (SCHIZO_CTRL_READ_8(sc, STX_CTRL_PERF_CNT));
both result in the same object code already with -O so there's no
real reason to use the latter, which IMO would be a clean-down.
In any case, the following approach is wrong as it breaks when
replacing CNT0 with CNT1 as long as tc_counter_mask is u_int
+ tc->tc_counter_mask = STX_CTRL_PERF_CNT_MASK <<
+ STX_CTRL_PERF_CNT_CNT0_SHIFT;
>
> > 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.
My point is that if you use the raw value, i.e. the full 64-bit,
of the counter register you should also use the corresponding raw
and 64-bit mask for tc_counter_mask for consistency. In both cases
these raw values will converted to 32-bit implicitly, which is why
I prefer the existing TC_COUNTER_MAX_MASK approach.
Marius
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110629230206.GM14797>