Date: Wed, 29 Jun 2011 17:31:34 -0400 From: Super Bisquit <superbisquit@gmail.com> To: Marius Strobl <marius@alchemy.franken.de> Cc: freebsd-sparc64@freebsd.org, Jung-uk Kim <jkim@freebsd.org> Subject: Re: [RFC] Clean up sparc64 timecounters Message-ID: <BANLkTimd=STf1bjBbpq9gkXUZfs0Qj4qJQ@mail.gmail.com> In-Reply-To: <20110629205646.GK14797@alchemy.franken.de> References: <201106281327.02537.jkim@FreeBSD.org> <201106281337.54901.jkim@FreeBSD.org> <20110629205646.GK14797@alchemy.franken.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jun 29, 2011 at 4:56 PM, Marius Strobl <marius@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
>
> _______________________________________________
> freebsd-sparc64@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-sparc64
> To unsubscribe, send any mail to "freebsd-sparc64-unsubscribe@freebsd.org"
>
I'm curious as to how this would improve performance on the overall SPARC64
port or is it only for one model?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTimd=STf1bjBbpq9gkXUZfs0Qj4qJQ>