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

From: Vadim Fedorenko <[email-masked]>
Date: Mon, 6 Mar 2023 19:12:59 +0000

On 06/03/2023 17:11, Pavan Chebbi wrote:
> On Mon, Mar 6, 2023 at 10:23 PM Vadim Fedorenko <[email-masked]> wrote:
>>
>> When using a PHC in shared between multiple hosts, the previous
>> frequency value may not be reset and could lead to host being unable to
>> compensate the offset with timecounter adjustments. To avoid such state
>> reset the hardware frequency of PHC to zero on init. Some refactoring is
>> needed to make code readable.
>>
> Thanks for the patch.
> I see what you are trying to do. But I think we have some build issues
> with this.
> Haven't looked at the whole patch, but one error I can spot is down at
> the bottom.
> 
>> Fixes: 85036aee1938 ("bnxt_en: Add a non-real time mode to access NIC clock")
>> Signed-off-by: Vadim Fedorenko <[email-masked]>
>> ---
>>   drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  6 +-
>>   drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  2 +
>>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 57 +++++++++++--------
>>   3 files changed, 36 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> index 5d4b1f2ebeac..8472ff79adf3 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -6989,11 +6989,9 @@ static int bnxt_hwrm_func_qcfg(struct bnxt *bp)
>>                  if (flags & FUNC_QCFG_RESP_FLAGS_FW_DCBX_AGENT_ENABLED)
>>                          bp->fw_cap |= BNXT_FW_CAP_DCBX_AGENT;
>>          }
>> -       if (BNXT_PF(bp) && (flags & FUNC_QCFG_RESP_FLAGS_MULTI_HOST)) {
>> +       if (BNXT_PF(bp) && (flags & FUNC_QCFG_RESP_FLAGS_MULTI_HOST))
>>                  bp->flags |= BNXT_FLAG_MULTI_HOST;
>> -               if (bp->fw_cap & BNXT_FW_CAP_PTP_RTC)
>> -                       bp->fw_cap &= ~BNXT_FW_CAP_PTP_RTC;
>> -       }
>> +
>>          if (flags & FUNC_QCFG_RESP_FLAGS_RING_MONITOR_ENABLED)
>>                  bp->fw_cap |= BNXT_FW_CAP_RING_MONITOR;
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>> index dcb09fbe4007..41e4bb7b8acb 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>> @@ -2000,6 +2000,8 @@ struct bnxt {
>>          u32                     fw_dbg_cap;
>>
>>   #define BNXT_NEW_RM(bp)                ((bp)->fw_cap & BNXT_FW_CAP_NEW_RM)
>> +#define BNXT_PTP_RTC(bp)       (!BNXT_MH(bp) && \
>> +                                ((bp)->fw_cap & BNXT_FW_CAP_PTP_RTC))
>>          u32                     hwrm_spec_code;
>>          u16                     hwrm_cmd_seq;
>>          u16                     hwrm_cmd_kong_seq;
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> index 4ec8bba18cdd..99c1a53231aa 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> @@ -63,7 +63,7 @@ static int bnxt_ptp_settime(struct ptp_clock_info *ptp_info,
>>                                                  ptp_info);
>>          u64 ns = timespec64_to_ns(ts);
>>
>> -       if (ptp->bp->fw_cap & BNXT_FW_CAP_PTP_RTC)
>> +       if (BNXT_PTP_RTC(ptp->bp))
>>                  return bnxt_ptp_cfg_settime(ptp->bp, ns);
>>
>>          spin_lock_bh(&ptp->ptp_lock);
>> @@ -196,7 +196,7 @@ static int bnxt_ptp_adjtime(struct ptp_clock_info *ptp_info, s64 delta)
>>          struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg,
>>                                                  ptp_info);
>>
>> -       if (ptp->bp->fw_cap & BNXT_FW_CAP_PTP_RTC)
>> +       if (BNXT_PTP_RTC(ptp->bp))
>>                  return bnxt_ptp_adjphc(ptp, delta);
>>
>>          spin_lock_bh(&ptp->ptp_lock);
>> @@ -205,34 +205,39 @@ static int bnxt_ptp_adjtime(struct ptp_clock_info *ptp_info, s64 delta)
>>          return 0;
>>   }
>>
>> +static int bnxt_ptp_adjfine_rtc(struct bnxt *bp, long scaled_ppm)
>> +{
>> +       s32 ppb = scaled_ppm_to_ppb(scaled_ppm);
>> +       struct hwrm_port_mac_cfg_input *req;
>> +       int rc;
>> +
>> +       rc = hwrm_req_init(bp, req, HWRM_PORT_MAC_CFG);
>> +       if (rc)
>> +               return rc;
>> +
>> +       req->ptp_freq_adj_ppb = cpu_to_le32(ppb);
>> +       req->enables = cpu_to_le32(PORT_MAC_CFG_REQ_ENABLES_PTP_FREQ_ADJ_PPB);
>> +       rc = hwrm_req_send(bp, req);
>> +       if (rc)
>> +               netdev_err(bp->dev,
>> +                          "ptp adjfine failed. rc = %d\n", rc);
>> +       return rc;
>> +}
>> +
>>   static int bnxt_ptp_adjfine(struct ptp_clock_info *ptp_info, long scaled_ppm)
>>   {
>>          struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg,
>>                                                  ptp_info);
>> -       struct hwrm_port_mac_cfg_input *req;
>>          struct bnxt *bp = ptp->bp;
>> -       int rc = 0;
>>
>> -       if (!(ptp->bp->fw_cap & BNXT_FW_CAP_PTP_RTC)) {
>> -               spin_lock_bh(&ptp->ptp_lock);
>> -               timecounter_read(&ptp->tc);
>> -               ptp->cc.mult = adjust_by_scaled_ppm(ptp->cmult, scaled_ppm);
>> -               spin_unlock_bh(&ptp->ptp_lock);
>> -       } else {
>> -               s32 ppb = scaled_ppm_to_ppb(scaled_ppm);
>> -
>> -               rc = hwrm_req_init(bp, req, HWRM_PORT_MAC_CFG);
>> -               if (rc)
>> -                       return rc;
>> +       if (BNXT_PTP_RTC(ptp->bp))
>> +               return bnxt_ptp_adjfine_rtc(bp, scaled_ppm);
>>
>> -               req->ptp_freq_adj_ppb = cpu_to_le32(ppb);
>> -               req->enables = cpu_to_le32(PORT_MAC_CFG_REQ_ENABLES_PTP_FREQ_ADJ_PPB);
>> -               rc = hwrm_req_send(ptp->bp, req);
>> -               if (rc)
>> -                       netdev_err(ptp->bp->dev,
>> -                                  "ptp adjfine failed. rc = %d\n", rc);
>> -       }
>> -       return rc;
>> +       spin_lock_bh(&ptp->ptp_lock);
>> +       timecounter_read(&ptp->tc);
>> +       ptp->cc.mult = adjust_by_scaled_ppm(ptp->cmult, scaled_ppm);
>> +       spin_unlock_bh(&ptp->ptp_lock);
>> +       return 0;
>>   }
>>
>>   void bnxt_ptp_pps_event(struct bnxt *bp, u32 data1, u32 data2)
>> @@ -879,7 +884,7 @@ int bnxt_ptp_init_rtc(struct bnxt *bp, bool phc_cfg)
>>          u64 ns;
>>          int rc;
>>
>> -       if (!bp->ptp_cfg || !(bp->fw_cap & BNXT_FW_CAP_PTP_RTC))
>> +       if (!bp->ptp_cfg || !BNXT_PTP_RTC(bp))
>>                  return -ENODEV;
>>
>>          if (!phc_cfg) {
>> @@ -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);
> 
> 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
>>