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

[Rui Sousa <rui.sousa@nxp.com>]

On 6/9/2021 11:03 AM, Xiaoliang Yang wrote:
> Hi Rui,
>

Hi,

> On 2021-06-02 18:18, Rui Sousa wrote:
>>> 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@nxp.com>
>>> ---
>>> .../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".
>>
> It is not the correct operation sequence for the user to configure Qbv before ptp clock synchronization.

The way I see it, a ptp clock discontinuity may happen at any time 
(change of grand master, grand master discontinuity, ...) outside of the 
user control. So having the driver handle all corner cases looked like a 
good solution to me.

> After adjusting the ptp clock time, it is no longer possible to determine whether the previously set basetime is what the user wants.

I may be assuming too much, but usually a Qbv schedule is determined by 
some central identity based on an absolute time (not related to the 
current time in the endpoint). So with this assumption I was considering 
the time specified by the user as "correct", independently of the 
current local ptp time.

> I think our driver only needs to ensure that the set basetime meets the hardware regulations, and the hardware can work normally. So I only updated the past basetime.

Understood, but from the point of view of the user I think the case you 
are already handling and the one I mentioned are very similar. Qbv 
schedule doesn't work as intended after the clock jump. Also, my 
suggestion simplifies a bit the code (no conversion from hardware to 
ktime), at the cost of adding extra data (software backup of the 
original user ktime).

> I am not sure if it is appropriate to reset EST configure in the PTP driver,

I'm not 100% sure either and I was hoping to see other people comments 
(and I haven't checked yet how other drivers are handling this).
That said, to handle this properly in userspace, IMHO you would need:
- some process monitoring the ptp clock and detecting jumps
- the process would need to be aware of the current Qbv schedule
- when a jump is detected, re-do the Qbv configuration

Overall, handling the issue transparently in the driver, seems like a 
better solution.

> but this case will cause the hardware to not work.
> 
>>> +        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.
> I will modify this.
>>
>>> +        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;
>>> }
> 
> Thanks,
> xiaoliang
> 

Br,
Rui

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel