[iwl-net] e1000e: correct maximum frequency adjustment values

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


base-commit: e307b5a845c5951dabafc48d00b6424ee64716c4

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>