Re: [Intel-wired-lan] [PATCH iwl-net] e1000e: correct maximum frequency adjustment values

["naamax.meir" <naamax.meir@linux.intel.com>]

On 12/12/2023 04:05, Jacob Keller wrote:
> The e1000e driver supports hardware with a variety of different clock
> speeds, and thus a variety of different increment values used for
> programming its PTP hardware clock.
> 
> The values currently programmed in e1000e_ptp_init are incorrect. In
> particular, only two maximum adjustments are used: 24000000 - 1, and
> 600000000 - 1. These were originally intended to be used with the 96 MHz
> clock and the 25 MHz clock.
> 
> Both of these values are actually slightly too high. For the 96 MHz clock,
> the actual maximum value that can safely be programmed is 23,999,938. For
> the 25 MHz clock, the maximum value is 599,999,904.
> 
> Worse, several devices use a 24 MHz clock or a 38.4 MHz clock. These parts
> are incorrectly assigned one of either the 24million or 600million values.
> For the 24 MHz clock, this is not a significant issue: its current
> increment value can support an adjustment up to 7billion in the positive
> direction. However, the 38.4 KHz clock uses an increment value which can
> only support up to 230,769,157 before it starts overflowing.
> 
> To understand where these values come from, consider that frequency
> adjustments have the form of:
> 
> new_incval = base_incval + (base_incval * adjustment) / (unit of adjustment)
> 
> The maximum adjustment is reported in terms of parts per billion:
> new_incval = base_incval + (base_incval * adjustment) / 1 billion
> 
> The largest possible adjustment is thus given by the following:
> max_incval = base_incval + (base_incval * max_adj) / 1 billion
> 
> Re-arranging to solve for max_adj:
> max_adj = (max_incval - base_incval) * 1 billion / base_incval
> 
> We also need to ensure that negative adjustments cannot underflow. This can
> be achieved simply by ensuring max_adj is always less than 1 billion.
> 
> Introduce new macros in e1000.h codifying the maximum adjustment in PPB for
> each frequency given its associated increment values. Also clarify where
> these values come from by commenting about the above equations.
> 
> Replace the switch statement in e1000e_ptp_init with one which mirrors the
> increment value switch statement from e1000e_get_base_timinica. For each
> device, assign the appropriate maximum adjustment based on its frequency.
> Some parts can have one of two frequency modes as determined by
> E1000_TSYNCRXCTL_SYSCFI.
> 
> Since the new flow directly matches the assignments in
> e1000e_get_base_timinca, and uses well defined macro names, it is much
> easier to verify that the resulting maximum adjustments are correct. It
> also avoids difficult to parse construction such as the "hw->mac.type <
> e1000_phc_lpt", and the use of fallthrough which was especially confusing
> when combined with a conditional block.
> 
> Note that I believe the current increment value configuration used for
> 24MHz clocks is sub-par, as it leaves at least 3 extra bits available in
> the INCVALUE register. However, fixing that requires more careful review of
> the clock rate and associated values.
> 
> Reported-by: Trey Harrison <harrisondigitalmedia@gmail.com>
> Fixes: 68fe1d5da548 ("e1000e: Add Support for 38.4MHZ frequency")
> Fixes: d89777bf0e42 ("e1000e: add support for IEEE-1588 PTP")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> Sasha, here's a better solution to what you proposed at [1], which fixes the
> issues I reported. It fixes the 24million and 600 million values, fixes the
> case statement so that its easier to follow, and introduces macros to avoid
> the confusion of the magic numbers.
> 
> The 24m and 600m max values appear to have originated all the way back when
> the driver first introduced the PTP support. I suspect no one directly
> tested whether such a large adjustment would behave correctly. They would
> only come up in normal practice rarely (such as if ptp4l when configured to
> avoid stepping the clock and only perform frequency slewing, and the remote
> clock had a large jump occur).
> 
> [1]: https://lore.kernel.org/intel-wired-lan/20231209170753.168989-1-sasha.neftin@intel.com/
> 
>   drivers/net/ethernet/intel/e1000e/e1000.h | 20 ++++++++++++++++++++
>   drivers/net/ethernet/intel/e1000e/ptp.c   | 22 +++++++++++++++-------
>   2 files changed, 35 insertions(+), 7 deletions(-)

Tested-by: Naama Meir <naamax.meir@linux.intel.com>