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

[Jacob Keller <jacob.e.keller@intel.com>]

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.