[PATCH v2 0/3] net: mvpp2: tai: add extts support

[PATCH v2 0/3] net: mvpp2: tai: add extts support

[Shmuel Hazan]

This patch series adds support for PTP event capture on the Aramda
80x0/70x0. This feature is mainly used by tools linux ts2phc(3) in order
to synchronize a timestamping unit (like the mvpp2's TAI) and a system
DPLL on the same PCB. 

The patch series includes 3 patches: the second one implements the
actual extts function.

Changes in v2:
	* Fixed a deadlock in the poll worker.
	* Removed tabs from comments.

Shmuel Hazan (3):
  net: mvpp2: tai: add refcount for ptp worker
  net: mvpp2: tai: add extts support
  dt-bindings: net: marvell,pp2: add extts docs

 .../devicetree/bindings/net/marvell,pp2.yaml  |  18 +
 .../net/ethernet/marvell/mvpp2/mvpp2_tai.c    | 334 ++++++++++++++++--
 2 files changed, 317 insertions(+), 35 deletions(-)


base-commit: 3e7bb4f2461710b70887704af7f175383251088e
-- 
2.40.0

[PATCH v2 1/3] net: mvpp2: tai: add refcount for ptp worker

[Shmuel Hazan]

In some configurations, a single TAI can be responsible for multiple
mvpp2 interfaces. However, the mvpp2 driver will call mvpp22_tai_stop
and mvpp22_tai_start per interface RX timestamp disable/enable.

As a result, disabling timestamping for one interface would stop the
worker and corrupt the other interface's RX timestamps.

This commit solves the issue by introducing a simpler ref count for each
TAI instance.

Fixes: ce3497e2072e ("net: mvpp2: ptp: add support for receive timestamping")
Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>
---
 .../net/ethernet/marvell/mvpp2/mvpp2_tai.c    | 30 ++++++++++++++++---
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
index 95862aff49f1..2e3d43b1bac1 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
@@ -61,6 +61,7 @@ struct mvpp2_tai {
 	u64 period;		// nanosecond period in 32.32 fixed point
 	/* This timestamp is updated every two seconds */
 	struct timespec64 stamp;
+	u16 poll_worker_refcount;
 };
 
 static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set)
@@ -368,18 +369,39 @@ void mvpp22_tai_tstamp(struct mvpp2_tai *tai, u32 tstamp,
 	hwtstamp->hwtstamp = timespec64_to_ktime(ts);
 }
 
+static void mvpp22_tai_start_unlocked(struct mvpp2_tai *tai)
+{
+	tai->poll_worker_refcount++;
+	if (tai->poll_worker_refcount > 1)
+		return;
+
+	ptp_schedule_worker(tai->ptp_clock, 0);
+}
+
 void mvpp22_tai_start(struct mvpp2_tai *tai)
 {
-	long delay;
+	unsigned long flags;
 
-	delay = mvpp22_tai_aux_work(&tai->caps);
+	spin_lock_irqsave(&tai->lock, flags);
+	mvpp22_tai_start_unlocked(tai);
+	spin_unlock_irqrestore(&tai->lock, flags);
+}
 
-	ptp_schedule_worker(tai->ptp_clock, delay);
+static void mvpp22_tai_stop_unlocked(struct mvpp2_tai *tai)
+{
+	tai->poll_worker_refcount--;
+	if (tai->poll_worker_refcount)
+		return;
+	ptp_cancel_worker_sync(tai->ptp_clock);
 }
 
 void mvpp22_tai_stop(struct mvpp2_tai *tai)
 {
-	ptp_cancel_worker_sync(tai->ptp_clock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&tai->lock, flags);
+	mvpp22_tai_stop_unlocked(tai);
+	spin_unlock_irqrestore(&tai->lock, flags);
 }
 
 static void mvpp22_tai_remove(void *priv)
-- 
2.40.0

Re: [PATCH v2 1/3] net: mvpp2: tai: add refcount for ptp worker

[Andrew Lunn]

