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 <[email protected]>
> Signed-off-by: Ahmad Fatoum <[email protected]>
> Signed-off-by: Holger Assmann <[email protected]>
> ---
> .../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
On Wed, Dec 16, 2020 at 05:13:34PM -0800, Jakub Kicinski wrote:
> 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.
+1 for keeping the PHC time continuous.
> Please remember to CC Richard, the PTP maintainer.
+1 to that, too!
Thanks,
Richard
Hello,
On 17.12.20 02:13, Jakub Kicinski wrote:
>> + 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?
To get a symbolic error name if support is compiled in (note the `e' after %p).
>
> 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?
Indeed, omitting the if condition here will lead to a needless warning on every reset.
Cheers,
Ahmad
>
>> /* reset the phy so that it's ready */
>> if (priv->mii)
>> stmmac_mdio_reset(priv->mii);
>>
>> base-commit: 3db1a3fa98808aa90f95ec3e0fa2fc7abf28f5c9
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, 17 Dec 2020 09:25:48 +0100 Ahmad Fatoum wrote:
> On 17.12.20 02:13, Jakub Kicinski wrote:
> >> + 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?
>
> To get a symbolic error name if support is compiled in (note the `e' after %p).
Cool, GTK. Kind of weird we there is no equivalent int decorator, tho.
Do you happen to know why?
On 17.12.20 18:59, Jakub Kicinski wrote:
> On Thu, 17 Dec 2020 09:25:48 +0100 Ahmad Fatoum wrote:
>> On 17.12.20 02:13, Jakub Kicinski wrote:
>>>> + 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?
>>
>> To get a symbolic error name if support is compiled in (note the `e' after %p).
>
> Cool, GTK. Kind of weird we there is no equivalent int decorator, tho.
> Do you happen to know why?
New format-specifiers should be using %p<extension>, which is already established,
said the reviewers:
https://lore.kernel.org/lkml/[email protected]/
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, 17.12.20 um 02:13 Jakub Kicinski wrote:
>
> Thanks for the patch, minor nits below.
Thanks for the Feedback! I will work it in for my v2.
>> +
>> + if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp))
>
> !a && !b reads better IMHO
We've chosen this variant because it is already used this way in
stmmac_main (e.g. in stmmac_hwtstamp_set(), stmmac_hwtstamp_get(), or
stmmac_validate()).
>> @@ -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?
This was not intended. Will be fixed in v2 to be optional again.
Regards,
Holger