| Message ID | 20230306165344.350387-1-vadfed@meta.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] bnxt_en: reset PHC frequency in free-running mode | expand |
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. > } > > ptp->ptp_info = bnxt_ptp_caps; > -- > 2.30.2 >
On Mon, 6 Mar 2023 08:53:44 -0800 Vadim Fedorenko wrote: > +#define BNXT_PTP_RTC(bp) (!BNXT_MH(bp) && \ > + ((bp)->fw_cap & BNXT_FW_CAP_PTP_RTC)) nit: maybe BNXT_PTP_USE_RTC() ?
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 >>
On 06/03/2023 18:06, Jakub Kicinski wrote: > On Mon, 6 Mar 2023 08:53:44 -0800 Vadim Fedorenko wrote: >> +#define BNXT_PTP_RTC(bp) (!BNXT_MH(bp) && \ >> + ((bp)->fw_cap & BNXT_FW_CAP_PTP_RTC)) > > nit: maybe BNXT_PTP_USE_RTC() ? That's fair point. I'll change it in v2. Thanks.
Hi Vadim,
I love your patch! Yet something to improve:
[auto build test ERROR on net/master]
url: https://github.com/intel-lab-lkp/linux/commits/Vadim-Fedorenko/bnxt_en-reset-PHC-frequency-in-free-running-mode/20230307-005700
patch link: https://lore.kernel.org/r/20230306165344.350387-1-vadfed%40meta.com
patch subject: [PATCH net] bnxt_en: reset PHC frequency in free-running mode
config: riscv-randconfig-r042-20230306 (https://download.01.org/0day-ci/archive/20230307/202303071016.BkAkCGA9-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/26d7b40eb9dd69f66a477fab7cf51d98c3fe63de
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Vadim-Fedorenko/bnxt_en-reset-PHC-frequency-in-free-running-mode/20230307-005700
git checkout 26d7b40eb9dd69f66a477fab7cf51d98c3fe63de
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/net/ethernet/broadcom/bnxt/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303071016.BkAkCGA9-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:948:4: error: call to undeclared function 'bnxt_ptp_adjfreq_rtc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
bnxt_ptp_adjfreq_rtc(bp, 0);
^
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:948:4: note: did you mean 'bnxt_ptp_adjfine_rtc'?
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:208:12: note: 'bnxt_ptp_adjfine_rtc' declared here
static int bnxt_ptp_adjfine_rtc(struct bnxt *bp, long scaled_ppm)
^
1 error generated.
vim +/bnxt_ptp_adjfreq_rtc +948 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
919
920 int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
921 {
922 struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
923 int rc;
924
925 if (!ptp)
926 return 0;
927
928 rc = bnxt_map_ptp_regs(bp);
929 if (rc)
930 return rc;
931
932 if (ptp->ptp_clock && bnxt_pps_config_ok(bp))
933 return 0;
934
935 bnxt_ptp_free(bp);
936
937 atomic_set(&ptp->tx_avail, BNXT_MAX_TX_TS);
938 spin_lock_init(&ptp->ptp_lock);
939
940 if (BNXT_PTP_RTC(ptp->bp)) {
941 bnxt_ptp_timecounter_init(bp, false);
942 rc = bnxt_ptp_init_rtc(bp, phc_cfg);
943 if (rc)
944 goto out;
945 } else {
946 bnxt_ptp_timecounter_init(bp, true);
947 if (bp->fw_cap & BNXT_FW_CAP_PTP_RTC)
> 948 bnxt_ptp_adjfreq_rtc(bp, 0);
949 }
950
951 ptp->ptp_info = bnxt_ptp_caps;
952 if ((bp->fw_cap & BNXT_FW_CAP_PTP_PPS)) {
953 if (bnxt_ptp_pps_init(bp))
954 netdev_err(bp->dev, "1pps not initialized, continuing without 1pps support\n");
955 }
956 ptp->ptp_clock = ptp_clock_register(&ptp->ptp_info, &bp->pdev->dev);
957 if (IS_ERR(ptp->ptp_clock)) {
958 int err = PTR_ERR(ptp->ptp_clock);
959
960 ptp->ptp_clock = NULL;
961 rc = err;
962 goto out;
963 }
964 if (bp->flags & BNXT_FLAG_CHIP_P5) {
965 spin_lock_bh(&ptp->ptp_lock);
966 bnxt_refclk_read(bp, NULL, &ptp->current_time);
967 WRITE_ONCE(ptp->old_time, ptp->current_time);
968 spin_unlock_bh(&ptp->ptp_lock);
969 ptp_schedule_worker(ptp->ptp_clock, 0);
970 }
971 return 0;
972
973 out:
974 bnxt_ptp_free(bp);
975 bnxt_unmap_ptp_regs(bp);
976 return rc;
977 }
978
On Tue, Mar 7, 2023 at 12:43 AM Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote: > > On 06/03/2023 17:11, Pavan Chebbi wrote: > > On Mon, Mar 6, 2023 at 10:23 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_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. If you want PHC to start counting from 0 on init, you may use bnxt_ptp_cfg_settime() with 0. 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. > > > > 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 > >> >
On 07/03/2023 05:07, Pavan Chebbi wrote: > On Tue, Mar 7, 2023 at 12:43 AM Vadim Fedorenko > <vadim.fedorenko@linux.dev> wrote: >> >> On 06/03/2023 17:11, Pavan Chebbi wrote: >>> On Mon, Mar 6, 2023 at 10:23 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_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 >>>> >>
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); } ptp->ptp_info = bnxt_ptp_caps;
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. 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(-)