From owner-freebsd-sparc64@FreeBSD.ORG Wed Jun 29 22:43:07 2011 Return-Path: Delivered-To: freebsd-sparc64@freebsd.org Received: from [127.0.0.1] (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by hub.freebsd.org (Postfix) with ESMTP id 741CD106566C; Wed, 29 Jun 2011 22:43:07 +0000 (UTC) (envelope-from jkim@FreeBSD.org) From: Jung-uk Kim To: Super Bisquit Date: Wed, 29 Jun 2011 18:42:58 -0400 User-Agent: KMail/1.6.2 References: <201106281327.02537.jkim@FreeBSD.org> <20110629205646.GK14797@alchemy.franken.de> In-Reply-To: MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201106291843.00030.jkim@FreeBSD.org> Cc: freebsd-sparc64@freebsd.org, Marius Strobl Subject: Re: [RFC] Clean up sparc64 timecounters X-BeenThere: freebsd-sparc64@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Porting FreeBSD to the Sparc List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Jun 2011 22:43:07 -0000 On Wednesday 29 June 2011 05:31 pm, Super Bisquit wrote: > On Wed, Jun 29, 2011 at 4: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. 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