Re: [PATCH v1 net-next 3/3] net: stmmac: ptp: update tas basetime after ptp adjust

[Rui Sousa]

On 6/1/2021 10:38 AM, Xiaoliang Yang wrote:

Hi Xiaoliang,

> After adjusting the ptp time, the Qbv base time may be the past time
> of the new current time. dwmac5 hardware limited the base time cannot
> be set as past time. This patch calculate the base time and reset the
> Qbv configuration after ptp time adjust.
>
> Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@xxxxxxx>
> ---
> .../net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 41 ++++++++++++++++++-
> 1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c 
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 4e86cdf2bc9f..c573bc8b2595 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -62,7 +62,8 @@ static int stmmac_adjust_time(struct ptp_clock_info 
> *ptp, s64 delta)
>      u32 sec, nsec;
>      u32 quotient, reminder;
>      int neg_adj = 0;
> -    bool xmac;
> +    bool xmac, est_rst = false;
> +    int ret;
>
>      xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
>
> @@ -75,10 +76,48 @@ static int stmmac_adjust_time(struct ptp_clock_info 
> *ptp, s64 delta)
>      sec = quotient;
>      nsec = reminder;
>
> +    /* If EST is enabled, disabled it before adjust ptp time. */
> +    if (priv->plat->est && priv->plat->est->enable) {
> +        est_rst = true;
> +        mutex_lock(&priv->plat->est->lock);
> +        priv->plat->est->enable = false;
> +        stmmac_est_configure(priv, priv->ioaddr, priv->plat->est,
> +                     priv->plat->clk_ptp_rate);
> +        mutex_unlock(&priv->plat->est->lock);
> +    }
> +
>      spin_lock_irqsave(&priv->ptp_lock, flags);
>      stmmac_adjust_systime(priv, priv->ptpaddr, sec, nsec, neg_adj, xmac);
>      spin_unlock_irqrestore(&priv->ptp_lock, flags);
>
> +    /* Caculate new basetime and re-configured EST after PTP time 
> adjust. */
> +    if (est_rst) {
> +        struct timespec64 current_time, time;
> +        ktime_t current_time_ns, basetime;
> +        u64 cycle_time;
> +
> +        priv->ptp_clock_ops.gettime64(&priv->ptp_clock_ops, 
> &current_time);
> +        current_time_ns = timespec64_to_ktime(current_time);
> +        time.tv_nsec = priv->plat->est->btr[0];
> +        time.tv_sec = priv->plat->est->btr[1];

This time may no longer be what the user specified originally, it was 
adjusted based on the gptp time when the configuration was first made.
IMHO, if we want to respect the user configuration then we need to do 
the calculation here based on the original time.
Typically (using arbitrary units):
a) User configures basetime of 0, at gptp time 1000
b) btr is update to 1000, schedule starts
c) later, gptp time is updated to 500
d-1) with current patch, schedule will restart at 1000 (i.e remains 
disabled for 500)
d-2) with my suggestion, schedule will restart at 500 (which matches the 
user request, "start as soon as possible".

> +        basetime = timespec64_to_ktime(time);
> +        cycle_time = priv->plat->est->ctr[1] * NSEC_PER_SEC +
> +                 priv->plat->est->ctr[0];
> +        time = stmmac_calc_tas_basetime(basetime,
> +                        current_time_ns,
> +                        cycle_time);
> +
> +        mutex_lock(&priv->plat->est->lock);

Hmm... the locking needs to move up. Reading + writting btr/ctr should 
be atomic.

> +        priv->plat->est->btr[0] = (u32)time.tv_nsec;
> +        priv->plat->est->btr[1] = (u32)time.tv_sec;
> +        priv->plat->est->enable = true;
> +        ret = stmmac_est_configure(priv, priv->ioaddr, priv->plat->est,
> +                       priv->plat->clk_ptp_rate);
> +        mutex_unlock(&priv->plat->est->lock);
> +        if (ret)
> +            netdev_err(priv->dev, "failed to configure EST\n");
> +    }
> +
>      return 0;
> }

Br,
Rui Sousa