| Message ID | 1557032106-28041-1-git-send-email-Po.Liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] timer:clock:ptp: add support the dynamic posix clock alarm set for ptp | expand |
On Sun, May 05, 2019 at 05:02:05AM +0000, Po Liu wrote: > Current kernel code do not support the dynamic posix clock alarm set. > This code would support it by the posix timer structure. > > 319 const struct k_clock clock_posix_dynamic = { > > 320 .clock_getres = pc_clock_getres, > 321 .clock_set = pc_clock_settime, > 322 .clock_get = pc_clock_gettime, > 323 .clock_adj = pc_clock_adjtime, > 324 + .timer_create = pc_timer_create, > 325 + .timer_del = pc_timer_delete, > 326 + .timer_set = pc_timer_set, > 327 + .timer_arm = pc_timer_arm, > } > Sorry, NAK, since we decided some time ago not to support timer_* operations on dynamic clocks. You get much better application level timer performance by synchronizing CLOCK_REALTIME to your PHC and using clock_nanosleep() with CLOCK_REALTIME or CLOCK_MONOTONIC. > This won't change the user space system call code. Normally the user > space set alarm by timer_create() and timer_settime(). Reference code > are tools/testing/selftests/ptp/testptp.c. That program still has misleading examples. Sorry about that. I'll submit a patch to remove them. > +static int pc_timer_create(struct k_itimer *new_timer) > +{ > + return 0; > +} > + This of course would never work. Consider what happens when two or more timers are created and armed. Thanks, Richard
Hi Richard, Thank you for your reply. Br, Po Liu > -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: 2019年5月7日 21:50 > To: Po Liu <po.liu@nxp.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; Y.b. Lu > <yangbo.lu@nxp.com>; Claudiu Manoil <claudiu.manoil@nxp.com>; > davem@davemloft.net; Leo Li <leoyang.li@nxp.com>; Roy Zang > <roy.zang@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; > deepa.kernel@gmail.com > Subject: [EXT] Re: [PATCH v1] timer:clock:ptp: add support the dynamic posix > clock alarm set for ptp > > Caution: EXT Email > > On Sun, May 05, 2019 at 05:02:05AM +0000, Po Liu wrote: > > Current kernel code do not support the dynamic posix clock alarm set. > > This code would support it by the posix timer structure. > > > > 319 const struct k_clock clock_posix_dynamic = { > > > > 320 .clock_getres = pc_clock_getres, > > 321 .clock_set = pc_clock_settime, > > 322 .clock_get = pc_clock_gettime, > > 323 .clock_adj = pc_clock_adjtime, > > 324 + .timer_create = pc_timer_create, > > 325 + .timer_del = pc_timer_delete, > > 326 + .timer_set = pc_timer_set, > > 327 + .timer_arm = pc_timer_arm, > > } > > > > Sorry, NAK, since we decided some time ago not to support timer_* operations > on dynamic clocks. You get much better application level timer performance > by synchronizing CLOCK_REALTIME to your PHC and using clock_nanosleep() > with CLOCK_REALTIME or CLOCK_MONOTONIC. The code intend to get alarm by interrupt of ptp hardware. The code to fix ptp not support to application layer to get the alarm interrupt. Do you mean the synchronizing at application layer by PHC (using clock_nanosleep()) to the CLOCK_REALTIME source? Then the kernel could using the hrtimer with CLOCK_REALTIME? > > > This won't change the user space system call code. Normally the user > > space set alarm by timer_create() and timer_settime(). Reference code > > are tools/testing/selftests/ptp/testptp.c. > > That program still has misleading examples. Sorry about that. I'll submit a > patch to remove them. Is there any replace method for an application code to get alarm interrupt by the ptp source? > > > +static int pc_timer_create(struct k_itimer *new_timer) { > > + return 0; > > +} > > + > > This of course would never work. Consider what happens when two or more > timers are created and armed. > > Thanks, > Richard
On Wed, May 08, 2019 at 03:30:01AM +0000, Po Liu wrote: > > Sorry, NAK, since we decided some time ago not to support timer_* operations > > on dynamic clocks. You get much better application level timer performance > > by synchronizing CLOCK_REALTIME to your PHC and using clock_nanosleep() > > with CLOCK_REALTIME or CLOCK_MONOTONIC. > > The code intend to get alarm by interrupt of ptp hardware. The code > to fix ptp not support to application layer to get the alarm > interrupt. Do you mean the synchronizing at application layer by > PHC (using clock_nanosleep()) to the CLOCK_REALTIME source? Then the > kernel could using the hrtimer with CLOCK_REALTIME? Yes, or with CLOCK_MONOTONIC. > > > This won't change the user space system call code. Normally the user > > > space set alarm by timer_create() and timer_settime(). Reference code > > > are tools/testing/selftests/ptp/testptp.c. > > > > That program still has misleading examples. Sorry about that. I'll submit a > > patch to remove them. > > Is there any replace method for an application code to get alarm interrupt by the ptp source? No the alarm functionality has been removed. It will not be coming back, unless there are really strong arguments to support it. Here is the result of a study of a prototype alarm method. It shows why the hrtimer method is better. https://sourceforge.net/p/linuxptp/mailman/message/35535965/ Thanks, Richard
On Wed, May 08, 2019 at 07:36:54AM -0700, Richard Cochran wrote: > No the alarm functionality has been removed. It will not be coming > back, unless there are really strong arguments to support it. Here is some more background: commit 3a06c7ac24f9f24ec059cd77c2dbdf7fbfd0aaaf Author: Thomas Gleixner <tglx@linutronix.de> Date: Tue May 30 23:15:38 2017 +0200 posix-clocks: Remove interval timer facility and mmap/fasync callbacks The only user of this facility is ptp_clock, which does not implement any of those functions. Remove them to prevent accidental users. Especially the interval timer interfaces are now more or less impossible to implement because the necessary infrastructure has been confined to the core code. Aside of that it's really complex to make these callbacks implemented according to spec as the alarm timer implementation demonstrates. If at all then a nanosleep callback might be a reasonable extension. For now keep just what ptp_clock needs. > Here is the result of a study of a prototype alarm method. It shows > why the hrtimer method is better. > > https://sourceforge.net/p/linuxptp/mailman/message/35535965/ That test was with a PCIe card. With a SoC that has a PHC as a built in peripheral, the hardware solution might outperform hrtimers. So you might consider adding clock_nanosleep() for dynamic posix clocks. But your code will have to support multiple users at the same time. Thanks, Richard
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ptp.c b/drivers/net/ethernet/freescale/enetc/enetc_ptp.c index 8c1497e..35e2f2a 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_ptp.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_ptp.c @@ -21,6 +21,7 @@ .gettime64 = ptp_qoriq_gettime, .settime64 = ptp_qoriq_settime, .enable = ptp_qoriq_enable, + .alarm = ptp_qoriq_alarm, }; static int enetc_ptp_probe(struct pci_dev *pdev, diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 79bd102..72d06a8 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -23,7 +23,9 @@ #include <linux/init.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/sched/task.h> #include <linux/posix-clock.h> +#include <linux/posix-timers.h> #include <linux/pps_kernel.h> #include <linux/slab.h> #include <linux/syscalls.h> @@ -166,12 +168,31 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx) return err; } +static int ptp_clock_alarm(struct posix_clock *pc, ktime_t expires, + bool absolute, struct k_itimer *timr) +{ + struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock); + struct ptp_clock_info *ops; + + ops = ptp->info; + if (!ops) + return -EINVAL; + + if (!ops->alarm) + return -EINVAL; + + ops->alarm(ops, expires, absolute, timr); + + return 0; +} + static struct posix_clock_operations ptp_clock_ops = { .owner = THIS_MODULE, .clock_adjtime = ptp_clock_adjtime, .clock_gettime = ptp_clock_gettime, .clock_getres = ptp_clock_getres, .clock_settime = ptp_clock_settime, + .clock_alarm = ptp_clock_alarm, .ioctl = ptp_ioctl, .open = ptp_open, .poll = ptp_poll, @@ -324,6 +345,20 @@ int ptp_clock_unregister(struct ptp_clock *ptp) } EXPORT_SYMBOL(ptp_clock_unregister); +int alarm_timer_event(struct k_itimer *timr, int si_private) +{ + int ret = -1; + + timr->sigq->info.si_sys_private = si_private; + + rcu_read_lock(); + ret = send_sigqueue(timr->sigq, timr->it_pid, PIDTYPE_PID); + rcu_read_unlock(); + + /* If we failed to send the signal the timer stops. */ + return ret > 0; +} + void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event) { struct pps_event_time evt; @@ -331,8 +366,10 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event) switch (event->type) { case PTP_CLOCK_ALARM: + if (!event->timr) + break; + alarm_timer_event(event->timr, 0); break; - case PTP_CLOCK_EXTTS: enqueue_external_timestamp(&ptp->tsevq, event); wake_up_interruptible(&ptp->tsev_wq); diff --git a/drivers/ptp/ptp_qoriq.c b/drivers/ptp/ptp_qoriq.c index 5377536..ce14d44 100644 --- a/drivers/ptp/ptp_qoriq.c +++ b/drivers/ptp/ptp_qoriq.c @@ -163,10 +163,15 @@ irqreturn_t ptp_qoriq_isr(int irq, void *priv) if (irqs & ALM2) { ack |= ALM2; + if (!ptp_qoriq->timr) { + ptp_qoriq->alarm_value = 0; + ptp_qoriq->alarm_interval = 0; + } if (ptp_qoriq->alarm_value) { event.type = PTP_CLOCK_ALARM; event.index = 0; event.timestamp = ptp_qoriq->alarm_value; + event.timr = ptp_qoriq->timr; ptp_clock_event(ptp_qoriq->clock, &event); } if (ptp_qoriq->alarm_interval) { @@ -341,6 +346,44 @@ int ptp_qoriq_enable(struct ptp_clock_info *ptp, } EXPORT_SYMBOL_GPL(ptp_qoriq_enable); +int ptp_qoriq_alarm(struct ptp_clock_info *ptp, ktime_t expires, + bool absolute, struct k_itimer *timr) +{ + u64 ns, now; + u32 lo, hi, mask; + + struct ptp_qoriq *ptp_qoriq = container_of(ptp, struct ptp_qoriq, caps); + struct ptp_qoriq_registers *regs = &ptp_qoriq->regs; + + if (!timr) + return -EINVAL; + + now = tmr_cnt_read(ptp_qoriq); + if (!absolute) + ns = now + ktime_to_ns(expires); + else if (ktime_to_ns(expires) < now) + ns = now; + else + ns = ktime_to_ns(expires); + + hi = ns >> 32; + lo = ns & 0xffffffff; + ptp_qoriq->write(®s->alarm_regs->tmr_alarm2_l, lo); + ptp_qoriq->write(®s->alarm_regs->tmr_alarm2_h, hi); + + spin_lock(&ptp_qoriq->lock); + mask = ptp_qoriq->read(®s->ctrl_regs->tmr_temask); + mask |= ALM2EN; + ptp_qoriq->write(®s->ctrl_regs->tmr_temask, mask); + spin_unlock(&ptp_qoriq->lock); + + ptp_qoriq->alarm_value = ns; + ptp_qoriq->alarm_interval = ktime_to_ns(timr->it_interval); + + ptp_qoriq->timr = timr; + return 0; +} + static const struct ptp_clock_info ptp_qoriq_caps = { .owner = THIS_MODULE, .name = "qoriq ptp clock", @@ -355,6 +398,7 @@ int ptp_qoriq_enable(struct ptp_clock_info *ptp, .gettime64 = ptp_qoriq_gettime, .settime64 = ptp_qoriq_settime, .enable = ptp_qoriq_enable, + .alarm = ptp_qoriq_alarm, }; /** diff --git a/include/linux/fsl/ptp_qoriq.h b/include/linux/fsl/ptp_qoriq.h index 992bf9f..2928df4 100644 --- a/include/linux/fsl/ptp_qoriq.h +++ b/include/linux/fsl/ptp_qoriq.h @@ -143,6 +143,7 @@ struct ptp_qoriq { spinlock_t lock; /* protects regs */ struct ptp_clock *clock; struct ptp_clock_info caps; + struct k_itimer *timr; struct resource *rsrc; struct dentry *debugfs_root; struct device *dev; @@ -190,6 +191,8 @@ int ptp_qoriq_init(struct ptp_qoriq *ptp_qoriq, void __iomem *base, int ptp_qoriq_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts); int ptp_qoriq_settime(struct ptp_clock_info *ptp, const struct timespec64 *ts); +int ptp_qoriq_alarm(struct ptp_clock_info *ptp, ktime_t expires, + bool absolute, struct k_itimer *timr); int ptp_qoriq_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *rq, int on); #ifdef CONFIG_DEBUG_FS diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h index 18674d7..80cc214 100644 --- a/include/linux/posix-clock.h +++ b/include/linux/posix-clock.h @@ -59,7 +59,8 @@ struct posix_clock_operations { int (*clock_settime)(struct posix_clock *pc, const struct timespec64 *ts); - + int (*clock_alarm)(struct posix_clock *pc, ktime_t expires, + bool absolute, struct k_itimer *timr); /* * Optional character device methods: */ diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h index 7121bbe..b51f64b 100644 --- a/include/linux/ptp_clock_kernel.h +++ b/include/linux/ptp_clock_kernel.h @@ -24,7 +24,7 @@ #include <linux/device.h> #include <linux/pps_kernel.h> #include <linux/ptp_clock.h> - +#include <linux/posix-timers.h> struct ptp_clock_request { enum { @@ -148,6 +148,8 @@ struct ptp_clock_info { int (*getcrosststamp)(struct ptp_clock_info *ptp, struct system_device_crosststamp *cts); int (*settime64)(struct ptp_clock_info *p, const struct timespec64 *ts); + int (*alarm)(struct ptp_clock_info *p, ktime_t expires, + bool absolute, struct k_itimer *timr); int (*enable)(struct ptp_clock_info *ptp, struct ptp_clock_request *request, int on); int (*verify)(struct ptp_clock_info *ptp, unsigned int pin, @@ -180,6 +182,7 @@ struct ptp_clock_event { u64 timestamp; struct pps_event_time pps_times; }; + struct k_itimer *timr; }; #if IS_REACHABLE(CONFIG_PTP_1588_CLOCK) diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index ec960bb..ac25d17 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -8,6 +8,7 @@ #include <linux/export.h> #include <linux/file.h> #include <linux/posix-clock.h> +#include <linux/posix-timers.h> #include <linux/slab.h> #include <linux/syscalls.h> #include <linux/uaccess.h> @@ -314,9 +315,68 @@ static int pc_clock_settime(clockid_t id, const struct timespec64 *ts) return err; } +static void pc_timer_arm(struct k_itimer *timr, ktime_t expires, + bool absolute, bool sigev_none) +{ + struct posix_clock_desc cd; + int err; + + err = get_clock_desc(timr->it_clock, &cd); + if (err) + return; + + cd.clk->ops.clock_alarm(cd.clk, expires, absolute, timr); +} + +static int pc_timer_set(struct k_itimer *timr, int flags, + struct itimerspec64 *new_setting, + struct itimerspec64 *old_setting) +{ + const struct k_clock *kc = timr->kclock; + bool sigev_none; + ktime_t expires; + + if (old_setting) + pr_err("old_setting not support!\n"); + + /* Prevent rearming by clearing the interval */ + timr->it_interval = 0; + + timr->it_active = 0; + timr->it_requeue_pending = (timr->it_requeue_pending + 2) & + ~REQUEUE_PENDING; + timr->it_overrun_last = 0; + + /* Switch off the timer when it_value is zero */ + if (!new_setting->it_value.tv_sec && !new_setting->it_value.tv_nsec) + return 0; + + timr->it_interval = timespec64_to_ktime(new_setting->it_interval); + expires = timespec64_to_ktime(new_setting->it_value); + sigev_none = timr->it_sigev_notify == SIGEV_NONE; + + kc->timer_arm(timr, expires, flags & TIMER_ABSTIME, sigev_none); + timr->it_active = !sigev_none; + return 0; +} + +static int pc_timer_create(struct k_itimer *new_timer) +{ + return 0; +} + +static int pc_timer_delete(struct k_itimer *new_timer) +{ + return 0; +} + const struct k_clock clock_posix_dynamic = { .clock_getres = pc_clock_getres, .clock_set = pc_clock_settime, .clock_get = pc_clock_gettime, .clock_adj = pc_clock_adjtime, + .timer_create = pc_timer_create, + .timer_del = pc_timer_delete, + .timer_set = pc_timer_set, + .timer_arm = pc_timer_arm, };
Current kernel code do not support the dynamic posix clock alarm set. This code would support it by the posix timer structure. 319 const struct k_clock clock_posix_dynamic = { 320 .clock_getres = pc_clock_getres, 321 .clock_set = pc_clock_settime, 322 .clock_get = pc_clock_gettime, 323 .clock_adj = pc_clock_adjtime, 324 + .timer_create = pc_timer_create, 325 + .timer_del = pc_timer_delete, 326 + .timer_set = pc_timer_set, 327 + .timer_arm = pc_timer_arm, } This won't change the user space system call code. Normally the user space set alarm by timer_create() and timer_settime(). Reference code are tools/testing/selftests/ptp/testptp.c. Some case requiring providing the alarm set for user space by ptp clock. Signed-off-by: Po Liu <Po.Liu@nxp.com> --- drivers/net/ethernet/freescale/enetc/enetc_ptp.c | 1 + drivers/ptp/ptp_clock.c | 39 ++++++++++++++- drivers/ptp/ptp_qoriq.c | 44 +++++++++++++++++ include/linux/fsl/ptp_qoriq.h | 3 ++ include/linux/posix-clock.h | 3 +- include/linux/ptp_clock_kernel.h | 5 +- kernel/time/posix-clock.c | 60 ++++++++++++++++++++++++ 7 files changed, 152 insertions(+), 3 deletions(-)