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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 09/03/2023 10:11, Pavan Chebbi wrote:
> On Thu, Mar 9, 2023 at 3:02 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 09.03.2023 04:40, Pavan Chebbi wrote:
>>> On Wed, Mar 8, 2023 at 8:12 PM Vadim Fedorenko <vadfed@meta.com> 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_USE_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)
>>>
>>> I understand from your response on v1 as to why it will not affect you
>>> if a new firmware does not report RTC on MH.
>>> However, once you update the fw, any subsequent kernels upgrades will
>>> prevent resetting the freq stored in the PHC.
>>> Would changing the check to if (BNXT_MH(bp)) instead be a better option?
>>
>> How will it affect hardware without RTC support? The one which doesn't have
>> BNXT_FW_CAP_PTP_RTC in a single-host configuration. Asking because if FW will
> 
> For single hosts, it should not matter if we reset the PHC frequency.
> bnxt_ptp_init() is [re]initializing the host-timecounter, and this
> function being called on a single host means everything is going to
> [re]start from scratch.
> 
>> not expose BNXT_FW_CAP_PTP_RTC, the check BNXT_PTP_USE_RTC() will be equal to
>> !BNXT_MH() and there will be no need for additional check in this else clause.
> 
> You are right, hence my original suggestion of resetting the PHC freq
> unconditionally is better.
> One more thing, the function bnxt_ptp_adjfine() should select
> non-realtime adjustments only for MH systems.
> You may need to flip the check, something like
> 
> if (!BNXT_MH(ptp->bp))
>      return bnxt_ptp_adjfine_rtc(bp, scaled_ppm);
> 
> This is because there can be a very old firmware which does not have
> RTC on single hosts but we still want to make HW adjustments.

Well, I just want to be sure that we will support all possible 
combinations of FW in the driver. AFAIU, there 3 different options:

1. Very old firmware. Doesn't have RTC support and doesn't expose 
BNXT_FW_CAP_PTP_RTC. Call to bnxt_ptp_adjfine_rtc on this variant may 
make HW unusable. We MUST not call it in this case. The timecounter is 
also not supported in this configuration, right?
2. Firmware which supports RTC and exposes BNXT_FW_CAP_PTP_RTC. 
Timecounter should be used only in MH case then.
3. Firmware which supports RTC, but doesn't expose BNXT_FW_CAP_PTP_RTC 
for MH configuration. How can we understand that it's not variant 1 in 
MH configuration? Or are we sure FUNC_QCAPS_RESP_FLAGS_PTP_SUPPORTED is 
not set on old firmware?