[PATCH net-next 0/3] Fully describe the waveform for PTP periodic output

[PATCH net-next 0/3] Fully describe the waveform for PTP periodic output

[Vladimir Oltean]

While using the ancillary pin functionality of PTP hardware clocks to
synchronize multiple DSA switches on a board, a need arised to be able
to configure the duty cycle of the master of this PPS hierarchy.

Also, the PPS master is not able to emit PPS starting from arbitrary
absolute times, so a new flag is introduced to support such hardware
without making guesses.

With these patches, struct ptp_perout_request now basically describes a
general-purpose square wave.

Vladimir Oltean (3):
  ptp: add ability to configure duty cycle for periodic output
  ptp: introduce a phase offset in the periodic output request
  net: mscc: ocelot: add support for PTP waveform configuration

 drivers/net/ethernet/mscc/ocelot_ptp.c | 74 +++++++++++++++++---------
 drivers/ptp/ptp_chardev.c              | 33 +++++++++---
 include/uapi/linux/ptp_clock.h         | 34 ++++++++++--
 3 files changed, 107 insertions(+), 34 deletions(-)

-- 
2.25.1

[PATCH net-next 1/3] ptp: add ability to configure duty cycle for periodic output

[Vladimir Oltean]

There are external event timestampers (PHCs with support for
PTP_EXTTS_REQUEST) that timestamp both event edges.

When those edges are very close (such as in the case of a short pulse),
there is a chance that the collected timestamp might be of the rising,
or of the falling edge, we never know.

There are also PHCs capable of generating periodic output with a
configurable duty cycle. This is good news, because we can space the
rising and falling edge out enough in time, that the risks to overrun
the 1-entry timestamp FIFO of the extts PHC are lower (example: the
perout PHC can be configured for a period of 1 second, and an "on" time
of 0.5 seconds, resulting in a duty cycle of 50%).

A flag is introduced for signaling that an on time is present in the
perout request structure, for preserving compatibility. Logically
speaking, the duty cycle cannot exceed 100% and the PTP core checks for
this.

PHC drivers that don't support this flag emit a periodic output of an
unspecified duty cycle, same as before.

The duty cycle is encoded as an "on" time, similar to the "start" and
"period" times, and reuses the reserved space while preserving overall
binary layout.

Pahole reported before:

struct ptp_perout_request {
        struct ptp_clock_time start;                     /*     0    16 */
        struct ptp_clock_time period;                    /*    16    16 */
        unsigned int               index;                /*    32     4 */
        unsigned int               flags;                /*    36     4 */
        unsigned int               rsv[4];               /*    40    16 */

        /* size: 56, cachelines: 1, members: 5 */
        /* last cacheline: 56 bytes */
};

And now:

struct ptp_perout_request {
        struct ptp_clock_time start;                     /*     0    16 */
        struct ptp_clock_time period;                    /*    16    16 */
        unsigned int               index;                /*    32     4 */
        unsigned int               flags;                /*    36     4 */
        union {
                struct ptp_clock_time on;                /*    40    16 */
                unsigned int       rsv[4];               /*    40    16 */
        };                                               /*    40    16 */

        /* size: 56, cachelines: 1, members: 5 */
        /* last cacheline: 56 bytes */
};

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/ptp/ptp_chardev.c      | 33 +++++++++++++++++++++++++++------
 include/uapi/linux/ptp_clock.h | 17 ++++++++++++++---
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 375cd6e4aade..e0e6f85966e1 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -191,12 +191,33 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			err = -EFAULT;
 			break;
 		}
-		if (((req.perout.flags & ~PTP_PEROUT_VALID_FLAGS) ||
-			req.perout.rsv[0] || req.perout.rsv[1] ||
-			req.perout.rsv[2] || req.perout.rsv[3]) &&
-			cmd == PTP_PEROUT_REQUEST2) {
-			err = -EINVAL;
-			break;
+		if (cmd == PTP_PEROUT_REQUEST2) {
+			struct ptp_perout_request *perout = &req.perout;
+
+			if (perout->flags & ~PTP_PEROUT_VALID_FLAGS) {
+				err = -EINVAL;
+				break;
+			}
+			/*
+			 * The "on" field has undefined meaning if
+			 * PTP_PEROUT_DUTY_CYCLE isn't set, we must still treat
+			 * it as reserved, which must be set to zero.
+			 */
+			if (!(perout->flags & PTP_PEROUT_DUTY_CYCLE) &&
+			    (perout->rsv[0] || perout->rsv[1] ||
+			     perout->rsv[2] || perout->rsv[3])) {
+				err = -EINVAL;
+				break;
+			}
+			if (perout->flags & PTP_PEROUT_DUTY_CYCLE) {
+				/* The duty cycle must be subunitary. */
+				if (perout->on.sec > perout->period.sec ||
+				    (perout->on.sec == perout->period.sec &&
+				     perout->on.nsec > perout->period.nsec)) {
+					err = -ERANGE;
+					break;
+				}
+			}
 		} else if (cmd == PTP_PEROUT_REQUEST) {
 			req.perout.flags &= PTP_PEROUT_V1_VALID_FLAGS;
 			req.perout.rsv[0] = 0;
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index ff070aa64278..1d2841155f7d 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -53,12 +53,14 @@
 /*
  * Bits of the ptp_perout_request.flags field:
  */
-#define PTP_PEROUT_ONE_SHOT (1<<0)
+#define PTP_PEROUT_ONE_SHOT		(1<<0)
+#define PTP_PEROUT_DUTY_CYCLE		(1<<1)
 
 /*
  * flag fields valid for the new PTP_PEROUT_REQUEST2 ioctl.
  */
-#define PTP_PEROUT_VALID_FLAGS	(PTP_PEROUT_ONE_SHOT)
+#define PTP_PEROUT_VALID_FLAGS		(PTP_PEROUT_ONE_SHOT | \
+					 PTP_PEROUT_DUTY_CYCLE)
 
 /*
  * No flags are valid for the original PTP_PEROUT_REQUEST ioctl
@@ -105,7 +107,16 @@ struct ptp_perout_request {
 	struct ptp_clock_time period; /* Desired period, zero means disable. */
 	unsigned int index;           /* Which channel to configure. */
 	unsigned int flags;
-	unsigned int rsv[4];          /* Reserved for future use. */
+	union {
+		/*
+		 * The "on" time of the signal.
+		 * Must be lower than the period.
+		 * Valid only if (flags & PTP_PEROUT_DUTY_CYCLE) is set.
+		 */
+		struct ptp_clock_time on;
+		/* Reserved for future use. */
+		unsigned int rsv[4];
+	};
 };
 
 #define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement samples. */
-- 
2.25.1

