From owner-freebsd-sparc64@FreeBSD.ORG Wed Jun 29 23:02:08 2011 Return-Path: Delivered-To: freebsd-sparc64@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4C8D1106566B; Wed, 29 Jun 2011 23:02:08 +0000 (UTC) (envelope-from marius@alchemy.franken.de) Received: from alchemy.franken.de (alchemy.franken.de [194.94.249.214]) by mx1.freebsd.org (Postfix) with ESMTP id DB32E8FC0A; Wed, 29 Jun 2011 23:02:07 +0000 (UTC) Received: from alchemy.franken.de (localhost [127.0.0.1]) by alchemy.franken.de (8.14.4/8.14.4/ALCHEMY.FRANKEN.DE) with ESMTP id p5TN26gl041556; Thu, 30 Jun 2011 01:02:06 +0200 (CEST) (envelope-from marius@alchemy.franken.de) Received: (from marius@localhost) by alchemy.franken.de (8.14.4/8.14.4/Submit) id p5TN26QN041555; Thu, 30 Jun 2011 01:02:06 +0200 (CEST) (envelope-from marius) Date: Thu, 30 Jun 2011 01:02:06 +0200 From: Marius Strobl To: Jung-uk Kim Message-ID: <20110629230206.GM14797@alchemy.franken.de> References: <201106281327.02537.jkim@FreeBSD.org> <201106281337.54901.jkim@FreeBSD.org> <20110629205646.GK14797@alchemy.franken.de> <201106291832.13735.jkim@FreeBSD.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201106291832.13735.jkim@FreeBSD.org> User-Agent: Mutt/1.4.2.3i Cc: freebsd-sparc64@FreeBSD.org 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 23:02:08 -0000 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