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