From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
To: Pavan Chebbi <pavan.chebbi@broadcom.com>,
Vadim Fedorenko <vadfed@meta.com>
Cc: Jakub Kicinski <kuba@kernel.org>,
Andy Gospodarek <andrew.gospodarek@broadcom.com>,
Michael Chan <michael.chan@broadcom.com>,
Richard Cochran <richardcochran@gmail.com>,
netdev@vger.kernel.org
Subject: Re: [PATCH net] bnxt_en: reset PHC frequency in free-running mode
Date: Mon, 6 Mar 2023 19:12:59 +0000 [thread overview]
Message-ID: <d8a49972-2875-2be9-a210-92d9dac32c03@linux.dev> (raw)
In-Reply-To: <CALs4sv1A1eTpH45Z=kyL3qtu7Yfu8JRW6Wc2r1d+UxjvB_EEEA@mail.gmail.com>
On 06/03/2023 17:11, Pavan Chebbi wrote:
> On Mon, Mar 6, 2023 at 10:23 PM Vadim Fedorenko <vadfed@meta.com> 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 <vadfed@meta.com>
>> ---
>> 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
>>
next prev parent reply other threads:[~2023-03-06 19:13 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-06 16:53 [PATCH net] bnxt_en: reset PHC frequency in free-running mode Vadim Fedorenko
2023-03-06 17:11 ` Pavan Chebbi
2023-03-06 19:12 ` Vadim Fedorenko [this message]
2023-03-07 5:07 ` Pavan Chebbi
2023-03-07 10:25 ` Vadim Fedorenko
2023-03-06 18:06 ` Jakub Kicinski
2023-03-06 19:13 ` Vadim Fedorenko
2023-03-07 2:40 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d8a49972-2875-2be9-a210-92d9dac32c03@linux.dev \
--to=vadim.fedorenko@linux.dev \
--cc=andrew.gospodarek@broadcom.com \
--cc=kuba@kernel.org \
--cc=michael.chan@broadcom.com \
--cc=netdev@vger.kernel.org \
--cc=pavan.chebbi@broadcom.com \
--cc=richardcochran@gmail.com \
--cc=vadfed@meta.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.