| Message ID | 20230821230554.236210-1-rrameshbabu@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 84a58e60038fa0366006977dba85eae16b2e3d78 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock | expand |
On 8/21/2023 4:05 PM, Rahul Rameshbabu wrote: > Use a dynamic calculation to determine the shift value for the internal > timer cyclecounter that will lead to the highest precision frequency > adjustments. Previously used a constant for the shift value assuming all > devices supported by the driver had a nominal frequency of 1GHz. However, > there are devices that operate at different frequencies. The previous shift > value constant would break the PHC functionality for those devices. > > Reported-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> > Closes: https://lore.kernel.org/netdev/20230815151507.3028503-1-vadfed@meta.com/ > Fixes: 6a4010927562 ("net/mlx5: Update cyclecounter shift value to improve ptp free running mode precision") > Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> > Tested-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> > --- > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Notes: > Devices tested on: > > * ConnectX 4 > * ConnectX 4-Lx > * ConnectX 5 > * ConnectX 6 > * ConnectX 6-Dx > * ConnectX 7 > > .../ethernet/mellanox/mlx5/core/lib/clock.c | 32 ++++++++++++++++--- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c > index 377372f0578a..aa29f09e8356 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c > @@ -32,16 +32,13 @@ > > #include <linux/clocksource.h> > #include <linux/highmem.h> > +#include <linux/log2.h> > #include <linux/ptp_clock_kernel.h> > #include <rdma/mlx5-abi.h> > #include "lib/eq.h" > #include "en.h" > #include "clock.h" > > -enum { > - MLX5_CYCLES_SHIFT = 31 > -}; > - > enum { > MLX5_PIN_MODE_IN = 0x0, > MLX5_PIN_MODE_OUT = 0x1, > @@ -93,6 +90,31 @@ static bool mlx5_modify_mtutc_allowed(struct mlx5_core_dev *mdev) > return MLX5_CAP_MCAM_FEATURE(mdev, ptpcyc2realtime_modify); > } > > +static u32 mlx5_ptp_shift_constant(u32 dev_freq_khz) > +{ > + /* Optimal shift constant leads to corrections above just 1 scaled ppm. > + * > + * Two sets of equations are needed to derive the optimal shift > + * constant for the cyclecounter. > + * > + * dev_freq_khz * 1000 / 2^shift_constant = 1 scaled_ppm > + * ppb = scaled_ppm * 1000 / 2^16 > + * > + * Using the two equations together > + * > + * dev_freq_khz * 1000 / 1 scaled_ppm = 2^shift_constant > + * dev_freq_khz * 2^16 / 1 ppb = 2^shift_constant > + * dev_freq_khz = 2^(shift_constant - 16) > + * > + * then yields > + * > + * shift_constant = ilog2(dev_freq_khz) + 16 > + */ > + I appreciate the derivation here. It helps understand the calculation here, and makes it clear why this is the best constant. Deriving it in terms of the frequency is useful since it makes supporting other frequencies much simpler in the future if thats ever necessary for the device family, rather than just adding a table of known frequencies. Nice! > + return min(ilog2(dev_freq_khz) + 16, > + ilog2((U32_MAX / NSEC_PER_MSEC) * dev_freq_khz)); > +} > + > static s32 mlx5_ptp_getmaxphase(struct ptp_clock_info *ptp) > { > struct mlx5_clock *clock = container_of(ptp, struct mlx5_clock, ptp_info); > @@ -909,7 +931,7 @@ static void mlx5_timecounter_init(struct mlx5_core_dev *mdev) > > dev_freq = MLX5_CAP_GEN(mdev, device_frequency_khz); > timer->cycles.read = read_internal_timer; > - timer->cycles.shift = MLX5_CYCLES_SHIFT; > + timer->cycles.shift = mlx5_ptp_shift_constant(dev_freq); > timer->cycles.mult = clocksource_khz2mult(dev_freq, > timer->cycles.shift); And you already derive the multiplier in terms of the frequency and shift, so the change in shift won't break the multiplier. Good. > timer->nominal_c_mult = timer->cycles.mult; Not really an issue of this patch, but a few drivers use a nominal multiplier in calculations with timecounter and cycle counter, I wonder if this could be baked into the cyclecounter code in the future... At any rate, this fix looks good to me.
On Wed, 23 Aug, 2023 12:54:21 -0700 Jacob Keller <jacob.e.keller@intel.com> wrote: > On 8/21/2023 4:05 PM, Rahul Rameshbabu wrote: >> Use a dynamic calculation to determine the shift value for the internal >> timer cyclecounter that will lead to the highest precision frequency >> adjustments. Previously used a constant for the shift value assuming all >> devices supported by the driver had a nominal frequency of 1GHz. However, >> there are devices that operate at different frequencies. The previous shift >> value constant would break the PHC functionality for those devices. >> >> Reported-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> Closes: https://lore.kernel.org/netdev/20230815151507.3028503-1-vadfed@meta.com/ >> Fixes: 6a4010927562 ("net/mlx5: Update cyclecounter shift value to improve ptp free running mode precision") >> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> >> Tested-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> --- >> > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > >> Notes: >> Devices tested on: >> >> * ConnectX 4 >> * ConnectX 4-Lx >> * ConnectX 5 >> * ConnectX 6 >> * ConnectX 6-Dx >> * ConnectX 7 >> >> .../ethernet/mellanox/mlx5/core/lib/clock.c | 32 ++++++++++++++++--- >> 1 file changed, 27 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c >> index 377372f0578a..aa29f09e8356 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c >> @@ -32,16 +32,13 @@ >> >> #include <linux/clocksource.h> >> #include <linux/highmem.h> >> +#include <linux/log2.h> >> #include <linux/ptp_clock_kernel.h> >> #include <rdma/mlx5-abi.h> >> #include "lib/eq.h" >> #include "en.h" >> #include "clock.h" >> >> -enum { >> - MLX5_CYCLES_SHIFT = 31 >> -}; >> - >> enum { >> MLX5_PIN_MODE_IN = 0x0, >> MLX5_PIN_MODE_OUT = 0x1, >> @@ -93,6 +90,31 @@ static bool mlx5_modify_mtutc_allowed(struct mlx5_core_dev *mdev) >> return MLX5_CAP_MCAM_FEATURE(mdev, ptpcyc2realtime_modify); >> } >> >> +static u32 mlx5_ptp_shift_constant(u32 dev_freq_khz) >> +{ >> + /* Optimal shift constant leads to corrections above just 1 scaled ppm. >> + * >> + * Two sets of equations are needed to derive the optimal shift >> + * constant for the cyclecounter. >> + * >> + * dev_freq_khz * 1000 / 2^shift_constant = 1 scaled_ppm >> + * ppb = scaled_ppm * 1000 / 2^16 >> + * >> + * Using the two equations together >> + * >> + * dev_freq_khz * 1000 / 1 scaled_ppm = 2^shift_constant >> + * dev_freq_khz * 2^16 / 1 ppb = 2^shift_constant >> + * dev_freq_khz = 2^(shift_constant - 16) >> + * >> + * then yields >> + * >> + * shift_constant = ilog2(dev_freq_khz) + 16 >> + */ >> + > > I appreciate the derivation here. It helps understand the calculation > here, and makes it clear why this is the best constant. Deriving it in > terms of the frequency is useful since it makes supporting other > frequencies much simpler in the future if thats ever necessary for the > device family, rather than just adding a table of known frequencies. Nice! > >> + return min(ilog2(dev_freq_khz) + 16, >> + ilog2((U32_MAX / NSEC_PER_MSEC) * dev_freq_khz)); >> +} >> + >> static s32 mlx5_ptp_getmaxphase(struct ptp_clock_info *ptp) >> { >> struct mlx5_clock *clock = container_of(ptp, struct mlx5_clock, ptp_info); >> @@ -909,7 +931,7 @@ static void mlx5_timecounter_init(struct mlx5_core_dev *mdev) >> >> dev_freq = MLX5_CAP_GEN(mdev, device_frequency_khz); >> timer->cycles.read = read_internal_timer; >> - timer->cycles.shift = MLX5_CYCLES_SHIFT; >> + timer->cycles.shift = mlx5_ptp_shift_constant(dev_freq); >> timer->cycles.mult = clocksource_khz2mult(dev_freq, >> timer->cycles.shift); > > And you already derive the multiplier in terms of the frequency and > shift, so the change in shift won't break the multiplier. Good. > >> timer->nominal_c_mult = timer->cycles.mult; > > > Not really an issue of this patch, but a few drivers use a nominal > multiplier in calculations with timecounter and cycle counter, I wonder > if this could be baked into the cyclecounter code in the future... This ran through my mind as I was making this patch. As you mentioned, the logic used here is not specific to mlx5. Rather, it's a general calculator for the shift value given a frequency. I wanted to look at all the use cases of cyclecounter before providing a general API for this. I will likely follow up with you if I have concerns with regards to the generalization for the cyclecounter API and hopefully can share an RFC. Thanks, Rahul Rameshbabu > > At any rate, this fix looks good to me.
> On 22 Aug 2023, at 01:05, Rahul Rameshbabu <rrameshbabu@nvidia.com> wrote: ... > +static u32 mlx5_ptp_shift_constant(u32 dev_freq_khz) > +{ > + /* Optimal shift constant leads to corrections above just 1 scaled ppm. > + * > + * Two sets of equations are needed to derive the optimal shift > + * constant for the cyclecounter. This is easy to follow, so I’m not suggesting you change it. But out of curiosity, have you considered using the more generic clocks_calc_mult_shift() to find a suitable shift value? > @@ -909,7 +931,7 @@ static void mlx5_timecounter_init(struct mlx5_core_dev *mdev) > > dev_freq = MLX5_CAP_GEN(mdev, device_frequency_khz); > timer->cycles.read = read_internal_timer; > - timer->cycles.shift = MLX5_CYCLES_SHIFT; > + timer->cycles.shift = mlx5_ptp_shift_constant(dev_freq); > timer->cycles.mult = clocksource_khz2mult(dev_freq, > timer->cycles.shift); > timer->nominal_c_mult = timer->cycles.mult; The mask assignment one line below nominal_c_mult could need updating to match the new shift, e.g., timer->cycles.mask = CLOCKSOURCE_MASK(64 - timer->cycles.shift); ? It’s currently fixed to 41, which (via 64-23 = 41) matches the cyclecounter bits shifted out constant of 23 in kernels v6.3 and before.
On Mon, Aug 21, 2023 at 04:05:54PM -0700, Rahul Rameshbabu wrote: > Use a dynamic calculation to determine the shift value for the internal > timer cyclecounter that will lead to the highest precision frequency > adjustments. Previously used a constant for the shift value assuming all > devices supported by the driver had a nominal frequency of 1GHz. However, > there are devices that operate at different frequencies. The previous shift > value constant would break the PHC functionality for those devices. > > Reported-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> > Closes: https://lore.kernel.org/netdev/20230815151507.3028503-1-vadfed@meta.com/ > Fixes: 6a4010927562 ("net/mlx5: Update cyclecounter shift value to improve ptp free running mode precision") > Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> > Tested-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> Reviewed-by: Simon Horman <horms@kernel.org>
On 21 Aug 16:05, Rahul Rameshbabu wrote: >Use a dynamic calculation to determine the shift value for the internal >timer cyclecounter that will lead to the highest precision frequency >adjustments. Previously used a constant for the shift value assuming all >devices supported by the driver had a nominal frequency of 1GHz. However, >there are devices that operate at different frequencies. The previous shift >value constant would break the PHC functionality for those devices. > >Reported-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >Closes: https://lore.kernel.org/netdev/20230815151507.3028503-1-vadfed@meta.com/ >Fixes: 6a4010927562 ("net/mlx5: Update cyclecounter shift value to improve ptp free running mode precision") >Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> >Tested-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> Acked-by: Saeed Mahameed <saeedm@nvidia.com> I have nothing else in my queue so just go ahead and apply directly to net. Thanks, Saeed.
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 21 Aug 2023 16:05:54 -0700 you wrote: > Use a dynamic calculation to determine the shift value for the internal > timer cyclecounter that will lead to the highest precision frequency > adjustments. Previously used a constant for the shift value assuming all > devices supported by the driver had a nominal frequency of 1GHz. However, > there are devices that operate at different frequencies. The previous shift > value constant would break the PHC functionality for those devices. > > [...] Here is the summary with links: - [net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock https://git.kernel.org/netdev/net/c/84a58e60038f You are awesome, thank you!
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c index 377372f0578a..aa29f09e8356 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c @@ -32,16 +32,13 @@ #include <linux/clocksource.h> #include <linux/highmem.h> +#include <linux/log2.h> #include <linux/ptp_clock_kernel.h> #include <rdma/mlx5-abi.h> #include "lib/eq.h" #include "en.h" #include "clock.h" -enum { - MLX5_CYCLES_SHIFT = 31 -}; - enum { MLX5_PIN_MODE_IN = 0x0, MLX5_PIN_MODE_OUT = 0x1, @@ -93,6 +90,31 @@ static bool mlx5_modify_mtutc_allowed(struct mlx5_core_dev *mdev) return MLX5_CAP_MCAM_FEATURE(mdev, ptpcyc2realtime_modify); } +static u32 mlx5_ptp_shift_constant(u32 dev_freq_khz) +{ + /* Optimal shift constant leads to corrections above just 1 scaled ppm. + * + * Two sets of equations are needed to derive the optimal shift + * constant for the cyclecounter. + * + * dev_freq_khz * 1000 / 2^shift_constant = 1 scaled_ppm + * ppb = scaled_ppm * 1000 / 2^16 + * + * Using the two equations together + * + * dev_freq_khz * 1000 / 1 scaled_ppm = 2^shift_constant + * dev_freq_khz * 2^16 / 1 ppb = 2^shift_constant + * dev_freq_khz = 2^(shift_constant - 16) + * + * then yields + * + * shift_constant = ilog2(dev_freq_khz) + 16 + */ + + return min(ilog2(dev_freq_khz) + 16, + ilog2((U32_MAX / NSEC_PER_MSEC) * dev_freq_khz)); +} + static s32 mlx5_ptp_getmaxphase(struct ptp_clock_info *ptp) { struct mlx5_clock *clock = container_of(ptp, struct mlx5_clock, ptp_info); @@ -909,7 +931,7 @@ static void mlx5_timecounter_init(struct mlx5_core_dev *mdev) dev_freq = MLX5_CAP_GEN(mdev, device_frequency_khz); timer->cycles.read = read_internal_timer; - timer->cycles.shift = MLX5_CYCLES_SHIFT; + timer->cycles.shift = mlx5_ptp_shift_constant(dev_freq); timer->cycles.mult = clocksource_khz2mult(dev_freq, timer->cycles.shift); timer->nominal_c_mult = timer->cycles.mult;
Use a dynamic calculation to determine the shift value for the internal timer cyclecounter that will lead to the highest precision frequency adjustments. Previously used a constant for the shift value assuming all devices supported by the driver had a nominal frequency of 1GHz. However, there are devices that operate at different frequencies. The previous shift value constant would break the PHC functionality for those devices. Reported-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> Closes: https://lore.kernel.org/netdev/20230815151507.3028503-1-vadfed@meta.com/ Fixes: 6a4010927562 ("net/mlx5: Update cyclecounter shift value to improve ptp free running mode precision") Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> Tested-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> --- Notes: Devices tested on: * ConnectX 4 * ConnectX 4-Lx * ConnectX 5 * ConnectX 6 * ConnectX 6-Dx * ConnectX 7 .../ethernet/mellanox/mlx5/core/lib/clock.c | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-)