Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753372AbdLDFNP (ORCPT ); Mon, 4 Dec 2017 00:13:15 -0500 Received: from mga03.intel.com ([134.134.136.65]:37478 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752968AbdLDFNM (ORCPT ); Mon, 4 Dec 2017 00:13:12 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,357,1508828400"; d="scan'208";a="183643347" Subject: Re: [PATCH 1/1] timecounter: Make cyclecounter struct part of timecounter struct To: Richard Cochran Cc: linux-kernel@vger.kernel.org, Chris Wilson , John Stultz , Thomas Gleixner , Stephen Boyd , linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org, linux-rdma@vger.kernel.org, alsa-devel@alsa-project.org, kvmarm@lists.cs.columbia.edu References: <1512189095-28654-1-git-send-email-sagar.a.kamble@intel.com> <20171202180458.winwznbstsi355ed@localhost> From: Sagar Arun Kamble Message-ID: <8cfdbb16-7053-1d8f-adae-4ce2487fe4ea@intel.com> Date: Mon, 4 Dec 2017 10:43:05 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171202180458.winwznbstsi355ed@localhost> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3962 Lines: 102 On 12/2/2017 11:34 PM, Richard Cochran wrote: > On Sat, Dec 02, 2017 at 10:01:35AM +0530, Sagar Arun Kamble wrote: >> There is no real need for the users of timecounters to define cyclecounter >> and timecounter variables separately. Since timecounter will always be >> based on cyclecounter, have cyclecounter struct as member of timecounter >> struct. > Overall, this is a welcome change. However, it doesn't go far enough, > IMHO, and I'll explain that more below. > >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c >> index 0247885..35987b5 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c > ... > > As it stands now, timecounter_init() is used for two totally different > reasons. Some callers only want to set the time, ... > >> @@ -207,7 +207,7 @@ static int mlx4_en_phc_settime(struct ptp_clock_info *ptp, >> >> /* reset the timecounter */ >> write_seqlock_irqsave(&mdev->clock_lock, flags); >> - timecounter_init(&mdev->clock, &mdev->cycles, ns); >> + timecounter_init(&mdev->clock, ns); >> write_sequnlock_irqrestore(&mdev->clock_lock, flags); >> >> return 0; > ... while others initialize the data structure the first time: > >> @@ -274,17 +274,17 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev) >> >> seqlock_init(&mdev->clock_lock); >> >> - memset(&mdev->cycles, 0, sizeof(mdev->cycles)); >> - mdev->cycles.read = mlx4_en_read_clock; >> - mdev->cycles.mask = CLOCKSOURCE_MASK(48); >> - mdev->cycles.shift = freq_to_shift(dev->caps.hca_core_clock); >> - mdev->cycles.mult = >> - clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift); >> - mdev->nominal_c_mult = mdev->cycles.mult; >> + memset(&mdev->clock.cc, 0, sizeof(mdev->clock.cc)); >> + mdev->clock.cc.read = mlx4_en_read_clock; >> + mdev->clock.cc.mask = CLOCKSOURCE_MASK(48); >> + mdev->clock.cc.shift = freq_to_shift(dev->caps.hca_core_clock); >> + mdev->clock.cc.mult = >> + clocksource_khz2mult(1000 * dev->caps.hca_core_clock, >> + mdev->clock.cc.shift); >> + mdev->nominal_c_mult = mdev->clock.cc.mult; >> >> write_seqlock_irqsave(&mdev->clock_lock, flags); >> - timecounter_init(&mdev->clock, &mdev->cycles, >> - ktime_to_ns(ktime_get_real())); >> + timecounter_init(&mdev->clock, ktime_to_ns(ktime_get_real())); > I'd like to see two followup patches to this one: > > 1. Convert timecounter_init() callers to a new timecounter_reset() > function where the intent is to reset the time. > > 2. Change timecounter_init() to take the cyclecounter fields as > arguments. > > void timecounter_init(struct timecounter *tc, > u64 (*read)(const struct cyclecounter *cc), > u64 mask, > u32 mult, > u32 shift, > u64 start_tstamp); > > Then we can clean up all this stuff: > > mdev->clock.cc.read = mlx4_en_read_clock; > mdev->clock.cc.mask = CLOCKSOURCE_MASK(48); > mdev->clock.cc.shift = freq_to_shift(dev->caps.hca_core_clock); > mdev->clock.cc.mult = clocksource_khz2mult(...); > > This second step can be phased in by calling the new function > timecounter_initialize() and converting the drivers one by one. Yes. Will make these changes and share new patchset. Thank you for the review Richard. Regards, Sagar >> diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h >> index 2496ad4..6daca06 100644 >> --- a/include/linux/timecounter.h >> +++ b/include/linux/timecounter.h > ... >> @@ -98,7 +98,6 @@ static inline void timecounter_adjtime(struct timecounter *tc, s64 delta) >> /** >> * timecounter_init - initialize a time counter >> * @tc: Pointer to time counter which is to be initialized/reset >> - * @cc: A cycle counter, ready to be used. > This "ready to used" requirement should go. The init() function > should make the instance ready to be used all at once. > > Thanks, > Richard