On Mon, Apr 17, 2023 at 08:07:39PM +0300, Shmuel Hazan wrote:
> In some configurations, a single TAI can be responsible for multiple
> mvpp2 interfaces. However, the mvpp2 driver will call mvpp22_tai_stop
> and mvpp22_tai_start per interface RX timestamp disable/enable.
> 
> As a result, disabling timestamping for one interface would stop the
> worker and corrupt the other interface's RX timestamps.
> 
> This commit solves the issue by introducing a simpler ref count for each
> TAI instance.
> 
> Fixes: ce3497e2072e ("net: mvpp2: ptp: add support for receive timestamping")
> Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>
> ---
>  .../net/ethernet/marvell/mvpp2/mvpp2_tai.c    | 30 ++++++++++++++++---
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> index 95862aff49f1..2e3d43b1bac1 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> @@ -61,6 +61,7 @@ struct mvpp2_tai {
>  	u64 period;		// nanosecond period in 32.32 fixed point
>  	/* This timestamp is updated every two seconds */
>  	struct timespec64 stamp;
> +	u16 poll_worker_refcount;
>  };
>  
>  static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set)
> @@ -368,18 +369,39 @@ void mvpp22_tai_tstamp(struct mvpp2_tai *tai, u32 tstamp,
>  	hwtstamp->hwtstamp = timespec64_to_ktime(ts);
>  }
>  
> +static void mvpp22_tai_start_unlocked(struct mvpp2_tai *tai)
> +{
> +	tai->poll_worker_refcount++;
> +	if (tai->poll_worker_refcount > 1)
> +		return;
> +
> +	ptp_schedule_worker(tai->ptp_clock, 0);

So the old code used to have delay here, not 0. And delay would be
returned by mvpp22_tai_aux_work() and have a value of 2000ms. So this
is a clear change in behaviour. Why is it O.K. not to delay for 2
seconds?  Should you actually still have the delay, pass it as a
parameter into this function?

     Andrew

Re: [PATCH v2 1/3] net: mvpp2: tai: add refcount for ptp worker

[Shmuel Hazan]

On Tue, 2023-04-18 at 02:44 +0200, Andrew Lunn wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments.
> 
> 
> On Mon, Apr 17, 2023 at 08:07:39PM +0300, Shmuel Hazan wrote:
> > In some configurations, a single TAI can be responsible for multiple
> > mvpp2 interfaces. However, the mvpp2 driver will call mvpp22_tai_stop
> > and mvpp22_tai_start per interface RX timestamp disable/enable.
> > 
> > As a result, disabling timestamping for one interface would stop the
> > worker and corrupt the other interface's RX timestamps.
> > 
> > This commit solves the issue by introducing a simpler ref count for each
> > TAI instance.
> > 
> > Fixes: ce3497e2072e ("net: mvpp2: ptp: add support for receive timestamping")
> > Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>
> > ---
> >  .../net/ethernet/marvell/mvpp2/mvpp2_tai.c    | 30 ++++++++++++++++---
> >  1 file changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> > index 95862aff49f1..2e3d43b1bac1 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> > @@ -61,6 +61,7 @@ struct mvpp2_tai {
> >       u64 period;             // nanosecond period in 32.32 fixed point
> >       /* This timestamp is updated every two seconds */
> >       struct timespec64 stamp;
> > +     u16 poll_worker_refcount;
> >  };
> > 
> >  static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set)
> > @@ -368,18 +369,39 @@ void mvpp22_tai_tstamp(struct mvpp2_tai *tai, u32 tstamp,
> >       hwtstamp->hwtstamp = timespec64_to_ktime(ts);
> >  }
> > 
> > +static void mvpp22_tai_start_unlocked(struct mvpp2_tai *tai)
> > +{
> > +     tai->poll_worker_refcount++;
> > +     if (tai->poll_worker_refcount > 1)
> > +             return;
> > +
> > +     ptp_schedule_worker(tai->ptp_clock, 0);
> 
> So the old code used to have delay here, not 0. And delay would be
> returned by mvpp22_tai_aux_work() and have a value of 2000ms. So this
> is a clear change in behaviour. Why is it O.K. not to delay for 2
> seconds?  Should you actually still have the delay, pass it as a
> parameter into this function?


Hi Andrew,

Thanks for your feedback. I will add more context about this change in
the next version. 

Before of this change, we ran the first iteration internally (on
mvpp22_tai_start), and then schedule another iteration in a delay to
run on the worker context. Howver, since the iteration itself locks
tai->lock, it is not possible to run it from mvpp22_tai_start anymore
as tai->lock is already locked. 

So, to summarize, from my point of view, this change should not
introduce any change in behavior as we will still run the first
iteration immediately, and then run it each 2s, as we did before. The
only change is that the first iteration will also be done from the
worker thread. However, I let me know if I am missing anything. 

---
Thanks,
Shmuel. 

> 
>      Andrew

[PATCH v2 2/3] net: mvpp2: tai: add extts support

[Shmuel Hazan]

This commit add support for capturing a timestamp in which the PTP_PULSE
pin, received a signal.

This feature is needed in order to synchronize multiple clocks in the
same board, using tools like ts2phc from the linuxptp project.

On the Armada 8040, this is the only way to do so as a result of
multiple erattas with the PTP_PULSE_IN interface that was designed to
synchronize the TAI on an external PPS signal (the errattas are
FE-6856276, FE-7382160 from document MV-S501388-00).

This patch introduces a pinctrl configuration "extts" that will be
selected once the user had enabled extts, and then will be returned back
to the "default" pinctrl config once it has been disabled. Additionally
these configurations will be also used in any case that the user asks us
to perform any action that involves "triggerering" the TAI subsystem, in
order to avoid a case where the external trigger would trigger with the
wrong action.

This pinctrl mess is needed due to the fact that there is no way for us
to distinguish betwee an external trigger (e.g. from the PTP_PULSE_IN
pin) or an internal one, triggered by the registers.

This feature has been tested on an Aramda
8040 based board, with linuxptp 3.1.1's ts2phc.

Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>
---
 .../net/ethernet/marvell/mvpp2/mvpp2_tai.c    | 304 ++++++++++++++++--
 1 file changed, 273 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
index 2e3d43b1bac1..ff57075c6ebc 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
@@ -3,8 +3,11 @@
  * Marvell PP2.2 TAI support
  *
  * Note:
- *   Do NOT use the event capture support.
- *   Do Not even set the MPP muxes to allow PTP_EVENT_REQ to be used.
+ *   In order to use the event capture support, please see the example
+ *   in marvell,pp2.yaml.
+ *   Do not manually (e.g. without pinctrl-1, as described in
+ *   marvell,pp2.yaml) set the MPP muxes to allow PTP_EVENT_REQ to be
+ *   used.
  *   It will disrupt the operation of this driver, and there is nothing
  *   that this driver can do to prevent that.  Even using PTP_EVENT_REQ
  *   as an output will be seen as a trigger input, which can't be masked.
@@ -33,6 +36,8 @@
  * Consequently, we support none of these.
  */
 #include <linux/io.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/ptp_clock.h>
 #include <linux/ptp_clock_kernel.h>
 #include <linux/slab.h>
 
@@ -53,6 +58,10 @@
 #define TCSR_CAPTURE_1_VALID		BIT(1)
 #define TCSR_CAPTURE_0_VALID		BIT(0)
 
+#define MVPP2_PINCTRL_EXTTS_STATE		"extts"
+#define MAX_PINS 1
+#define EXTTS_PERIOD_MS 95
+
 struct mvpp2_tai {
 	struct ptp_clock_info caps;
 	struct ptp_clock *ptp_clock;
@@ -61,7 +70,12 @@ struct mvpp2_tai {
 	u64 period;		// nanosecond period in 32.32 fixed point
 	/* This timestamp is updated every two seconds */
 	struct timespec64 stamp;
+	struct pinctrl *extts_pinctrl;
+	struct pinctrl_state *default_pinctrl_state;
+	struct pinctrl_state *extts_pinctrl_state;
+	struct ptp_pin_desc pin_config[MAX_PINS];
 	u16 poll_worker_refcount;
+	bool extts_enabled:1;
 };
 
 static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set)
@@ -73,6 +87,38 @@ static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set)
 	writel(val, reg);
 }
 
+/* mvpp2_tai_{pause,resume}_external_trigger are used as guards
+ * to mask external triggers where it is undesirable. For example,
+ * in case that the action is "increment", we may want to perform it
+ * once; however, we may trigger it once internally and once from
+ * an external pulse, which will cause an issue.
+ * In order to work around this issue, we need perform the following sequence:
+ *  1. call mvpp2_tai_pause_external_trigger
+ *  2. save the current trigger operation.
+ *  3. update the trigger operation.
+ *  4. perform an internal trigger.
+ *  5. restore the previous trigger operation.
+ *  6. call mvpp2_tai_restore_external_trigger.
+ */
+static int mvpp2_tai_pause_external_trigger(struct mvpp2_tai *tai)
+{
+	if (tai->extts_enabled && tai->extts_pinctrl &&
+	    tai->extts_pinctrl_state)
+		return pinctrl_select_state(tai->extts_pinctrl,
+					    tai->default_pinctrl_state);
+
+	return 0;
+}
+
+static int mvpp2_tai_resume_external_trigger(struct mvpp2_tai *tai)
+{
+	if (tai->extts_enabled && tai->extts_pinctrl &&
+	    tai->extts_pinctrl_state)
+		return pinctrl_select_state(tai->extts_pinctrl,
+					    tai->extts_pinctrl_state);
+	return 0;
+}
+
 static void mvpp2_tai_write(u32 val, void __iomem *reg)
 {
 	writel_relaxed(val & 0xffff, reg);
@@ -102,6 +148,28 @@ static void mvpp22_tai_read_ts(struct timespec64 *ts, void __iomem *base)
 	readl_relaxed(base + 24);
 }
 
