Re: [PATCH net-next v2 1/1] net: stmmac: Add support for external trigger timestamping

2021-04-10 Thread Wong Vee Khee 
On Fri, Apr 09, 2021 at 05:50:04PM -0700, Jakub Kicinski wrote:
> Other than the minor nit below LGTM. Let's give Richard one more day.
> 
> On Thu,  8 Apr 2021 01:04:42 +0800 Wong Vee Khee wrote:
> > +static void timestamp_interrupt(struct stmmac_priv *priv)
> > +{
> > +   struct ptp_clock_event event;
> > +   unsigned long flags;
> > +   u32 num_snapshot;
> > +   u32 ts_status;
> > +   u32 tsync_int;
> > +   u64 ptp_time;
> > +   int i;
> > +
> > +   tsync_int = readl(priv->ioaddr + GMAC_INT_STATUS) & GMAC_INT_TSIE;
> > +
> > +   if (!tsync_int)
> > +   return;
> > +
> > +   /* Read timestamp status to clear interrupt from either external
> > +* timestamp or start/end of PPS.
> > +*/
> > +   ts_status = readl(priv->ioaddr + GMAC_TIMESTAMP_STATUS);
> > +
> > +   if (priv->plat->ext_snapshot_en) {
> 
> Are you intending to add more code after this if? Otherwise you could
> flip the condition and return early instead of having the extra level
> of indentation.
>

Thanks fo the suggestion.
There's no plan to add more code after this as per STMMAC features that
required this interrupt. I will flip the condition.

> > +   num_snapshot = (ts_status & GMAC_TIMESTAMP_ATSNS_MASK) >>
> > +  GMAC_TIMESTAMP_ATSNS_SHIFT;
> > +
> > +   for (i = 0; i < num_snapshot; i++) {
> > +   spin_lock_irqsave(>ptp_lock, flags);
> > +   get_ptptime(priv->ptpaddr, _time);
> > +   spin_unlock_irqrestore(>ptp_lock, flags);
> > +   event.type = PTP_CLOCK_EXTTS;
> > +   event.index = 0;
> > +   event.timestamp = ptp_time;
> > +   ptp_clock_event(priv->ptp_clock, );
> > +   }
> > +   }
> > +}
> 
> Not really related to this patch but how does stmmac set IRQF_SHARED
> and yet not track if it indeed generated the interrupt? Isn't that
> against the rules?
>

Good point! Thanks for pointing that out. I looked at how STMMAC
interrupt handlers are coded, and indeed there are no tracking. Will
work on that and send as a seperate patch in near future.

Re: [PATCH net-next v2 1/1] net: stmmac: Add support for external trigger timestamping

2021-04-09 Thread Jakub Kicinski 
Other than the minor nit below LGTM. Let's give Richard one more day.

On Thu,  8 Apr 2021 01:04:42 +0800 Wong Vee Khee wrote:
> +static void timestamp_interrupt(struct stmmac_priv *priv)
> +{
> + struct ptp_clock_event event;
> + unsigned long flags;
> + u32 num_snapshot;
> + u32 ts_status;
> + u32 tsync_int;
> + u64 ptp_time;
> + int i;
> +
> + tsync_int = readl(priv->ioaddr + GMAC_INT_STATUS) & GMAC_INT_TSIE;
> +
> + if (!tsync_int)
> + return;
> +
> + /* Read timestamp status to clear interrupt from either external
> +  * timestamp or start/end of PPS.
> +  */
> + ts_status = readl(priv->ioaddr + GMAC_TIMESTAMP_STATUS);
> +
> + if (priv->plat->ext_snapshot_en) {

Are you intending to add more code after this if? Otherwise you could
flip the condition and return early instead of having the extra level
of indentation.

> + num_snapshot = (ts_status & GMAC_TIMESTAMP_ATSNS_MASK) >>
> +GMAC_TIMESTAMP_ATSNS_SHIFT;
> +
> + for (i = 0; i < num_snapshot; i++) {
> + spin_lock_irqsave(>ptp_lock, flags);
> + get_ptptime(priv->ptpaddr, _time);
> + spin_unlock_irqrestore(>ptp_lock, flags);
> + event.type = PTP_CLOCK_EXTTS;
> + event.index = 0;
> + event.timestamp = ptp_time;
> + ptp_clock_event(priv->ptp_clock, );
> + }
> + }
> +}

Not really related to this patch but how does stmmac set IRQF_SHARED
and yet not track if it indeed generated the interrupt? Isn't that
against the rules?

[PATCH net-next v2 1/1] net: stmmac: Add support for external trigger timestamping

