* Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: Correct the maximum frequency adjustment
2023-12-09 17:07 [Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: Correct the maximum frequency adjustment Sasha Neftin
@ 2023-12-12 2:23 ` Jacob Keller
0 siblings, 0 replies; 2+ messages in thread
From: Jacob Keller @ 2023-12-12 2:23 UTC (permalink / raw)
To: intel-wired-lan, Tony Nguyen, Sasha Levin
On 12/9/2023 9:07 AM, Sasha Neftin wrote:
> The latest Intel platform used two clocks for 1588 time synchronization
> dependent on the HW strap: 24 MHz and 38.4 MHz. The maximum possible
> frequency adjustment, in parts per billion, calculated as follows:
> max ppb = ((MAX_INCVAL – BASE_INCVAL)*1billion) / BASE_INCVAL.
> Where MAX_INCVAL is TIMINCA resolution (2^24 -1) and BASE_INCVAL is
> depends on the clock.
> For 24 MHz the max ppb value should be 600,000,000 and for the 38.4MHz
> the max ppb value is 230,000,000.
>
> Reported-by: Trey Harrison <harrisondigitalmedia@gmail.com>
> Fixes: d89777bf0e42 ("e1000e: add support for IEEE-1588 PTP")
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> ---
So, I did review this before, but it looks like I missed some things.
Tony had some questions and asked me to clarify, so we walked through
this and it looks like there are a few issues remaining, as well as some
difficulty following the logic.
I'm going to try and repeat the analysis here, and will likely send a v2
based on the feedback.
In order to understand where these max_adj values come from, we need to
be aware of the relevant code from netdev.c:
> switch (hw->mac.type) {
> case e1000_pch2lan:
> /* Stable 96MHz frequency */
> incperiod = INCPERIOD_96MHZ;
> incvalue = INCVALUE_96MHZ;
> shift = INCVALUE_SHIFT_96MHZ;
> adapter->cc.shift = shift + INCPERIOD_SHIFT_96MHZ;
> break;
> case e1000_pch_lpt:
> if (er32(TSYNCRXCTL) & E1000_TSYNCRXCTL_SYSCFI) {
> /* Stable 96MHz frequency */
> incperiod = INCPERIOD_96MHZ;
> incvalue = INCVALUE_96MHZ;
> shift = INCVALUE_SHIFT_96MHZ;
> adapter->cc.shift = shift + INCPERIOD_SHIFT_96MHZ;
> } else {
> /* Stable 25MHz frequency */
> incperiod = INCPERIOD_25MHZ;
> incvalue = INCVALUE_25MHZ;
> shift = INCVALUE_SHIFT_25MHZ;
> adapter->cc.shift = shift;
> }
> break;
> case e1000_pch_spt:
> /* Stable 24MHz frequency */
> incperiod = INCPERIOD_24MHZ;
> incvalue = INCVALUE_24MHZ;
> shift = INCVALUE_SHIFT_24MHZ;
> adapter->cc.shift = shift;
> break;
> case e1000_pch_cnp:
> case e1000_pch_tgp:
> case e1000_pch_adp:
> case e1000_pch_mtp:
> case e1000_pch_lnp:
> case e1000_pch_ptp:
> case e1000_pch_nvp:
> if (er32(TSYNCRXCTL) & E1000_TSYNCRXCTL_SYSCFI) {
> /* Stable 24MHz frequency */
> incperiod = INCPERIOD_24MHZ;
> incvalue = INCVALUE_24MHZ;
> shift = INCVALUE_SHIFT_24MHZ;
> adapter->cc.shift = shift;
> } else {
> /* Stable 38400KHz frequency */
> incperiod = INCPERIOD_38400KHZ;
> incvalue = INCVALUE_38400KHZ;
> shift = INCVALUE_SHIFT_38400KHZ;
> adapter->cc.shift = shift;
> }
> break;
> case e1000_82574:
> case e1000_82583:
> /* Stable 25MHz frequency */
> incperiod = INCPERIOD_25MHZ;
> incvalue = INCVALUE_25MHZ;
> shift = INCVALUE_SHIFT_25MHZ;
> adapter->cc.shift = shift;
> break;
> default:
> return -EINVAL;
> }
And the relevant macro definitions for the increment values:
> /* The system time is maintained by a 64-bit counter comprised of the 32-bit
> * SYSTIMH and SYSTIML registers. How the counter increments (and therefore
> * its resolution) is based on the contents of the TIMINCA register - it
> * increments every incperiod (bits 31:24) clock ticks by incvalue (bits 23:0).
> * For the best accuracy, the incperiod should be as small as possible. The
> * incvalue is scaled by a factor as large as possible (while still fitting
> * in bits 23:0) so that relatively small clock corrections can be made.
> *
> * As a result, a shift of INCVALUE_SHIFT_n is used to fit a value of
> * INCVALUE_n into the TIMINCA register allowing 32+8+(24-INCVALUE_SHIFT_n)
> * bits to count nanoseconds leaving the rest for fractional nonseconds.
> */
> #define INCVALUE_96MHZ 125
> #define INCVALUE_SHIFT_96MHZ 17
> #define INCPERIOD_SHIFT_96MHZ 2
> #define INCPERIOD_96MHZ (12 >> INCPERIOD_SHIFT_96MHZ)
>
> #define INCVALUE_25MHZ 40
> #define INCVALUE_SHIFT_25MHZ 18
> #define INCPERIOD_25MHZ 1
>
> #define INCVALUE_24MHZ 125
> #define INCVALUE_SHIFT_24MHZ 14
> #define INCPERIOD_24MHZ 3
>
> #define INCVALUE_38400KHZ 26
> #define INCVALUE_SHIFT_38400KHZ 19
> #define INCPERIOD_38400KHZ 1
>
The key thing here is that max_adj is a function of the increment value.
When we make an adjustment we slightly modify the increment value using
the following equation:
new_incval = base_incval + ( base_incval * ppb_adj / 1billion)
We do use mul_u64_u64_divu64 to do the calculation, so the step with
"base_incval * ppb_adj / 1 billion) will never overflow as long as
ppb_adj is <1billion. However, we do need to make sure that the new
increment value calculated is never > the maximum possible increment value.
For the e1000e parts, this is always 2^24-1, 0xFFFFFF.
Thus, to calculate the maximum possible adjustment we can re-arrange the
equation as:
0xFFFFFF = base_incval + ((base_incval * max_adj) / 1billion)
0xFFFFFF - base_incval = (base_incval * max_adj) / 1billion
(0xFFFFFF - base_incval) * 1billion = base_incval * max_adj
((0xFFFFFF - base_incval) * 1billion) / base_incval = max_adj
Performing these steps for each increment value we get the following for
each set of incvalues:
> #define INCVALUE_96MHZ 125
> #define INCVALUE_SHIFT_96MHZ 17
> #define INCPERIOD_SHIFT_96MHZ 2
> #define INCPERIOD_96MHZ (12 >> INCPERIOD_SHIFT_96MHZ)
>
(0xFFFFFF - (125 << 17)) * 1bilion / (125 <<17):
23,999,938.96484375 or slightly less than 24 million
We can confirm this by taking the max increment value and multiplying by
600m and then dividing by 1billion:
393,214.984192 + 16,384,000 = 16,777,214.984192 which is *just below*
0xFFFFFF (16,777,215)
Thus, the maximum adjust for 96MHz clock here should be a bit less than
24 million, 23,999,938 but we can round this to 23,999,900.
> #define INCVALUE_25MHZ 40
> #define INCVALUE_SHIFT_25MHZ 18
> #define INCPERIOD_25MHZ 1
>
Doing the same calculation here, we get:
(0xFFFFFF - (40 << 18)) * 1billion / (40 << 18)
40 << 18 = 10,485,760
max_adj = 599,999,904.632568359375, or slightly less than 600 million.
Again, we need to round down to below 599,999,904, so rounding to a 100
would be 599,999,900
> #define INCVALUE_24MHZ 125
> #define INCVALUE_SHIFT_24MHZ 14
> #define INCPERIOD_24MHZ 3
>
This one gets a bit weird:
(0xFFFFFF - (125 << 14)) * 1billion / (125 << 14)
125 << 14 = 2,048,000
max_adj = 7,191,999,511.71875
actually much *greater* than 1billion. This appears to be due to the way
that the base_incval was chosen to be so small. We're actually adding
125<<14 once every 3rd clock tick instead. This is likely because 24MHz
has a clock period of 41+2/3 ns, which is hard to represent accurately.
Anyways, any maximum adjust value <1billion is going to be safe. We
could investigate and come up with a better increment value and a better
maximum adjustment, but I don't think thats warranted in a small bug fix.
Using 600million is probably reasonable, though we could make this as
high as 1billion.
The existing code uses either 600m or 24m depending on the part.
> #define INCVALUE_38400KHZ 26
> #define INCVALUE_SHIFT_38400KHZ 19
> #define INCPERIOD_38400KHZ 1
This is the one which is missing from the maximum adjustments.
(0xFFFFFF - (26<<19)) * 1billion / (26<<19)
26<<19 = 13,631,488
max_adj = 230,769,157.40966796875
> switch (hw->mac.type) {
> case e1000_pch2lan:
> /* Stable 96MHz frequency */
> incperiod = INCPERIOD_96MHZ;
> incvalue = INCVALUE_96MHZ;
> shift = INCVALUE_SHIFT_96MHZ;
> adapter->cc.shift = shift + INCPERIOD_SHIFT_96MHZ;
> break;
phc2lan should always use the 96MHz max adj of 23,999,900
> case e1000_pch_lpt:
> if (er32(TSYNCRXCTL) & E1000_TSYNCRXCTL_SYSCFI) {
> /* Stable 96MHz frequency */
> incperiod = INCPERIOD_96MHZ;
> incvalue = INCVALUE_96MHZ;
> shift = INCVALUE_SHIFT_96MHZ;
> adapter->cc.shift = shift + INCPERIOD_SHIFT_96MHZ;
> } else {
> /* Stable 25MHz frequency */
> incperiod = INCPERIOD_25MHZ;
> incvalue = INCVALUE_25MHZ;
> shift = INCVALUE_SHIFT_25MHZ;
> adapter->cc.shift = shift;
> }
> break;
phc_lpt should use 96Mhz value of 23,999,900 when SYSCFI is set, and
should use 599,999,900 for 25MHz when its not.
> case e1000_pch_spt:
> /* Stable 24MHz frequency */
> incperiod = INCPERIOD_24MHZ;
> incvalue = INCVALUE_24MHZ;
> shift = INCVALUE_SHIFT_24MHZ;
> adapter->cc.shift = shift;
> break;
phc_spt should always use a maximum adjustment value for 24Mhz, which
could be anything up to 1billion.
> case e1000_pch_cnp:
> case e1000_pch_tgp:
> case e1000_pch_adp:
> case e1000_pch_mtp:
> case e1000_pch_lnp:
> case e1000_pch_ptp:
> case e1000_pch_nvp:
> if (er32(TSYNCRXCTL) & E1000_TSYNCRXCTL_SYSCFI) {
> /* Stable 24MHz frequency */
> incperiod = INCPERIOD_24MHZ;
> incvalue = INCVALUE_24MHZ;
> shift = INCVALUE_SHIFT_24MHZ;
> adapter->cc.shift = shift;
> } else {
> /* Stable 38400KHz frequency */
> incperiod = INCPERIOD_38400KHZ;
> incvalue = INCVALUE_38400KHZ;
> shift = INCVALUE_SHIFT_38400KHZ;
> adapter->cc.shift = shift;
> }
> break;
cnp through nvp should use the 24Mhz value when SYSCFI is set, and
should use the 230,769,100 value when its not.
> case e1000_82574:
> case e1000_82583:
> /* Stable 25MHz frequency */
> incperiod = INCPERIOD_25MHZ;
> incvalue = INCVALUE_25MHZ;
> shift = INCVALUE_SHIFT_25MHZ;
> adapter->cc.shift = shift;
> break;
82574 and 82583 should use the 599,999,900 value for 25Mhz clock.
Currently we use the following table:
> switch (hw->mac.type) {
> case e1000_pch2lan:
> case e1000_pch_lpt:
> case e1000_pch_spt:
> case e1000_pch_cnp:
> case e1000_pch_tgp:
> case e1000_pch_adp:
> case e1000_pch_mtp:
> case e1000_pch_lnp:
> case e1000_pch_ptp:
> case e1000_pch_nvp:
> if ((hw->mac.type < e1000_pch_lpt) ||
> (er32(TSYNCRXCTL) & E1000_TSYNCRXCTL_SYSCFI)) {
> adapter->ptp_clock_info.max_adj = 24000000 - 1;
> break;
> }
> fallthrough;
This is incredibly confusing because it requires knowing that phc2lan is
less than lpt, but no other values are. And it requires processing a
fallthrough block.
phc2lan will use 24million -1, which is *slightly* above the actual
maximum value we can support.
lpt will use 24million - 1 of SYSCFI is set, and fall through to use
600million -1 otherwise. Both of these are mostly correct but again
slightly too high.
spt should fall through and use the 600million value which is safe, if
slightly lower than we need.
cnp through npv will artificially limit themselves to 24million if
SYSCFI is set (24 MHz clock!) and will use the incorrect value of
600million when its not.
They can use up to 1billion for the 24MHz clock as-is, and should be
limited to ~230million for the 38.4 MHz clock.
> case e1000_82574:
> case e1000_82583:
> adapter->ptp_clock_info.max_adj = 600000000 - 1;
> break;
82574 and 82583 use 600million -1, which is just slightly too high.
> default:
> break;
> }
This patch leaves several holes and is extremely difficult to follow. I
think we should reject it and submit a proper fix that cleans up the
600million and 24million values, introduces macros to define the values,
and makes the case tables for both functions match better. (It would be
nice to refactor them to only have a single case table, but that may not
be straight forward.
I also think the 24MHz clock should be adjusted to better fit without
sacrificing precision... but that would require a lot of investigation
that I don't have time to do.
> drivers/net/ethernet/intel/e1000e/ptp.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/ptp.c b/drivers/net/ethernet/intel/e1000e/ptp.c
> index 02d871bc112a..792dfe602ca0 100644
> --- a/drivers/net/ethernet/intel/e1000e/ptp.c
> +++ b/drivers/net/ethernet/intel/e1000e/ptp.c
> @@ -283,17 +283,24 @@ void e1000e_ptp_init(struct e1000_adapter *adapter)
> case e1000_pch_lpt:
> case e1000_pch_spt:
> case e1000_pch_cnp:
> + if (hw->mac.type < e1000_pch_lpt ||
> + (er32(TSYNCRXCTL) & E1000_TSYNCRXCTL_SYSCFI)) {
> + adapter->ptp_clock_info.max_adj = 24000000 - 1;
> + break;
> + }
> + fallthrough;
In particular here, the use of fallthrough is extremely confusing. We'll
make the SYSCFI check, and then fall through and repeat the check. For
devices <phc_lpt, (phc2lan) this will use 24million, but if SYSCFI is
not set, LPT and CNP will fall through and fall into the 230million
calculation below.
For CNP, this artificially limits the maximum adjustment for the 24MHz
clock variation.
> case e1000_pch_tgp:
> case e1000_pch_adp:
> case e1000_pch_mtp:
> case e1000_pch_lnp:
> case e1000_pch_ptp:
> case e1000_pch_nvp:
> - if ((hw->mac.type < e1000_pch_lpt) ||
> - (er32(TSYNCRXCTL) & E1000_TSYNCRXCTL_SYSCFI)) {
> - adapter->ptp_clock_info.max_adj = 24000000 - 1;
> - break;
> - }
> + if (hw->mac.type < e1000_pch_lpt ||
> + (er32(TSYNCRXCTL) & E1000_TSYNCRXCTL_SYSCFI))
> + adapter->ptp_clock_info.max_adj = 600000000 - 1;
> + else
> + adapter->ptp_clock_info.max_adj = 230000000 - 1;
> + break;
> fallthrough;
This has an extra unnecessary fallthrough, and the fact that we fall
into this b lock from above is confusing to process.
In addition, since this does not mirror the switch statement in
e1000e_get_base_timinca, it is extremely difficult to verify the logic.
You essentially have to reverse engineer where these values came from
each time you look at this block.
> case e1000_82574:
> case e1000_82583:
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 2+ messages in thread