| Message ID | 20210701002713.3486336-5-jacob.e.keller@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ice: implement support for PTP on E822 hardware | expand |
Dear Jacob, Am 01.07.21 um 02:27 schrieb Jacob Keller: > Wen we enabled auxiliary input/output support for the E810 device, we When > forgot to add logic to restart the output when we change time. This is > important as the periodic output will be incorrect after a time change > otherwise. > > This unfortunately includes the adjust time function, even though it > uses an atomic hardware interface. The atomic adjustment can still cause > the pin output to stall permanently, so we need to stop and restart it. > > Fixes: 172db5f91d5f ("ice: add support for auxiliary input/output pins") > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_ptp.c | 30 ++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > index 83ba0bf2817a..08acdb2494ed 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > @@ -776,6 +776,7 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts) > struct ice_pf *pf = ptp_info_to_pf(info); > struct timespec64 ts64 = *ts; > struct ice_hw *hw = &pf->hw; > + u8 i; For count variables, it’s better to use native types, size_t/unsigned int in this case. static int ice_ptp_cfg_clkout(struct ice_pf *pf, unsigned int chan, struct ice_perout_channel *config, bool store) > int err; > > if (!ice_ptp_lock(hw)) { > @@ -783,12 +784,22 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts) > goto exit; > } > > + /* Disable periodic outputs */ > + for (i = 0; i < info->n_per_out; i++) > + if (pf->ptp.perout_channels[i].ena) > + ice_ptp_cfg_clkout(pf, i, NULL, false); > + > err = ice_ptp_write_init(pf, &ts64); > ice_ptp_unlock(hw); > > if (!err) > ice_ptp_update_cached_phctime(pf); > > + /* Reenable periodic outputs */ > + for (i = 0; i < info->n_per_out; i++) > + if (pf->ptp.perout_channels[i].ena) > + ice_ptp_cfg_clkout(pf, i, &pf->ptp.perout_channels[i], > + false); > exit: > if (err) { > dev_err(ice_pf_to_dev(pf), "PTP failed to set time %d\n", err); > @@ -825,6 +836,7 @@ static int ice_ptp_adjtime(struct ptp_clock_info *info, s64 delta) > struct ice_hw *hw = &pf->hw; > struct device *dev; > int err; > + u8 i; > > dev = ice_pf_to_dev(pf); > > @@ -842,8 +854,19 @@ static int ice_ptp_adjtime(struct ptp_clock_info *info, s64 delta) > return -EBUSY; > } > > + /* Disable periodic outputs */ > + for (i = 0; i < info->n_per_out; i++) > + if (pf->ptp.perout_channels[i].ena) > + ice_ptp_cfg_clkout(pf, i, NULL, false); > + > err = ice_ptp_write_adj(pf, delta); > > + /* Reenable periodic outputs */ > + for (i = 0; i < info->n_per_out; i++) > + if (pf->ptp.perout_channels[i].ena) > + ice_ptp_cfg_clkout(pf, i, &pf->ptp.perout_channels[i], > + false); > + > ice_ptp_unlock(hw); > > if (err) { > @@ -1526,6 +1549,8 @@ void ice_ptp_init(struct ice_pf *pf) > */ > void ice_ptp_release(struct ice_pf *pf) > { > + int i; > + > /* Disable timestamping for both Tx and Rx */ > ice_ptp_cfg_timestamp(pf, false); > > @@ -1543,6 +1568,11 @@ void ice_ptp_release(struct ice_pf *pf) > if (!pf->ptp.clock) > return; > > + /* Disable periodic outputs */ > + for (i = 0; i < pf->ptp.info.n_per_out; i++) > + if (pf->ptp.perout_channels[i].ena) > + ice_ptp_cfg_clkout(pf, i, NULL, false); > + Could this be put into a dedicated enable/disable function (only pf seems to be needed to be passed). > ice_clear_ptp_clock_index(pf); > ptp_clock_unregister(pf->ptp.clock); > pf->ptp.clock = NULL; Kind regards, Paul
> -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: Monday, July 05, 2021 12:52 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: intel-wired-lan@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [net-next 04/13] ice: restart periodic outputs around > time changes > > Dear Jacob, > > > Am 01.07.21 um 02:27 schrieb Jacob Keller: > > Wen we enabled auxiliary input/output support for the E810 device, we > > When > > > forgot to add logic to restart the output when we change time. This is > > important as the periodic output will be incorrect after a time change > > otherwise. > > > > This unfortunately includes the adjust time function, even though it > > uses an atomic hardware interface. The atomic adjustment can still cause > > the pin output to stall permanently, so we need to stop and restart it. > > > > Fixes: 172db5f91d5f ("ice: add support for auxiliary input/output pins") > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_ptp.c | 30 ++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c > b/drivers/net/ethernet/intel/ice/ice_ptp.c > > index 83ba0bf2817a..08acdb2494ed 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > > @@ -776,6 +776,7 @@ ice_ptp_settime64(struct ptp_clock_info *info, const > struct timespec64 *ts) > > struct ice_pf *pf = ptp_info_to_pf(info); > > struct timespec64 ts64 = *ts; > > struct ice_hw *hw = &pf->hw; > > + u8 i; > > For count variables, it’s better to use native types, size_t/unsigned > int in this case. > Sure. > static int ice_ptp_cfg_clkout(struct ice_pf *pf, unsigned int chan, > struct ice_perout_channel *config, > bool store) > > > int err; > > > > if (!ice_ptp_lock(hw)) { > > @@ -783,12 +784,22 @@ ice_ptp_settime64(struct ptp_clock_info *info, const > struct timespec64 *ts) > > goto exit; > > } > > > > + /* Disable periodic outputs */ > > + for (i = 0; i < info->n_per_out; i++) > > + if (pf->ptp.perout_channels[i].ena) > > + ice_ptp_cfg_clkout(pf, i, NULL, false); > > + > > err = ice_ptp_write_init(pf, &ts64); > > ice_ptp_unlock(hw); > > > > if (!err) > > ice_ptp_update_cached_phctime(pf); > > > > + /* Reenable periodic outputs */ > > + for (i = 0; i < info->n_per_out; i++) > > + if (pf->ptp.perout_channels[i].ena) > > + ice_ptp_cfg_clkout(pf, i, &pf->ptp.perout_channels[i], > > + false); > > exit: > > if (err) { > > dev_err(ice_pf_to_dev(pf), "PTP failed to set time %d\n", err); > > @@ -825,6 +836,7 @@ static int ice_ptp_adjtime(struct ptp_clock_info *info, > s64 delta) > > struct ice_hw *hw = &pf->hw; > > struct device *dev; > > int err; > > + u8 i; > > > > dev = ice_pf_to_dev(pf); > > > > @@ -842,8 +854,19 @@ static int ice_ptp_adjtime(struct ptp_clock_info *info, > s64 delta) > > return -EBUSY; > > } > > > > + /* Disable periodic outputs */ > > + for (i = 0; i < info->n_per_out; i++) > > + if (pf->ptp.perout_channels[i].ena) > > + ice_ptp_cfg_clkout(pf, i, NULL, false); > > + > > err = ice_ptp_write_adj(pf, delta); > > > > + /* Reenable periodic outputs */ > > + for (i = 0; i < info->n_per_out; i++) > > + if (pf->ptp.perout_channels[i].ena) > > + ice_ptp_cfg_clkout(pf, i, &pf->ptp.perout_channels[i], > > + false); > > + > > ice_ptp_unlock(hw); > > > > if (err) { > > @@ -1526,6 +1549,8 @@ void ice_ptp_init(struct ice_pf *pf) > > */ > > void ice_ptp_release(struct ice_pf *pf) > > { > > + int i; > > + > > /* Disable timestamping for both Tx and Rx */ > > ice_ptp_cfg_timestamp(pf, false); > > > > @@ -1543,6 +1568,11 @@ void ice_ptp_release(struct ice_pf *pf) > > if (!pf->ptp.clock) > > return; > > > > + /* Disable periodic outputs */ > > + for (i = 0; i < pf->ptp.info.n_per_out; i++) > > + if (pf->ptp.perout_channels[i].ena) > > + ice_ptp_cfg_clkout(pf, i, NULL, false); > > + > > Could this be put into a dedicated enable/disable function (only pf > seems to be needed to be passed). > Yea that seems like a good cleanup. > > ice_clear_ptp_clock_index(pf); > > ptp_clock_unregister(pf->ptp.clock); > > pf->ptp.clock = NULL; > > > Kind regards, > > Paul
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index 83ba0bf2817a..08acdb2494ed 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -776,6 +776,7 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts) struct ice_pf *pf = ptp_info_to_pf(info); struct timespec64 ts64 = *ts; struct ice_hw *hw = &pf->hw; + u8 i; int err; if (!ice_ptp_lock(hw)) { @@ -783,12 +784,22 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts) goto exit; } + /* Disable periodic outputs */ + for (i = 0; i < info->n_per_out; i++) + if (pf->ptp.perout_channels[i].ena) + ice_ptp_cfg_clkout(pf, i, NULL, false); + err = ice_ptp_write_init(pf, &ts64); ice_ptp_unlock(hw); if (!err) ice_ptp_update_cached_phctime(pf); + /* Reenable periodic outputs */ + for (i = 0; i < info->n_per_out; i++) + if (pf->ptp.perout_channels[i].ena) + ice_ptp_cfg_clkout(pf, i, &pf->ptp.perout_channels[i], + false); exit: if (err) { dev_err(ice_pf_to_dev(pf), "PTP failed to set time %d\n", err); @@ -825,6 +836,7 @@ static int ice_ptp_adjtime(struct ptp_clock_info *info, s64 delta) struct ice_hw *hw = &pf->hw; struct device *dev; int err; + u8 i; dev = ice_pf_to_dev(pf); @@ -842,8 +854,19 @@ static int ice_ptp_adjtime(struct ptp_clock_info *info, s64 delta) return -EBUSY; } + /* Disable periodic outputs */ + for (i = 0; i < info->n_per_out; i++) + if (pf->ptp.perout_channels[i].ena) + ice_ptp_cfg_clkout(pf, i, NULL, false); + err = ice_ptp_write_adj(pf, delta); + /* Reenable periodic outputs */ + for (i = 0; i < info->n_per_out; i++) + if (pf->ptp.perout_channels[i].ena) + ice_ptp_cfg_clkout(pf, i, &pf->ptp.perout_channels[i], + false); + ice_ptp_unlock(hw); if (err) { @@ -1526,6 +1549,8 @@ void ice_ptp_init(struct ice_pf *pf) */ void ice_ptp_release(struct ice_pf *pf) { + int i; + /* Disable timestamping for both Tx and Rx */ ice_ptp_cfg_timestamp(pf, false); @@ -1543,6 +1568,11 @@ void ice_ptp_release(struct ice_pf *pf) if (!pf->ptp.clock) return; + /* Disable periodic outputs */ + for (i = 0; i < pf->ptp.info.n_per_out; i++) + if (pf->ptp.perout_channels[i].ena) + ice_ptp_cfg_clkout(pf, i, NULL, false); + ice_clear_ptp_clock_index(pf); ptp_clock_unregister(pf->ptp.clock); pf->ptp.clock = NULL;
Wen we enabled auxiliary input/output support for the E810 device, we forgot to add logic to restart the output when we change time. This is important as the periodic output will be incorrect after a time change otherwise. This unfortunately includes the adjust time function, even though it uses an atomic hardware interface. The atomic adjustment can still cause the pin output to stall permanently, so we need to stop and restart it. Fixes: 172db5f91d5f ("ice: add support for auxiliary input/output pins") Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- drivers/net/ethernet/intel/ice/ice_ptp.c | 30 ++++++++++++++++++++++++ 1 file changed, 30 insertions(+)