[PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock

[PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock

[Rahul Rameshbabu]

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(-)

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;
-- 
2.40.1

Re: [PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock

[Jacob Keller]

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.

Re: [PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock

[Rahul Rameshbabu]

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.

Re: [PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock

[Kenneth Klette Jonassen]

> 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.

Re: [PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock

[Simon Horman]

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>

Re: [PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock

[Saeed Mahameed]

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.

Re: [PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html