From: Rahul Rameshbabu <rrameshbabu@nvidia•com>
To: Jacob Keller <jacob.e.keller@intel•com>
Cc: <netdev@vger•kernel.org>, Saeed Mahameed <saeed@kernel•org>,
Jakub Kicinski <kuba@kernel•org>,
Richard Cochran <richardcochran@gmail•com>,
"David S. Miller" <davem@davemloft•net>,
Paolo Abeni <pabeni@redhat•com>,
Vadim Fedorenko <vadfed@meta•com>,
Kenneth Klette Jonassen <kenneth.jonassen@bridgetech•tv>
Subject: Re: [PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock
Date: Wed, 23 Aug 2023 14:46:20 -0700 [thread overview]
Message-ID: <87wmxl5t5f.fsf@nvidia.com> (raw)
In-Reply-To: <82ddc62b-4d2c-cf6f-f5c8-812ce795a494@intel.com> (Jacob Keller's message of "Wed, 23 Aug 2023 12:54:21 -0700")
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.
next prev parent reply other threads:[~2023-08-23 21:46 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-21 23:05 [PATCH net] net/mlx5: Dynamic cyclecounter shift calculation for PTP free running clock Rahul Rameshbabu
2023-08-23 19:54 ` Jacob Keller
2023-08-23 21:46 ` Rahul Rameshbabu [this message]
2023-08-23 23:31 ` Kenneth Klette Jonassen
2023-08-24 8:40 ` Simon Horman
2023-08-24 20:40 ` Saeed Mahameed
2023-08-25 2:10 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wmxl5t5f.fsf@nvidia.com \
--to=rrameshbabu@nvidia$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=jacob.e.keller@intel$(echo .)com \
--cc=kenneth.jonassen@bridgetech$(echo .)tv \
--cc=kuba@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=richardcochran@gmail$(echo .)com \
--cc=saeed@kernel$(echo .)org \
--cc=vadfed@meta$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox