diff mbox series

[net-next,v2,1/5] ptp: Add cycles support for virtual clocks

Message ID 20220403175544.26556-2-gerhard@engleder-embedded.com (mailing list archive)
State Deferred
Delegated to: Netdev Maintainers
Headers show
Series ptp: Support hardware clocks with additional free running cycle counter | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 204 this patch: 204
netdev/cc_maintainers success CCed 3 of 3 maintainers
netdev/build_clang success Errors and warnings before: 17 this patch: 17
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 235 this patch: 235
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 176 lines checked
netdev/kdoc success Errors and warnings before: 5 this patch: 5
netdev/source_inline success Was 0 now: 0

Commit Message

Gerhard Engleder April 3, 2022, 5:55 p.m. UTC
ptp vclocks require a free running time for their timecounter.
Currently only a physical clock forced to free running is supported.
If vclocks are used, then the physical clock cannot be synchronized
anymore. The synchronized time is not available in hardware in this
case. As a result, timed transmission with TAPRIO hardware support
is not possible anymore.

If hardware would support a free running time additionally to the
physical clock, then the physical clock does not need to be forced to
free running. Thus, the physical clocks can still be synchronized
while vclocks are in use.

The physical clock could be used to synchronize the time domain of the
TSN network and trigger TAPRIO. In parallel vclocks can be used to
synchronize other time domains.

Introduce support for a free running cycle counter called cycles to
physical clocks. Rework ptp vclocks to use this free running cycle
counter. Default implementation is based on time of physical clock.
Thus, behavior of ptp vclocks based on physical clocks without free
running cycle counter is identical to previous behavior.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/ptp/ptp_clock.c          | 31 +++++++++++++++++++++++++++----
 drivers/ptp/ptp_private.h        | 10 ++++++++++
 drivers/ptp/ptp_sysfs.c          | 10 ++++++----
 drivers/ptp/ptp_vclock.c         | 13 +++++--------
 include/linux/ptp_clock_kernel.h | 31 +++++++++++++++++++++++++++++++
 5 files changed, 79 insertions(+), 16 deletions(-)

Comments

Richard Cochran April 10, 2022, 6:26 a.m. UTC | #1
On Sun, Apr 03, 2022 at 07:55:40PM +0200, Gerhard Engleder wrote:

> @@ -225,6 +233,21 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  	mutex_init(&ptp->n_vclocks_mux);
>  	init_waitqueue_head(&ptp->tsev_wq);
>  
> +	if (!ptp->info->getcycles64 && !ptp->info->getcyclesx64) {

Please swap blocks, using non-negated logical test:

	if (ptp->info->getcycles64 || ptp->info->getcyclesx64)

> +		/* Free running cycle counter not supported, use time. */
> +		ptp->info->getcycles64 = ptp_getcycles64;
> +
> +		if (ptp->info->gettimex64)
> +			ptp->info->getcyclesx64 = ptp->info->gettimex64;
> +
> +		if (ptp->info->getcrosststamp)
> +			ptp->info->getcrosscycles = ptp->info->getcrosststamp;
> +	} else {
> +		ptp->has_cycles = true;
> +		if (!ptp->info->getcycles64 && ptp->info->getcyclesx64)
> +			ptp->info->getcycles64 = ptp_getcycles64;
> +	}
> +
>  	if (ptp->info->do_aux_work) {
>  		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
>  		ptp->kworker = kthread_create_worker(0, "ptp%d", ptp->index);


> @@ -231,10 +231,12 @@ static ssize_t n_vclocks_store(struct device *dev,
>  			*(ptp->vclock_index + ptp->n_vclocks - i) = -1;
>  	}
>  
> -	if (num == 0)
> -		dev_info(dev, "only physical clock in use now\n");
> -	else
> -		dev_info(dev, "guarantee physical clock free running\n");
> +	if (!ptp->has_cycles) {

Not sure what this test means ...

> +		if (num == 0)
> +			dev_info(dev, "only physical clock in use now\n");

Shouldn't this one print even if has_cycles == false?

> +		else
> +			dev_info(dev, "guarantee physical clock free running\n");
> +	}
>  
>  	ptp->n_vclocks = num;
>  	mutex_unlock(&ptp->n_vclocks_mux);

Thanks,
Richard
Gerhard Engleder April 10, 2022, 12:32 p.m. UTC | #2
> > @@ -225,6 +233,21 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> >       mutex_init(&ptp->n_vclocks_mux);
> >       init_waitqueue_head(&ptp->tsev_wq);
> >
> > +     if (!ptp->info->getcycles64 && !ptp->info->getcyclesx64) {
>
> Please swap blocks, using non-negated logical test:
>
>         if (ptp->info->getcycles64 || ptp->info->getcyclesx64)

I will change it as suggested.

> > +             /* Free running cycle counter not supported, use time. */
> > +             ptp->info->getcycles64 = ptp_getcycles64;
> > +
> > +             if (ptp->info->gettimex64)
> > +                     ptp->info->getcyclesx64 = ptp->info->gettimex64;
> > +
> > +             if (ptp->info->getcrosststamp)
> > +                     ptp->info->getcrosscycles = ptp->info->getcrosststamp;
> > +     } else {
> > +             ptp->has_cycles = true;
> > +             if (!ptp->info->getcycles64 && ptp->info->getcyclesx64)
> > +                     ptp->info->getcycles64 = ptp_getcycles64;
> > +     }
> > +
> >       if (ptp->info->do_aux_work) {
> >               kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
> >               ptp->kworker = kthread_create_worker(0, "ptp%d", ptp->index);
>
>
> > @@ -231,10 +231,12 @@ static ssize_t n_vclocks_store(struct device *dev,
> >                       *(ptp->vclock_index + ptp->n_vclocks - i) = -1;
> >       }
> >
> > -     if (num == 0)
> > -             dev_info(dev, "only physical clock in use now\n");
> > -     else
> > -             dev_info(dev, "guarantee physical clock free running\n");
> > +     if (!ptp->has_cycles) {
>
> Not sure what this test means ...

I thought these dev_info() are useless if the free running cycle
counter is supported,
because the behavior of the physical clock does not change in this case.

> > +             if (num == 0)
> > +                     dev_info(dev, "only physical clock in use now\n");
>
> Shouldn't this one print even if has_cycles == false?

It will print if has_cycles == false.

In my opinion this dev_info() tells the user that the physical clock can be used
again. So it does not carry any interesting information if has_cycles == true.

> > +             else
> > +                     dev_info(dev, "guarantee physical clock free running\n");
> > +     }
> >
> >       ptp->n_vclocks = num;
> >       mutex_unlock(&ptp->n_vclocks_mux);

Thank you!

Gerhard
diff mbox series

Patch

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index b6f2cfd15dd2..11b8190807c3 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -77,8 +77,8 @@  static int ptp_clock_settime(struct posix_clock *pc, const struct timespec64 *tp
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 
-	if (ptp_vclock_in_use(ptp)) {
-		pr_err("ptp: virtual clock in use\n");
+	if (ptp_clock_freerun(ptp)) {
+		pr_err("ptp: physical clock is free running\n");
 		return -EBUSY;
 	}
 
@@ -103,8 +103,8 @@  static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
 	struct ptp_clock_info *ops;
 	int err = -EOPNOTSUPP;
 
-	if (ptp_vclock_in_use(ptp)) {
-		pr_err("ptp: virtual clock in use\n");
+	if (ptp_clock_freerun(ptp)) {
+		pr_err("ptp: physical clock is free running\n");
 		return -EBUSY;
 	}
 
@@ -178,6 +178,14 @@  static void ptp_clock_release(struct device *dev)
 	kfree(ptp);
 }
 
+static int ptp_getcycles64(struct ptp_clock_info *info, struct timespec64 *ts)
+{
+	if (info->getcyclesx64)
+		return info->getcyclesx64(info, ts, NULL);
+	else
+		return info->gettime64(info, ts);
+}
+
 static void ptp_aux_kworker(struct kthread_work *work)
 {
 	struct ptp_clock *ptp = container_of(work, struct ptp_clock,
@@ -225,6 +233,21 @@  struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	mutex_init(&ptp->n_vclocks_mux);
 	init_waitqueue_head(&ptp->tsev_wq);
 
+	if (!ptp->info->getcycles64 && !ptp->info->getcyclesx64) {
+		/* Free running cycle counter not supported, use time. */
+		ptp->info->getcycles64 = ptp_getcycles64;
+
+		if (ptp->info->gettimex64)
+			ptp->info->getcyclesx64 = ptp->info->gettimex64;
+
+		if (ptp->info->getcrosststamp)
+			ptp->info->getcrosscycles = ptp->info->getcrosststamp;
+	} else {
+		ptp->has_cycles = true;
+		if (!ptp->info->getcycles64 && ptp->info->getcyclesx64)
+			ptp->info->getcycles64 = ptp_getcycles64;
+	}
+
 	if (ptp->info->do_aux_work) {
 		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
 		ptp->kworker = kthread_create_worker(0, "ptp%d", ptp->index);
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index dba6be477067..ab47c10b3874 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -52,6 +52,7 @@  struct ptp_clock {
 	int *vclock_index;
 	struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */
 	bool is_virtual_clock;
+	bool has_cycles;
 };
 
 #define info_to_vclock(d) container_of((d), struct ptp_vclock, info)
@@ -96,6 +97,15 @@  static inline bool ptp_vclock_in_use(struct ptp_clock *ptp)
 	return in_use;
 }
 
+/* Check if ptp clock shall be free running */
+static inline bool ptp_clock_freerun(struct ptp_clock *ptp)
+{
+	if (ptp->has_cycles)
+		return false;
+
+	return ptp_vclock_in_use(ptp);
+}
+
 extern struct class *ptp_class;
 
 /*
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 9233bfedeb17..414a70d32571 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -231,10 +231,12 @@  static ssize_t n_vclocks_store(struct device *dev,
 			*(ptp->vclock_index + ptp->n_vclocks - i) = -1;
 	}
 
-	if (num == 0)
-		dev_info(dev, "only physical clock in use now\n");
-	else
-		dev_info(dev, "guarantee physical clock free running\n");
+	if (!ptp->has_cycles) {
+		if (num == 0)
+			dev_info(dev, "only physical clock in use now\n");
+		else
+			dev_info(dev, "guarantee physical clock free running\n");
+	}
 
 	ptp->n_vclocks = num;
 	mutex_unlock(&ptp->n_vclocks_mux);
diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index cb179a3ea508..3a095eab9cc5 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -68,7 +68,7 @@  static int ptp_vclock_gettimex(struct ptp_clock_info *ptp,
 	int err;
 	u64 ns;
 
-	err = pptp->info->gettimex64(pptp->info, &pts, sts);
+	err = pptp->info->getcyclesx64(pptp->info, &pts, sts);
 	if (err)
 		return err;
 
@@ -104,7 +104,7 @@  static int ptp_vclock_getcrosststamp(struct ptp_clock_info *ptp,
 	int err;
 	u64 ns;
 
-	err = pptp->info->getcrosststamp(pptp->info, xtstamp);
+	err = pptp->info->getcrosscycles(pptp->info, xtstamp);
 	if (err)
 		return err;
 
@@ -143,10 +143,7 @@  static u64 ptp_vclock_read(const struct cyclecounter *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);
+	ptp->info->getcycles64(ptp->info, &ts);
 
 	return timespec64_to_ns(&ts);
 }
@@ -168,11 +165,11 @@  struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 
 	vclock->pclock = pclock;
 	vclock->info = ptp_vclock_info;
-	if (pclock->info->gettimex64)
+	if (pclock->info->getcyclesx64)
 		vclock->info.gettimex64 = ptp_vclock_gettimex;
 	else
 		vclock->info.gettime64 = ptp_vclock_gettime;
-	if (pclock->info->getcrosststamp)
+	if (pclock->info->getcrosscycles)
 		vclock->info.getcrosststamp = ptp_vclock_getcrosststamp;
 	vclock->cc = ptp_vclock_cc;
 
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 554454cb8693..3ea7110a9d70 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -108,6 +108,32 @@  struct ptp_system_timestamp {
  * @settime64:  Set the current time on the hardware clock.
  *              parameter ts: Time value to set.
  *
+ * @getcycles64:  Reads the current free running cycle counter from the hardware
+ *                clock.
+ *                If @getcycles64 and @getcyclesx64 are not supported, then
+ *                @gettime64 or @gettimex64 will be used as default
+ *                implementation.
+ *                parameter ts: Holds the result.
+ *
+ * @getcyclesx64:  Reads the current free running cycle counter from the
+ *                 hardware clock and optionally also the system clock.
+ *                 If @getcycles64 and @getcyclesx64 are not supported, then
+ *                 @gettimex64 will be used as default implementation if
+ *                 available.
+ *                 parameter ts: Holds the PHC timestamp.
+ *                 parameter sts: If not NULL, it holds a pair of timestamps
+ *                 from the system clock. The first reading is made right before
+ *                 reading the lowest bits of the PHC timestamp and the second
+ *                 reading immediately follows that.
+ *
+ * @getcrosscycles:  Reads the current free running cycle counter from the
+ *                   hardware clock and system clock simultaneously.
+ *                   If @getcycles64 and @getcyclesx64 are not supported, then
+ *                   @getcrosststamp will be used as default implementation if
+ *                   available.
+ *                   parameter cts: Contains timestamp (device,system) pair,
+ *                   where system time is realtime and monotonic.
+ *
  * @enable:   Request driver to enable or disable an ancillary feature.
  *            parameter request: Desired resource to enable or disable.
  *            parameter on: Caller passes one to enable or zero to disable.
@@ -155,6 +181,11 @@  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 (*getcycles64)(struct ptp_clock_info *ptp, struct timespec64 *ts);
+	int (*getcyclesx64)(struct ptp_clock_info *ptp, struct timespec64 *ts,
+			    struct ptp_system_timestamp *sts);
+	int (*getcrosscycles)(struct ptp_clock_info *ptp,
+			      struct system_device_crosststamp *cts);
 	int (*enable)(struct ptp_clock_info *ptp,
 		      struct ptp_clock_request *request, int on);
 	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,