On Wed, 16 Dec 2020 12:32:38 +0100 Holger Assmann wrote:
> As it is, valid SIOCSHWTSTAMP ioctl calls - i.e. enable/disable time
> stamping or changing filter settings - lead to synchronization of the
> NIC's hardware clock with CLOCK_REALTIME. This might be necessary
> during system initialization, but at runtime, when the PTP clock has
> already been synchronized to a grand master, a reset of the timestamp
> settings might result in a clock jump.
>
> This further differs from how drivers like IGB and FEC behave: Those
> initialize the PTP system time less frequently - on interface up and
> at probe time, respectively.
>
> We consequently introduce the new function stmmac_init_hwtstamp(), which
> gets called during ndo_open(). It contains the code snippet moved
> from stmmac_hwtstamp_set() that manages the time synchronization. Besides,
> the sub second increment configuration is also moved here since the
> related values are hardware dependent and do not change during runtime.
>
> Furthermore, the hardware clock must be kept running even when no time
> stamping mode is selected in order to retain the once synced time basis.
> That way, time stamping can be enabled again at any time only with the
> need for compensation of the clock's natural drifting.
>
> As a side effect, this patch fixes a potential race between SIOCSHWTSTAMP
> and ptp_clock_info::enable regarding priv->systime_flags. Subsequently,
> since this variable becomes deprecated by this commit, it should be
> removed completely in a follow-up patch.
>
> Fixes: 92ba6888510c ("stmmac: add the support for PTP hw clock driver")
> Fixes: cc4c9001ce31 ("net: stmmac: Switch stmmac_hwtimestamp to generic
> HW Interface Helpers")
>
Thanks for the patch, minor nits below.
If you post a v2 please don't wrap fixes tags and no space between
those and the other tags.
Also please put the tree in the subject, like [PATCH net 1/2].
Please remember to CC Richard, the PTP maintainer.
> Reported-by: Michael Olbrich <m.olbr...@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fat...@pengutronix.de>
> Signed-off-by: Holger Assmann <h.assm...@pengutronix.de>
> ---
> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 121 ++++++++++++------
> 1 file changed, 80 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 5b1c12ff98c0..55f5e6cd1cad 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -46,6 +46,13 @@
> #include "dwxgmac2.h"
> #include "hwif.h"
>
> +
Spurious new line
> +/* As long the interface is active, we keep the timestamping HW enabled with
> + * fine resolution and binary rollover. This avoid non-monotonic behavior
> + * when changing timestamp settings at runtime
> + * */
The */ should be on a line of its own.
> +#define STMMAC_HWTS_ACTIVE (PTP_TCR_TSENA | PTP_TCR_TSCFUPDT |
> PTP_TCR_TSCTRLSSR)
> +
> #define STMMAC_ALIGN(x) ALIGN(ALIGN(x, SMP_CACHE_BYTES), 16)
> #define TSO_MAX_BUFF_SIZE (SZ_16K - 1)
> @@ -791,6 +772,63 @@ static void stmmac_release_ptp(struct stmmac_priv *priv)
> stmmac_ptp_unregister(priv);
> }
>
> +/**
> + * stmmac_init_hwtstamp - init Timestamping Hardware
> + * @priv: driver private structure
> + * Description: Initialize hardware for Timestamping use
> + * This is valid as long as the interface is open and not suspended.
> + * Will be rerun after resume from suspension.
> + */
> +static int stmmac_init_hwtstamp(struct stmmac_priv *priv)
> +{
> + bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
> + struct timespec64 now;
> + u32 sec_inc = 0;
> + u64 temp = 0;
> + u32 value;
> + int ret;
> +
> + ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> + if (ret < 0) {
> + netdev_warn(priv->dev, "failed to enable PTP reference clock:
> %d\n", ret);
> + return ret;
> + }
> +
> + if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp))
!a && !b reads better IMHO
> + return -EOPNOTSUPP;
> +
> + value = STMMAC_HWTS_ACTIVE;
> + stmmac_config_hw_tstamping(priv, priv->ptpaddr, value);
> +
> + /* program Sub Second Increment reg */
> + stmmac_config_sub_second_increment(priv,
> + priv->ptpaddr, priv->plat->clk_ptp_rate,
> + xmac, &sec_inc);
Now that this code is not indented as much any more please align the
continuation lines under the opening bracket.
> + temp = div_u64(1000000000ULL, sec_inc);
> +
> + /* Store sub second increment and flags for later use */
> + priv->sub_second_inc = sec_inc;
> + priv->systime_flags = value;
> +
> + /* calculate default added value:
> + * formula is :
> + * addend = (2^32)/freq_div_ratio;
> + * where, freq_div_ratio = 1e9ns/sec_inc
> + */
> + temp = (u64)(temp << 32);
> + priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);
> + stmmac_config_addend(priv, priv->ptpaddr, priv->default_addend);
> +
> + /* initialize system time */
> + ktime_get_real_ts64(&now);
> +
> + /* lower 32 bits of tv_sec are safe until y2106 */
> + stmmac_init_systime(priv, priv->ptpaddr,
> + (u32)now.tv_sec, now.tv_nsec);
ditto
> +
> + return 0;
> +}
> +
> /**
> * stmmac_mac_flow_ctrl - Configure flow control in all queues
> * @priv: driver private structure
> @@ -2713,15 +2751,17 @@ static int stmmac_hw_setup(struct net_device *dev,
> bool init_ptp)
> stmmac_mmc_setup(priv);
>
> if (init_ptp) {
> - ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> - if (ret < 0)
> - netdev_warn(priv->dev, "failed to enable PTP reference
> clock: %d\n", ret);
> -
> - ret = stmmac_init_ptp(priv);
> - if (ret == -EOPNOTSUPP)
> - netdev_warn(priv->dev, "PTP not supported by HW\n");
> - else if (ret)
> - netdev_warn(priv->dev, "PTP init failed\n");
> + ret = stmmac_init_hwtstamp(priv);
> + if (ret) {
> + netdev_warn(priv->dev, "HW Timestamping init failed:
> %pe\n",
> + ERR_PTR(ret));
why convert to ERR_PTR and use %pe and not just %d?
also continuation misaligned
> + } else {
> + ret = stmmac_init_ptp(priv);
> + if (ret == -EOPNOTSUPP)
> + netdev_warn(priv->dev, "PTP not supported by
> HW\n");
> + else if (ret)
> + netdev_warn(priv->dev, "PTP init failed\n");
> + }
> }
>
> priv->eee_tw_timer = STMMAC_DEFAULT_TWT_LS;
> @@ -5290,8 +5330,7 @@ int stmmac_resume(struct device *dev)
> /* enable the clk previously disabled */
> clk_prepare_enable(priv->plat->stmmac_clk);
> clk_prepare_enable(priv->plat->pclk);
> - if (priv->plat->clk_ptp_ref)
> - clk_prepare_enable(priv->plat->clk_ptp_ref);
> + stmmac_init_hwtstamp(priv);
This was optional, now you always init?
> /* reset the phy so that it's ready */
> if (priv->mii)
> stmmac_mdio_reset(priv->mii);
>
> base-commit: 3db1a3fa98808aa90f95ec3e0fa2fc7abf28f5c9
