| Message ID | 20210411024028.14586-1-vee.khee.wong@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3,1/1] net: stmmac: Add support for external trigger timestamping | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: alexandre.torgue@foss.st.com rusaimi.amira.rusaimi@intel.com vee.khee.wong@intel.com qiangqing.zhang@nxp.com tee.min.tan@intel.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 22 this patch: 22 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 217 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 22 this patch: 22 |
| netdev/header_inline | success | Link |
On Sun, Apr 11, 2021 at 10:40:28AM +0800, Wong Vee Khee wrote: > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c > index 60566598d644..60e17fd24aba 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c > @@ -296,6 +296,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(&priv->aux_ts_lock); Lock, then ... > /* Enable Internal snapshot trigger */ > acr_value = readl(ptpaddr + PTP_ACR); > acr_value &= ~PTP_ACR_MASK; > @@ -321,6 +328,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(&priv->aux_ts_lock); unlock, then ... > /* Trigger Internal snapshot signal > * Create a rising edge by just toggle the GPO1 to low > @@ -355,6 +363,8 @@ static int intel_crosststamp(ktime_t *device, > *system = convert_art_to_tsc(art_time); > } > > + /* Release the mutex */ > + mutex_unlock(&priv->aux_ts_lock); unlock again? Huh? > 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; In the comment, please be specific about which data are protected. For example: /* Protects auxiliary snapshot registers from concurrent access. */ > @@ -163,6 +166,43 @@ static void get_ptptime(void __iomem *ptpaddr, u64 *ptp_time) > *ptp_time = ns; > } > > +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; Please group same types together (u32) in a one-line list. > + 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); Reading this register has a side effect of clearing status? If so, doesn't it need protection against concurrent access? The function, intel_crosststamp() also reads this bit. > + if (!priv->plat->ext_snapshot_en) > + return; Doesn't this test come too late? Setting ts_status just cleared the bit used by the other code path. > + num_snapshot = (ts_status & GMAC_TIMESTAMP_ATSNS_MASK) >> > + GMAC_TIMESTAMP_ATSNS_SHIFT; > + > + for (i = 0; i < num_snapshot; i++) { > + spin_lock_irqsave(&priv->ptp_lock, flags); > + get_ptptime(priv->ptpaddr, &ptp_time); > + spin_unlock_irqrestore(&priv->ptp_lock, flags); > + event.type = PTP_CLOCK_EXTTS; > + event.index = 0; > + event.timestamp = ptp_time; > + ptp_clock_event(priv->ptp_clock, &event); > + } > +} > + > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c > index b164ae22e35f..d668c33a0746 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c > @@ -135,9 +135,13 @@ static int stmmac_enable(struct ptp_clock_info *ptp, > { > struct stmmac_priv *priv = > container_of(ptp, struct stmmac_priv, ptp_clock_ops); > + void __iomem *ptpaddr = priv->ptpaddr; > + void __iomem *ioaddr = priv->hw->pcsr; > struct stmmac_pps_cfg *cfg; > int ret = -EOPNOTSUPP; > unsigned long flags; > + u32 intr_value; > + u32 acr_value; Please group same types together. Thanks, Richard
On Sun, Apr 11, 2021 at 08:10:55AM -0700, Richard Cochran wrote: > On Sun, Apr 11, 2021 at 10:40:28AM +0800, Wong Vee Khee wrote: > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c > > index 60566598d644..60e17fd24aba 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c > > @@ -296,6 +296,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(&priv->aux_ts_lock); > > Lock, then ... > > > /* Enable Internal snapshot trigger */ > > acr_value = readl(ptpaddr + PTP_ACR); > > acr_value &= ~PTP_ACR_MASK; > > @@ -321,6 +328,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(&priv->aux_ts_lock); > > unlock, then ... > > > /* Trigger Internal snapshot signal > > * Create a rising edge by just toggle the GPO1 to low > > @@ -355,6 +363,8 @@ static int intel_crosststamp(ktime_t *device, > > *system = convert_art_to_tsc(art_time); > > } > > > > + /* Release the mutex */ > > + mutex_unlock(&priv->aux_ts_lock); > > unlock again? Huh? > Nice catch. Fix in v4. > > 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; > > In the comment, please be specific about which data are protected. > For example: > > /* Protects auxiliary snapshot registers from concurrent access. */ > > > @@ -163,6 +166,43 @@ static void get_ptptime(void __iomem *ptpaddr, u64 *ptp_time) > > *ptp_time = ns; > > } > > > > +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; > > Please group same types together (u32) in a one-line list. > > > + 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); > > Reading this register has a side effect of clearing status? If so, > doesn't it need protection against concurrent access? > > The function, intel_crosststamp() also reads this bit. > The following check is introduced in intel_crosststamp() to avoid this: /* Both internal crosstimestamping and external triggered event * timestamping cannot be run concurrently. */ if (priv->plat->ext_snapshot_en) return -EBUSY; > > + if (!priv->plat->ext_snapshot_en) > > + return; > > Doesn't this test come too late? Setting ts_status just cleared the > bit used by the other code path. > As per Synopsys's design, all bits except Bits[27:25] gets cleared when this register is read. > > + num_snapshot = (ts_status & GMAC_TIMESTAMP_ATSNS_MASK) >> > > + GMAC_TIMESTAMP_ATSNS_SHIFT; > > + > > + for (i = 0; i < num_snapshot; i++) { > > + spin_lock_irqsave(&priv->ptp_lock, flags); > > + get_ptptime(priv->ptpaddr, &ptp_time); > > + spin_unlock_irqrestore(&priv->ptp_lock, flags); > > + event.type = PTP_CLOCK_EXTTS; > > + event.index = 0; > > + event.timestamp = ptp_time; > > + ptp_clock_event(priv->ptp_clock, &event); > > + } > > +} > > + > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c > > index b164ae22e35f..d668c33a0746 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c > > @@ -135,9 +135,13 @@ static int stmmac_enable(struct ptp_clock_info *ptp, > > { > > struct stmmac_priv *priv = > > container_of(ptp, struct stmmac_priv, ptp_clock_ops); > > + void __iomem *ptpaddr = priv->ptpaddr; > > + void __iomem *ioaddr = priv->hw->pcsr; > > struct stmmac_pps_cfg *cfg; > > int ret = -EOPNOTSUPP; > > unsigned long flags; > > + u32 intr_value; > > + u32 acr_value; > > Please group same types together. > Fix in v4. > Thanks, > Richard
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c index 60566598d644..60e17fd24aba 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c @@ -296,6 +296,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(&priv->aux_ts_lock); /* Enable Internal snapshot trigger */ acr_value = readl(ptpaddr + PTP_ACR); acr_value &= ~PTP_ACR_MASK; @@ -321,6 +328,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(&priv->aux_ts_lock); /* Trigger Internal snapshot signal * Create a rising edge by just toggle the GPO1 to low @@ -355,6 +363,8 @@ static int intel_crosststamp(ktime_t *device, *system = convert_art_to_tsc(art_time); } + /* Release the mutex */ + mutex_unlock(&priv->aux_ts_lock); system->cycles *= intel_priv->crossts_adj; return 0; @@ -520,6 +530,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..f6bdd3cde824 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c @@ -12,8 +12,11 @@ #include <linux/io.h> #include <linux/iopoll.h> #include <linux/delay.h> +#include <linux/ptp_clock_kernel.h> #include "common.h" #include "stmmac_ptp.h" +#include "dwmac4.h" +#include "stmmac.h" static void config_hw_tstamping(void __iomem *ioaddr, u32 data) { @@ -163,6 +166,43 @@ static void get_ptptime(void __iomem *ptpaddr, u64 *ptp_time) *ptp_time = ns; } +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) + return; + + num_snapshot = (ts_status & GMAC_TIMESTAMP_ATSNS_MASK) >> + GMAC_TIMESTAMP_ATSNS_SHIFT; + + for (i = 0; i < num_snapshot; i++) { + spin_lock_irqsave(&priv->ptp_lock, flags); + get_ptptime(priv->ptpaddr, &ptp_time); + spin_unlock_irqrestore(&priv->ptp_lock, flags); + event.type = PTP_CLOCK_EXTTS; + event.index = 0; + event.timestamp = ptp_time; + ptp_clock_event(priv->ptp_clock, &event); + } +} + const struct stmmac_hwtimestamp stmmac_ptp = { .config_hw_tstamping = config_hw_tstamping, .init_systime = init_systime, @@ -171,4 +211,5 @@ const struct stmmac_hwtimestamp stmmac_ptp = { .adjust_systime = adjust_systime, .get_systime = get_systime, .get_ptptime = get_ptptime, + .timestamp_interrupt = timestamp_interrupt, }; diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 77285646c5fc..9e57bc3d00a3 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -4989,6 +4989,8 @@ static void stmmac_common_interrupt(struct stmmac_priv *priv) else netif_carrier_off(priv->dev); } + + stmmac_timestamp_interrupt(priv, priv); } } diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c index b164ae22e35f..d668c33a0746 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c @@ -135,9 +135,13 @@ static int stmmac_enable(struct ptp_clock_info *ptp, { struct stmmac_priv *priv = container_of(ptp, struct stmmac_priv, ptp_clock_ops); + void __iomem *ptpaddr = priv->ptpaddr; + void __iomem *ioaddr = priv->hw->pcsr; struct stmmac_pps_cfg *cfg; int ret = -EOPNOTSUPP; unsigned long flags; + u32 intr_value; + u32 acr_value; switch (rq->type) { case PTP_CLK_REQ_PEROUT: @@ -159,6 +163,37 @@ static int stmmac_enable(struct ptp_clock_info *ptp, priv->systime_flags); spin_unlock_irqrestore(&priv->ptp_lock, flags); break; + case PTP_CLK_REQ_EXTTS: + priv->plat->ext_snapshot_en = on; + mutex_lock(&priv->aux_ts_lock); + acr_value = readl(ptpaddr + PTP_ACR); + acr_value &= ~PTP_ACR_MASK; + if (on) { + /* Enable External snapshot trigger */ + acr_value |= priv->plat->ext_snapshot_num; + acr_value |= PTP_ACR_ATSFC; + netdev_dbg(priv->dev, "Auxiliary Snapshot %d enabled.\n", + priv->plat->ext_snapshot_num >> + PTP_ACR_ATSEN_SHIFT); + /* Enable Timestamp Interrupt */ + intr_value = readl(ioaddr + GMAC_INT_EN); + intr_value |= GMAC_INT_TSIE; + writel(intr_value, ioaddr + GMAC_INT_EN); + + } else { + netdev_dbg(priv->dev, "Auxiliary Snapshot %d disabled.\n", + priv->plat->ext_snapshot_num >> + PTP_ACR_ATSEN_SHIFT); + /* Disable Timestamp Interrupt */ + intr_value = readl(ioaddr + GMAC_INT_EN); + intr_value &= ~GMAC_INT_TSIE; + writel(intr_value, ioaddr + GMAC_INT_EN); + } + writel(acr_value, ptpaddr + PTP_ACR); + mutex_unlock(&priv->aux_ts_lock); + ret = 0; + break; + default: break; } @@ -202,7 +237,7 @@ static struct ptp_clock_info stmmac_ptp_clock_ops = { .name = "stmmac ptp", .max_adj = 62500000, .n_alarm = 0, - .n_ext_ts = 0, + .n_ext_ts = 0, /* will be overwritten in stmmac_ptp_register */ .n_per_out = 0, /* will be overwritten in stmmac_ptp_register */ .n_pins = 0, .pps = 0, @@ -237,8 +272,10 @@ void stmmac_ptp_register(struct stmmac_priv *priv) stmmac_ptp_clock_ops.max_adj = priv->plat->ptp_max_adj; stmmac_ptp_clock_ops.n_per_out = priv->dma_cap.pps_out_num; + stmmac_ptp_clock_ops.n_ext_ts = priv->dma_cap.aux_snapshot_n; spin_lock_init(&priv->ptp_lock); + mutex_init(&priv->aux_ts_lock); priv->ptp_clock_ops = stmmac_ptp_clock_ops; priv->ptp_clock = ptp_clock_register(&priv->ptp_clock_ops, diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h index f88727ce4d30..53172a439810 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h @@ -73,6 +73,7 @@ #define PTP_ACR_ATSEN1 BIT(5) /* Auxiliary Snapshot 1 Enable */ #define PTP_ACR_ATSEN2 BIT(6) /* Auxiliary Snapshot 2 Enable */ #define PTP_ACR_ATSEN3 BIT(7) /* Auxiliary Snapshot 3 Enable */ +#define PTP_ACR_ATSEN_SHIFT 5 /* Auxiliary Snapshot shift */ #define PTP_ACR_MASK GENMASK(7, 4) /* Aux Snapshot Mask */ #define PMC_ART_VALUE0 0x01 /* PMC_ART[15:0] timer value */ #define PMC_ART_VALUE1 0x02 /* PMC_ART[31:16] timer value */ diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index e338ef7abc00..97edb31d6310 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -238,6 +238,8 @@ struct plat_stmmacenet_data { struct pci_dev *pdev; bool has_crossts; int int_snapshot_num; + int ext_snapshot_num; + bool ext_snapshot_en; bool multi_msi_en; int msi_mac_vec; int msi_wol_vec;