2021-04-07 Thread Wong Vee Khee 
From: Tan Tee Min 

The Synopsis MAC controller supports auxiliary snapshot feature that
allows user to store a snapshot of the system time based on an external
event.

This patch add supports to the above mentioned feature. Users will be
able to triggered capturing the time snapshot from user-space using
application such as testptp or any other applications that uses the
PTP_EXTTS_REQUEST ioctl request.

Cc: Richard Cochran 
Signed-off-by: Tan Tee Min 
Co-developed-by: Wong Vee Khee 
Signed-off-by: Wong Vee Khee 
---
v1 -> v2:
Changed from pr_info() to netdev_dbg().

 .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 11 +
 drivers/net/ethernet/stmicro/stmmac/hwif.h|  5 +++
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  3 ++
 .../ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 40 +++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  2 +
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 39 +-
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.h  |  1 +
 include/linux/stmmac.h|  2 +
 8 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 8ba87c0be976..aee3e0b5fb46 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -286,6 +286,13 @@ static int intel_crosststamp(ktime_t *device,
 
intel_priv = priv->plat->bsp_priv;
 
+   /* Both internal crosstimestamping and external triggered event
+* timestamping cannot be run concurrently.
+*/
+   if (priv->plat->ext_snapshot_en)
+   return -EBUSY;
+
+   mutex_lock(>aux_ts_lock);
/* Enable Internal snapshot trigger */
acr_value = readl(ptpaddr + PTP_ACR);
acr_value &= ~PTP_ACR_MASK;
@@ -311,6 +318,7 @@ static int intel_crosststamp(ktime_t *device,
acr_value = readl(ptpaddr + PTP_ACR);
acr_value |= PTP_ACR_ATSFC;
writel(acr_value, ptpaddr + PTP_ACR);
+   mutex_unlock(>aux_ts_lock);
 
/* Trigger Internal snapshot signal
 * Create a rising edge by just toggle the GPO1 to low
@@ -345,6 +353,8 @@ static int intel_crosststamp(ktime_t *device,
*system = convert_art_to_tsc(art_time);
}
 
+   /* Release the mutex */
+   mutex_unlock(>aux_ts_lock);
system->cycles *= intel_priv->crossts_adj;
 
return 0;
@@ -510,6 +520,7 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
plat->mdio_bus_data->phy_mask |= 1 << INTEL_MGBE_XPCS_ADDR;
 
plat->int_snapshot_num = AUX_SNAPSHOT1;
+   plat->ext_snapshot_num = AUX_SNAPSHOT0;
 
plat->has_crossts = true;
plat->crosststamp = intel_crosststamp;
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h 
b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 2b5022ef1e52..2cc91759b91f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -504,6 +504,8 @@ struct stmmac_ops {
 #define stmmac_fpe_irq_status(__priv, __args...) \
stmmac_do_callback(__priv, mac, fpe_irq_status, __args)
 
+struct stmmac_priv;
+
 /* PTP and HW Timer helpers */
 struct stmmac_hwtimestamp {
void (*config_hw_tstamping) (void __iomem *ioaddr, u32 data);
@@ -515,6 +517,7 @@ struct stmmac_hwtimestamp {
   int add_sub, int gmac4);
void (*get_systime) (void __iomem *ioaddr, u64 *systime);
void (*get_ptptime)(void __iomem *ioaddr, u64 *ptp_time);
+   void (*timestamp_interrupt)(struct stmmac_priv *priv);
 };
 
 #define stmmac_config_hw_tstamping(__priv, __args...) \
@@ -531,6 +534,8 @@ struct stmmac_hwtimestamp {
stmmac_do_void_callback(__priv, ptp, get_systime, __args)
 #define stmmac_get_ptptime(__priv, __args...) \
stmmac_do_void_callback(__priv, ptp, get_ptptime, __args)
+#define stmmac_timestamp_interrupt(__priv, __args...) \
+   stmmac_do_void_callback(__priv, ptp, timestamp_interrupt, __args)
 
 /* Helpers to manage the descriptors for chain and ring modes */
 struct stmmac_mode_ops {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h 
b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index c49debb62b05..abadcd8cdc41 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -239,6 +239,9 @@ struct stmmac_priv {
int use_riwt;
int irq_wake;
spinlock_t ptp_lock;
+   /* Mutex lock for Auxiliary Snapshots */
+   struct mutex aux_ts_lock;
+
void __iomem *mmcaddr;
void __iomem *ptpaddr;
unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c 
b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index 113c51bcc0b5..ec7ec926f27c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++