[PATCH net-next 2/3] ptp: introduce a phase offset in the periodic output request

[Vladimir Oltean]

Some PHCs like the ocelot/felix switch cannot emit generic periodic
output, but just PPS (pulse per second) signals, which:
- don't start from arbitrary absolute times, but are rather
  phase-aligned to the beginning of [the closest next] second.
- have an optional phase offset relative to that beginning of the
  second.

For those, it was initially established that they should reject any
other absolute time for the PTP_PEROUT_REQUEST than 0.000000000 [1].

But when it actually came to writing an application [2] that makes use
of this functionality, we realized that we can't really deal generically
with PHCs that support absolute start time, and with PHCs that don't,
without an explicit interface. Namely, in an ideal world, PHC drivers
would ensure that the "perout.start" value written to hardware will
result in a functional output. This means that if the PTP time has
become in the past of this PHC's current time, it should be
automatically fast-forwarded by the driver into a close enough future
time that is known to work (note: this is necessary only if the hardware
doesn't do this fast-forward by itself). But we don't really know what
is the status for PHC drivers in use today, so in the general sense,
user space would be risking to have a non-functional periodic output if
it simply asked for a start time of 0.000000000.

So let's introduce a flag for this type of reduced-functionality
hardware, named PTP_PEROUT_PHASE. The start time is just "soon", the
only thing we know for sure about this signal is that its rising edge
events, Rn, occur at:

Rn = period.phase + n * perout.period

The "phase" in the periodic output structure is simply an alias to the
"start" time, since both cannot logically be specified at the same time.
Therefore, the binary layout of the structure is not affected.

[1]: https://patchwork.ozlabs.org/project/netdev/patch/20200320103726.32559-7-yangbo.lu@nxp.com/
[2]: https://www.mail-archive.com/linuxptp-devel@lists.sourceforge.net/msg04142.html

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 include/uapi/linux/ptp_clock.h | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 1d2841155f7d..1d108d597f66 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -55,12 +55,14 @@
  */
 #define PTP_PEROUT_ONE_SHOT		(1<<0)
 #define PTP_PEROUT_DUTY_CYCLE		(1<<1)
+#define PTP_PEROUT_PHASE		(1<<2)
 
 /*
  * flag fields valid for the new PTP_PEROUT_REQUEST2 ioctl.
  */
 #define PTP_PEROUT_VALID_FLAGS		(PTP_PEROUT_ONE_SHOT | \
-					 PTP_PEROUT_DUTY_CYCLE)
+					 PTP_PEROUT_DUTY_CYCLE | \
+					 PTP_PEROUT_PHASE)
 
 /*
  * No flags are valid for the original PTP_PEROUT_REQUEST ioctl
@@ -103,7 +105,20 @@ struct ptp_extts_request {
 };
 
 struct ptp_perout_request {
-	struct ptp_clock_time start;  /* Absolute start time. */
+	union {
+		/*
+		 * Absolute start time.
+		 * Valid only if (flags & PTP_PEROUT_PHASE) is unset.
+		 */
+		struct ptp_clock_time start;
+		/*
+		 * Phase offset. The signal should start toggling at an
+		 * unspecified integer multiple of the period, plus this value.
+		 * The start time should be "as soon as possible".
+		 * Valid only if (flags & PTP_PEROUT_PHASE) is set.
+		 */
+		struct ptp_clock_time phase;
+	};
 	struct ptp_clock_time period; /* Desired period, zero means disable. */
 	unsigned int index;           /* Which channel to configure. */
 	unsigned int flags;
-- 
2.25.1

[PATCH net-next 3/3] net: mscc: ocelot: add support for PTP waveform configuration

[Vladimir Oltean]

For PPS output (perout period is 1.000000000), accept the new "phase"
parameter from the periodic output request structure.

For both PPS and freeform output, accept the new "on" argument for
specifying the duty cycle of the generated signal. Preserve the old
defaults for this "on" time: 1 us for PPS, and half the period for
freeform output.

Also preserve the old behavior that accepted the "phase" via the "start"
argument.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/ethernet/mscc/ocelot_ptp.c | 74 +++++++++++++++++---------
 1 file changed, 50 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_ptp.c b/drivers/net/ethernet/mscc/ocelot_ptp.c
