[PATCH net] net: dsa: sja1105: the PTP_CLK extts input reacts on both edges

[PATCH net] net: dsa: sja1105: the PTP_CLK extts input reacts on both edges

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

It looks like the sja1105 external timestamping input is not as generic
as we thought. When fed a signal with 50% duty cycle, it will timestamp
both the rising and the falling edge. When fed a short pulse signal,
only the timestamp of the falling edge will be seen in the PTPSYNCTS
register, because that of the rising edge had been overwritten. So the
moral is: don't feed it short pulse inputs.

Luckily this is not a complete deal breaker, as we can still work with
1 Hz square waves. But the problem is that the extts polling period was
not dimensioned enough for this input signal. If we leave the period at
half a second, we risk losing timestamps due to jitter in the measuring
process. So we need to increase it to 4 times per second.

Also, the very least we can do to inform the user is to deny any other
flags combination than with PTP_RISING_EDGE and PTP_FALLING_EDGE both
set.

Fixes: 747e5eb31d59 ("net: dsa: sja1105: configure the PTP_CLK pin as EXT_TS or PER_OUT")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_ptp.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index a22f8e3fc06b..bc0e47c1dbb9 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -16,14 +16,15 @@
 
 /* PTPSYNCTS has no interrupt or update mechanism, because the intended
  * hardware use case is for the timestamp to be collected synchronously,
- * immediately after the CAS_MASTER SJA1105 switch has triggered a CASSYNC
- * pulse on the PTP_CLK pin. When used as a generic extts source, it needs
- * polling and a comparison with the old value. The polling interval is just
- * the Nyquist rate of a canonical PPS input (e.g. from a GPS module).
- * Anything of higher frequency than 1 Hz will be lost, since there is no
- * timestamp FIFO.
+ * immediately after the CAS_MASTER SJA1105 switch has performed a CASSYNC
+ * one-shot toggle (no return to level) on the PTP_CLK pin. When used as a
+ * generic extts source, the PTPSYNCTS register needs polling and a comparison
+ * with the old value. The polling interval is configured as the Nyquist rate
+ * of a signal with 50% duty cycle and 1Hz frequency, which is sadly all that
+ * this hardware can do (but may be enough for some setups). Anything of higher
+ * frequency than 1 Hz will be lost, since there is no timestamp FIFO.
  */
-#define SJA1105_EXTTS_INTERVAL		(HZ / 2)
+#define SJA1105_EXTTS_INTERVAL		(HZ / 4)
 
 /*            This range is actually +/- SJA1105_MAX_ADJ_PPB
  *            divided by 1000 (ppb -> ppm) and with a 16-bit
@@ -754,7 +755,16 @@ static int sja1105_extts_enable(struct sja1105_private *priv,
 		return -EOPNOTSUPP;
 
 	/* Reject requests with unsupported flags */
-	if (extts->flags)
+	if (extts->flags & ~(PTP_ENABLE_FEATURE |
+			     PTP_RISING_EDGE |
+			     PTP_FALLING_EDGE |
+			     PTP_STRICT_FLAGS))
+		return -EOPNOTSUPP;
+
+	/* We can only enable time stamping on both edges, sadly. */
+	if ((extts->flags & PTP_STRICT_FLAGS) &&
+	    (extts->flags & PTP_ENABLE_FEATURE) &&
+	    (extts->flags & PTP_EXTTS_EDGES) != PTP_EXTTS_EDGES)
 		return -EOPNOTSUPP;
 
 	rc = sja1105_change_ptp_clk_pin_func(priv, PTP_PF_EXTTS);
-- 
2.17.1

Re: [PATCH net] net: dsa: sja1105: the PTP_CLK extts input reacts on both edges

[Richard Cochran]

On Wed, May 06, 2020 at 08:48:13PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> It looks like the sja1105 external timestamping input is not as generic
> as we thought. When fed a signal with 50% duty cycle, it will timestamp
> both the rising and the falling edge. When fed a short pulse signal,
> only the timestamp of the falling edge will be seen in the PTPSYNCTS
> register, because that of the rising edge had been overwritten. So the
> moral is: don't feed it short pulse inputs.
> 
> Luckily this is not a complete deal breaker, as we can still work with
> 1 Hz square waves. But the problem is that the extts polling period was
> not dimensioned enough for this input signal. If we leave the period at
> half a second, we risk losing timestamps due to jitter in the measuring
> process. So we need to increase it to 4 times per second.
> 
> Also, the very least we can do to inform the user is to deny any other
> flags combination than with PTP_RISING_EDGE and PTP_FALLING_EDGE both
> set.
> 
> Fixes: 747e5eb31d59 ("net: dsa: sja1105: configure the PTP_CLK pin as EXT_TS or PER_OUT")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH net] net: dsa: sja1105: the PTP_CLK extts input reacts on both edges

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Wed,  6 May 2020 20:48:13 +0300

> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> It looks like the sja1105 external timestamping input is not as generic
> as we thought. When fed a signal with 50% duty cycle, it will timestamp
> both the rising and the falling edge. When fed a short pulse signal,
> only the timestamp of the falling edge will be seen in the PTPSYNCTS
> register, because that of the rising edge had been overwritten. So the
> moral is: don't feed it short pulse inputs.
> 
> Luckily this is not a complete deal breaker, as we can still work with
> 1 Hz square waves. But the problem is that the extts polling period was
> not dimensioned enough for this input signal. If we leave the period at
> half a second, we risk losing timestamps due to jitter in the measuring
> process. So we need to increase it to 4 times per second.
> 
> Also, the very least we can do to inform the user is to deny any other
> flags combination than with PTP_RISING_EDGE and PTP_FALLING_EDGE both
> set.
> 
> Fixes: 747e5eb31d59 ("net: dsa: sja1105: configure the PTP_CLK pin as EXT_TS or PER_OUT")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Applied.