+static int mvpp22_tai_try_read_ts(struct timespec64 *ts, void __iomem *base)
+{
+	long tcsr = readl(base + MVPP22_TAI_TCSR);
+	/* If both captures are not valid, return EBUSY */
+	int ret = -EBUSY;
+
+	if (tcsr & TCSR_CAPTURE_1_VALID) {
+		mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV1_SEC_HIGH);
+		ret = 0;
+	}
+
+	/* If both capture 1 and capture 0 are valid, use capture 0
+	 * but also read and clear capture 1.
+	 */
+	if (tcsr & TCSR_CAPTURE_0_VALID) {
+		mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV0_SEC_HIGH);
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static void mvpp2_tai_write_tlv(const struct timespec64 *ts, u32 frac,
 			        void __iomem *base)
 {
@@ -114,16 +182,30 @@ static void mvpp2_tai_write_tlv(const struct timespec64 *ts, u32 frac,
 	mvpp2_tai_write(frac, base + MVPP22_TAI_TLV_FRAC_LOW);
 }
 
-static void mvpp2_tai_op(u32 op, void __iomem *base)
+static int mvpp2_tai_op(u32 op, void __iomem *base, struct mvpp2_tai *tai)
 {
+	u32 reg_val;
+	int ret;
+
+	reg_val = mvpp2_tai_read(base + MVPP22_TAI_TCFCR0);
 	/* Trigger the operation. Note that an external unmaskable
 	 * event on PTP_EVENT_REQ will also trigger this action.
+	 * therefore, pause possible (known) external triggers.
 	 */
+	ret = mvpp2_tai_pause_external_trigger(tai);
+	if (ret)
+		goto out;
+
 	mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
 			 TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER,
 			 op | TCFCR0_TCF_TRIGGER);
-	mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
-			 TCFCR0_TCF_NOP);
+	mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
+			 TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER, reg_val);
+
+	mvpp2_tai_resume_external_trigger(tai);
+
+out:
+	return ret;
 }
 
 /* The adjustment has a range of +0.5ns to -0.5ns in 2^32 steps, so has units
@@ -170,6 +252,7 @@ static int mvpp22_tai_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	bool neg_adj;
 	s32 frac;
 	u64 val;
+	int ret;
 
 	neg_adj = scaled_ppm < 0;
 	if (neg_adj)
@@ -197,10 +280,10 @@ static int mvpp22_tai_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	spin_lock_irqsave(&tai->lock, flags);
 	mvpp2_tai_write(frac >> 16, base + MVPP22_TAI_TLV_FRAC_HIGH);
 	mvpp2_tai_write(frac, base + MVPP22_TAI_TLV_FRAC_LOW);
-	mvpp2_tai_op(TCFCR0_TCF_FREQUPDATE, base);
+	ret = mvpp2_tai_op(TCFCR0_TCF_FREQUPDATE, base, tai);
 	spin_unlock_irqrestore(&tai->lock, flags);
 
-	return 0;
+	return ret;
 }
 
 static int mvpp22_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
@@ -210,6 +293,7 @@ static int mvpp22_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	unsigned long flags;
 	void __iomem *base;
 	u32 tcf;
+	int ret;
 
 	/* We can't deal with S64_MIN */
 	if (delta == S64_MIN)
@@ -227,10 +311,10 @@ static int mvpp22_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	base = tai->base;
 	spin_lock_irqsave(&tai->lock, flags);
 	mvpp2_tai_write_tlv(&ts, 0, base);
-	mvpp2_tai_op(tcf, base);
+	ret = mvpp2_tai_op(tcf, base, tai);
 	spin_unlock_irqrestore(&tai->lock, flags);
 
-	return 0;
+	return ret;
 }
 
 static int mvpp22_tai_gettimex64(struct ptp_clock_info *ptp,
@@ -240,35 +324,34 @@ static int mvpp22_tai_gettimex64(struct ptp_clock_info *ptp,
 	struct mvpp2_tai *tai = ptp_to_tai(ptp);
 	unsigned long flags;
 	void __iomem *base;
-	u32 tcsr;
+	u32 reg_val;
 	int ret;
 
 	base = tai->base;
 	spin_lock_irqsave(&tai->lock, flags);
 	/* XXX: the only way to read the PTP time is for the CPU to trigger
 	 * an event. However, there is no way to distinguish between the CPU
-	 * triggered event, and an external event on PTP_EVENT_REQ. So this
-	 * is incompatible with external use of PTP_EVENT_REQ.
+	 * triggered event, and an external event on PTP_EVENT_REQ. As a result
+	 * we are pausing here external triggers by switching the pinctrl state
+	 * to the default state (if applicable).
 	 */
+	ret = mvpp2_tai_pause_external_trigger(tai);
+	if (ret)
+		goto unlock_out;
+
+	reg_val = mvpp2_tai_read(base + MVPP22_TAI_TCFCR0);
 	ptp_read_system_prets(sts);
 	mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
 			 TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER,
 			 TCFCR0_TCF_CAPTURE | TCFCR0_TCF_TRIGGER);
 	ptp_read_system_postts(sts);
-	mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
-			 TCFCR0_TCF_NOP);
+	mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
+			 TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER, reg_val);
 
-	tcsr = readl(base + MVPP22_TAI_TCSR);
-	if (tcsr & TCSR_CAPTURE_1_VALID) {
-		mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV1_SEC_HIGH);
-		ret = 0;
-	} else if (tcsr & TCSR_CAPTURE_0_VALID) {
-		mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV0_SEC_HIGH);
-		ret = 0;
-	} else {
-		/* We don't seem to have a reading... */
-		ret = -EBUSY;
-	}
+	ret = mvpp22_tai_try_read_ts(ts, base);
+	mvpp2_tai_resume_external_trigger(tai);
+
+unlock_out:
 	spin_unlock_irqrestore(&tai->lock, flags);
 
 	return ret;
@@ -280,32 +363,71 @@ static int mvpp22_tai_settime64(struct ptp_clock_info *ptp,
 	struct mvpp2_tai *tai = ptp_to_tai(ptp);
 	unsigned long flags;
 	void __iomem *base;
+	u32 reg_val;
+	int ret;
 
 	base = tai->base;
 	spin_lock_irqsave(&tai->lock, flags);
 	mvpp2_tai_write_tlv(ts, 0, base);
 
+	ret = mvpp2_tai_pause_external_trigger(tai);
+	if (ret)
+		goto unlock_out;
+
 	/* Trigger an update to load the value from the TLV registers
 	 * into the TOD counter. Note that an external unmaskable event on
 	 * PTP_EVENT_REQ will also trigger this action.
 	 */
+	reg_val = mvpp2_tai_read(base + MVPP22_TAI_TCFCR0);
 	mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
-			 TCFCR0_PHASE_UPDATE_ENABLE |
-			 TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER,
+			 TCFCR0_PHASE_UPDATE_ENABLE | TCFCR0_TCF_MASK |
+				 TCFCR0_TCF_TRIGGER,
 			 TCFCR0_TCF_UPDATE | TCFCR0_TCF_TRIGGER);
-	mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
-			 TCFCR0_TCF_NOP);
+	mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
+			 TCFCR0_PHASE_UPDATE_ENABLE | TCFCR0_TCF_MASK |
+				 TCFCR0_TCF_TRIGGER,
+			 reg_val);
+
+	mvpp2_tai_resume_external_trigger(tai);
+
+unlock_out:
 	spin_unlock_irqrestore(&tai->lock, flags);
 
-	return 0;
+	return ret;
+}
+
+static void do_aux_work_extts(struct mvpp2_tai *tai)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&tai->lock, flags);
+
+	ret = mvpp22_tai_try_read_ts(&tai->stamp, tai->base);
+	/* We are not managed to read a TS, try again later */
+	if (!ret) {
+		struct ptp_clock_event event;
+
+		/* Triggered - save timestamp */
+		event.type = PTP_CLOCK_EXTTS;
+		event.index = 0; /* We only have one channel */
+		event.timestamp = timespec64_to_ns(&tai->stamp);
+		ptp_clock_event(tai->ptp_clock, &event);
+	}
+
+	spin_unlock_irqrestore(&tai->lock, flags);
 }
 
 static long mvpp22_tai_aux_work(struct ptp_clock_info *ptp)
 {
 	struct mvpp2_tai *tai = ptp_to_tai(ptp);
 
-	mvpp22_tai_gettimex64(ptp, &tai->stamp, NULL);
+	if (tai->extts_enabled) {
+		do_aux_work_extts(tai);
+		return msecs_to_jiffies(EXTTS_PERIOD_MS);
+	}
 
+	mvpp22_tai_gettimex64(ptp, &tai->stamp, NULL);
 	return msecs_to_jiffies(2000);
 }
 
