| Message ID | 20241106090331.56519-5-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support external snapshots on dwmac1000 | expand |
On Wed, 6 Nov 2024 10:03:25 +0100 Maxime Chevallier wrote: > + mutex_unlock(&priv->aux_ts_lock); > + > + /* wait for auxts fifo clear to finish */ > + ret = readl_poll_timeout(ptpaddr + PTP_TCR, tcr_val, > + !(tcr_val & GMAC_PTP_TCR_ATSFC), > + 10, 10000); Is there a good reason to wait for the flush to complete outside of the mutex?
On 11/12/24 01:12, Jakub Kicinski wrote: > On Wed, 6 Nov 2024 10:03:25 +0100 Maxime Chevallier wrote: >> + mutex_unlock(&priv->aux_ts_lock); >> + >> + /* wait for auxts fifo clear to finish */ >> + ret = readl_poll_timeout(ptpaddr + PTP_TCR, tcr_val, >> + !(tcr_val & GMAC_PTP_TCR_ATSFC), >> + 10, 10000); > > Is there a good reason to wait for the flush to complete outside of > the mutex? Indeed looking at other `ptpaddr` access use-case, it looks like the mutex protects both read and write accesses. @Maxime: is the above intentional? looks race-prone Thanks, Paolo
Hello Jakub, Paolo, On Tue, 12 Nov 2024 10:28:21 +0100 Paolo Abeni <pabeni@redhat.com> wrote: > On 11/12/24 01:12, Jakub Kicinski wrote: > > On Wed, 6 Nov 2024 10:03:25 +0100 Maxime Chevallier wrote: > >> + mutex_unlock(&priv->aux_ts_lock); > >> + > >> + /* wait for auxts fifo clear to finish */ > >> + ret = readl_poll_timeout(ptpaddr + PTP_TCR, tcr_val, > >> + !(tcr_val & GMAC_PTP_TCR_ATSFC), > >> + 10, 10000); > > > > Is there a good reason to wait for the flush to complete outside of > > the mutex? > > Indeed looking at other `ptpaddr` access use-case, it looks like the > mutex protects both read and write accesses. > > @Maxime: is the above intentional? looks race-prone You're right, this is racy... It wasn't intentionnal, it's actually the same logic as dwmac4 uses so looks like dwmac4 is also incorrect in that regard. I'll send a v4 with that change, and a fix for dwmac4 along the way then. Thanks for spotting this, Maxime
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h index 4a0a1708c391..6f68a6b298c9 100644 --- a/drivers/net/ethernet/stmicro/stmmac/common.h +++ b/drivers/net/ethernet/stmicro/stmmac/common.h @@ -552,6 +552,7 @@ extern const struct stmmac_hwtimestamp stmmac_ptp; extern const struct stmmac_mode_ops dwmac4_ring_mode_ops; extern const struct ptp_clock_info stmmac_ptp_clock_ops; +extern const struct ptp_clock_info dwmac1000_ptp_clock_ops; struct mac_link { u32 caps; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h index 4296ddda8aaa..01eafeb1272f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h @@ -329,5 +329,10 @@ enum rtc_control { #define GMAC_MMC_RX_CSUM_OFFLOAD 0x208 #define GMAC_EXTHASH_BASE 0x500 +/* PTP and timestamping registers */ + +#define GMAC_PTP_TCR_ATSFC BIT(24) +#define GMAC_PTP_TCR_ATSEN0 BIT(25) + extern const struct stmmac_dma_ops dwmac1000_dma_ops; #endif /* __DWMAC1000_H__ */ diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c index d413d76a8936..b6930009ea06 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c @@ -18,6 +18,7 @@ #include <linux/io.h> #include "stmmac.h" #include "stmmac_pcs.h" +#include "stmmac_ptp.h" #include "dwmac1000.h" static void dwmac1000_core_init(struct mac_device_info *hw, @@ -551,3 +552,47 @@ int dwmac1000_setup(struct stmmac_priv *priv) return 0; } + +/* DWMAC 1000 ptp_clock_info ops */ + +int dwmac1000_ptp_enable(struct ptp_clock_info *ptp, + struct ptp_clock_request *rq, int on) +{ + struct stmmac_priv *priv = + container_of(ptp, struct stmmac_priv, ptp_clock_ops); + void __iomem *ptpaddr = priv->ptpaddr; + int ret = -EOPNOTSUPP; + u32 tcr_val; + + switch (rq->type) { + case PTP_CLK_REQ_EXTTS: + mutex_lock(&priv->aux_ts_lock); + tcr_val = readl(ptpaddr + PTP_TCR); + + if (on) { + tcr_val |= GMAC_PTP_TCR_ATSEN0; + tcr_val |= GMAC_PTP_TCR_ATSFC; + priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN; + } else { + tcr_val &= ~GMAC_PTP_TCR_ATSEN0; + priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN; + } + + netdev_dbg(priv->dev, "Auxiliary Snapshot %s.\n", + on ? "enabled" : "disabled"); + writel(tcr_val, ptpaddr + PTP_TCR); + + mutex_unlock(&priv->aux_ts_lock); + + /* wait for auxts fifo clear to finish */ + ret = readl_poll_timeout(ptpaddr + PTP_TCR, tcr_val, + !(tcr_val & GMAC_PTP_TCR_ATSFC), + 10, 10000); + break; + + default: + break; + } + + return ret; +} diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c index 47458cbcbc94..1f508843fb5a 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c @@ -135,7 +135,7 @@ static const struct stmmac_hwif_entry { .dma = &dwmac100_dma_ops, .mac = &dwmac100_ops, .hwtimestamp = &stmmac_ptp, - .ptp = &stmmac_ptp_clock_ops, + .ptp = &dwmac1000_ptp_clock_ops, .mode = NULL, .tc = NULL, .mmc = &dwmac_mmc_ops, @@ -154,7 +154,7 @@ static const struct stmmac_hwif_entry { .dma = &dwmac1000_dma_ops, .mac = &dwmac1000_ops, .hwtimestamp = &stmmac_ptp, - .ptp = &stmmac_ptp_clock_ops, + .ptp = &dwmac1000_ptp_clock_ops, .mode = NULL, .tc = NULL, .mmc = &dwmac_mmc_ops, diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c index 8ea2b4226234..430905f591b2 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c @@ -282,6 +282,24 @@ const struct ptp_clock_info stmmac_ptp_clock_ops = { .getcrosststamp = stmmac_getcrosststamp, }; +/* structure describing a PTP hardware clock */ +const struct ptp_clock_info dwmac1000_ptp_clock_ops = { + .owner = THIS_MODULE, + .name = "stmmac ptp", + .max_adj = 62500000, + .n_alarm = 0, + .n_ext_ts = 1, + .n_per_out = 0, + .n_pins = 0, + .pps = 0, + .adjfine = stmmac_adjust_freq, + .adjtime = stmmac_adjust_time, + .gettime64 = stmmac_get_time, + .settime64 = stmmac_set_time, + .enable = dwmac1000_ptp_enable, + .getcrosststamp = stmmac_getcrosststamp, +}; + /** * stmmac_ptp_register * @priv: driver private structure diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h index fce3fba2ffd2..fa4611855311 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h @@ -94,4 +94,10 @@ enum aux_snapshot { AUX_SNAPSHOT3 = 0x80, }; +struct ptp_clock_info; +struct ptp_clock_request; + +int dwmac1000_ptp_enable(struct ptp_clock_info *ptp, + struct ptp_clock_request *rq, int on); + #endif /* __STMMAC_PTP_H__ */
The PTP configuration for GMAC3_X differs from the other implementations in several ways : - There's only one external snapshot trigger - The snapshot configuration is done through the PTP_TCR register, whereas the other dwmac variants have a dedicated ACR (auxiliary control reg) for that purpose - The layout for the PTP_TCR register also differs, as bits 24/25 are used for the snapshot configuration. These bits are reserved on other variants. On GMAC3_X, we also can't discover the number of snapshot triggers automatically. The GMAC3_X has one PPS output, however it's configuration isn't supported yet so report 0 n_per_out for now. Introduce a dedicated set of ptp_clock_info ops and configuration parameters to reflect these differences specific to GMAC3_X. This was tested on dwmac_socfpga. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- drivers/net/ethernet/stmicro/stmmac/common.h | 1 + .../net/ethernet/stmicro/stmmac/dwmac1000.h | 5 +++ .../ethernet/stmicro/stmmac/dwmac1000_core.c | 45 +++++++++++++++++++ drivers/net/ethernet/stmicro/stmmac/hwif.c | 4 +- .../net/ethernet/stmicro/stmmac/stmmac_ptp.c | 18 ++++++++ .../net/ethernet/stmicro/stmmac/stmmac_ptp.h | 6 +++ 6 files changed, 77 insertions(+), 2 deletions(-)