From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Cochran Subject: Re: [PATCH 1/1] timecounter: Make cyclecounter struct part of timecounter struct Date: Sat, 2 Dec 2017 10:04:58 -0800 Message-ID: <20171202180458.winwznbstsi355ed@localhost> References: <1512189095-28654-1-git-send-email-sagar.a.kamble@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pg0-f67.google.com (mail-pg0-f67.google.com [74.125.83.67]) by alsa0.perex.cz (Postfix) with ESMTP id 35DB12671E8 for ; Sat, 2 Dec 2017 19:05:03 +0100 (CET) Received: by mail-pg0-f67.google.com with SMTP id f12so5844753pgo.5 for ; Sat, 02 Dec 2017 10:05:03 -0800 (PST) Content-Disposition: inline In-Reply-To: <1512189095-28654-1-git-send-email-sagar.a.kamble@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Sagar Arun Kamble Cc: alsa-devel@alsa-project.org, linux-rdma@vger.kernel.org, netdev@vger.kernel.org, Stephen Boyd , linux-kernel@vger.kernel.org, Chris Wilson , John Stultz , intel-wired-lan@lists.osuosl.org, Thomas Gleixner , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org List-Id: alsa-devel@alsa-project.org 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. > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Cochran Date: Sat, 2 Dec 2017 10:04:58 -0800 Subject: [Intel-wired-lan] [PATCH 1/1] timecounter: Make cyclecounter struct part of timecounter struct In-Reply-To: <1512189095-28654-1-git-send-email-sagar.a.kamble@intel.com> References: <1512189095-28654-1-git-send-email-sagar.a.kamble@intel.com> Message-ID: <20171202180458.winwznbstsi355ed@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: 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. > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: richardcochran@gmail.com (Richard Cochran) Date: Sat, 2 Dec 2017 10:04:58 -0800 Subject: [PATCH 1/1] timecounter: Make cyclecounter struct part of timecounter struct In-Reply-To: <1512189095-28654-1-git-send-email-sagar.a.kamble@intel.com> References: <1512189095-28654-1-git-send-email-sagar.a.kamble@intel.com> Message-ID: <20171202180458.winwznbstsi355ed@localhost> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752308AbdLBSFG (ORCPT ); Sat, 2 Dec 2017 13:05:06 -0500 Received: from mail-pg0-f66.google.com ([74.125.83.66]:38958 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752214AbdLBSFC (ORCPT ); Sat, 2 Dec 2017 13:05:02 -0500 X-Google-Smtp-Source: AGs4zMaFChjk3CMb67YQ5j00K9dKX5dkuO/asamGbnsNNjS/YdKOCPpV5Pb7psy2wkhGjVSjTduO+Q== Date: Sat, 2 Dec 2017 10:04:58 -0800 From: Richard Cochran To: Sagar Arun Kamble 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 Subject: Re: [PATCH 1/1] timecounter: Make cyclecounter struct part of timecounter struct Message-ID: <20171202180458.winwznbstsi355ed@localhost> References: <1512189095-28654-1-git-send-email-sagar.a.kamble@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1512189095-28654-1-git-send-email-sagar.a.kamble@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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