@@ -404,6 +526,94 @@ void mvpp22_tai_stop(struct mvpp2_tai *tai)
 	spin_unlock_irqrestore(&tai->lock, flags);
 }
 
+static void mvpp22_tai_capture_enable(struct mvpp2_tai *tai, bool enable)
+{
+	mvpp2_tai_modify(tai->base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
+			 enable ? TCFCR0_TCF_CAPTURE : TCFCR0_TCF_NOP);
+}
+
+static int mvpp22_tai_req_extts_enable(struct mvpp2_tai *tai,
+				       struct ptp_clock_request *rq, int on)
+{
+	u8 index = rq->extts.index;
+	int ret = 0;
+
+	if (!tai->extts_pinctrl)
+		return -EINVAL;
+
+	/* Reject requests with unsupported flags */
+	if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | PTP_RISING_EDGE |
+				PTP_FALLING_EDGE | PTP_STRICT_FLAGS))
+		return -EOPNOTSUPP;
+
+	/* Reject requests to enable time stamping on falling edge */
+	if ((rq->extts.flags & PTP_ENABLE_FEATURE) &&
+	    (rq->extts.flags & PTP_FALLING_EDGE))
+		return -EOPNOTSUPP;
+
+	if (index >= MAX_PINS)
+		return -EINVAL;
+
+	if (on)
+		ret = pinctrl_select_state(tai->extts_pinctrl,
+					   tai->extts_pinctrl_state);
+	else
+		ret = pinctrl_select_state(tai->extts_pinctrl,
+					   tai->default_pinctrl_state);
+	if (ret)
+		goto out;
+
+	tai->extts_enabled = on != 0;
+	mvpp22_tai_capture_enable(tai, tai->extts_enabled);
+
+	/* We need to enable the poll worker in order for events to be polled */
+	if (on)
+		mvpp22_tai_start_unlocked(tai);
+	else
+		mvpp22_tai_stop_unlocked(tai);
+
+out:
+	return ret;
+}
+
+static int mvpp22_tai_enable(struct ptp_clock_info *ptp,
+			     struct ptp_clock_request *rq, int on)
+{
+	struct mvpp2_tai *tai = ptp_to_tai(ptp);
+	int err = -EOPNOTSUPP;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tai->lock, flags);
+
+	switch (rq->type) {
+	case PTP_CLK_REQ_EXTTS:
+		err = mvpp22_tai_req_extts_enable(tai, rq, on);
+		break;
+	default:
+		break;
+	}
+
+	spin_unlock_irqrestore(&tai->lock, flags);
+	return err;
+}
+
+static int mvpp22_tai_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
+				 enum ptp_pin_function func, unsigned int chan)
+{
+	if (chan != 0)
+		return -1;
+
+	switch (func) {
+	case PTP_PF_NONE:
+	case PTP_PF_EXTTS:
+		break;
+	case PTP_PF_PEROUT:
+	case PTP_PF_PHYSYNC:
+		return -1;
+	}
+	return 0;
+}
+
 static void mvpp22_tai_remove(void *priv)
 {
 	struct mvpp2_tai *tai = priv;
@@ -423,6 +633,24 @@ int mvpp22_tai_probe(struct device *dev, struct mvpp2 *priv)
 
 	spin_lock_init(&tai->lock);
 
+	tai->extts_pinctrl = devm_pinctrl_get_select_default(dev);
+	if (!IS_ERR(tai->extts_pinctrl)) {
+		tai->default_pinctrl_state = pinctrl_lookup_state
+			(tai->extts_pinctrl, PINCTRL_STATE_DEFAULT);
+		tai->extts_pinctrl_state = pinctrl_lookup_state
+			(tai->extts_pinctrl, MVPP2_PINCTRL_EXTTS_STATE);
+
+		if (IS_ERR(tai->default_pinctrl_state) ||
+		    IS_ERR(tai->extts_pinctrl_state)) {
+			pinctrl_put(tai->extts_pinctrl);
+			tai->extts_pinctrl = NULL;
+			tai->default_pinctrl_state = NULL;
+			tai->extts_pinctrl_state = NULL;
+		}
+	} else {
+		tai->extts_pinctrl = NULL;
+	}
+
 	tai->base = priv->iface_base;
 
 	/* The step size consists of three registers - a 16-bit nanosecond step
@@ -458,12 +686,26 @@ int mvpp22_tai_probe(struct device *dev, struct mvpp2 *priv)
 
 	tai->caps.owner = THIS_MODULE;
 	strscpy(tai->caps.name, "Marvell PP2.2", sizeof(tai->caps.name));
+	tai->caps.n_ext_ts = MAX_PINS;
+	tai->caps.n_pins = MAX_PINS;
 	tai->caps.max_adj = mvpp22_calc_max_adj(tai);
 	tai->caps.adjfine = mvpp22_tai_adjfine;
 	tai->caps.adjtime = mvpp22_tai_adjtime;
 	tai->caps.gettimex64 = mvpp22_tai_gettimex64;
 	tai->caps.settime64 = mvpp22_tai_settime64;
 	tai->caps.do_aux_work = mvpp22_tai_aux_work;
+	tai->caps.enable = mvpp22_tai_enable;
+	tai->caps.verify = mvpp22_tai_verify_pin;
+	tai->caps.pin_config = tai->pin_config;
+
+	for (int i = 0; i < tai->caps.n_pins; ++i) {
+		struct ptp_pin_desc *ppd = &tai->caps.pin_config[i];
+
+		snprintf(ppd->name, sizeof(ppd->name), "PTP_PULSE_IN%d", i);
+		ppd->index = i;
+		ppd->func = PTP_PF_NONE;
+		ppd->chan = 0;
+	}
 
 	ret = devm_add_action(dev, mvpp22_tai_remove, tai);
 	if (ret)
-- 
2.40.0

Re: [PATCH v2 2/3] net: mvpp2: tai: add extts support

[Horatiu Vultur]

The 04/17/2023 20:07, Shmuel Hazan wrote:
> 
> This commit add support for capturing a timestamp in which the PTP_PULSE
> pin, received a signal.
> 
> This feature is needed in order to synchronize multiple clocks in the
> same board, using tools like ts2phc from the linuxptp project.
> 
> On the Armada 8040, this is the only way to do so as a result of
> multiple erattas with the PTP_PULSE_IN interface that was designed to
> synchronize the TAI on an external PPS signal (the errattas are
> FE-6856276, FE-7382160 from document MV-S501388-00).
> 
> This patch introduces a pinctrl configuration "extts" that will be
> selected once the user had enabled extts, and then will be returned back
> to the "default" pinctrl config once it has been disabled. Additionally
> these configurations will be also used in any case that the user asks us
> to perform any action that involves "triggerering" the TAI subsystem, in
> order to avoid a case where the external trigger would trigger with the
> wrong action.
> 
> This pinctrl mess is needed due to the fact that there is no way for us
> to distinguish betwee an external trigger (e.g. from the PTP_PULSE_IN
> pin) or an internal one, triggered by the registers.
> 
> This feature has been tested on an Aramda
> 8040 based board, with linuxptp 3.1.1's ts2phc.

It looks good to me, just one more questions bellow.
Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> 
> Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>
> ---
>  .../net/ethernet/marvell/mvpp2/mvpp2_tai.c    | 304 ++++++++++++++++--
>  1 file changed, 273 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> index 2e3d43b1bac1..ff57075c6ebc 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> @@ -3,8 +3,11 @@
>   * Marvell PP2.2 TAI support
>   *
>   * Note:
> - *   Do NOT use the event capture support.
> - *   Do Not even set the MPP muxes to allow PTP_EVENT_REQ to be used.
> + *   In order to use the event capture support, please see the example
> + *   in marvell,pp2.yaml.
> + *   Do not manually (e.g. without pinctrl-1, as described in
> + *   marvell,pp2.yaml) set the MPP muxes to allow PTP_EVENT_REQ to be
> + *   used.
>   *   It will disrupt the operation of this driver, and there is nothing
>   *   that this driver can do to prevent that.  Even using PTP_EVENT_REQ
>   *   as an output will be seen as a trigger input, which can't be masked.
> @@ -33,6 +36,8 @@
>   * Consequently, we support none of these.
>   */
>  #include <linux/io.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/ptp_clock.h>
>  #include <linux/ptp_clock_kernel.h>
>  #include <linux/slab.h>
> 
> @@ -53,6 +58,10 @@
>  #define TCSR_CAPTURE_1_VALID           BIT(1)
>  #define TCSR_CAPTURE_0_VALID           BIT(0)
> 
> +#define MVPP2_PINCTRL_EXTTS_STATE              "extts"
> +#define MAX_PINS 1
> +#define EXTTS_PERIOD_MS 95

How have you come with this 95 value?

> +
>  struct mvpp2_tai {
>         struct ptp_clock_info caps;
>         struct ptp_clock *ptp_clock;
> @@ -61,7 +70,12 @@ struct mvpp2_tai {
>         u64 period;             // nanosecond period in 32.32 fixed point
>         /* This timestamp is updated every two seconds */
>         struct timespec64 stamp;
> +       struct pinctrl *extts_pinctrl;
> +       struct pinctrl_state *default_pinctrl_state;
> +       struct pinctrl_state *extts_pinctrl_state;
> +       struct ptp_pin_desc pin_config[MAX_PINS];
>         u16 poll_worker_refcount;
> +       bool extts_enabled:1;
>  };
> 
>  static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set)
> @@ -73,6 +87,38 @@ static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set)
>         writel(val, reg);
>  }
> 
> +/* mvpp2_tai_{pause,resume}_external_trigger are used as guards
> + * to mask external triggers where it is undesirable. For example,
> + * in case that the action is "increment", we may want to perform it
> + * once; however, we may trigger it once internally and once from
> + * an external pulse, which will cause an issue.
> + * In order to work around this issue, we need perform the following sequence:
> + *  1. call mvpp2_tai_pause_external_trigger
> + *  2. save the current trigger operation.
> + *  3. update the trigger operation.
> + *  4. perform an internal trigger.
> + *  5. restore the previous trigger operation.
> + *  6. call mvpp2_tai_restore_external_trigger.
> + */
> +static int mvpp2_tai_pause_external_trigger(struct mvpp2_tai *tai)
> +{
> +       if (tai->extts_enabled && tai->extts_pinctrl &&
> +           tai->extts_pinctrl_state)
> +               return pinctrl_select_state(tai->extts_pinctrl,
> +                                           tai->default_pinctrl_state);
> +
> +       return 0;
> +}
> +
> +static int mvpp2_tai_resume_external_trigger(struct mvpp2_tai *tai)
> +{
> +       if (tai->extts_enabled && tai->extts_pinctrl &&
> +           tai->extts_pinctrl_state)
> +               return pinctrl_select_state(tai->extts_pinctrl,
> +                                           tai->extts_pinctrl_state);
> +       return 0;
> +}
> +
>  static void mvpp2_tai_write(u32 val, void __iomem *reg)
>  {
>         writel_relaxed(val & 0xffff, reg);
> @@ -102,6 +148,28 @@ static void mvpp22_tai_read_ts(struct timespec64 *ts, void __iomem *base)
>         readl_relaxed(base + 24);
>  }
> 
> +static int mvpp22_tai_try_read_ts(struct timespec64 *ts, void __iomem *base)
> +{
> +       long tcsr = readl(base + MVPP22_TAI_TCSR);
> +       /* If both captures are not valid, return EBUSY */
> +       int ret = -EBUSY;
> +
> +       if (tcsr & TCSR_CAPTURE_1_VALID) {
> +               mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV1_SEC_HIGH);
> +               ret = 0;
> +       }
> +
> +       /* If both capture 1 and capture 0 are valid, use capture 0
> +        * but also read and clear capture 1.
> +        */
> +       if (tcsr & TCSR_CAPTURE_0_VALID) {
> +               mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV0_SEC_HIGH);
> +               ret = 0;
> +       }
> +
> +       return ret;
> +}
> +
>  static void mvpp2_tai_write_tlv(const struct timespec64 *ts, u32 frac,
>                                 void __iomem *base)
>  {
> @@ -114,16 +182,30 @@ static void mvpp2_tai_write_tlv(const struct timespec64 *ts, u32 frac,
>         mvpp2_tai_write(frac, base + MVPP22_TAI_TLV_FRAC_LOW);
>  }
> 
> -static void mvpp2_tai_op(u32 op, void __iomem *base)
> +static int mvpp2_tai_op(u32 op, void __iomem *base, struct mvpp2_tai *tai)
>  {
> +       u32 reg_val;
> +       int ret;
> +
> +       reg_val = mvpp2_tai_read(base + MVPP22_TAI_TCFCR0);
>         /* Trigger the operation. Note that an external unmaskable
>          * event on PTP_EVENT_REQ will also trigger this action.
> +        * therefore, pause possible (known) external triggers.
>          */
> +       ret = mvpp2_tai_pause_external_trigger(tai);
> +       if (ret)
> +               goto out;
> +
>         mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
>                          TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER,
>                          op | TCFCR0_TCF_TRIGGER);
> -       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
> -                        TCFCR0_TCF_NOP);
> +       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
> +                        TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER, reg_val);
> +
> +       mvpp2_tai_resume_external_trigger(tai);
> +
> +out:
> +       return ret;
>  }
> 
>  /* The adjustment has a range of +0.5ns to -0.5ns in 2^32 steps, so has units
> @@ -170,6 +252,7 @@ static int mvpp22_tai_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>         bool neg_adj;
>         s32 frac;
>         u64 val;
> +       int ret;
> 
>         neg_adj = scaled_ppm < 0;
>         if (neg_adj)
> @@ -197,10 +280,10 @@ static int mvpp22_tai_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>         spin_lock_irqsave(&tai->lock, flags);
>         mvpp2_tai_write(frac >> 16, base + MVPP22_TAI_TLV_FRAC_HIGH);
>         mvpp2_tai_write(frac, base + MVPP22_TAI_TLV_FRAC_LOW);
> -       mvpp2_tai_op(TCFCR0_TCF_FREQUPDATE, base);
> +       ret = mvpp2_tai_op(TCFCR0_TCF_FREQUPDATE, base, tai);
>         spin_unlock_irqrestore(&tai->lock, flags);
> 
> -       return 0;
> +       return ret;
>  }
> 
>  static int mvpp22_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
> @@ -210,6 +293,7 @@ static int mvpp22_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
>         unsigned long flags;
>         void __iomem *base;
>         u32 tcf;
> +       int ret;
> 
>         /* We can't deal with S64_MIN */
>         if (delta == S64_MIN)
> @@ -227,10 +311,10 @@ static int mvpp22_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
>         base = tai->base;
>         spin_lock_irqsave(&tai->lock, flags);
>         mvpp2_tai_write_tlv(&ts, 0, base);
> -       mvpp2_tai_op(tcf, base);
> +       ret = mvpp2_tai_op(tcf, base, tai);
>         spin_unlock_irqrestore(&tai->lock, flags);
> 
> -       return 0;
> +       return ret;
>  }
> 
>  static int mvpp22_tai_gettimex64(struct ptp_clock_info *ptp,
> @@ -240,35 +324,34 @@ static int mvpp22_tai_gettimex64(struct ptp_clock_info *ptp,
>         struct mvpp2_tai *tai = ptp_to_tai(ptp);
>         unsigned long flags;
>         void __iomem *base;
> -       u32 tcsr;
> +       u32 reg_val;
>         int ret;
> 
>         base = tai->base;
>         spin_lock_irqsave(&tai->lock, flags);
>         /* XXX: the only way to read the PTP time is for the CPU to trigger
>          * an event. However, there is no way to distinguish between the CPU
> -        * triggered event, and an external event on PTP_EVENT_REQ. So this
> -        * is incompatible with external use of PTP_EVENT_REQ.
> +        * triggered event, and an external event on PTP_EVENT_REQ. As a result
> +        * we are pausing here external triggers by switching the pinctrl state
> +        * to the default state (if applicable).
>          */
> +       ret = mvpp2_tai_pause_external_trigger(tai);
> +       if (ret)
> +               goto unlock_out;
> +
> +       reg_val = mvpp2_tai_read(base + MVPP22_TAI_TCFCR0);
>         ptp_read_system_prets(sts);
>         mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
>                          TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER,
>                          TCFCR0_TCF_CAPTURE | TCFCR0_TCF_TRIGGER);
>         ptp_read_system_postts(sts);
> -       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
> -                        TCFCR0_TCF_NOP);
> +       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
> +                        TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER, reg_val);
> 
> -       tcsr = readl(base + MVPP22_TAI_TCSR);
> -       if (tcsr & TCSR_CAPTURE_1_VALID) {
> -               mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV1_SEC_HIGH);
> -               ret = 0;
> -       } else if (tcsr & TCSR_CAPTURE_0_VALID) {
> -               mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV0_SEC_HIGH);
> -               ret = 0;
> -       } else {
> -               /* We don't seem to have a reading... */
> -               ret = -EBUSY;
> -       }
> +       ret = mvpp22_tai_try_read_ts(ts, base);
> +       mvpp2_tai_resume_external_trigger(tai);
> +
> +unlock_out:
>         spin_unlock_irqrestore(&tai->lock, flags);
> 
>         return ret;
> @@ -280,32 +363,71 @@ static int mvpp22_tai_settime64(struct ptp_clock_info *ptp,
>         struct mvpp2_tai *tai = ptp_to_tai(ptp);
>         unsigned long flags;
>         void __iomem *base;
> +       u32 reg_val;
> +       int ret;
> 
>         base = tai->base;
>         spin_lock_irqsave(&tai->lock, flags);
>         mvpp2_tai_write_tlv(ts, 0, base);
> 
> +       ret = mvpp2_tai_pause_external_trigger(tai);
> +       if (ret)
> +               goto unlock_out;
> +
>         /* Trigger an update to load the value from the TLV registers
>          * into the TOD counter. Note that an external unmaskable event on
>          * PTP_EVENT_REQ will also trigger this action.
>          */
> +       reg_val = mvpp2_tai_read(base + MVPP22_TAI_TCFCR0);
>         mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
> -                        TCFCR0_PHASE_UPDATE_ENABLE |
> -                        TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER,
> +                        TCFCR0_PHASE_UPDATE_ENABLE | TCFCR0_TCF_MASK |
> +                                TCFCR0_TCF_TRIGGER,
>                          TCFCR0_TCF_UPDATE | TCFCR0_TCF_TRIGGER);
> -       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
> -                        TCFCR0_TCF_NOP);
> +       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
> +                        TCFCR0_PHASE_UPDATE_ENABLE | TCFCR0_TCF_MASK |
> +                                TCFCR0_TCF_TRIGGER,
> +                        reg_val);
> +
> +       mvpp2_tai_resume_external_trigger(tai);
> +
> +unlock_out:
>         spin_unlock_irqrestore(&tai->lock, flags);
> 
> -       return 0;
> +       return ret;
> +}
> +
> +static void do_aux_work_extts(struct mvpp2_tai *tai)
> +{
> +       unsigned long flags;
> +       int ret;
> +
> +       spin_lock_irqsave(&tai->lock, flags);
> +
> +       ret = mvpp22_tai_try_read_ts(&tai->stamp, tai->base);
> +       /* We are not managed to read a TS, try again later */
> +       if (!ret) {
> +               struct ptp_clock_event event;
> +
> +               /* Triggered - save timestamp */
> +               event.type = PTP_CLOCK_EXTTS;
> +               event.index = 0; /* We only have one channel */
> +               event.timestamp = timespec64_to_ns(&tai->stamp);
> +               ptp_clock_event(tai->ptp_clock, &event);
> +       }
> +
> +       spin_unlock_irqrestore(&tai->lock, flags);
>  }
> 
>  static long mvpp22_tai_aux_work(struct ptp_clock_info *ptp)
>  {
>         struct mvpp2_tai *tai = ptp_to_tai(ptp);
> 
> -       mvpp22_tai_gettimex64(ptp, &tai->stamp, NULL);
> +       if (tai->extts_enabled) {
> +               do_aux_work_extts(tai);
> +               return msecs_to_jiffies(EXTTS_PERIOD_MS);
> +       }
> 
> +       mvpp22_tai_gettimex64(ptp, &tai->stamp, NULL);
>         return msecs_to_jiffies(2000);
>  }
> 
> @@ -404,6 +526,94 @@ void mvpp22_tai_stop(struct mvpp2_tai *tai)
>         spin_unlock_irqrestore(&tai->lock, flags);
>  }
> 
> +static void mvpp22_tai_capture_enable(struct mvpp2_tai *tai, bool enable)
> +{
> +       mvpp2_tai_modify(tai->base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
> +                        enable ? TCFCR0_TCF_CAPTURE : TCFCR0_TCF_NOP);
> +}
> +
> +static int mvpp22_tai_req_extts_enable(struct mvpp2_tai *tai,
> +                                      struct ptp_clock_request *rq, int on)
> +{
> +       u8 index = rq->extts.index;
> +       int ret = 0;
> +
> +       if (!tai->extts_pinctrl)
> +               return -EINVAL;
> +
> +       /* Reject requests with unsupported flags */
> +       if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | PTP_RISING_EDGE |
> +                               PTP_FALLING_EDGE | PTP_STRICT_FLAGS))
> +               return -EOPNOTSUPP;
> +
> +       /* Reject requests to enable time stamping on falling edge */
> +       if ((rq->extts.flags & PTP_ENABLE_FEATURE) &&
> +           (rq->extts.flags & PTP_FALLING_EDGE))
> +               return -EOPNOTSUPP;
> +
> +       if (index >= MAX_PINS)
> +               return -EINVAL;
> +
> +       if (on)
> +               ret = pinctrl_select_state(tai->extts_pinctrl,
> +                                          tai->extts_pinctrl_state);
> +       else
> +               ret = pinctrl_select_state(tai->extts_pinctrl,
> +                                          tai->default_pinctrl_state);
> +       if (ret)
> +               goto out;
> +
> +       tai->extts_enabled = on != 0;
> +       mvpp22_tai_capture_enable(tai, tai->extts_enabled);
> +
> +       /* We need to enable the poll worker in order for events to be polled */
> +       if (on)
> +               mvpp22_tai_start_unlocked(tai);
> +       else
> +               mvpp22_tai_stop_unlocked(tai);
> +
> +out:
> +       return ret;
> +}
> +
> +static int mvpp22_tai_enable(struct ptp_clock_info *ptp,
> +                            struct ptp_clock_request *rq, int on)
> +{
> +       struct mvpp2_tai *tai = ptp_to_tai(ptp);
> +       int err = -EOPNOTSUPP;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&tai->lock, flags);
> +
> +       switch (rq->type) {
> +       case PTP_CLK_REQ_EXTTS:
> +               err = mvpp22_tai_req_extts_enable(tai, rq, on);
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       spin_unlock_irqrestore(&tai->lock, flags);
> +       return err;
> +}
> +
> +static int mvpp22_tai_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
> +                                enum ptp_pin_function func, unsigned int chan)
> +{
> +       if (chan != 0)
> +               return -1;
> +
> +       switch (func) {
> +       case PTP_PF_NONE:
> +       case PTP_PF_EXTTS:
> +               break;
> +       case PTP_PF_PEROUT:
> +       case PTP_PF_PHYSYNC:
> +               return -1;
> +       }
> +       return 0;
> +}
> +
>  static void mvpp22_tai_remove(void *priv)
>  {
>         struct mvpp2_tai *tai = priv;
> @@ -423,6 +633,24 @@ int mvpp22_tai_probe(struct device *dev, struct mvpp2 *priv)
> 
>         spin_lock_init(&tai->lock);
> 
> +       tai->extts_pinctrl = devm_pinctrl_get_select_default(dev);
> +       if (!IS_ERR(tai->extts_pinctrl)) {
> +               tai->default_pinctrl_state = pinctrl_lookup_state
> +                       (tai->extts_pinctrl, PINCTRL_STATE_DEFAULT);
> +               tai->extts_pinctrl_state = pinctrl_lookup_state
> +                       (tai->extts_pinctrl, MVPP2_PINCTRL_EXTTS_STATE);
> +
> +               if (IS_ERR(tai->default_pinctrl_state) ||
> +                   IS_ERR(tai->extts_pinctrl_state)) {
> +                       pinctrl_put(tai->extts_pinctrl);
> +                       tai->extts_pinctrl = NULL;
> +                       tai->default_pinctrl_state = NULL;
> +                       tai->extts_pinctrl_state = NULL;
> +               }
> +       } else {
> +               tai->extts_pinctrl = NULL;
> +       }
> +
>         tai->base = priv->iface_base;
> 
>         /* The step size consists of three registers - a 16-bit nanosecond step
> @@ -458,12 +686,26 @@ int mvpp22_tai_probe(struct device *dev, struct mvpp2 *priv)
> 
>         tai->caps.owner = THIS_MODULE;
>         strscpy(tai->caps.name, "Marvell PP2.2", sizeof(tai->caps.name));
> +       tai->caps.n_ext_ts = MAX_PINS;
> +       tai->caps.n_pins = MAX_PINS;
>         tai->caps.max_adj = mvpp22_calc_max_adj(tai);
>         tai->caps.adjfine = mvpp22_tai_adjfine;
>         tai->caps.adjtime = mvpp22_tai_adjtime;
>         tai->caps.gettimex64 = mvpp22_tai_gettimex64;
>         tai->caps.settime64 = mvpp22_tai_settime64;
>         tai->caps.do_aux_work = mvpp22_tai_aux_work;
> +       tai->caps.enable = mvpp22_tai_enable;
> +       tai->caps.verify = mvpp22_tai_verify_pin;
> +       tai->caps.pin_config = tai->pin_config;
> +
> +       for (int i = 0; i < tai->caps.n_pins; ++i) {
> +               struct ptp_pin_desc *ppd = &tai->caps.pin_config[i];
> +
> +               snprintf(ppd->name, sizeof(ppd->name), "PTP_PULSE_IN%d", i);
> +               ppd->index = i;
> +               ppd->func = PTP_PF_NONE;
> +               ppd->chan = 0;
> +       }
> 
>         ret = devm_add_action(dev, mvpp22_tai_remove, tai);
>         if (ret)
> --
> 2.40.0
> 

-- 
/Horatiu

Re: [PATCH v2 2/3] net: mvpp2: tai: add extts support

[Shmuel Hazan]

On Tue, 2023-04-18 at 11:37 +0200, Horatiu Vultur wrote:
> 
> The 04/17/2023 20:07, Shmuel Hazan wrote:
> > 
> > This commit add support for capturing a timestamp in which the PTP_PULSE
> > pin, received a signal.
> > 
> > This feature is needed in order to synchronize multiple clocks in the
> > same board, using tools like ts2phc from the linuxptp project.
> > 
> > On the Armada 8040, this is the only way to do so as a result of
> > multiple erattas with the PTP_PULSE_IN interface that was designed to
> > synchronize the TAI on an external PPS signal (the errattas are
> > FE-6856276, FE-7382160 from document MV-S501388-00).
> > 
> > This patch introduces a pinctrl configuration "extts" that will be
> > selected once the user had enabled extts, and then will be returned back
> > to the "default" pinctrl config once it has been disabled. Additionally
> > these configurations will be also used in any case that the user asks us
> > to perform any action that involves "triggerering" the TAI subsystem, in
> > order to avoid a case where the external trigger would trigger with the
> > wrong action.
> > 
> > This pinctrl mess is needed due to the fact that there is no way for us
> > to distinguish betwee an external trigger (e.g. from the PTP_PULSE_IN
> > pin) or an internal one, triggered by the registers.
> > 
> > This feature has been tested on an Aramda
> > 8040 based board, with linuxptp 3.1.1's ts2phc.
> 
> It looks good to me, just one more questions bellow.
> Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> 
> > 
> > Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>
> > ---
> >  .../net/ethernet/marvell/mvpp2/mvpp2_tai.c    | 304 ++++++++++++++++--
> >  1 file changed, 273 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> > index 2e3d43b1bac1..ff57075c6ebc 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> > @@ -3,8 +3,11 @@
> >   * Marvell PP2.2 TAI support
> >   *
> >   * Note:
> > - *   Do NOT use the event capture support.
> > - *   Do Not even set the MPP muxes to allow PTP_EVENT_REQ to be used.
> > + *   In order to use the event capture support, please see the example
> > + *   in marvell,pp2.yaml.
> > + *   Do not manually (e.g. without pinctrl-1, as described in
> > + *   marvell,pp2.yaml) set the MPP muxes to allow PTP_EVENT_REQ to be
> > + *   used.
> >   *   It will disrupt the operation of this driver, and there is nothing
> >   *   that this driver can do to prevent that.  Even using PTP_EVENT_REQ
> >   *   as an output will be seen as a trigger input, which can't be masked.
> > @@ -33,6 +36,8 @@
> >   * Consequently, we support none of these.
> >   */
> >  #include <linux/io.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/ptp_clock.h>
> >  #include <linux/ptp_clock_kernel.h>
> >  #include <linux/slab.h>
> > 
> > @@ -53,6 +58,10 @@
> >  #define TCSR_CAPTURE_1_VALID           BIT(1)
> >  #define TCSR_CAPTURE_0_VALID           BIT(0)
> > 
> > +#define MVPP2_PINCTRL_EXTTS_STATE              "extts"
> > +#define MAX_PINS 1
> > +#define EXTTS_PERIOD_MS 95
> 
> How have you come with this 95 value?

Hi Horatiu,

Thanks for your review. 

As for the 95 value, I just borrowed it from a similar driver
(ptp_clockmatrix) that also employs a similar polling mechanism for
extts support. It seems to work pretty well in testing so I had no
reason to change it.

> 
> > <snip>
> --
> /Horatiu

[PATCH v2 3/3] dt-bindings: net: marvell,pp2: add extts docs

[Shmuel Hazan]

Add some documentation and example for enabling extts on the marvell
mvpp2 TAI.

Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>
---
 .../devicetree/bindings/net/marvell,pp2.yaml   | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/marvell,pp2.yaml b/Documentation/devicetree/bindings/net/marvell,pp2.yaml
index 4eadafc43d4f..5e4fc9c5dc92 100644
--- a/Documentation/devicetree/bindings/net/marvell,pp2.yaml
+++ b/Documentation/devicetree/bindings/net/marvell,pp2.yaml
@@ -31,6 +31,21 @@ properties:
   "#size-cells":
     const: 0
 
+  pinctrl-0: true
+  pinctrl-1: true
+
+  pinctrl-names:
+    description:
+      When present, must have one state named "default",
+      and may contain a second name named "extts". The former
+      state sets up pins for ordinary operation without extts
+      support whereas the latter state will enable receiving
+      external timestamp events.
+    minItems: 1
+    items:
+      - const: default
+      - const: extts
+
   clocks:
     minItems: 2
     items:
@@ -241,6 +256,9 @@ examples:
                  <&cp0_clk 1 5>, <&cp0_clk 1 6>, <&cp0_clk 1 18>;
         clock-names = "pp_clk", "gop_clk", "mg_clk", "mg_core_clk", "axi_clk";
         marvell,system-controller = <&cp0_syscon0>;
+        pinctrl-names = "default", "extts";
+        pinctrl-0 = <&cp1_mpp6_gpio>;
+        pinctrl-1 = <&cp1_mpp6_ptp>;
 
         ethernet-port@0 {
             interrupts = <ICU_GRP_NSR 39 IRQ_TYPE_LEVEL_HIGH>,
-- 
2.40.0

Re: [PATCH v2 3/3] dt-bindings: net: marvell,pp2: add extts docs

[Krzysztof Kozlowski]

On 17/04/2023 19:07, Shmuel Hazan wrote:
> Add some documentation and example for enabling extts on the marvell
> mvpp2 TAI.
> 
> Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed not only people but also list - at least DT one - so this
won't be tested.

Change looks good though, but still needs testing, so please resend.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: net: marvell,pp2: add extts docs

[Shmuel Hazan]

On Mon, 2023-04-17 at 19:47 +0200, Krzysztof Kozlowski wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments.
> 
> 
> On 17/04/2023 19:07, Shmuel Hazan wrote:
> > Add some documentation and example for enabling extts on the marvell
> > mvpp2 TAI.
> > 
> > Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> You missed not only people but also list - at least DT one - so this
> won't be tested.
> 
> Change looks good though, but still needs testing, so please resend.
> 

Hi Krzysztof,

Thanks for the feedback. I will resend the series later to the include
also the DT list and relevant maintainers. 

> Best regards,
> Krzysztof
> 

Re: [PATCH v2 0/3] net: mvpp2: tai: add extts support

[Jakub Kicinski]

On Mon, 17 Apr 2023 20:07:38 +0300 Shmuel Hazan wrote:
> This patch series adds support for PTP event capture on the Aramda
> 80x0/70x0. This feature is mainly used by tools linux ts2phc(3) in order
> to synchronize a timestamping unit (like the mvpp2's TAI) and a system
> DPLL on the same PCB. 
> 
> The patch series includes 3 patches: the second one implements the
> actual extts function.

Please wait at least 24h between resends.
Please read the rules:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

Re: [PATCH v2 0/3] net: mvpp2: tai: add extts support

[Shmuel Hazan]

On Mon, 2023-04-17 at 12:01 -0700, Jakub Kicinski wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments.
> 
> 
> On Mon, 17 Apr 2023 20:07:38 +0300 Shmuel Hazan wrote:
> > This patch series adds support for PTP event capture on the Aramda
> > 80x0/70x0. This feature is mainly used by tools linux ts2phc(3) in order
> > to synchronize a timestamping unit (like the mvpp2's TAI) and a system
> > DPLL on the same PCB.
> > 
> > The patch series includes 3 patches: the second one implements the
> > actual extts function.
> 
> Please wait at least 24h between resends.
> Please read the rules:
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

Hi Jakub,

Sorry about it. Thanks for letting me know and linking the rules. 

-- Shmuel. 

Re: [PATCH v2 0/3] net: mvpp2: tai: add extts support

[Andrew Lunn]

On Mon, Apr 17, 2023 at 08:07:38PM +0300, Shmuel Hazan wrote:
> This patch series adds support for PTP event capture on the Aramda
> 80x0/70x0. This feature is mainly used by tools linux ts2phc(3) in order
> to synchronize a timestamping unit (like the mvpp2's TAI) and a system
> DPLL on the same PCB. 
> 
> The patch series includes 3 patches: the second one implements the
> actual extts function.
> 
> Changes in v2:
> 	* Fixed a deadlock in the poll worker.
> 	* Removed tabs from comments.

The other think the NETDEV FAQ says is to wait at least 24 hours
before posting a new version. That give people time to actually review
the code, and likely the current version, not an old version.

    Andrew

Re: [PATCH v2 0/3] net: mvpp2: tai: add extts support

[Shmuel Hazan]

On Tue, 2023-04-18 at 02:36 +0200, Andrew Lunn wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments.
> 
> 
> On Mon, Apr 17, 2023 at 08:07:38PM +0300, Shmuel Hazan wrote:
> > This patch series adds support for PTP event capture on the Aramda
> > 80x0/70x0. This feature is mainly used by tools linux ts2phc(3) in order
> > to synchronize a timestamping unit (like the mvpp2's TAI) and a system
> > DPLL on the same PCB.
> > 
> > The patch series includes 3 patches: the second one implements the
> > actual extts function.
> > 
> > Changes in v2:
> >       * Fixed a deadlock in the poll worker.
> >       * Removed tabs from comments.
> 
> The other think the NETDEV FAQ says is to wait at least 24 hours
> before posting a new version. That give people time to actually review
> the code, and likely the current version, not an old version.
> 

Hi Andrew,

Sorry about it. Thanks for letting me know. 

>     Andrew