| Message ID | 20240627151127.284884-16-karol.kolacinski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: Cleanup and refactor PTP pin handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Dear Karol, dear Sergey, Thank you for the patch. Am 27.06.24 um 17:09 schrieb Karol Kolacinski: > From: Sergey Temerkhanov <sergey.temerkhanov@intel.com> > > Implement 1PPS signal enabling/disabling in CGU. The amplitude is > always the maximum in this implementation (Please add a dot/period at the end of sentences.) Could you please elaborate why using the maximum is alright, that means what are the downsides, and what is the alternative approahc. > Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com> > Co-developed-by: Karol Kolacinski <karol.kolacinski@intel.com> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_ptp.c | 10 ++++++++++ > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 21 +++++++++++++++++++++ > drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + > 3 files changed, 32 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > index d9ff94a4b293..b97ea2b61e5c 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > @@ -4,6 +4,7 @@ > #include "ice.h" > #include "ice_lib.h" > #include "ice_trace.h" > +#include "ice_cgu_regs.h" > > static const char ice_pin_names[][64] = { > "SDP0", > @@ -1708,6 +1709,15 @@ static int ice_ptp_write_perout(struct ice_hw *hw, unsigned int chan, > /* 0. Reset mode & out_en in AUX_OUT */ > wr32(hw, GLTSYN_AUX_OUT(chan, tmr_idx), 0); > > + if (ice_is_e825c(hw)) { > + int err; > + > + /* Enable/disable CGU 1PPS output for E825C */ > + err = ice_cgu_ena_pps_out(hw, !!period); > + if (err) > + return err; > + } Does only E825C products support this feature? > + > /* 1. Write perout with half of required period value. > * HW toggles output when source clock hits the TGT and then adds > * GLTSYN_CLKO value to the target, so it ends up with 50% duty cycle. > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > index 07ecf2a86742..fa7cf8453b88 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > @@ -661,6 +661,27 @@ static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw, > return 0; > } > > +#define ICE_ONE_PPS_OUT_AMP_MAX 3 > + > +/** > + * ice_cgu_ena_pps_out - Enable/disable 1PPS output > + * @hw: pointer to the HW struct > + * @ena: Enable/disable 1PPS output > + */ > +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena) Is `ena` short for enable? > +{ > + union nac_cgu_dword9 dw9; > + int err; > + > + err = ice_read_cgu_reg_e82x(hw, NAC_CGU_DWORD9, &dw9.val); > + if (err) > + return err; > + > + dw9.one_pps_out_en = ena; > + dw9.one_pps_out_amp = ena * ICE_ONE_PPS_OUT_AMP_MAX; > + return ice_write_cgu_reg_e82x(hw, NAC_CGU_DWORD9, dw9.val); > +} > + > /** > * ice_cfg_cgu_pll_dis_sticky_bits_e82x - disable TS PLL sticky bits > * @hw: pointer to the HW struct > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > index ff98f76969e3..382e84568256 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > @@ -331,6 +331,7 @@ extern const struct ice_vernier_info_e82x e822_vernier[NUM_ICE_PTP_LNK_SPD]; > > /* Device agnostic functions */ > u8 ice_get_ptp_src_clock_index(struct ice_hw *hw); > +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena); If *ena* means “enable”, I do not like this pattern very much, and I’d prefer an enable and a disable function. > bool ice_ptp_lock(struct ice_hw *hw); > void ice_ptp_unlock(struct ice_hw *hw); > void ice_ptp_src_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd); Kind regards, Paul
On Thu, Jun 27, 2024 at 05:09:31PM +0200, Karol Kolacinski wrote: > From: Sergey Temerkhanov <sergey.temerkhanov@intel.com> > > Implement 1PPS signal enabling/disabling in CGU. The amplitude is > always the maximum in this implementation > > Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com> > Co-developed-by: Karol Kolacinski <karol.kolacinski@intel.com> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> ... > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > index 07ecf2a86742..fa7cf8453b88 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > @@ -661,6 +661,27 @@ static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw, > return 0; > } > > +#define ICE_ONE_PPS_OUT_AMP_MAX 3 > + > +/** > + * ice_cgu_ena_pps_out - Enable/disable 1PPS output > + * @hw: pointer to the HW struct > + * @ena: Enable/disable 1PPS output Please include a "Returns: " or "Return: " section in the kernel doc for functions that have a return value. NIPA has recently got more picky about this. Flagged by kernel-doc -none --Warn > + */ > +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena) > +{ > + union nac_cgu_dword9 dw9; > + int err; > + > + err = ice_read_cgu_reg_e82x(hw, NAC_CGU_DWORD9, &dw9.val); > + if (err) > + return err; > + > + dw9.one_pps_out_en = ena; > + dw9.one_pps_out_amp = ena * ICE_ONE_PPS_OUT_AMP_MAX; > + return ice_write_cgu_reg_e82x(hw, NAC_CGU_DWORD9, dw9.val); > +} > + > /** > * ice_cfg_cgu_pll_dis_sticky_bits_e82x - disable TS PLL sticky bits > * @hw: pointer to the HW struct ...
Hi Paul, Thank you for your feedback! On 6/27/2024 5:37 PM, Paul Menzel wrote: > > @@ -1708,6 +1709,15 @@ static int ice_ptp_write_perout(struct ice_hw *hw, unsigned int chan, > > /* 0. Reset mode & out_en in AUX_OUT */ > > wr32(hw, GLTSYN_AUX_OUT(chan, tmr_idx), 0); > > > > + if (ice_is_e825c(hw)) { > > + int err; > > + > > + /* Enable/disable CGU 1PPS output for E825C */ > > + err = ice_cgu_ena_pps_out(hw, !!period); > > + if (err) > > + return err; > > + } > > Does only E825C products support this feature? Yes, it's only necessary for E825C, other products don't need or support it. > > +/** > > + * ice_cgu_ena_pps_out - Enable/disable 1PPS output > > + * @hw: pointer to the HW struct > > + * @ena: Enable/disable 1PPS output > > + */ > > +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena) > > Is `ena` short for enable? Yes. I guess for the function argument 'enable' would be better. > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > index ff98f76969e3..382e84568256 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > @@ -331,6 +331,7 @@ extern const struct ice_vernier_info_e82x e822_vernier[NUM_ICE_PTP_LNK_SPD]; > > > > /* Device agnostic functions */ > > u8 ice_get_ptp_src_clock_index(struct ice_hw *hw); > > +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena); > > If *ena* means “enable”, I do not like this pattern very much, and I’d > prefer an enable and a disable function. Good point regarding the name, I guess I should use 'cfg' instead of 'ena' in this case to be clear that this function doesn't only enable PPS output. That said, I don't see a point to use two separate functions for a simple enable/disable functionality. From my point of view, ~20 lines for 2 lines of actual code difference would be unnecessarily redundant. Kind regards, Karol
On 7/1/24 15:27, Simon Horman wrote: > On Thu, Jun 27, 2024 at 05:09:31PM +0200, Karol Kolacinski wrote: >> From: Sergey Temerkhanov <sergey.temerkhanov@intel.com> >> >> Implement 1PPS signal enabling/disabling in CGU. The amplitude is >> always the maximum in this implementation >> >> Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >> Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com> >> Co-developed-by: Karol Kolacinski <karol.kolacinski@intel.com> >> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > > ... > >> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c >> index 07ecf2a86742..fa7cf8453b88 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c >> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c >> @@ -661,6 +661,27 @@ static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw, >> return 0; >> } >> >> +#define ICE_ONE_PPS_OUT_AMP_MAX 3 >> + >> +/** >> + * ice_cgu_ena_pps_out - Enable/disable 1PPS output >> + * @hw: pointer to the HW struct >> + * @ena: Enable/disable 1PPS output > > Please include a "Returns: " or "Return: " section in the kernel doc > for functions that have a return value. last time I have checked only the singular "Return:" was supported (aka non-complained) by kdoc checker on W=2 builds > > NIPA has recently got more picky about this. > Flagged by kernel-doc -none --Warn > >> + */ >> +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena) >> +{ >> + union nac_cgu_dword9 dw9; >> + int err; >> + >> + err = ice_read_cgu_reg_e82x(hw, NAC_CGU_DWORD9, &dw9.val); >> + if (err) >> + return err; >> + >> + dw9.one_pps_out_en = ena; >> + dw9.one_pps_out_amp = ena * ICE_ONE_PPS_OUT_AMP_MAX; >> + return ice_write_cgu_reg_e82x(hw, NAC_CGU_DWORD9, dw9.val); >> +} >> + >> /** >> * ice_cfg_cgu_pll_dis_sticky_bits_e82x - disable TS PLL sticky bits >> * @hw: pointer to the HW struct > > ... >
On Mon, Jul 01, 2024 at 05:08:09PM +0200, Przemek Kitszel wrote: > On 7/1/24 15:27, Simon Horman wrote: > > On Thu, Jun 27, 2024 at 05:09:31PM +0200, Karol Kolacinski wrote: > > > From: Sergey Temerkhanov <sergey.temerkhanov@intel.com> > > > > > > Implement 1PPS signal enabling/disabling in CGU. The amplitude is > > > always the maximum in this implementation > > > > > > Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > > > Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com> > > > Co-developed-by: Karol Kolacinski <karol.kolacinski@intel.com> > > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > > > > ... > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > > > index 07ecf2a86742..fa7cf8453b88 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > > > @@ -661,6 +661,27 @@ static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw, > > > return 0; > > > } > > > +#define ICE_ONE_PPS_OUT_AMP_MAX 3 > > > + > > > +/** > > > + * ice_cgu_ena_pps_out - Enable/disable 1PPS output > > > + * @hw: pointer to the HW struct > > > + * @ena: Enable/disable 1PPS output > > > > Please include a "Returns: " or "Return: " section in the kernel doc > > for functions that have a return value. > > last time I have checked only the singular "Return:" was supported (aka > non-complained) by kdoc checker on W=2 builds Yes, agreed. My point was that the kernel document should have a (correct :) section to document the return value. > > > > > NIPA has recently got more picky about this. > > Flagged by kernel-doc -none --Warn > > > > > + */ > > > +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena) > > > +{ > > > + union nac_cgu_dword9 dw9; > > > + int err; > > > + > > > + err = ice_read_cgu_reg_e82x(hw, NAC_CGU_DWORD9, &dw9.val); > > > + if (err) > > > + return err; > > > + > > > + dw9.one_pps_out_en = ena; > > > + dw9.one_pps_out_amp = ena * ICE_ONE_PPS_OUT_AMP_MAX; > > > + return ice_write_cgu_reg_e82x(hw, NAC_CGU_DWORD9, dw9.val); > > > +} > > > + > > > /** > > > * ice_cfg_cgu_pll_dis_sticky_bits_e82x - disable TS PLL sticky bits > > > * @hw: pointer to the HW struct > > > > ... > > >
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index d9ff94a4b293..b97ea2b61e5c 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -4,6 +4,7 @@ #include "ice.h" #include "ice_lib.h" #include "ice_trace.h" +#include "ice_cgu_regs.h" static const char ice_pin_names[][64] = { "SDP0", @@ -1708,6 +1709,15 @@ static int ice_ptp_write_perout(struct ice_hw *hw, unsigned int chan, /* 0. Reset mode & out_en in AUX_OUT */ wr32(hw, GLTSYN_AUX_OUT(chan, tmr_idx), 0); + if (ice_is_e825c(hw)) { + int err; + + /* Enable/disable CGU 1PPS output for E825C */ + err = ice_cgu_ena_pps_out(hw, !!period); + if (err) + return err; + } + /* 1. Write perout with half of required period value. * HW toggles output when source clock hits the TGT and then adds * GLTSYN_CLKO value to the target, so it ends up with 50% duty cycle. diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c index 07ecf2a86742..fa7cf8453b88 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c @@ -661,6 +661,27 @@ static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw, return 0; } +#define ICE_ONE_PPS_OUT_AMP_MAX 3 + +/** + * ice_cgu_ena_pps_out - Enable/disable 1PPS output + * @hw: pointer to the HW struct + * @ena: Enable/disable 1PPS output + */ +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena) +{ + union nac_cgu_dword9 dw9; + int err; + + err = ice_read_cgu_reg_e82x(hw, NAC_CGU_DWORD9, &dw9.val); + if (err) + return err; + + dw9.one_pps_out_en = ena; + dw9.one_pps_out_amp = ena * ICE_ONE_PPS_OUT_AMP_MAX; + return ice_write_cgu_reg_e82x(hw, NAC_CGU_DWORD9, dw9.val); +} + /** * ice_cfg_cgu_pll_dis_sticky_bits_e82x - disable TS PLL sticky bits * @hw: pointer to the HW struct diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h index ff98f76969e3..382e84568256 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h @@ -331,6 +331,7 @@ extern const struct ice_vernier_info_e82x e822_vernier[NUM_ICE_PTP_LNK_SPD]; /* Device agnostic functions */ u8 ice_get_ptp_src_clock_index(struct ice_hw *hw); +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena); bool ice_ptp_lock(struct ice_hw *hw); void ice_ptp_unlock(struct ice_hw *hw); void ice_ptp_src_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd);