| Message ID | 20231209170753.168989-1-sasha.neftin@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-net,v1,1/1] e1000e: Correct the maximum frequency adjustment | expand |
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:
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; 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; case e1000_82574: case e1000_82583:
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> --- drivers/net/ethernet/intel/e1000e/ptp.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)