| Message ID | 20210615094517.48752-2-yangbo.lu@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ptp: support virtual clocks and timestamping | expand |
On Tue, Jun 15, 2021 at 05:45:08PM +0800, Yangbo Lu wrote: > diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile > index 8673d1743faa..3c6a905760e2 100644 > --- a/drivers/ptp/Makefile > +++ b/drivers/ptp/Makefile > @@ -3,7 +3,7 @@ > # Makefile for PTP 1588 clock support. > # > > -ptp-y := ptp_clock.o ptp_chardev.o ptp_sysfs.o > +ptp-y := ptp_clock.o ptp_vclock.o ptp_chardev.o ptp_sysfs.o Nit: Please place ptp_vclock.o at the end of the list. > ptp_kvm-$(CONFIG_X86) := ptp_kvm_x86.o ptp_kvm_common.o > ptp_kvm-$(CONFIG_HAVE_ARM_SMCCC) := ptp_kvm_arm.o ptp_kvm_common.o > obj-$(CONFIG_PTP_1588_CLOCK) += ptp.o > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h > index 6b97155148f1..3f388d63904c 100644 > --- a/drivers/ptp/ptp_private.h > +++ b/drivers/ptp/ptp_private.h > @@ -48,6 +48,20 @@ struct ptp_clock { > struct kthread_delayed_work aux_work; > }; > > +#define info_to_vclock(d) container_of((d), struct ptp_vclock, info) > +#define cc_to_vclock(d) container_of((d), struct ptp_vclock, cc) > +#define dw_to_vclock(d) container_of((d), struct ptp_vclock, refresh_work) > + > +struct ptp_vclock { > + struct ptp_clock *pclock; > + struct ptp_clock_info info; > + struct ptp_clock *clock; > + struct cyclecounter cc; > + struct timecounter tc; > + spinlock_t lock; /* protects tc/cc */ > + struct delayed_work refresh_work; Can we please have a kthread worker here instead of work? Experience shows that plain work can be delayed for a long, long time on busy systems. > +}; > + > /* > * The function queue_cnt() is safe for readers to call without > * holding q->lock. Readers use this function to verify that the queue > @@ -89,4 +103,6 @@ extern const struct attribute_group *ptp_groups[]; > int ptp_populate_pin_groups(struct ptp_clock *ptp); > void ptp_cleanup_pin_groups(struct ptp_clock *ptp); > > +struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock); > +void ptp_vclock_unregister(struct ptp_vclock *vclock); > #endif > diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c > new file mode 100644 > index 000000000000..b8f500677314 > --- /dev/null > +++ b/drivers/ptp/ptp_vclock.c > @@ -0,0 +1,154 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * PTP virtual clock driver > + * > + * Copyright 2021 NXP > + */ > +#include <linux/slab.h> > +#include "ptp_private.h" > + > +#define PTP_VCLOCK_CC_MULT (1 << 31) > +#define PTP_VCLOCK_CC_SHIFT 31 These two are okay, but ... > +#define PTP_VCLOCK_CC_MULT_NUM (1 << 9) > +#define PTP_VCLOCK_CC_MULT_DEM 15625ULL the similarity of naming is confusing for these two. They are only used in the .adjfine method. How about this? PTP_VCLOCK_FADJ_NUMERATOR, or even PTP_VCLOCK_FADJ_SHIFT (see below) PTP_VCLOCK_FADJ_DENOMINATOR > +#define PTP_VCLOCK_CC_REFRESH_INTERVAL (HZ * 2) Consider dropping CC from the name. PTP_VCLOCK_REFRESH_INTERVAL sounds good to me. > +static int ptp_vclock_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) > +{ > + struct ptp_vclock *vclock = info_to_vclock(ptp); > + unsigned long flags; > + s64 adj; > + > + adj = (s64)scaled_ppm * PTP_VCLOCK_CC_MULT_NUM; Rather than scaled_ppm * (1 << 9) I suggest scaled_ppm << 9 instead. I suppose a good compiler would replace the multiplication with a bit shift, but it never hurts to spell it out. > + adj = div_s64(adj, PTP_VCLOCK_CC_MULT_DEM); > + > + spin_lock_irqsave(&vclock->lock, flags); > + timecounter_read(&vclock->tc); > + vclock->cc.mult = PTP_VCLOCK_CC_MULT + adj; > + spin_unlock_irqrestore(&vclock->lock, flags); > + > + return 0; > +} Thanks, Richard
Hi Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: 2021年6月18日 1:33 > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-kselftest@vger.kernel.org; mptcp@lists.linux.dev; David S . Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Mat Martineau > <mathew.j.martineau@linux.intel.com>; Matthieu Baerts > <matthieu.baerts@tessares.net>; Shuah Khan <shuah@kernel.org>; Michal > Kubecek <mkubecek@suse.cz>; Florian Fainelli <f.fainelli@gmail.com>; > Andrew Lunn <andrew@lunn.ch>; Rui Sousa <rui.sousa@nxp.com>; Sebastien > Laveze <sebastien.laveze@nxp.com> > Subject: Re: [net-next, v3, 01/10] ptp: add ptp virtual clock driver framework > > On Tue, Jun 15, 2021 at 05:45:08PM +0800, Yangbo Lu wrote: > > diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile index > > 8673d1743faa..3c6a905760e2 100644 > > --- a/drivers/ptp/Makefile > > +++ b/drivers/ptp/Makefile > > @@ -3,7 +3,7 @@ > > # Makefile for PTP 1588 clock support. > > # > > > > -ptp-y := ptp_clock.o ptp_chardev.o ptp_sysfs.o > > +ptp-y := ptp_clock.o ptp_vclock.o ptp_chardev.o > ptp_sysfs.o > > Nit: Please place ptp_vclock.o at the end of the list. Ok. > > > ptp_kvm-$(CONFIG_X86) := ptp_kvm_x86.o ptp_kvm_common.o > > ptp_kvm-$(CONFIG_HAVE_ARM_SMCCC) := ptp_kvm_arm.o > ptp_kvm_common.o > > obj-$(CONFIG_PTP_1588_CLOCK) += ptp.o > > > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h > > index 6b97155148f1..3f388d63904c 100644 > > --- a/drivers/ptp/ptp_private.h > > +++ b/drivers/ptp/ptp_private.h > > @@ -48,6 +48,20 @@ struct ptp_clock { > > struct kthread_delayed_work aux_work; }; > > > > +#define info_to_vclock(d) container_of((d), struct ptp_vclock, info) > > +#define cc_to_vclock(d) container_of((d), struct ptp_vclock, cc) > > +#define dw_to_vclock(d) container_of((d), struct ptp_vclock, > > +refresh_work) > > + > > +struct ptp_vclock { > > + struct ptp_clock *pclock; > > + struct ptp_clock_info info; > > + struct ptp_clock *clock; > > + struct cyclecounter cc; > > + struct timecounter tc; > > + spinlock_t lock; /* protects tc/cc */ > > + struct delayed_work refresh_work; > > Can we please have a kthread worker here instead of work? > > Experience shows that plain work can be delayed for a long, long time on busy > systems. > I think do_aux_work callback could be utilized for ptp virtual clock, right? > > +}; > > + > > /* > > * The function queue_cnt() is safe for readers to call without > > * holding q->lock. Readers use this function to verify that the > > queue @@ -89,4 +103,6 @@ extern const struct attribute_group > > *ptp_groups[]; int ptp_populate_pin_groups(struct ptp_clock *ptp); > > void ptp_cleanup_pin_groups(struct ptp_clock *ptp); > > > > +struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock); > > +void ptp_vclock_unregister(struct ptp_vclock *vclock); > > #endif > > > diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c new > > file mode 100644 index 000000000000..b8f500677314 > > --- /dev/null > > +++ b/drivers/ptp/ptp_vclock.c > > @@ -0,0 +1,154 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * PTP virtual clock driver > > + * > > + * Copyright 2021 NXP > > + */ > > +#include <linux/slab.h> > > +#include "ptp_private.h" > > + > > +#define PTP_VCLOCK_CC_MULT (1 << 31) > > +#define PTP_VCLOCK_CC_SHIFT 31 > > These two are okay, but ... > > > +#define PTP_VCLOCK_CC_MULT_NUM (1 << 9) > > +#define PTP_VCLOCK_CC_MULT_DEM 15625ULL > > the similarity of naming is confusing for these two. They are only used in > the .adjfine method. How about this? > > PTP_VCLOCK_FADJ_NUMERATOR, or even PTP_VCLOCK_FADJ_SHIFT (see > below) > PTP_VCLOCK_FADJ_DENOMINATOR > > > +#define PTP_VCLOCK_CC_REFRESH_INTERVAL (HZ * 2) > > Consider dropping CC from the name. > PTP_VCLOCK_REFRESH_INTERVAL sounds good to me. > Thanks. Will rename the MACROs per your suggestion. > > +static int ptp_vclock_adjfine(struct ptp_clock_info *ptp, long > > +scaled_ppm) { > > + struct ptp_vclock *vclock = info_to_vclock(ptp); > > + unsigned long flags; > > + s64 adj; > > + > > + adj = (s64)scaled_ppm * PTP_VCLOCK_CC_MULT_NUM; > > Rather than > > scaled_ppm * (1 << 9) > > I suggest > > scaled_ppm << 9 > > instead. I suppose a good compiler would replace the multiplication with a > bit shift, but it never hurts to spell it out. Ok. > > > + adj = div_s64(adj, PTP_VCLOCK_CC_MULT_DEM); > > + > > + spin_lock_irqsave(&vclock->lock, flags); > > + timecounter_read(&vclock->tc); > > + vclock->cc.mult = PTP_VCLOCK_CC_MULT + adj; > > + spin_unlock_irqrestore(&vclock->lock, flags); > > + > > + return 0; > > +} > > Thanks, > Richard
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile index 8673d1743faa..3c6a905760e2 100644 --- a/drivers/ptp/Makefile +++ b/drivers/ptp/Makefile @@ -3,7 +3,7 @@ # Makefile for PTP 1588 clock support. # -ptp-y := ptp_clock.o ptp_chardev.o ptp_sysfs.o +ptp-y := ptp_clock.o ptp_vclock.o ptp_chardev.o ptp_sysfs.o ptp_kvm-$(CONFIG_X86) := ptp_kvm_x86.o ptp_kvm_common.o ptp_kvm-$(CONFIG_HAVE_ARM_SMCCC) := ptp_kvm_arm.o ptp_kvm_common.o obj-$(CONFIG_PTP_1588_CLOCK) += ptp.o diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h index 6b97155148f1..3f388d63904c 100644 --- a/drivers/ptp/ptp_private.h +++ b/drivers/ptp/ptp_private.h @@ -48,6 +48,20 @@ struct ptp_clock { struct kthread_delayed_work aux_work; }; +#define info_to_vclock(d) container_of((d), struct ptp_vclock, info) +#define cc_to_vclock(d) container_of((d), struct ptp_vclock, cc) +#define dw_to_vclock(d) container_of((d), struct ptp_vclock, refresh_work) + +struct ptp_vclock { + struct ptp_clock *pclock; + struct ptp_clock_info info; + struct ptp_clock *clock; + struct cyclecounter cc; + struct timecounter tc; + spinlock_t lock; /* protects tc/cc */ + struct delayed_work refresh_work; +}; + /* * The function queue_cnt() is safe for readers to call without * holding q->lock. Readers use this function to verify that the queue @@ -89,4 +103,6 @@ extern const struct attribute_group *ptp_groups[]; int ptp_populate_pin_groups(struct ptp_clock *ptp); void ptp_cleanup_pin_groups(struct ptp_clock *ptp); +struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock); +void ptp_vclock_unregister(struct ptp_vclock *vclock); #endif diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c new file mode 100644 index 000000000000..b8f500677314 --- /dev/null +++ b/drivers/ptp/ptp_vclock.c @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * PTP virtual clock driver + * + * Copyright 2021 NXP + */ +#include <linux/slab.h> +#include "ptp_private.h" + +#define PTP_VCLOCK_CC_MULT (1 << 31) +#define PTP_VCLOCK_CC_SHIFT 31 +#define PTP_VCLOCK_CC_MULT_NUM (1 << 9) +#define PTP_VCLOCK_CC_MULT_DEM 15625ULL +#define PTP_VCLOCK_CC_REFRESH_INTERVAL (HZ * 2) + +static int ptp_vclock_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) +{ + struct ptp_vclock *vclock = info_to_vclock(ptp); + unsigned long flags; + s64 adj; + + adj = (s64)scaled_ppm * PTP_VCLOCK_CC_MULT_NUM; + adj = div_s64(adj, PTP_VCLOCK_CC_MULT_DEM); + + spin_lock_irqsave(&vclock->lock, flags); + timecounter_read(&vclock->tc); + vclock->cc.mult = PTP_VCLOCK_CC_MULT + adj; + spin_unlock_irqrestore(&vclock->lock, flags); + + return 0; +} + +static int ptp_vclock_adjtime(struct ptp_clock_info *ptp, s64 delta) +{ + struct ptp_vclock *vclock = info_to_vclock(ptp); + unsigned long flags; + + spin_lock_irqsave(&vclock->lock, flags); + timecounter_adjtime(&vclock->tc, delta); + spin_unlock_irqrestore(&vclock->lock, flags); + + return 0; +} + +static int ptp_vclock_gettime(struct ptp_clock_info *ptp, + struct timespec64 *ts) +{ + struct ptp_vclock *vclock = info_to_vclock(ptp); + unsigned long flags; + u64 ns; + + spin_lock_irqsave(&vclock->lock, flags); + ns = timecounter_read(&vclock->tc); + spin_unlock_irqrestore(&vclock->lock, flags); + *ts = ns_to_timespec64(ns); + + return 0; +} + +static int ptp_vclock_settime(struct ptp_clock_info *ptp, + const struct timespec64 *ts) +{ + struct ptp_vclock *vclock = info_to_vclock(ptp); + u64 ns = timespec64_to_ns(ts); + unsigned long flags; + + spin_lock_irqsave(&vclock->lock, flags); + timecounter_init(&vclock->tc, &vclock->cc, ns); + spin_unlock_irqrestore(&vclock->lock, flags); + + return 0; +} + +static const struct ptp_clock_info ptp_vclock_info = { + .owner = THIS_MODULE, + .name = "ptp virtual clock", + /* The maximum ppb value that long scaled_ppm can support */ + .max_adj = 32767999, + .adjfine = ptp_vclock_adjfine, + .adjtime = ptp_vclock_adjtime, + .gettime64 = ptp_vclock_gettime, + .settime64 = ptp_vclock_settime, +}; + +static void ptp_vclock_refresh(struct work_struct *work) +{ + struct delayed_work *dw = to_delayed_work(work); + struct ptp_vclock *vclock = dw_to_vclock(dw); + struct timespec64 ts; + + ptp_vclock_gettime(&vclock->info, &ts); + schedule_delayed_work(&vclock->refresh_work, + PTP_VCLOCK_CC_REFRESH_INTERVAL); +} + +static u64 ptp_vclock_read(const struct cyclecounter *cc) +{ + struct ptp_vclock *vclock = cc_to_vclock(cc); + struct ptp_clock *ptp = vclock->pclock; + struct timespec64 ts = {}; + + if (ptp->info->gettimex64) + ptp->info->gettimex64(ptp->info, &ts, NULL); + else + ptp->info->gettime64(ptp->info, &ts); + + return timespec64_to_ns(&ts); +} + +static const struct cyclecounter ptp_vclock_cc = { + .read = ptp_vclock_read, + .mask = CYCLECOUNTER_MASK(32), + .mult = PTP_VCLOCK_CC_MULT, + .shift = PTP_VCLOCK_CC_SHIFT, +}; + +struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock) +{ + struct ptp_vclock *vclock; + + vclock = kzalloc(sizeof(*vclock), GFP_KERNEL); + if (!vclock) + return NULL; + + vclock->pclock = pclock; + vclock->info = ptp_vclock_info; + vclock->cc = ptp_vclock_cc; + + snprintf(vclock->info.name, PTP_CLOCK_NAME_LEN, "ptp%d_virt", + pclock->index); + + spin_lock_init(&vclock->lock); + + vclock->clock = ptp_clock_register(&vclock->info, &pclock->dev); + if (IS_ERR_OR_NULL(vclock->clock)) { + kfree(vclock); + return NULL; + } + + timecounter_init(&vclock->tc, &vclock->cc, 0); + + INIT_DELAYED_WORK(&vclock->refresh_work, ptp_vclock_refresh); + schedule_delayed_work(&vclock->refresh_work, + PTP_VCLOCK_CC_REFRESH_INTERVAL); + + return vclock; +} + +void ptp_vclock_unregister(struct ptp_vclock *vclock) +{ + cancel_delayed_work_sync(&vclock->refresh_work); + ptp_clock_unregister(vclock->clock); + kfree(vclock); +} diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h index a311bddd9e85..af12cc1e76d7 100644 --- a/include/linux/ptp_clock_kernel.h +++ b/include/linux/ptp_clock_kernel.h @@ -11,7 +11,9 @@ #include <linux/device.h> #include <linux/pps_kernel.h> #include <linux/ptp_clock.h> +#include <linux/timecounter.h> +#define PTP_CLOCK_NAME_LEN 32 /** * struct ptp_clock_request - request PTP clock event * @@ -134,7 +136,7 @@ struct ptp_system_timestamp { struct ptp_clock_info { struct module *owner; - char name[16]; + char name[PTP_CLOCK_NAME_LEN]; s32 max_adj; int n_alarm; int n_ext_ts;
This patch is to add ptp virtual clock driver framework utilizing timecounter/cyclecounter. The patch just exports two essential APIs for PTP driver. - ptp_vclock_register() - ptp_vclock_unregister() Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> --- Changes for v2: - Split from v1 patch #1. - Fixed build warning. - Updated copyright. Changes for v3: - Supported PTP virtual clock in default in PTP driver. --- drivers/ptp/Makefile | 2 +- drivers/ptp/ptp_private.h | 16 ++++ drivers/ptp/ptp_vclock.c | 154 +++++++++++++++++++++++++++++++ include/linux/ptp_clock_kernel.h | 4 +- 4 files changed, 174 insertions(+), 2 deletions(-) create mode 100644 drivers/ptp/ptp_vclock.c