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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
