Subject: Re: [PATCH net] bnxt_en: reset PHC frequency in free-running mode

From: Vadim Fedorenko <[email-masked]>
Date: Tue, 7 Mar 2023 10:25:04 +0000

On 07/03/2023 05:07, Pavan Chebbi wrote:
> On Tue, Mar 7, 2023 at 12:43 AM Vadim Fedorenko
> <[email-masked]> wrote:
>>
>> On 06/03/2023 17:11, Pavan Chebbi wrote:
>>> On Mon, Mar 6, 2023 at 10:23 PM Vadim Fedorenko <[email-masked]> wrote:
>>>>
>>>> @@ -932,13 +937,15 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
>>>>           atomic_set(&ptp->tx_avail, BNXT_MAX_TX_TS);
>>>>           spin_lock_init(&ptp->ptp_lock);
>>>>
>>>> -       if (bp->fw_cap & BNXT_FW_CAP_PTP_RTC) {
>>>> +       if (BNXT_PTP_RTC(ptp->bp)) {
>>>>                   bnxt_ptp_timecounter_init(bp, false);
>>>>                   rc = bnxt_ptp_init_rtc(bp, phc_cfg);
>>>>                   if (rc)
>>>>                           goto out;
>>>>           } else {
>>>>                   bnxt_ptp_timecounter_init(bp, true);
>>>> +               if (bp->fw_cap & BNXT_FW_CAP_PTP_RTC)
>>>> +                       bnxt_ptp_adjfreq_rtc(bp, 0);
> 
> I am not sure if the intended objective of resetting the PHC is going
> to be achieved with this. The FW will always apply the new ppb on the
> base PHC frequency. I am not sure what you mean by "reset the hardware
> frequency of PHC to zero" in the commit message.

Well, I meant reset it to base frequency and remove any adjustments 
applied before. I'll re-phrase it in the next version.

> If you want PHC to
> start counting from 0 on init, you may use bnxt_ptp_cfg_settime() with
> 0.

That's not what we want, the current counter behavior is ok.

> Also, the RTC flag may not be set on newer firmwares running on MH
> systems. I see no harm in resetting the PHC unconditionally if we are
> not in RTC mode.

And that's perfectly fine! Each firmware upgrade requires full NIC reset 
and it will end up with reset of PHC to base frequency. But in the 
situation when we have several version of kernel running on the SLED, on 
kernel upgrade we end up with adjustment stored in the PHC using old 
kernel which was tuning RTC, but then booted into new kernel which stops 
tuning RTC. If the last stored adjustment was close to boundaries for 
some reasons, timecounter will never compensate the difference and all 
hosts in the sled will be drifting hard. The only way to bring it back 
to working state is to power reset the sled, which is disruptive action.

The fix was proved to work actually in our setup.

>>>
>>> You meant bnxt_ptp_adjfine_rtc(), right.
>>> Anyway, let me go through the patch in detail, while you may submit
>>> corrections for the build.
>>>
>> Oh, yeah, right, artefact of rebasing. Will fix it in v2, thanks.
>>
>>
>>>>           }
>>>>
>>>>           ptp->ptp_info = bnxt_ptp_caps;
>>>> --
>>>> 2.30.2
>>>>
>>