| Message ID | 20240229070202.107488-2-michael.chan@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt_en: Support configurable PTP TX timeout | expand |
On 29/02/2024 07:02, Michael Chan wrote: > From: Pavan Chebbi <pavan.chebbi@broadcom.com> > > Sometimes, the current 1ms value that driver waits for firmware > to obtain a tx timestamp for a PTP packet may not be sufficient. > User may want the driver to wait for a longer custom period before > timing out. > > Introduce a new runtime driver param for devlink "ptp_tx_timeout". > Using this parameter the driver can wait for up to the specified > time, when it is querying for a TX timestamp from firmware. By > default the value is set to 1s. > > Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com> > Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com> > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > --- > .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 42 +++++++++++++++++++ > drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 1 + > drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 3 ++ > 3 files changed, 46 insertions(+) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c > index ae4529c043f0..0df0baa9d18c 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c > @@ -652,6 +652,7 @@ static const struct devlink_ops bnxt_vf_dl_ops; > enum bnxt_dl_param_id { > BNXT_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, > + BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO, > }; > > static const struct bnxt_dl_nvm_param nvm_params[] = { > @@ -1077,6 +1078,42 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg, > return rc; > } > > +static int bnxt_dl_ptp_param_get(struct devlink *dl, u32 id, > + struct devlink_param_gset_ctx *ctx) > +{ > + struct bnxt *bp = bnxt_get_bp_from_dl(dl); > + > + if (!bp->ptp_cfg) > + return -EOPNOTSUPP; > + > + ctx->val.vu32 = bp->ptp_cfg->txts_tmo; > + return 0; > +} > + > +static int bnxt_dl_ptp_param_set(struct devlink *dl, u32 id, > + struct devlink_param_gset_ctx *ctx) > +{ > + struct bnxt *bp = bnxt_get_bp_from_dl(dl); > + > + if (!bp->ptp_cfg) > + return -EOPNOTSUPP; > + > + bp->ptp_cfg->txts_tmo = ctx->val.vu32; > + return 0; > +} > + > +static int bnxt_dl_ptp_param_validate(struct devlink *dl, u32 id, > + union devlink_param_value val, > + struct netlink_ext_ack *extack) > +{ > + if (val.vu32 > BNXT_PTP_MAX_TX_TMO) { > + NL_SET_ERR_MSG_FMT_MOD(extack, "TX timeout value exceeds the maximum (%d ms)", > + BNXT_PTP_MAX_TX_TMO); > + return -EINVAL; > + } > + return 0; > +} > + > static int bnxt_dl_nvm_param_get(struct devlink *dl, u32 id, > struct devlink_param_gset_ctx *ctx) > { > @@ -1180,6 +1217,11 @@ static const struct devlink_param bnxt_dl_params[] = { > BIT(DEVLINK_PARAM_CMODE_PERMANENT), > bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set, > NULL), > + DEVLINK_PARAM_DRIVER(BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO, > + "ptp_tx_timeout", DEVLINK_PARAM_TYPE_U32, > + BIT(DEVLINK_PARAM_CMODE_RUNTIME), > + bnxt_dl_ptp_param_get, bnxt_dl_ptp_param_set, > + bnxt_dl_ptp_param_validate), > /* keep REMOTE_DEV_RESET last, it is excluded based on caps */ > DEVLINK_PARAM_GENERIC(ENABLE_REMOTE_DEV_RESET, > BIT(DEVLINK_PARAM_CMODE_RUNTIME), > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c > index cc07660330f5..4b50b07b9771 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c > @@ -965,6 +965,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg) > spin_unlock_bh(&ptp->ptp_lock); > ptp_schedule_worker(ptp->ptp_clock, 0); > } > + ptp->txts_tmo = BNXT_PTP_DFLT_TX_TMO; > return 0; > > out: > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h > index fce8dc39a7d0..ee977620d33e 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h > @@ -22,6 +22,8 @@ > #define BNXT_LO_TIMER_MASK 0x0000ffffffffUL > #define BNXT_HI_TIMER_MASK 0xffff00000000UL > > +#define BNXT_PTP_DFLT_TX_TMO 1000 /* ms */ I'm not happy with such huge timeout, but looks like other vendors do expect the same timeouts, I'm ok. Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> > +#define BNXT_PTP_MAX_TX_TMO 5000 /* ms */ > #define BNXT_PTP_QTS_TIMEOUT 1000 > #define BNXT_PTP_QTS_TX_ENABLES (PORT_TS_QUERY_REQ_ENABLES_PTP_SEQ_ID | \ > PORT_TS_QUERY_REQ_ENABLES_TS_REQ_TIMEOUT | \ > @@ -120,6 +122,7 @@ struct bnxt_ptp_cfg { > > u32 refclk_regs[2]; > u32 refclk_mapped_regs[2]; > + u32 txts_tmo; > }; > > #if BITS_PER_LONG == 32
Thu, Feb 29, 2024 at 08:02:01AM CET, michael.chan@broadcom.com wrote: >From: Pavan Chebbi <pavan.chebbi@broadcom.com> > >Sometimes, the current 1ms value that driver waits for firmware >to obtain a tx timestamp for a PTP packet may not be sufficient. >User may want the driver to wait for a longer custom period before >timing out. > >Introduce a new runtime driver param for devlink "ptp_tx_timeout". >Using this parameter the driver can wait for up to the specified >time, when it is querying for a TX timestamp from firmware. By >default the value is set to 1s. > >Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com> >Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com> >Signed-off-by: Michael Chan <michael.chan@broadcom.com> >--- > .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 42 +++++++++++++++++++ > drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 1 + > drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 3 ++ > 3 files changed, 46 insertions(+) > >diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c >index ae4529c043f0..0df0baa9d18c 100644 >--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c >+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c >@@ -652,6 +652,7 @@ static const struct devlink_ops bnxt_vf_dl_ops; > enum bnxt_dl_param_id { > BNXT_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, >+ BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO, > }; > > static const struct bnxt_dl_nvm_param nvm_params[] = { >@@ -1077,6 +1078,42 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg, > return rc; > } > >+static int bnxt_dl_ptp_param_get(struct devlink *dl, u32 id, >+ struct devlink_param_gset_ctx *ctx) >+{ >+ struct bnxt *bp = bnxt_get_bp_from_dl(dl); >+ >+ if (!bp->ptp_cfg) >+ return -EOPNOTSUPP; >+ >+ ctx->val.vu32 = bp->ptp_cfg->txts_tmo; >+ return 0; >+} >+ >+static int bnxt_dl_ptp_param_set(struct devlink *dl, u32 id, >+ struct devlink_param_gset_ctx *ctx) >+{ >+ struct bnxt *bp = bnxt_get_bp_from_dl(dl); >+ >+ if (!bp->ptp_cfg) >+ return -EOPNOTSUPP; >+ >+ bp->ptp_cfg->txts_tmo = ctx->val.vu32; >+ return 0; >+} >+ >+static int bnxt_dl_ptp_param_validate(struct devlink *dl, u32 id, >+ union devlink_param_value val, >+ struct netlink_ext_ack *extack) >+{ >+ if (val.vu32 > BNXT_PTP_MAX_TX_TMO) { >+ NL_SET_ERR_MSG_FMT_MOD(extack, "TX timeout value exceeds the maximum (%d ms)", >+ BNXT_PTP_MAX_TX_TMO); >+ return -EINVAL; >+ } >+ return 0; >+} >+ > static int bnxt_dl_nvm_param_get(struct devlink *dl, u32 id, > struct devlink_param_gset_ctx *ctx) > { >@@ -1180,6 +1217,11 @@ static const struct devlink_param bnxt_dl_params[] = { > BIT(DEVLINK_PARAM_CMODE_PERMANENT), > bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set, > NULL), >+ DEVLINK_PARAM_DRIVER(BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO, >+ "ptp_tx_timeout", DEVLINK_PARAM_TYPE_U32, >+ BIT(DEVLINK_PARAM_CMODE_RUNTIME), >+ bnxt_dl_ptp_param_get, bnxt_dl_ptp_param_set, >+ bnxt_dl_ptp_param_validate), Idk. This does not look sane to me at all. Will we have custom knobs to change timeout for arbitrary FW commands as this as a common thing? Driver is the one to take care of timeouts of FW gracefully, he should know the FW, not the user. Therefore exposing user knobs like this sounds pure wrong to me. nack for adding this to devlink. If this is some maybe-to-be-common ptp thing, can that be done as part of ptp api perhaps? pw-bot: cr > /* keep REMOTE_DEV_RESET last, it is excluded based on caps */ > DEVLINK_PARAM_GENERIC(ENABLE_REMOTE_DEV_RESET, > BIT(DEVLINK_PARAM_CMODE_RUNTIME), >diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c >index cc07660330f5..4b50b07b9771 100644 >--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c >+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c >@@ -965,6 +965,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg) > spin_unlock_bh(&ptp->ptp_lock); > ptp_schedule_worker(ptp->ptp_clock, 0); > } >+ ptp->txts_tmo = BNXT_PTP_DFLT_TX_TMO; > return 0; > > out: >diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h >index fce8dc39a7d0..ee977620d33e 100644 >--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h >+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h >@@ -22,6 +22,8 @@ > #define BNXT_LO_TIMER_MASK 0x0000ffffffffUL > #define BNXT_HI_TIMER_MASK 0xffff00000000UL > >+#define BNXT_PTP_DFLT_TX_TMO 1000 /* ms */ >+#define BNXT_PTP_MAX_TX_TMO 5000 /* ms */ > #define BNXT_PTP_QTS_TIMEOUT 1000 > #define BNXT_PTP_QTS_TX_ENABLES (PORT_TS_QUERY_REQ_ENABLES_PTP_SEQ_ID | \ > PORT_TS_QUERY_REQ_ENABLES_TS_REQ_TIMEOUT | \ >@@ -120,6 +122,7 @@ struct bnxt_ptp_cfg { > > u32 refclk_regs[2]; > u32 refclk_mapped_regs[2]; >+ u32 txts_tmo; > }; > > #if BITS_PER_LONG == 32 >-- >2.30.1 >
On Thu, 29 Feb 2024 18:11:49 +0100 Jiri Pirko wrote: > Idk. This does not look sane to me at all. Will we have custom knobs to > change timeout for arbitrary FW commands as this as a common thing? > Driver is the one to take care of timeouts of FW gracefully, he should > know the FW, not the user. Therefore exposing user knobs like this > sounds pure wrong to me. > > nack for adding this to devlink. +1 BTW why is the documentation in a different patch that the param :( > If this is some maybe-to-be-common ptp thing, can that be done as part > of ptp api perhaps? Perhaps, but also I think it's fairly impractical. Specialized users may be able to tune this, but in DC environment PTP is handled at the host level, and the applications come and go. So all the poor admin can do is set this to the max value. While in the driver you can actually try to be a bit more intelligent. Expecting the user to tune this strikes me as trying to take the easy way out..
On 29/02/2024 17:30, Jakub Kicinski wrote: > On Thu, 29 Feb 2024 18:11:49 +0100 Jiri Pirko wrote: >> Idk. This does not look sane to me at all. Will we have custom knobs to >> change timeout for arbitrary FW commands as this as a common thing? >> Driver is the one to take care of timeouts of FW gracefully, he should >> know the FW, not the user. Therefore exposing user knobs like this >> sounds pure wrong to me. >> >> nack for adding this to devlink. > > +1 > > BTW why is the documentation in a different patch that the param :( > >> If this is some maybe-to-be-common ptp thing, can that be done as part >> of ptp api perhaps? Jiri, do you mean extend current ioctl used to enable/disable HW timestamps? > > Perhaps, but also I think it's fairly impractical. Specialized users may > be able to tune this, but in DC environment PTP is handled at the host That's correct, only 1 app is actually doing syncronization > level, and the applications come and go. So all the poor admin can do Container/VM level applications don't care about PTP packets timestamps. They only care about the time being synchronized. > is set this to the max value. While in the driver you can actually try Pure admin will tune it according to the host level app configuration which may differ because of environment. > to be a bit more intelligent. Expecting the user to tune this strikes me > as trying to take the easy way out.. There is no actual way for application to signal down to driver that it gave up waiting for TX timestamp, what other kind of smartness can we expect here?
On Thu, 29 Feb 2024 21:22:19 +0000 Vadim Fedorenko wrote: > > Perhaps, but also I think it's fairly impractical. Specialized users may > > be able to tune this, but in DC environment PTP is handled at the host > > That's correct, only 1 app is actually doing syncronization > > > level, and the applications come and go. So all the poor admin can do > > Container/VM level applications don't care about PTP packets timestamps. > They only care about the time being synchronized. What I was saying is that in the PTP daemon you don't know whether the app running is likely to cause delays or not, and how long. > > is set this to the max value. While in the driver you can actually try > > Pure admin will tune it according to the host level app configuration > which may differ because of environment. Concrete example? > > to be a bit more intelligent. Expecting the user to tune this strikes me > > as trying to take the easy way out.. > > There is no actual way for application to signal down to driver that it > gave up waiting for TX timestamp, what other kind of smartness can we > expect here? Let's figure out why the timeouts happen, before we create uAPIs. If it's because there's buffer bloat or a pause storm, the next TS request that gets queued will get stuck in the same exact way.
On Fri, Mar 1, 2024 at 7:19 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 29 Feb 2024 21:22:19 +0000 Vadim Fedorenko wrote: > > > Perhaps, but also I think it's fairly impractical. Specialized users may > > > be able to tune this, but in DC environment PTP is handled at the host > > > > That's correct, only 1 app is actually doing syncronization > > > > > level, and the applications come and go. So all the poor admin can do > > > > Container/VM level applications don't care about PTP packets timestamps. > > They only care about the time being synchronized. > > What I was saying is that in the PTP daemon you don't know whether > the app running is likely to cause delays or not, and how long. > As such timeouts are rare but still normal. But if you've an environment where you want to have some kind of sync between the application and the NIC, as to when should both conclude that the timestamp is absolutely lost, we need some knob like this. Like you pointed out it's for an informed user who has knowledge of the workloads/flow control and how (badly) could they affect the TX. Of course the user cannot make an accurate estimation of the exact time out, but he can always experiment with the knob. We are not sure if others need this as well, hence the private devlink parameter. For most common users, the default 1s timeout should serve well. > > > is set this to the max value. While in the driver you can actually try > > > > Pure admin will tune it according to the host level app configuration > > which may differ because of environment. > > Concrete example? > > > > to be a bit more intelligent. Expecting the user to tune this strikes me > > > as trying to take the easy way out.. > > > > There is no actual way for application to signal down to driver that it > > gave up waiting for TX timestamp, what other kind of smartness can we > > expect here? > > Let's figure out why the timeouts happen, before we create uAPIs. > If it's because there's buffer bloat or a pause storm, the next TS > request that gets queued will get stuck in the same exact way.
Thu, Feb 29, 2024 at 10:22:19PM CET, vadim.fedorenko@linux.dev wrote: >On 29/02/2024 17:30, Jakub Kicinski wrote: >> On Thu, 29 Feb 2024 18:11:49 +0100 Jiri Pirko wrote: >> > Idk. This does not look sane to me at all. Will we have custom knobs to >> > change timeout for arbitrary FW commands as this as a common thing? >> > Driver is the one to take care of timeouts of FW gracefully, he should >> > know the FW, not the user. Therefore exposing user knobs like this >> > sounds pure wrong to me. >> > >> > nack for adding this to devlink. >> >> +1 >> >> BTW why is the documentation in a different patch that the param :( >> >> > If this is some maybe-to-be-common ptp thing, can that be done as part >> > of ptp api perhaps? > >Jiri, do you mean extend current ioctl used to enable/disable HW >timestamps? Maybe.
On Fri, 1 Mar 2024 13:09:30 +0530 Pavan Chebbi wrote: > > What I was saying is that in the PTP daemon you don't know whether > > the app running is likely to cause delays or not, and how long. > > As such timeouts are rare but still normal. Normal, because...? Why do they happen? > But if you've an environment where you want to have some kind of sync > between the application and the NIC, as to when should both conclude > that the timestamp is absolutely lost, we need some knob like this. > Like you pointed out it's for an informed user who has knowledge of > the Let's start from informing the user why timeout happens. > workloads/flow control and how (badly) could they affect the TX. Of > course the user cannot make an accurate estimation of the exact time > out, but he can always experiment with the knob. > We are not sure if others need this as well, hence the private devlink > parameter. For most common users, the default 1s timeout should serve > well.
On Fri, Mar 1, 2024 at 10:48 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 1 Mar 2024 13:09:30 +0530 Pavan Chebbi wrote: > > > What I was saying is that in the PTP daemon you don't know whether > > > the app running is likely to cause delays or not, and how long. > > > > As such timeouts are rare but still normal. > > Normal, because...? Why do they happen? Excuse me for the late reply. In my experience so far, it's primarily because of flow control and how stressed the underlying HW queue is. (I am sure it's not unique to our hardware alone) Hence we wanted to accommodate cases where the expected wait time is higher than what is default in the driver, for the packets to go out. But it's disappointing to know that even private devlink params are discouraged for such purposes. I'd think that non-generic driver params in devlink serve exactly such requirements and having such a knob would be useful for an advanced user. Not to mention, in my view, such additions to devlink would make it more popular and would help in its wider adoption. For now, we will drop this series and try to get back with a different solution. > > > But if you've an environment where you want to have some kind of sync > > between the application and the NIC, as to when should both conclude > > that the timestamp is absolutely lost, we need some knob like this. > > Like you pointed out it's for an informed user who has knowledge of > > the > > Let's start from informing the user why timeout happens. > > > workloads/flow control and how (badly) could they affect the TX. Of > > course the user cannot make an accurate estimation of the exact time > > out, but he can always experiment with the knob. > > We are not sure if others need this as well, hence the private devlink > > parameter. For most common users, the default 1s timeout should serve > > well.
On Thu, 7 Mar 2024 09:20:44 +0530 Pavan Chebbi wrote: > > > As such timeouts are rare but still normal. > > > > Normal, because...? Why do they happen? > > Excuse me for the late reply. > In my experience so far, it's primarily because of flow control and > how stressed the underlying HW queue is. (I am sure it's not unique to > our hardware alone) > Hence we wanted to accommodate cases where the expected wait time is > higher than what is default in the driver, for the packets to go out. > But it's disappointing to know that even private devlink params are > discouraged for such purposes. > I'd think that non-generic driver params in devlink serve exactly such > requirements and having such a knob would be useful for an advanced > user. > Not to mention, in my view, such additions to devlink would make it > more popular and would help in its wider adoption. The problem can be solved more intelligently.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c index ae4529c043f0..0df0baa9d18c 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c @@ -652,6 +652,7 @@ static const struct devlink_ops bnxt_vf_dl_ops; enum bnxt_dl_param_id { BNXT_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, + BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO, }; static const struct bnxt_dl_nvm_param nvm_params[] = { @@ -1077,6 +1078,42 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg, return rc; } +static int bnxt_dl_ptp_param_get(struct devlink *dl, u32 id, + struct devlink_param_gset_ctx *ctx) +{ + struct bnxt *bp = bnxt_get_bp_from_dl(dl); + + if (!bp->ptp_cfg) + return -EOPNOTSUPP; + + ctx->val.vu32 = bp->ptp_cfg->txts_tmo; + return 0; +} + +static int bnxt_dl_ptp_param_set(struct devlink *dl, u32 id, + struct devlink_param_gset_ctx *ctx) +{ + struct bnxt *bp = bnxt_get_bp_from_dl(dl); + + if (!bp->ptp_cfg) + return -EOPNOTSUPP; + + bp->ptp_cfg->txts_tmo = ctx->val.vu32; + return 0; +} + +static int bnxt_dl_ptp_param_validate(struct devlink *dl, u32 id, + union devlink_param_value val, + struct netlink_ext_ack *extack) +{ + if (val.vu32 > BNXT_PTP_MAX_TX_TMO) { + NL_SET_ERR_MSG_FMT_MOD(extack, "TX timeout value exceeds the maximum (%d ms)", + BNXT_PTP_MAX_TX_TMO); + return -EINVAL; + } + return 0; +} + static int bnxt_dl_nvm_param_get(struct devlink *dl, u32 id, struct devlink_param_gset_ctx *ctx) { @@ -1180,6 +1217,11 @@ static const struct devlink_param bnxt_dl_params[] = { BIT(DEVLINK_PARAM_CMODE_PERMANENT), bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set, NULL), + DEVLINK_PARAM_DRIVER(BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO, + "ptp_tx_timeout", DEVLINK_PARAM_TYPE_U32, + BIT(DEVLINK_PARAM_CMODE_RUNTIME), + bnxt_dl_ptp_param_get, bnxt_dl_ptp_param_set, + bnxt_dl_ptp_param_validate), /* keep REMOTE_DEV_RESET last, it is excluded based on caps */ DEVLINK_PARAM_GENERIC(ENABLE_REMOTE_DEV_RESET, BIT(DEVLINK_PARAM_CMODE_RUNTIME), diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c index cc07660330f5..4b50b07b9771 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c @@ -965,6 +965,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg) spin_unlock_bh(&ptp->ptp_lock); ptp_schedule_worker(ptp->ptp_clock, 0); } + ptp->txts_tmo = BNXT_PTP_DFLT_TX_TMO; return 0; out: diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h index fce8dc39a7d0..ee977620d33e 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h @@ -22,6 +22,8 @@ #define BNXT_LO_TIMER_MASK 0x0000ffffffffUL #define BNXT_HI_TIMER_MASK 0xffff00000000UL +#define BNXT_PTP_DFLT_TX_TMO 1000 /* ms */ +#define BNXT_PTP_MAX_TX_TMO 5000 /* ms */ #define BNXT_PTP_QTS_TIMEOUT 1000 #define BNXT_PTP_QTS_TX_ENABLES (PORT_TS_QUERY_REQ_ENABLES_PTP_SEQ_ID | \ PORT_TS_QUERY_REQ_ENABLES_TS_REQ_TIMEOUT | \ @@ -120,6 +122,7 @@ struct bnxt_ptp_cfg { u32 refclk_regs[2]; u32 refclk_mapped_regs[2]; + u32 txts_tmo; }; #if BITS_PER_LONG == 32