Time Counter fixes and improvements
Subsystems (11)
cover Time Counter fixes and improvements
Several PTP Hardware Clock (PHC) drivers implement the clock in software using the timecounter/cyclecounter code. This series adds one simple improvement and one more subtle fix to the shared timecounter facility. Credit for this series goes to Janusz Użycki, who pointed the issues out to me off list.
Patch #1 simply move the timecounter code into its own file. When working on this series, it was really annoying to see half the kernel recompile after every tweak to the timecounter stuff. There is no reason to keep this together with the clocksource code.
Patch #2 implements an improved adjtime() method, and patches 3-10 convert all of the drivers over to the new method.
Patch #11 fixes a subtle but important issue with the timecounter WRT frequency adjustment. As it stands now, a timecounter based PHC will exhibit a variable frequency resolution (and variable time error) depending on how often the clock is read.
In timecounter_read_delta(), the expression
(delta * cc->mult) >> cc->shift;
can lose resolution from the adjusted value of 'mult'. If the value of 'delta' is too small, then small changes in 'mult' have no effect. However, if the delta value is large enough, then small changes in 'mult' will have an effect.
Reading the clock too often means smaller 'delta' values which in turn will spoil the fine adjustments made to 'mult'. Up until now, this effect did not show up in my testing. The following example explains why.
The CPTS has an input clock of 250 MHz, and the clock source uses mult=0x80000000 and shift=29, making the ticks to nanoseconds conversion like this:
ticks * 2^31
------------
2^29
Imagine what happens if the clock is read every 10 milliseconds. Ten milliseconds are about 2500000 ticks, which corresponds to about 21 bits. The product in the numerator has then 52 bits. After the shift operation, 23 bits are preserved. This results in a frequency adjustment resolution of about 0.1 ppm (not _too_ bad.)
A frequency resolution of 1 ppm requires 20 bits.
A frequency resolution of 1 ppb requires 30 bits.
For the 250 MHz CPTS clock, reading every 4 seconds yields a 1 ppb resolution (which is the finest that our API allows).
However, the error can be much higher if the clock is read too often or if time stamps occur close in time to read operations. In general it is really not acceptable to allow the rate of clock readings to influence the clock accuracy.
Thanks,
Richard
Richard Cochran (11):
time: move the timecounter/cyclecounter code into its own file.
timecounter: provide a helper function to shift the time.
net: xgbe: convert to timecounter adjtime.
net: bnx2x: convert to timecounter adjtime.
net: fec: convert to timecounter adjtime.
net: e1000e: convert to timecounter adjtime.
net: igb: convert to timecounter adjtime.
net: ixgbe: convert to timecounter adjtime.
net: mlx4: convert to timecounter adjtime.
net: cpts: convert to timecounter adjtime.
timecounter: keep track of accumulated fractional nanoseconds
drivers/net/ethernet/amd/xgbe/xgbe-ptp.c | 8 +- drivers/net/ethernet/amd/xgbe/xgbe.h | 2 +- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 +- drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 6 +- drivers/net/ethernet/freescale/fec.h | 1 + drivers/net/ethernet/freescale/fec_ptp.c | 16 +-- drivers/net/ethernet/intel/e1000e/e1000.h | 2 +- drivers/net/ethernet/intel/e1000e/ptp.c | 5 +- drivers/net/ethernet/intel/igb/igb.h | 2 +- drivers/net/ethernet/intel/igb/igb_ptp.c | 7 +- drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 +- drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 11 +- drivers/net/ethernet/mellanox/mlx4/en_clock.c | 9 +- drivers/net/ethernet/ti/cpts.c | 5 +- drivers/net/ethernet/ti/cpts.h | 1 + include/clocksource/arm_arch_timer.h | 2 +- include/linux/clocksource.h | 102 ---------------- include/linux/mlx4/device.h | 2 +- include/linux/timecounter.h | 136 ++++++++++++++++++++++ include/linux/types.h | 3 + kernel/time/Makefile | 2 +- kernel/time/clocksource.c | 76 ------------ kernel/time/timecounter.c | 112 ++++++++++++++++++ sound/pci/hda/hda_priv.h | 2 +- virt/kvm/arm/arch_timer.c | 3 +- 25 files changed, 274 insertions(+), 245 deletions(-) create mode 100644 include/linux/timecounter.h create mode 100644 kernel/time/timecounter.c
Signature · 1 line
1.7.10.4
PATCH 2/11 timecounter: provide a helper function to shift the time.
Some PTP Hardware Clock drivers use a struct timecounter to represent their clock. To adjust the time by a given offset, these drivers all perform a two step read/write of their timecounter. However, it is better and simpler just to adjust the offset in one step. This patch introduces a little routine to help drivers implement the adjtime method.
--- include/linux/timecounter.h | 9 +++++++++ 1 file changed, 9 insertions(+)
include/linux/timecounter.h +9 -0
@@ -79,6 +79,15 @@ static inline u64 cyclecounter_cyc2ns(const struct cyclecounter *cc, }/** +* timecounter_adjtime - Shifts the time of the clock. +* @delta: Desired change in nanoseconds. +*/ +static inline void timecounter_adjtime (struct timecounter *tc,s64 delta) +{ + tc->nsec += 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.
Signature · 1 line
1.7.10.4
PATCH 4/11 net: bnx2x: convert to timecounter adjtime.
In reply to Richard CochranThis patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
Compile tested only.
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +1 -5
@@ -13265,14 +13265,10 @@ static int bnx2x_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)static int bnx2x_ptp_adjtime (struct ptp_clock_info *ptp,s64 delta) { struct bnx2x *bp = container_of(ptp, struct bnx2x, ptp_clock_info); -u64 now;DP (BNX2X_MSG_PTP ,"PTP adjtime called, delta = %llx\n" , delta); - now =timecounter_read (&bp->timecounter); - now += delta; -/* Re-init the timecounter */ -timecounter_init (&bp->timecounter, &bp->cyclecounter, now); +timecounter_adjtime (&bp->timecounter, delta);return 0 ; }
Signature · 1 line
1.7.10.4
PATCH 5/11 net: fec: convert to timecounter adjtime.
In reply to Richard CochranThis patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
Compile tested only.
--- drivers/net/ethernet/freescale/fec_ptp.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)
drivers/net/ethernet/freescale/fec_ptp.c +1 -15
@@ -374,23 +374,9 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)struct fec_enet_private *fep = container_of(ptp, struct fec_enet_private, ptp_caps);unsigned long flags; -u64 now; -u32 counter;spin_lock_irqsave (&fep->tmreg_lock, flags); - - now =timecounter_read (&fep->tc); - now += delta; - -/* Get the timer value based on adjusted timestamp. -* Update the counter with the masked value. -*/ - counter = now & fep->cc.mask; -writel (counter, fep->hwp +FEC_ATIME ); - -/* reset the timecounter */ -timecounter_init (&fep->tc, &fep->cc, now); - +timecounter_adjtime (&fep->tc, delta);spin_unlock_irqrestore (&fep->tmreg_lock, flags);return 0 ;
Signature · 1 line
1.7.10.4
PATCH 6/11 net: e1000e: convert to timecounter adjtime.
In reply to Richard CochranThis patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
--- drivers/net/ethernet/intel/e1000e/ptp.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/net/ethernet/intel/e1000e/ptp.c +1 -4
@@ -90,12 +90,9 @@ static int e1000e_phc_adjtime(struct ptp_clock_info *ptp, s64 delta) struct e1000_adapter *adapter = container_of(ptp, struct e1000_adapter, ptp_clock_info);unsigned long flags; -s64 now;spin_lock_irqsave (&adapter->systim_lock, flags); - now =timecounter_read (&adapter->tc); - now += delta; -timecounter_init (&adapter->tc, &adapter->cc, now); +timecounter_adjtime (&adapter->tc, delta);spin_unlock_irqrestore (&adapter->systim_lock, flags);return 0 ;
Signature · 1 line
1.7.10.4
PATCH 7/11 net: igb: convert to timecounter adjtime.
In reply to Richard CochranThis patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
--- drivers/net/ethernet/intel/igb/igb_ptp.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/net/ethernet/intel/igb/igb_ptp.c +1 -6
@@ -256,14 +256,9 @@ static int igb_ptp_adjtime_82576(struct ptp_clock_info *ptp, s64 delta) struct igb_adapter *igb = container_of(ptp, struct igb_adapter, ptp_caps);unsigned long flags; -s64 now;spin_lock_irqsave (&igb->tmreg_lock, flags); - - now =timecounter_read (&igb->tc); - now += delta; -timecounter_init (&igb->tc, &igb->cc, now); - +timecounter_adjtime (&igb->tc, delta);spin_unlock_irqrestore (&igb->tmreg_lock, flags);return 0 ;
Signature · 1 line
1.7.10.4
PATCH 8/11 net: ixgbe: convert to timecounter adjtime.
In reply to Richard CochranThis patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
Compile tested only.
--- drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c +1 -10
@@ -261,18 +261,9 @@ static int ixgbe_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)struct ixgbe_adapter *adapter = container_of(ptp, struct ixgbe_adapter, ptp_caps);unsigned long flags; -u64 now;spin_lock_irqsave (&adapter->tmreg_lock, flags); - - now =timecounter_read (&adapter->tc); - now += delta; - -/* reset the timecounter */ -timecounter_init (&adapter->tc, - &adapter->cc, - now); - +timecounter_adjtime (&adapter->tc, delta);spin_unlock_irqrestore (&adapter->tmreg_lock, flags);ixgbe_ptp_setup_sdp (adapter);
Signature · 1 line
1.7.10.4
PATCH 9/11 net: mlx4: convert to timecounter adjtime.
In reply to Richard CochranThis patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
Compile tested only.
--- drivers/net/ethernet/mellanox/mlx4/en_clock.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlx4/en_clock.c +1 -4
@@ -147,12 +147,9 @@ static int mlx4_en_phc_adjtime(struct ptp_clock_info *ptp, s64 delta) struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev, ptp_clock_info);unsigned long flags; -s64 now;write_lock_irqsave (&mdev->clock_lock, flags); - now =timecounter_read (&mdev->clock); - now += delta; -timecounter_init (&mdev->clock, &mdev->cycles, now); +timecounter_adjtime (&mdev->clock, delta);write_unlock_irqrestore (&mdev->clock_lock, flags);return 0 ;
Signature · 1 line
1.7.10.4
PATCH 10/11 net: cpts: convert to timecounter adjtime.
In reply to Richard CochranThis patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
--- drivers/net/ethernet/ti/cpts.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/net/ethernet/ti/cpts.c +1 -4
@@ -157,14 +157,11 @@ static int cpts_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)static int cpts_ptp_adjtime (struct ptp_clock_info *ptp,s64 delta) { -s64 now;unsigned long flags; struct cpts *cpts = container_of(ptp, struct cpts, info);spin_lock_irqsave (&cpts->lock, flags); - now =timecounter_read (&cpts->tc); - now += delta; -timecounter_init (&cpts->tc, &cpts->cc, now); +timecounter_adjtime (&cpts->tc, delta);spin_unlock_irqrestore (&cpts->lock, flags);return 0 ;
Signature · 1 line
1.7.10.4
PATCH 1/11 time: move the timecounter/cyclecounter code into its own file.
In reply to Richard CochranThe timecounter code has almost nothing to do with the clocksource code. Let it live in its own file. This will help isolate the timecounter users from the clocksource users in the source tree.
--- drivers/net/ethernet/amd/xgbe/xgbe.h | 2 +- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 +- drivers/net/ethernet/freescale/fec.h | 1 + drivers/net/ethernet/intel/e1000e/e1000.h | 2 +- drivers/net/ethernet/intel/igb/igb.h | 2 +- drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 +- drivers/net/ethernet/ti/cpts.h | 1 + include/clocksource/arm_arch_timer.h | 2 +- include/linux/clocksource.h | 102 ---------------------- include/linux/mlx4/device.h | 2 +- include/linux/timecounter.h | 122 +++++++++++++++++++++++++++ include/linux/types.h | 3 + kernel/time/Makefile | 2 +- kernel/time/clocksource.c | 76 ----------------- kernel/time/timecounter.c | 95 +++++++++++++++++++++ sound/pci/hda/hda_priv.h | 2 +- 16 files changed, 231 insertions(+), 187 deletions(-) create mode 100644 include/linux/timecounter.h create mode 100644 kernel/time/timecounter.c
drivers/net/ethernet/amd/xgbe/xgbe.h +1 -1
@@ -124,7 +124,7 @@#include <linux/if_vlan.h> #include <linux/bitops.h> #include <linux/ptp_clock_kernel.h> -#include <linux/clocksource.h> +#include <linux/timecounter.h> #include <linux/net_tstamp.h> #include <net/dcbnl.h>
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +1 -1
@@ -22,7 +22,7 @@#include <linux/ptp_clock_kernel.h> #include <linux/net_tstamp.h> -#include <linux/clocksource.h> +#include <linux/timecounter.h> /* compilation time flags */
drivers/net/ethernet/freescale/fec.h +1 -0
@@ -16,6 +16,7 @@#include <linux/clocksource.h> #include <linux/net_tstamp.h> #include <linux/ptp_clock_kernel.h> +#include <linux/timecounter.h> #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \ defined(CONFIG_M520x) || defined(CONFIG_M532x) || \
drivers/net/ethernet/intel/e1000e/e1000.h +1 -1
@@ -34,7 +34,7 @@#include <linux/pci-aspm.h> #include <linux/crc32.h> #include <linux/if_vlan.h> -#include <linux/clocksource.h> +#include <linux/timecounter.h> #include <linux/net_tstamp.h> #include <linux/ptp_clock_kernel.h> #include <linux/ptp_classify.h>
drivers/net/ethernet/intel/igb/igb.h +1 -1
@@ -29,7 +29,7 @@#include "e1000_mac.h" #include "e1000_82575.h" -#include <linux/clocksource.h> +#include <linux/timecounter.h> #include <linux/net_tstamp.h> #include <linux/ptp_clock_kernel.h> #include <linux/bitops.h>
drivers/net/ethernet/intel/ixgbe/ixgbe.h +1 -1
@@ -38,7 +38,7 @@#include <linux/if_vlan.h> #include <linux/jiffies.h> -#include <linux/clocksource.h> +#include <linux/timecounter.h> #include <linux/net_tstamp.h> #include <linux/ptp_clock_kernel.h>
drivers/net/ethernet/ti/cpts.h +1 -0
@@ -27,6 +27,7 @@#include <linux/list.h> #include <linux/ptp_clock_kernel.h> #include <linux/skbuff.h> +#include <linux/timecounter.h> struct cpsw_cpts {u32 idver;/* Identification and version */
include/clocksource/arm_arch_timer.h +1 -1
@@ -16,7 +16,7 @@#ifndef __CLKSOURCE_ARM_ARCH_TIMER_H#define __CLKSOURCE_ARM_ARCH_TIMER_H -#include <linux/clocksource.h> +#include <linux/timecounter.h> #include <linux/types.h> #define ARCH_TIMER_CTRL_ENABLE (1 << 0)
include/linux/clocksource.h +0 -102
114 lines not on this page · raw message
include/linux/mlx4/device.h +1 -1
@@ -42,7 +42,7 @@#include <linux/atomic.h> -#include <linux/clocksource.h> +#include <linux/timecounter.h> #define MAX_MSIX_P_PORT 17#define MAX_MSIX 64
include/linux/timecounter.h new file +122 -0
122 lines not on this page · raw message
include/linux/types.h +3 -0
@@ -213,5 +213,8 @@ struct callback_head { };#define rcu_head callback_head +/* clocksource cycle base type */ +typedef u64 cycle_t ; +#endif /* __ASSEMBLY__ */ #endif /* _LINUX_TYPES_H */
kernel/time/Makefile +1 -1
@@ -1,6 +1,6 @@ obj-y += time.o timer.o hrtimer.o itimer.o posix-timers.o posix-cpu-timers.o obj-y += timekeeping.o ntp.o clocksource.o jiffies.o timer_list.o -obj-y += timeconv.o posix-clock.o alarmtimer.o +obj-y += timeconv.o timecounter.o posix-clock.o alarmtimer.o obj-$(CONFIG_GENERIC_CLOCKEVENTS_BUILD) += clockevents.o obj-$(CONFIG_GENERIC_CLOCKEVENTS) += tick-common.o
kernel/time/clocksource.c +0 -76
82 lines not on this page · raw message
kernel/time/timecounter.c new file +95 -0
95 lines not on this page · raw message
sound/pci/hda/hda_priv.h +1 -1
@@ -15,7 +15,7 @@#ifndef __SOUND_HDA_PRIV_H#define __SOUND_HDA_PRIV_H -#include <linux/clocksource.h> +#include <linux/timecounter.h> #include <sound/core.h> #include <sound/pcm.h>
Signature · 1 line
1.7.10.4
PATCH 3/11 net: xgbe: convert to timecounter adjtime.
In reply to Richard CochranThis patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
Compile tested only.
--- drivers/net/ethernet/amd/xgbe/xgbe-ptp.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
drivers/net/ethernet/amd/xgbe/xgbe-ptp.c +1 -7
@@ -171,15 +171,9 @@ static int xgbe_adjtime(struct ptp_clock_info *info, s64 delta) struct xgbe_prv_data, ptp_clock_info);unsigned long flags; -u64 nsec;spin_lock_irqsave (&pdata->tstamp_lock, flags); - - nsec =timecounter_read (&pdata->tstamp_tc); - - nsec += delta; -timecounter_init (&pdata->tstamp_tc, &pdata->tstamp_cc, nsec); - +timecounter_adjtime (&pdata->tstamp_tc, delta);spin_unlock_irqrestore (&pdata->tstamp_lock, flags);return 0 ;
Signature · 1 line
1.7.10.4
PATCH 11/11 timecounter: keep track of accumulated fractional nanoseconds
In reply to Richard CochranThe current timecounter implementation will drop a variable amount of resolution, depending on the magnitude of the time delta. In other words, reading the clock too often or too close to a time stamp conversion will introduce errors into the time values. This patch fixes the issue by introducing a fractional nanosecond field that accumulates the low order bits.
--- drivers/net/ethernet/mellanox/mlx4/en_clock.c | 4 ++-- include/linux/timecounter.h | 19 +++++++++------ kernel/time/timecounter.c | 31 +++++++++++++++++++------ virt/kvm/arm/arch_timer.c | 3 ++- 4 files changed, 40 insertions(+), 17 deletions(-)
drivers/net/ethernet/mellanox/mlx4/en_clock.c +2 -2
@@ -240,7 +240,7 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev) {struct mlx4_dev *dev = mdev->dev;unsigned long flags; -u64 ns; +u64 ns, zero =0 ;rwlock_init (&mdev->clock_lock);
@@ -265,7 +265,7 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)/* Calculate period in seconds to call the overflow watchdog - to make * sure counter is checked at least once every wrap around. */ - ns =cyclecounter_cyc2ns (&mdev->cycles, mdev->cycles.mask); + ns =cyclecounter_cyc2ns (&mdev->cycles, mdev->cycles.mask, zero, &zero);do_div (ns,NSEC_PER_SEC /2 /HZ ); mdev->overflow_period = ns;
include/linux/timecounter.h +12 -7
@@ -55,27 +55,32 @@ struct cyclecounter {* @cycle_last: most recent cycle counter value seen by * timecounter_read() * @nsec: continuously increasing count +* @mask: bit mask for maintaining the 'frac' field +* @frac: accumulated fractional nanoseconds */ struct timecounter {const struct cyclecounter *cc;cycle_t cycle_last;u64 nsec; +u64 mask; +u64 frac; };/** * cyclecounter_cyc2ns - converts cycle counter cycles to nanoseconds * @cc: Pointer to cycle counter. * @cycles: Cycles -* -* XXX - This could use some mult_lxl_ll() asm optimization. Same code -* as in cyc2ns, but with unsigned result. +* @mask: bit mask for maintaining the 'frac' field +* @frac: pointer to storage for the fractional nanoseconds. */ static inline u64 cyclecounter_cyc2ns (const struct cyclecounter *cc, -cycle_t cycles) +cycle_t cycles,u64 mask,u64 *frac) { -u64 ret = (u64 )cycles; - ret = (ret * cc->mult) >> cc->shift; -return ret; +u64 ns = (u64 ) cycles; + + ns = (ns * cc->mult) + *frac; + *frac = ns & mask; +return ns >> cc->shift; }/**
kernel/time/timecounter.c +24 -7
@@ -25,6 +25,8 @@ void timecounter_init(struct timecounter *tc, tc->cc = cc; tc->cycle_last = cc->read (cc); tc->nsec = start_tstamp; + tc->mask = (1ULL << cc->shift) -1 ; + tc->frac =0 ; }EXPORT_SYMBOL_GPL (timecounter_init);
@@ -51,7 +53,8 @@ static u64 timecounter_read_delta(struct timecounter *tc) cycle_delta = (cycle_now - tc->cycle_last) & tc->cc->mask;/* convert to nanoseconds: */ - ns_offset =cyclecounter_cyc2ns (tc->cc, cycle_delta); + ns_offset =cyclecounter_cyc2ns (tc->cc, cycle_delta, + tc->mask, &tc->frac);/* update time stamp of timecounter_read_delta() call: */ tc->cycle_last = cycle_now;
@@ -72,22 +75,36 @@ u64 timecounter_read(struct timecounter *tc) }EXPORT_SYMBOL_GPL (timecounter_read); +/* +* This is like cyclecounter_cyc2ns(), but it is used for computing a +* time previous to the time stored in the cycle counter. +*/ +static u64 cc_cyc2ns_backwards (const struct cyclecounter *cc, +cycle_t cycles,u64 mask,u64 frac) +{ +u64 ns = (u64 ) cycles; + + ns = ((ns * cc->mult) - frac) >> cc->shift; + +return ns; +} +u64 timecounter_cyc2time (struct timecounter *tc,cycle_t cycle_tstamp) { -u64 cycle_delta = (cycle_tstamp - tc->cycle_last) & tc->cc->mask; -u64 nsec; +u64 delta = (cycle_tstamp - tc->cycle_last) & tc->cc->mask; +u64 nsec = tc->nsec, frac = tc->frac;/* * Instead of always treating cycle_tstamp as more recent * than tc->cycle_last, detect when it is too far in the * future and treat it as old time stamp instead. */ -if (cycle_delta > tc->cc->mask /2 ) { - cycle_delta = (tc->cycle_last - cycle_tstamp) & tc->cc->mask; - nsec = tc->nsec -cyclecounter_cyc2ns (tc->cc, cycle_delta); +if (delta > tc->cc->mask /2 ) { + delta = (tc->cycle_last - cycle_tstamp) & tc->cc->mask; + nsec -=cc_cyc2ns_backwards (tc->cc, delta, tc->mask, frac); }else { - nsec =cyclecounter_cyc2ns (tc->cc, cycle_delta) + tc->nsec; + nsec +=cyclecounter_cyc2ns (tc->cc, delta, tc->mask, &frac); }return nsec;
virt/kvm/arm/arch_timer.c +2 -1
@@ -150,7 +150,8 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)return ; } - ns =cyclecounter_cyc2ns (timecounter->cc, cval - now); + ns =cyclecounter_cyc2ns (timecounter->cc, cval - now, timecounter->mask, + &timecounter->frac);timer_arm (timer, ns); }
Signature · 1 line
1.7.10.4
Re: [PATCH net-next 04/11] net: bnx2x: convert to timecounter adjtime.
In reply to Richard CochranQuoted 2 lines · Richard Cochran
This patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
BTW, I noticed that this driver does not serialize access to the timecounter in any way, but probably it should. Ariel?
Thanks,
Richard
Re: [PATCH net-next 01/11] time: move the timecounter/cyclecounter code into its own file.
In reply to Richard CochranQuoted 5 lines · Richard Cochran
The timecounter code has almost nothing to do with the clocksource code. Let it live in its own file. This will help isolate the timecounter users from the clocksource users in the source tree.
Signed-off-by Richard Cochran <[email protected]>
For the Intel driver changes...
Quoted 23 lines
--- drivers/net/ethernet/amd/xgbe/xgbe.h | 2 +- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 +- drivers/net/ethernet/freescale/fec.h | 1 + drivers/net/ethernet/intel/e1000e/e1000.h | 2 +- drivers/net/ethernet/intel/igb/igb.h | 2 +- drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 +- drivers/net/ethernet/ti/cpts.h | 1 + include/clocksource/arm_arch_timer.h | 2 +- include/linux/clocksource.h | 102 ---------------------- include/linux/mlx4/device.h | 2 +- include/linux/timecounter.h | 122 +++++++++++++++++++++++++++ include/linux/types.h | 3 + kernel/time/Makefile | 2 +- kernel/time/clocksource.c | 76 ----------------- kernel/time/timecounter.c | 95 +++++++++++++++++++++ sound/pci/hda/hda_priv.h | 2 +- 16 files changed, 231 insertions(+), 187 deletions(-) create mode 100644 include/linux/timecounter.h create mode 100644 kernel/time/timecounter.c
Re: [PATCH net-next 06/11] net: e1000e: convert to timecounter adjtime.
In reply to Richard CochranQuoted 4 lines · Richard Cochran
This patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
Signed-off-by Richard Cochran <[email protected]>
Quoted 3 lines
--- drivers/net/ethernet/intel/e1000e/ptp.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
Re: [PATCH net-next 07/11] net: igb: convert to timecounter adjtime.
In reply to Richard CochranQuoted 4 lines · Richard Cochran
This patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
Signed-off-by Richard Cochran <[email protected]>
Quoted 3 lines
--- drivers/net/ethernet/intel/igb/igb_ptp.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
Re: [PATCH net-next 08/11] net: ixgbe: convert to timecounter adjtime.
In reply to Richard CochranQuoted 6 lines · Richard Cochran
This patch changes the driver to use the new and improved method for adjusting the offset of a timecounter.
Compile tested only.
Signed-off-by Richard Cochran <[email protected]>
Just for sanity sake, I will have Phillip test the changes, but there is no need to hold this up from being committed in the mean time.
Quoted 3 lines
--- drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
Re: [PATCH net-next 08/11] net: ixgbe: convert to timecounter adjtime.
Quoted 2 lines · Jeff Kirsher
Just for sanity sake, I will have Phillip test the changes, but there is no need to hold this up from being committed in the mean time.
FYI, here is a test that may show the improved behavior when comparing before/after.
1. Start ptp4l, let it run long enough to find the frequency offset.
2. Stop ptp4l with ^C.
3. Wait a bit, to let the clock drift away.
4. Restart ptp4l, it will now start with the frequency offset learned in step 1.
5. Notice the the offsets in states s0 -> s1 -> s2. There will likely be less offset error with the patches applied.
Thanks,
Richard
David Miller
maintainerFrom: Richard Cochran <[email protected]>
Date: Sun, 21 Dec 2014 19:46:55 +0100
Quoted 5 lines
Several PTP Hardware Clock (PHC) drivers implement the clock in software using the timecounter/cyclecounter code. This series adds one simple improvement and one more subtle fix to the shared timecounter facility. Credit for this series goes to Janusz Użycki, who pointed the issues out to me off list.
Series applied, thanks Richard.
Quoted 2 lines · David Miller
Series applied, thanks Richard.
I got an automated email from
kbuild test robot <[email protected]>
showing new warnings and errors introduced by this series. I'll follow up with fixes soon.
Thanks,
Richard
Quoted 10 lines · Richard Cochran
Several PTP Hardware Clock (PHC) drivers implement the clock in software using the timecounter/cyclecounter code. This series adds one simple improvement and one more subtle fix to the shared timecounter facility. Credit for this series goes to Janusz Użycki, who pointed the issues out to me off list.
Patch #1 simply move the timecounter code into its own file. When working on this series, it was really annoying to see half the kernel recompile after every tweak to the timecounter stuff. There is no reason to keep this together with the clocksource code.
I did have some faint hope we could merge the cyclecounter and clocksource struct at some point, and while we've gotten closer with much of the time-specific values being kept in the timekeeper, the full cleanup hasn't happened in a few years here, so its clearly not something I've prioritized.
So no objection in concept.
Quoted 39 lines
Patch #2 implements an improved
adjtime()method, and patches 3-10 convert all of the drivers over to the new method.Patch #11 fixes a subtle but important issue with the timecounter WRT frequency adjustment. As it stands now, a timecounter based PHC will exhibit a variable frequency resolution (and variable time error) depending on how often the clock is read.
In
timecounter_read_delta(), the expression(delta * cc->mult) >> cc->shift;can lose resolution from the adjusted value of 'mult'. If the value of 'delta' is too small, then small changes in 'mult' have no effect. However, if the delta value is large enough, then small changes in 'mult' will have an effect.
Reading the clock too often means smaller 'delta' values which in turn will spoil the fine adjustments made to 'mult'. Up until now, this effect did not show up in my testing. The following example explains why.
The CPTS has an input clock of 250 MHz, and the clock source uses mult=0x80000000 and shift=29, making the ticks to nanoseconds conversion like this:
ticks * 2^31 ------------ 2^29Imagine what happens if the clock is read every 10 milliseconds. Ten milliseconds are about 2500000 ticks, which corresponds to about 21 bits. The product in the numerator has then 52 bits. After the shift operation, 23 bits are preserved. This results in a frequency adjustment resolution of about 0.1 ppm (not _too_ bad.)
A frequency resolution of 1 ppm requires 20 bits.
A frequency resolution of 1 ppb requires 30 bits.
Quoted 7 lines
For the 250 MHz CPTS clock, reading every 4 seconds yields a 1 ppb resolution (which is the finest that our API allows).
However, the error can be much higher if the clock is read too often or if time stamps occur close in time to read operations. In general it is really not acceptable to allow the rate of clock readings to influence the clock accuracy.
Though, this does start to sound like issues the timekeeping code had to resolve, so while its probably time to let some flowers bloom and see what happens, we should be somewhat watchful for too much logic duplication.
The patch set itself seems fairly straight forward (at least to my back from vacation brain), so..
thanks
-john
Quoted 4 lines · John Stultz
Though, this does start to sound like issues the timekeeping code had to resolve, so while its probably time to let some flowers bloom and see what happens, we should be somewhat watchful for too much logic duplication.
You took the words right out of my mouth.
My feeling is that this timecounter code might become more complicated, and if the core time keeping becomes more straightforward, then maybe one day the two will join.
Thanks,
Richard