index 62188772a75d..1e08fe4daaef 100644
--- a/drivers/net/ethernet/mscc/ocelot_ptp.c
+++ b/drivers/net/ethernet/mscc/ocelot_ptp.c
@@ -184,18 +184,20 @@ int ocelot_ptp_enable(struct ptp_clock_info *ptp,
 		      struct ptp_clock_request *rq, int on)
 {
 	struct ocelot *ocelot = container_of(ptp, struct ocelot, ptp_info);
-	struct timespec64 ts_start, ts_period;
+	struct timespec64 ts_phase, ts_period;
 	enum ocelot_ptp_pins ptp_pin;
 	unsigned long flags;
 	bool pps = false;
 	int pin = -1;
+	s64 wf_high;
+	s64 wf_low;
 	u32 val;
-	s64 ns;
 
 	switch (rq->type) {
 	case PTP_CLK_REQ_PEROUT:
 		/* Reject requests with unsupported flags */
-		if (rq->perout.flags)
+		if (rq->perout.flags & ~(PTP_PEROUT_DUTY_CYCLE |
+					 PTP_PEROUT_PHASE))
 			return -EOPNOTSUPP;
 
 		pin = ptp_find_pin(ocelot->ptp_clock, PTP_PF_PEROUT,
@@ -211,22 +213,12 @@ int ocelot_ptp_enable(struct ptp_clock_info *ptp,
 		else
 			return -EBUSY;
 
-		ts_start.tv_sec = rq->perout.start.sec;
-		ts_start.tv_nsec = rq->perout.start.nsec;
 		ts_period.tv_sec = rq->perout.period.sec;
 		ts_period.tv_nsec = rq->perout.period.nsec;
 
 		if (ts_period.tv_sec == 1 && ts_period.tv_nsec == 0)
 			pps = true;
 
-		if (ts_start.tv_sec || (ts_start.tv_nsec && !pps)) {
-			dev_warn(ocelot->dev,
-				 "Absolute start time not supported!\n");
-			dev_warn(ocelot->dev,
-				 "Accept nsec for PPS phase adjustment, otherwise start time should be 0 0.\n");
-			return -EINVAL;
-		}
-
 		/* Handle turning off */
 		if (!on) {
 			spin_lock_irqsave(&ocelot->ptp_clock_lock, flags);
@@ -236,16 +228,48 @@ int ocelot_ptp_enable(struct ptp_clock_info *ptp,
 			break;
 		}
 
+		if (rq->perout.flags & PTP_PEROUT_PHASE) {
+			ts_phase.tv_sec = rq->perout.phase.sec;
+			ts_phase.tv_nsec = rq->perout.phase.nsec;
+		} else {
+			/* Compatibility */
+			ts_phase.tv_sec = rq->perout.start.sec;
+			ts_phase.tv_nsec = rq->perout.start.nsec;
+		}
+		if (ts_phase.tv_sec || (ts_phase.tv_nsec && !pps)) {
+			dev_warn(ocelot->dev,
+				 "Absolute start time not supported!\n");
+			dev_warn(ocelot->dev,
+				 "Accept nsec for PPS phase adjustment, otherwise start time should be 0 0.\n");
+			return -EINVAL;
+		}
+
+		/* Calculate waveform high and low times */
+		if (rq->perout.flags & PTP_PEROUT_DUTY_CYCLE) {
+			struct timespec64 ts_on;
+
+			ts_on.tv_sec = rq->perout.on.sec;
+			ts_on.tv_nsec = rq->perout.on.nsec;
+
+			wf_high = timespec64_to_ns(&ts_on);
+		} else {
+			if (pps) {
+				wf_high = 1000;
+			} else {
+				wf_high = timespec64_to_ns(&ts_period);
+				wf_high = div_s64(wf_high, 2);
+			}
+		}
+
+		wf_low = timespec64_to_ns(&ts_period);
+		wf_low -= wf_high;
+
 		/* Handle PPS request */
 		if (pps) {
 			spin_lock_irqsave(&ocelot->ptp_clock_lock, flags);
-			/* Pulse generated perout.start.nsec after TOD has
-			 * increased seconds.
-			 * Pulse width is set to 1us.
-			 */
-			ocelot_write_rix(ocelot, ts_start.tv_nsec,
+			ocelot_write_rix(ocelot, ts_phase.tv_nsec,
 					 PTP_PIN_WF_LOW_PERIOD, ptp_pin);
-			ocelot_write_rix(ocelot, NSEC_PER_SEC / 2,
+			ocelot_write_rix(ocelot, wf_high,
 					 PTP_PIN_WF_HIGH_PERIOD, ptp_pin);
 			val = PTP_PIN_CFG_ACTION(PTP_PIN_ACTION_CLOCK);
 			val |= PTP_PIN_CFG_SYNC;
@@ -255,14 +279,16 @@ int ocelot_ptp_enable(struct ptp_clock_info *ptp,
 		}
 
 		/* Handle periodic clock */
-		ns = timespec64_to_ns(&ts_period);
-		ns = ns >> 1;
-		if (ns > 0x3fffffff || ns <= 0x6)
+		if (wf_high > 0x3fffffff || wf_high <= 0x6)
+			return -EINVAL;
+		if (wf_low > 0x3fffffff || wf_low <= 0x6)
 			return -EINVAL;
 
 		spin_lock_irqsave(&ocelot->ptp_clock_lock, flags);
-		ocelot_write_rix(ocelot, ns, PTP_PIN_WF_LOW_PERIOD, ptp_pin);
-		ocelot_write_rix(ocelot, ns, PTP_PIN_WF_HIGH_PERIOD, ptp_pin);
+		ocelot_write_rix(ocelot, wf_low, PTP_PIN_WF_LOW_PERIOD,
+				 ptp_pin);
+		ocelot_write_rix(ocelot, wf_high, PTP_PIN_WF_HIGH_PERIOD,
+				 ptp_pin);
 		val = PTP_PIN_CFG_ACTION(PTP_PIN_ACTION_CLOCK);
 		ocelot_write_rix(ocelot, val, PTP_PIN_CFG, ptp_pin);
 		spin_unlock_irqrestore(&ocelot->ptp_clock_lock, flags);
-- 
2.25.1

Re: [PATCH net-next 0/3] Fully describe the waveform for PTP periodic output

[Jacob Keller]

On 7/16/2020 2:20 PM, Vladimir Oltean wrote:
> While using the ancillary pin functionality of PTP hardware clocks to
> synchronize multiple DSA switches on a board, a need arised to be able
> to configure the duty cycle of the master of this PPS hierarchy.
> 
> Also, the PPS master is not able to emit PPS starting from arbitrary
> absolute times, so a new flag is introduced to support such hardware
> without making guesses.
> 
> With these patches, struct ptp_perout_request now basically describes a
> general-purpose square wave.

Nice!

> 
> Vladimir Oltean (3):
>   ptp: add ability to configure duty cycle for periodic output
>   ptp: introduce a phase offset in the periodic output request
>   net: mscc: ocelot: add support for PTP waveform configuration
> 
>  drivers/net/ethernet/mscc/ocelot_ptp.c | 74 +++++++++++++++++---------
>  drivers/ptp/ptp_chardev.c              | 33 +++++++++---
>  include/uapi/linux/ptp_clock.h         | 34 ++++++++++--
>  3 files changed, 107 insertions(+), 34 deletions(-)
> 

Re: [PATCH net-next 3/3] net: mscc: ocelot: add support for PTP waveform configuration

[Vladimir Oltean]

On Fri, Jul 17, 2020 at 12:20:32AM +0300, Vladimir Oltean wrote:
> For PPS output (perout period is 1.000000000), accept the new "phase"
> parameter from the periodic output request structure.
> 
> For both PPS and freeform output, accept the new "on" argument for
> specifying the duty cycle of the generated signal. Preserve the old
> defaults for this "on" time: 1 us for PPS, and half the period for
> freeform output.
> 
> Also preserve the old behavior that accepted the "phase" via the "start"
> argument.
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  drivers/net/ethernet/mscc/ocelot_ptp.c | 74 +++++++++++++++++---------
>  1 file changed, 50 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot_ptp.c b/drivers/net/ethernet/mscc/ocelot_ptp.c
> index 62188772a75d..1e08fe4daaef 100644
> --- a/drivers/net/ethernet/mscc/ocelot_ptp.c
> +++ b/drivers/net/ethernet/mscc/ocelot_ptp.c
> @@ -184,18 +184,20 @@ int ocelot_ptp_enable(struct ptp_clock_info *ptp,
>  		      struct ptp_clock_request *rq, int on)
>  {
>  	struct ocelot *ocelot = container_of(ptp, struct ocelot, ptp_info);
> -	struct timespec64 ts_start, ts_period;
> +	struct timespec64 ts_phase, ts_period;
>  	enum ocelot_ptp_pins ptp_pin;
>  	unsigned long flags;
>  	bool pps = false;
>  	int pin = -1;
> +	s64 wf_high;
> +	s64 wf_low;
>  	u32 val;
> -	s64 ns;
>  
>  	switch (rq->type) {
>  	case PTP_CLK_REQ_PEROUT:
>  		/* Reject requests with unsupported flags */
> -		if (rq->perout.flags)
> +		if (rq->perout.flags & ~(PTP_PEROUT_DUTY_CYCLE |
> +					 PTP_PEROUT_PHASE))
>  			return -EOPNOTSUPP;
>  
>  		pin = ptp_find_pin(ocelot->ptp_clock, PTP_PF_PEROUT,
> @@ -211,22 +213,12 @@ int ocelot_ptp_enable(struct ptp_clock_info *ptp,
>  		else
>  			return -EBUSY;
>  
> -		ts_start.tv_sec = rq->perout.start.sec;
> -		ts_start.tv_nsec = rq->perout.start.nsec;
>  		ts_period.tv_sec = rq->perout.period.sec;
>  		ts_period.tv_nsec = rq->perout.period.nsec;
>  
>  		if (ts_period.tv_sec == 1 && ts_period.tv_nsec == 0)
>  			pps = true;
>  
> -		if (ts_start.tv_sec || (ts_start.tv_nsec && !pps)) {
> -			dev_warn(ocelot->dev,
> -				 "Absolute start time not supported!\n");
> -			dev_warn(ocelot->dev,
> -				 "Accept nsec for PPS phase adjustment, otherwise start time should be 0 0.\n");
> -			return -EINVAL;
> -		}
> -
>  		/* Handle turning off */
>  		if (!on) {
>  			spin_lock_irqsave(&ocelot->ptp_clock_lock, flags);
> @@ -236,16 +228,48 @@ int ocelot_ptp_enable(struct ptp_clock_info *ptp,
>  			break;
>  		}
>  
> +		if (rq->perout.flags & PTP_PEROUT_PHASE) {
> +			ts_phase.tv_sec = rq->perout.phase.sec;
> +			ts_phase.tv_nsec = rq->perout.phase.nsec;
> +		} else {
> +			/* Compatibility */
> +			ts_phase.tv_sec = rq->perout.start.sec;
> +			ts_phase.tv_nsec = rq->perout.start.nsec;
> +		}
> +		if (ts_phase.tv_sec || (ts_phase.tv_nsec && !pps)) {
> +			dev_warn(ocelot->dev,
> +				 "Absolute start time not supported!\n");
> +			dev_warn(ocelot->dev,
> +				 "Accept nsec for PPS phase adjustment, otherwise start time should be 0 0.\n");
> +			return -EINVAL;
> +		}
> +
> +		/* Calculate waveform high and low times */
> +		if (rq->perout.flags & PTP_PEROUT_DUTY_CYCLE) {
> +			struct timespec64 ts_on;
> +
> +			ts_on.tv_sec = rq->perout.on.sec;
> +			ts_on.tv_nsec = rq->perout.on.nsec;
> +
> +			wf_high = timespec64_to_ns(&ts_on);
> +		} else {
> +			if (pps) {
> +				wf_high = 1000;
> +			} else {
> +				wf_high = timespec64_to_ns(&ts_period);
> +				wf_high = div_s64(wf_high, 2);
> +			}
> +		}
> +
> +		wf_low = timespec64_to_ns(&ts_period);
> +		wf_low -= wf_high;
> +
>  		/* Handle PPS request */
>  		if (pps) {
>  			spin_lock_irqsave(&ocelot->ptp_clock_lock, flags);
> -			/* Pulse generated perout.start.nsec after TOD has
> -			 * increased seconds.
> -			 * Pulse width is set to 1us.
> -			 */
> -			ocelot_write_rix(ocelot, ts_start.tv_nsec,
> +			ocelot_write_rix(ocelot, ts_phase.tv_nsec,
>  					 PTP_PIN_WF_LOW_PERIOD, ptp_pin);
> -			ocelot_write_rix(ocelot, NSEC_PER_SEC / 2,

Damn. I had this patch in my tree:

diff --git a/drivers/net/ethernet/mscc/ocelot_ptp.c b/drivers/net/ethernet/mscc/ocelot_ptp.c
index a3088a1676ed..62188772a75d 100644
--- a/drivers/net/ethernet/mscc/ocelot_ptp.c
+++ b/drivers/net/ethernet/mscc/ocelot_ptp.c
@@ -245,7 +245,7 @@ int ocelot_ptp_enable(struct ptp_clock_info *ptp,
 			 */
 			ocelot_write_rix(ocelot, ts_start.tv_nsec,
 					 PTP_PIN_WF_LOW_PERIOD, ptp_pin);
-			ocelot_write_rix(ocelot, 1000,
+			ocelot_write_rix(ocelot, NSEC_PER_SEC / 2,
 					 PTP_PIN_WF_HIGH_PERIOD, ptp_pin);
 			val = PTP_PIN_CFG_ACTION(PTP_PIN_ACTION_CLOCK);
 			val |= PTP_PIN_CFG_SYNC;

which I used for testing until I exposed this into an ioctl. I knew I
forgot to do something, and that was to squash that patch into this one.
So I'll need to send a v2, because otherwise this doesn't apply to
mainline. Please review taking this into consideration.

> +			ocelot_write_rix(ocelot, wf_high,
>  					 PTP_PIN_WF_HIGH_PERIOD, ptp_pin);
>  			val = PTP_PIN_CFG_ACTION(PTP_PIN_ACTION_CLOCK);
>  			val |= PTP_PIN_CFG_SYNC;
> @@ -255,14 +279,16 @@ int ocelot_ptp_enable(struct ptp_clock_info *ptp,
>  		}
>  
>  		/* Handle periodic clock */
> -		ns = timespec64_to_ns(&ts_period);
> -		ns = ns >> 1;
> -		if (ns > 0x3fffffff || ns <= 0x6)
> +		if (wf_high > 0x3fffffff || wf_high <= 0x6)
> +			return -EINVAL;
> +		if (wf_low > 0x3fffffff || wf_low <= 0x6)
>  			return -EINVAL;
>  
>  		spin_lock_irqsave(&ocelot->ptp_clock_lock, flags);
> -		ocelot_write_rix(ocelot, ns, PTP_PIN_WF_LOW_PERIOD, ptp_pin);
> -		ocelot_write_rix(ocelot, ns, PTP_PIN_WF_HIGH_PERIOD, ptp_pin);
> +		ocelot_write_rix(ocelot, wf_low, PTP_PIN_WF_LOW_PERIOD,
> +				 ptp_pin);
> +		ocelot_write_rix(ocelot, wf_high, PTP_PIN_WF_HIGH_PERIOD,
> +				 ptp_pin);
>  		val = PTP_PIN_CFG_ACTION(PTP_PIN_ACTION_CLOCK);
>  		ocelot_write_rix(ocelot, val, PTP_PIN_CFG, ptp_pin);
>  		spin_unlock_irqrestore(&ocelot->ptp_clock_lock, flags);
> -- 
> 2.25.1
> 

Thanks,
-Vladimir

Re: [PATCH net-next 1/3] ptp: add ability to configure duty cycle for periodic output

[Jacob Keller]

On 7/16/2020 2:20 PM, Vladimir Oltean wrote:
> There are external event timestampers (PHCs with support for
> PTP_EXTTS_REQUEST) that timestamp both event edges.
> 
> When those edges are very close (such as in the case of a short pulse),
> there is a chance that the collected timestamp might be of the rising,
> or of the falling edge, we never know.
> 
> There are also PHCs capable of generating periodic output with a
> configurable duty cycle. This is good news, because we can space the
> rising and falling edge out enough in time, that the risks to overrun
> the 1-entry timestamp FIFO of the extts PHC are lower (example: the
> perout PHC can be configured for a period of 1 second, and an "on" time
> of 0.5 seconds, resulting in a duty cycle of 50%).
> 
> A flag is introduced for signaling that an on time is present in the
> perout request structure, for preserving compatibility. Logically
> speaking, the duty cycle cannot exceed 100% and the PTP core checks for
> this.

I was thinking whether it made sense to support over 50% since in theory
you could change start time and the duty cycle to specify the shifted
wave over? but I guess it doesn't really make much of a difference to
support all the way up to 100%.

> 
> PHC drivers that don't support this flag emit a periodic output of an
> unspecified duty cycle, same as before.
> 
> The duty cycle is encoded as an "on" time, similar to the "start" and
> "period" times, and reuses the reserved space while preserving overall
> binary layout.
> 
> Pahole reported before:
> 
> struct ptp_perout_request {
>         struct ptp_clock_time start;                     /*     0    16 */
>         struct ptp_clock_time period;                    /*    16    16 */
>         unsigned int               index;                /*    32     4 */
>         unsigned int               flags;                /*    36     4 */
>         unsigned int               rsv[4];               /*    40    16 */
> 
>         /* size: 56, cachelines: 1, members: 5 */
>         /* last cacheline: 56 bytes */
> };
> 
> And now:
> 
> struct ptp_perout_request {
>         struct ptp_clock_time start;                     /*     0    16 */
>         struct ptp_clock_time period;                    /*    16    16 */
>         unsigned int               index;                /*    32     4 */
>         unsigned int               flags;                /*    36     4 */
>         union {
>                 struct ptp_clock_time on;                /*    40    16 */
>                 unsigned int       rsv[4];               /*    40    16 */
>         };                                               /*    40    16 */
> 
>         /* size: 56, cachelines: 1, members: 5 */
>         /* last cacheline: 56 bytes */
> };
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  drivers/ptp/ptp_chardev.c      | 33 +++++++++++++++++++++++++++------
>  include/uapi/linux/ptp_clock.h | 17 ++++++++++++++---
>  2 files changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 375cd6e4aade..e0e6f85966e1 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -191,12 +191,33 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			err = -EFAULT;
>  			break;
>  		}
> -		if (((req.perout.flags & ~PTP_PEROUT_VALID_FLAGS) ||
> -			req.perout.rsv[0] || req.perout.rsv[1] ||
> -			req.perout.rsv[2] || req.perout.rsv[3]) &&
> -			cmd == PTP_PEROUT_REQUEST2) {
> -			err = -EINVAL;
> -			break;
> +		if (cmd == PTP_PEROUT_REQUEST2) {
> +			struct ptp_perout_request *perout = &req.perout;
> +
> +			if (perout->flags & ~PTP_PEROUT_VALID_FLAGS) {
> +				err = -EINVAL;
> +				break;
> +			}
> +			/*
> +			 * The "on" field has undefined meaning if
> +			 * PTP_PEROUT_DUTY_CYCLE isn't set, we must still treat
> +			 * it as reserved, which must be set to zero.
> +			 */
> +			if (!(perout->flags & PTP_PEROUT_DUTY_CYCLE) &&
> +			    (perout->rsv[0] || perout->rsv[1] ||
> +			     perout->rsv[2] || perout->rsv[3])) {
> +				err = -EINVAL;
> +				break;
> +			}
> +			if (perout->flags & PTP_PEROUT_DUTY_CYCLE) {
> +				/* The duty cycle must be subunitary. */

I'm sure this means "smaller than the period" but I can't help thinking
just spelling that out would be clearer.

> +				if (perout->on.sec > perout->period.sec ||
> +				    (perout->on.sec == perout->period.sec &&
> +				     perout->on.nsec > perout->period.nsec)) {
> +					err = -ERANGE;
> +					break;
> +				}
> +			}
>  		} else if (cmd == PTP_PEROUT_REQUEST) {
>  			req.perout.flags &= PTP_PEROUT_V1_VALID_FLAGS;
>  			req.perout.rsv[0] = 0;
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index ff070aa64278..1d2841155f7d 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -53,12 +53,14 @@
>  /*
>   * Bits of the ptp_perout_request.flags field:
>   */
> -#define PTP_PEROUT_ONE_SHOT (1<<0)
> +#define PTP_PEROUT_ONE_SHOT		(1<<0)
> +#define PTP_PEROUT_DUTY_CYCLE		(1<<1)
>  
>  /*
>   * flag fields valid for the new PTP_PEROUT_REQUEST2 ioctl.
>   */
> -#define PTP_PEROUT_VALID_FLAGS	(PTP_PEROUT_ONE_SHOT)
> +#define PTP_PEROUT_VALID_FLAGS		(PTP_PEROUT_ONE_SHOT | \
> +					 PTP_PEROUT_DUTY_CYCLE)
>  
>  /*
>   * No flags are valid for the original PTP_PEROUT_REQUEST ioctl
> @@ -105,7 +107,16 @@ struct ptp_perout_request {
>  	struct ptp_clock_time period; /* Desired period, zero means disable. */
>  	unsigned int index;           /* Which channel to configure. */
>  	unsigned int flags;
> -	unsigned int rsv[4];          /* Reserved for future use. */
> +	union {
> +		/*
> +		 * The "on" time of the signal.
> +		 * Must be lower than the period.
> +		 * Valid only if (flags & PTP_PEROUT_DUTY_CYCLE) is set.
> +		 */
> +		struct ptp_clock_time on;
> +		/* Reserved for future use. */
> +		unsigned int rsv[4];
> +	};

Hmmm. So the idea is that if PTP_PEROUT_DUTY_CYCLE is not set, then we
keep this as reserved and then if it *is* set we allow it to be the "on"
time?

Is it possible for us to still use the reserved bits for another
purpose? Or should we just remove it entirely and leave only the "on"
timestamp. Any future extension would by definition *have* to be
exclusive with PTP_PEROUT_DUTY_CYCLE if it wants to use these reserved
fields anyways...

>  };
>  
>  #define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement samples. */
> 

Re: [PATCH net-next 2/3] ptp: introduce a phase offset in the periodic output request

[Jacob Keller]

On 7/16/2020 2:20 PM, Vladimir Oltean wrote:
> Some PHCs like the ocelot/felix switch cannot emit generic periodic
> output, but just PPS (pulse per second) signals, which:
> - don't start from arbitrary absolute times, but are rather
>   phase-aligned to the beginning of [the closest next] second.
> - have an optional phase offset relative to that beginning of the
>   second.
> 
> For those, it was initially established that they should reject any
> other absolute time for the PTP_PEROUT_REQUEST than 0.000000000 [1].
> 
> But when it actually came to writing an application [2] that makes use
> of this functionality, we realized that we can't really deal generically
> with PHCs that support absolute start time, and with PHCs that don't,
> without an explicit interface. Namely, in an ideal world, PHC drivers
> would ensure that the "perout.start" value written to hardware will
> result in a functional output. This means that if the PTP time has
> become in the past of this PHC's current time, it should be
> automatically fast-forwarded by the driver into a close enough future
> time that is known to work (note: this is necessary only if the hardware
> doesn't do this fast-forward by itself). But we don't really know what
> is the status for PHC drivers in use today, so in the general sense,
> user space would be risking to have a non-functional periodic output if
> it simply asked for a start time of 0.000000000.
> 
> So let's introduce a flag for this type of reduced-functionality
> hardware, named PTP_PEROUT_PHASE. The start time is just "soon", the
> only thing we know for sure about this signal is that its rising edge
> events, Rn, occur at:
> 
> Rn = period.phase + n * perout.period
> 
> The "phase" in the periodic output structure is simply an alias to the
> "start" time, since both cannot logically be specified at the same time.
> Therefore, the binary layout of the structure is not affected.
> 
> [1]: https://patchwork.ozlabs.org/project/netdev/patch/20200320103726.32559-7-yangbo.lu@nxp.com/
> [2]: https://www.mail-archive.com/linuxptp-devel@lists.sourceforge.net/msg04142.html
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  include/uapi/linux/ptp_clock.h | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 1d2841155f7d..1d108d597f66 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -55,12 +55,14 @@
>   */
>  #define PTP_PEROUT_ONE_SHOT		(1<<0)
>  #define PTP_PEROUT_DUTY_CYCLE		(1<<1)
> +#define PTP_PEROUT_PHASE		(1<<2)
>  
>  /*
>   * flag fields valid for the new PTP_PEROUT_REQUEST2 ioctl.
>   */
>  #define PTP_PEROUT_VALID_FLAGS		(PTP_PEROUT_ONE_SHOT | \
> -					 PTP_PEROUT_DUTY_CYCLE)
> +					 PTP_PEROUT_DUTY_CYCLE | \
> +					 PTP_PEROUT_PHASE)
>  
>  /*
>   * No flags are valid for the original PTP_PEROUT_REQUEST ioctl
> @@ -103,7 +105,20 @@ struct ptp_extts_request {
>  };
>  
>  struct ptp_perout_request {
> -	struct ptp_clock_time start;  /* Absolute start time. */
> +	union {
> +		/*
> +		 * Absolute start time.
> +		 * Valid only if (flags & PTP_PEROUT_PHASE) is unset.
> +		 */
> +		struct ptp_clock_time start;
> +		/*
> +		 * Phase offset. The signal should start toggling at an
> +		 * unspecified integer multiple of the period, plus this value.
> +		 * The start time should be "as soon as possible".
> +		 * Valid only if (flags & PTP_PEROUT_PHASE) is set.
> +		 */
> +		struct ptp_clock_time phase;
> +	};

Ok. Since when using the PHASE mode the start time is "meaningless" we
can re-use it for this purpose without breaking the binary structure.
Makes sense.

>  	struct ptp_clock_time period; /* Desired period, zero means disable. */
>  	unsigned int index;           /* Which channel to configure. */
>  	unsigned int flags;
> 

Re: [PATCH net-next 1/3] ptp: add ability to configure duty cycle for periodic output

[Vladimir Oltean]

On Thu, Jul 16, 2020 at 02:36:45PM -0700, Jacob Keller wrote:
> 
> 
> On 7/16/2020 2:20 PM, Vladimir Oltean wrote:
> > There are external event timestampers (PHCs with support for
> > PTP_EXTTS_REQUEST) that timestamp both event edges.
> > 
> > When those edges are very close (such as in the case of a short pulse),
> > there is a chance that the collected timestamp might be of the rising,
> > or of the falling edge, we never know.
> > 
> > There are also PHCs capable of generating periodic output with a
> > configurable duty cycle. This is good news, because we can space the
> > rising and falling edge out enough in time, that the risks to overrun
> > the 1-entry timestamp FIFO of the extts PHC are lower (example: the
> > perout PHC can be configured for a period of 1 second, and an "on" time
> > of 0.5 seconds, resulting in a duty cycle of 50%).
> > 
> > A flag is introduced for signaling that an on time is present in the
> > perout request structure, for preserving compatibility. Logically
> > speaking, the duty cycle cannot exceed 100% and the PTP core checks for
> > this.
> 
> I was thinking whether it made sense to support over 50% since in theory
> you could change start time and the duty cycle to specify the shifted
> wave over? but I guess it doesn't really make much of a difference to
> support all the way up to 100%.
> 

Only if you also support polarity, and we don't support polarity. It's
always high first, then low.

   +------+  +------+  +------+  +------+  +------+  +------+  +------+
   |      |  |      |  |      |  |      |  |      |  |      |  |      |
 --+      +--+      +--+      +--+      +--+      +--+      +--+      +

 +---------+---------+---------+---------+---------+---------+--------->
 period=10                                                          time
 phase=2
 on = 7

There's no other way to obtain this signal which has a duty cycle > 50%
by specifying a duty cycle < 50%.

> > 
> > PHC drivers that don't support this flag emit a periodic output of an
> > unspecified duty cycle, same as before.
> > 
> > The duty cycle is encoded as an "on" time, similar to the "start" and
> > "period" times, and reuses the reserved space while preserving overall
> > binary layout.
> > 
> > Pahole reported before:
> > 
> > struct ptp_perout_request {
> >         struct ptp_clock_time start;                     /*     0    16 */
> >         struct ptp_clock_time period;                    /*    16    16 */
> >         unsigned int               index;                /*    32     4 */
> >         unsigned int               flags;                /*    36     4 */
> >         unsigned int               rsv[4];               /*    40    16 */
> > 
> >         /* size: 56, cachelines: 1, members: 5 */
> >         /* last cacheline: 56 bytes */
> > };
> > 
> > And now:
> > 
> > struct ptp_perout_request {
> >         struct ptp_clock_time start;                     /*     0    16 */
> >         struct ptp_clock_time period;                    /*    16    16 */
> >         unsigned int               index;                /*    32     4 */
> >         unsigned int               flags;                /*    36     4 */
> >         union {
> >                 struct ptp_clock_time on;                /*    40    16 */
> >                 unsigned int       rsv[4];               /*    40    16 */
> >         };                                               /*    40    16 */
> > 
> >         /* size: 56, cachelines: 1, members: 5 */
> >         /* last cacheline: 56 bytes */
> > };
> > 
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
> >  drivers/ptp/ptp_chardev.c      | 33 +++++++++++++++++++++++++++------
> >  include/uapi/linux/ptp_clock.h | 17 ++++++++++++++---
> >  2 files changed, 41 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> > index 375cd6e4aade..e0e6f85966e1 100644
> > --- a/drivers/ptp/ptp_chardev.c
> > +++ b/drivers/ptp/ptp_chardev.c
> > @@ -191,12 +191,33 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> >  			err = -EFAULT;
> >  			break;
> >  		}
> > -		if (((req.perout.flags & ~PTP_PEROUT_VALID_FLAGS) ||
> > -			req.perout.rsv[0] || req.perout.rsv[1] ||
> > -			req.perout.rsv[2] || req.perout.rsv[3]) &&
> > -			cmd == PTP_PEROUT_REQUEST2) {
> > -			err = -EINVAL;
> > -			break;
> > +		if (cmd == PTP_PEROUT_REQUEST2) {
> > +			struct ptp_perout_request *perout = &req.perout;
> > +
> > +			if (perout->flags & ~PTP_PEROUT_VALID_FLAGS) {
> > +				err = -EINVAL;
> > +				break;
> > +			}
> > +			/*
> > +			 * The "on" field has undefined meaning if
> > +			 * PTP_PEROUT_DUTY_CYCLE isn't set, we must still treat
> > +			 * it as reserved, which must be set to zero.
> > +			 */
> > +			if (!(perout->flags & PTP_PEROUT_DUTY_CYCLE) &&
> > +			    (perout->rsv[0] || perout->rsv[1] ||
> > +			     perout->rsv[2] || perout->rsv[3])) {
> > +				err = -EINVAL;
> > +				break;
> > +			}
> > +			if (perout->flags & PTP_PEROUT_DUTY_CYCLE) {
> > +				/* The duty cycle must be subunitary. */
> 
> I'm sure this means "smaller than the period" but I can't help thinking
> just spelling that out would be clearer.
> 

Duty cycle is by definition a fraction. In my example above, on/period
is 70%, or 0.7. So it is not incorrect to say that the duty cycle is
subunitary. The alternative phrasing would be that the on time must be
lower than the period, and I've used that already in the header file. I
was just trying to avoid repetition.

> > +				if (perout->on.sec > perout->period.sec ||
> > +				    (perout->on.sec == perout->period.sec &&
> > +				     perout->on.nsec > perout->period.nsec)) {
> > +					err = -ERANGE;
> > +					break;
> > +				}
> > +			}
> >  		} else if (cmd == PTP_PEROUT_REQUEST) {
> >  			req.perout.flags &= PTP_PEROUT_V1_VALID_FLAGS;
> >  			req.perout.rsv[0] = 0;
> > diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> > index ff070aa64278..1d2841155f7d 100644
> > --- a/include/uapi/linux/ptp_clock.h
> > +++ b/include/uapi/linux/ptp_clock.h
> > @@ -53,12 +53,14 @@
> >  /*
> >   * Bits of the ptp_perout_request.flags field:
> >   */
> > -#define PTP_PEROUT_ONE_SHOT (1<<0)
> > +#define PTP_PEROUT_ONE_SHOT		(1<<0)
> > +#define PTP_PEROUT_DUTY_CYCLE		(1<<1)
> >  
> >  /*
> >   * flag fields valid for the new PTP_PEROUT_REQUEST2 ioctl.
> >   */
> > -#define PTP_PEROUT_VALID_FLAGS	(PTP_PEROUT_ONE_SHOT)
> > +#define PTP_PEROUT_VALID_FLAGS		(PTP_PEROUT_ONE_SHOT | \
> > +					 PTP_PEROUT_DUTY_CYCLE)
> >  
> >  /*
> >   * No flags are valid for the original PTP_PEROUT_REQUEST ioctl
> > @@ -105,7 +107,16 @@ struct ptp_perout_request {
> >  	struct ptp_clock_time period; /* Desired period, zero means disable. */
> >  	unsigned int index;           /* Which channel to configure. */
> >  	unsigned int flags;
> > -	unsigned int rsv[4];          /* Reserved for future use. */
> > +	union {
> > +		/*
> > +		 * The "on" time of the signal.
> > +		 * Must be lower than the period.
> > +		 * Valid only if (flags & PTP_PEROUT_DUTY_CYCLE) is set.
> > +		 */
> > +		struct ptp_clock_time on;
> > +		/* Reserved for future use. */
> > +		unsigned int rsv[4];
> > +	};
> 
> Hmmm. So the idea is that if PTP_PEROUT_DUTY_CYCLE is not set, then we
> keep this as reserved and then if it *is* set we allow it to be the "on"
> time?
> 
> Is it possible for us to still use the reserved bits for another
> purpose? Or should we just remove it entirely and leave only the "on"
> timestamp. Any future extension would by definition *have* to be
> exclusive with PTP_PEROUT_DUTY_CYCLE if it wants to use these reserved
> fields anyways...
> 

You're right about the mutual exclusion, but I can't predict the future
and I don't know if the reserved field is going to be practically useful
or not.

There is one subtlety though, and that is that we have been exposing to
user space, previously, a structure with a field named "rsv" in it. So,
application writers may have been accessing that "rsv" field, to memset
it to zero, for instance. It wouldn't be nice if it disappeared, it
would break their code.

> >  };
> >  
> >  #define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement samples. */
> > 

Thanks,
-Vladimir

Re: [PATCH net-next 1/3] ptp: add ability to configure duty cycle for periodic output

[Vladimir Oltean]

On Fri, Jul 17, 2020 at 12:49:27AM +0300, Vladimir Oltean wrote:
> On Thu, Jul 16, 2020 at 02:36:45PM -0700, Jacob Keller wrote:
> > 
> > 
> > On 7/16/2020 2:20 PM, Vladimir Oltean wrote:
> > > There are external event timestampers (PHCs with support for
> > > PTP_EXTTS_REQUEST) that timestamp both event edges.
> > > 
> > > When those edges are very close (such as in the case of a short pulse),
> > > there is a chance that the collected timestamp might be of the rising,
> > > or of the falling edge, we never know.
> > > 
> > > There are also PHCs capable of generating periodic output with a
> > > configurable duty cycle. This is good news, because we can space the
> > > rising and falling edge out enough in time, that the risks to overrun
> > > the 1-entry timestamp FIFO of the extts PHC are lower (example: the
> > > perout PHC can be configured for a period of 1 second, and an "on" time
> > > of 0.5 seconds, resulting in a duty cycle of 50%).
> > > 
> > > A flag is introduced for signaling that an on time is present in the
> > > perout request structure, for preserving compatibility. Logically
> > > speaking, the duty cycle cannot exceed 100% and the PTP core checks for
> > > this.
> > 
> > I was thinking whether it made sense to support over 50% since in theory
> > you could change start time and the duty cycle to specify the shifted
> > wave over? but I guess it doesn't really make much of a difference to
> > support all the way up to 100%.
> > 
> 
> Only if you also support polarity, and we don't support polarity. It's
> always high first, then low.
> 

Sorry for the imprecise statement.
If you look at things from the perspective of the signal itself, the
statement is correct.
If you look at them from the perspective of the imaginary grid drawn by
the integer multiples of the period, in the PHC's time (a digital
counter), the correct statement would be that "it's always rising edge
first, then falling edge".  And then the phase is just the delta between
these 2 points of reference.

Let me annotate this:

     t_on
     <------>
     t_period
     <--------->
phase
   <->
>    +------+  +------+  +------+  +------+  +------+  +------+  +------+
>    |      |  |      |  |      |  |      |  |      |  |      |  |      |
>  --+      +--+      +--+      +--+      +--+      +--+      +--+      +
> 
>  +---------+---------+---------+---------+---------+---------+--------->
   t=1000    t=1010    t=1020    t=1030    t=1040    t=1050    t=1060
>  period=10                                                          time
>  phase=2
>  on = 7
> 
> There's no other way to obtain this signal which has a duty cycle > 50%
> by specifying a duty cycle < 50%.
> 

Thanks,
-Vladimir

Re: [PATCH net-next 0/3] Fully describe the waveform for PTP periodic output

[Jakub Kicinski]

On Fri, 17 Jul 2020 00:20:29 +0300 Vladimir Oltean wrote:
> While using the ancillary pin functionality of PTP hardware clocks to
> synchronize multiple DSA switches on a board, a need arised to be able
> to configure the duty cycle of the master of this PPS hierarchy.
> 
> Also, the PPS master is not able to emit PPS starting from arbitrary
> absolute times, so a new flag is introduced to support such hardware
> without making guesses.
> 
> With these patches, struct ptp_perout_request now basically describes a
> general-purpose square wave.

error: patch failed: drivers/net/ethernet/mscc/ocelot_ptp.c:236
error: drivers/net/ethernet/mscc/ocelot_ptp.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: ptp: add ability to configure duty cycle for periodic output
Applying: ptp: introduce a phase offset in the periodic output request
Applying: net: mscc: ocelot: add support for PTP waveform configuration
Patch failed at 0003 net: mscc: ocelot: add support for PTP waveform configuration
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [PATCH net-next 3/3] net: mscc: ocelot: add support for PTP waveform configuration

[Jakub Kicinski]

On Fri, 17 Jul 2020 00:36:22 +0300 Vladimir Oltean wrote:
> which I used for testing until I exposed this into an ioctl. I knew I
> forgot to do something, and that was to squash that patch into this one.
> So I'll need to send a v2, because otherwise this doesn't apply to
> mainline. Please review taking this into consideration.

Ah, only noticed this now.

Re: [PATCH net-next 1/3] ptp: add ability to configure duty cycle for periodic output

[Jacob Keller]

On 7/16/2020 3:09 PM, Vladimir Oltean wrote:
> On Fri, Jul 17, 2020 at 12:49:27AM +0300, Vladimir Oltean wrote:
>> On Thu, Jul 16, 2020 at 02:36:45PM -0700, Jacob Keller wrote:
>>>
>>>
>>> On 7/16/2020 2:20 PM, Vladimir Oltean wrote:
>>>> There are external event timestampers (PHCs with support for
>>>> PTP_EXTTS_REQUEST) that timestamp both event edges.
>>>>
>>>> When those edges are very close (such as in the case of a short pulse),
>>>> there is a chance that the collected timestamp might be of the rising,
>>>> or of the falling edge, we never know.
>>>>
>>>> There are also PHCs capable of generating periodic output with a
>>>> configurable duty cycle. This is good news, because we can space the
>>>> rising and falling edge out enough in time, that the risks to overrun
>>>> the 1-entry timestamp FIFO of the extts PHC are lower (example: the
>>>> perout PHC can be configured for a period of 1 second, and an "on" time
>>>> of 0.5 seconds, resulting in a duty cycle of 50%).
>>>>
>>>> A flag is introduced for signaling that an on time is present in the
>>>> perout request structure, for preserving compatibility. Logically
>>>> speaking, the duty cycle cannot exceed 100% and the PTP core checks for
>>>> this.
>>>
>>> I was thinking whether it made sense to support over 50% since in theory
>>> you could change start time and the duty cycle to specify the shifted
>>> wave over? but I guess it doesn't really make much of a difference to
>>> support all the way up to 100%.
>>>
>>
>> Only if you also support polarity, and we don't support polarity. It's
>> always high first, then low.
>>
> 
> Sorry for the imprecise statement.
> If you look at things from the perspective of the signal itself, the
> statement is correct.
> If you look at them from the perspective of the imaginary grid drawn by
> the integer multiples of the period, in the PHC's time (a digital
> counter), the correct statement would be that "it's always rising edge
> first, then falling edge".  And then the phase is just the delta between
> these 2 points of reference.
> 
> Let me annotate this:
> 
>      t_on
>      <------>
>      t_period
>      <--------->
> phase
>    <->
>>    +------+  +------+  +------+  +------+  +------+  +------+  +------+
>>    |      |  |      |  |      |  |      |  |      |  |      |  |      |
>>  --+      +--+      +--+      +--+      +--+      +--+      +--+      +
>>
>>  +---------+---------+---------+---------+---------+---------+--------->
>    t=1000    t=1010    t=1020    t=1030    t=1040    t=1050    t=1060
>>  period=10                                                          time
>>  phase=2
>>  on = 7
>>
>> There's no other way to obtain this signal which has a duty cycle > 50%
>> by specifying a duty cycle < 50%.
>>
> 
> Thanks,
> -Vladimir
> 

Right this makes sense now, thanks for the detailed explanation!

Regards,
Jake