| Message ID | 20240417164316.1755299-3-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add support for TimeSync path delays | expand |
On Wed, Apr 17, 2024 at 06:43:14PM +0200, Oleksij Rempel wrote: > Set default PTP latency values to provide realistic path delay > measurements and reflecting internal PHY latency asymetry. > > This values are based on ptp4l measurements for the path delay against > identical PHY as link partner and latency asymmetry extracted from > documented SOF Latency values of this PHY. > > Documented SOF Latency values are: > TX 138ns/RX 430ns @ 1000Mbps > TX 140ns/RX 615ns @ 100Mbps (fixed latency mode) > TX 140ns/RX 488-524ns @ 100Mbps (variable latency mode) > TX 654ns/227-2577ns @ 10Mbps Does Half Duplex vs Full Duplex make a difference here? > +static int lan8841_ptp_latency_init(struct phy_device *phydev) > +{ > + int ret; > + > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > + LAN8841_PTP_RX_LATENCY_10M, > + LAN8841_PTP_RX_LATENCY_10M_VAL); > + if (ret) > + return ret; > + > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > + LAN8841_PTP_TX_LATENCY_10M, > + LAN8841_PTP_TX_LATENCY_10M_VAL); > + if (ret) > + return ret; > + > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > + LAN8841_PTP_RX_LATENCY_100M, > + LAN8841_PTP_RX_LATENCY_100M_VAL); > + if (ret) > + return ret; > + > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > + LAN8841_PTP_TX_LATENCY_100M, > + LAN8841_PTP_TX_LATENCY_100M_VAL); > + if (ret) > + return ret; > + > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > + LAN8841_PTP_RX_LATENCY_1000M, > + LAN8841_PTP_RX_LATENCY_1000M_VAL); > + if (ret) > + return ret; > + > + return phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > + LAN8841_PTP_TX_LATENCY_1000M, > + LAN8841_PTP_TX_LATENCY_1000M_VAL); > +} What affect does this have on systems which have already applied adjustments in user space to correct for this? Will this cause regressions for such systems? I know Richard has rejected changes like this in the past. Andrew
On Wed, Apr 17, 2024 at 08:39:54PM +0200, Andrew Lunn wrote: > On Wed, Apr 17, 2024 at 06:43:14PM +0200, Oleksij Rempel wrote: > > Set default PTP latency values to provide realistic path delay > > measurements and reflecting internal PHY latency asymetry. > > > > This values are based on ptp4l measurements for the path delay against > > identical PHY as link partner and latency asymmetry extracted from > > documented SOF Latency values of this PHY. > > > > Documented SOF Latency values are: > > TX 138ns/RX 430ns @ 1000Mbps > > TX 140ns/RX 615ns @ 100Mbps (fixed latency mode) > > TX 140ns/RX 488-524ns @ 100Mbps (variable latency mode) > > TX 654ns/227-2577ns @ 10Mbps Here is a typo 2277-2577ns > Does Half Duplex vs Full Duplex make a difference here? Yes, Half Duplex will be even less predictable. It would make no sense to use it for the time sync. > > +static int lan8841_ptp_latency_init(struct phy_device *phydev) > > +{ > > + int ret; > > + > > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > > + LAN8841_PTP_RX_LATENCY_10M, > > + LAN8841_PTP_RX_LATENCY_10M_VAL); > > + if (ret) > > + return ret; > > + > > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > > + LAN8841_PTP_TX_LATENCY_10M, > > + LAN8841_PTP_TX_LATENCY_10M_VAL); > > + if (ret) > > + return ret; > > + > > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > > + LAN8841_PTP_RX_LATENCY_100M, > > + LAN8841_PTP_RX_LATENCY_100M_VAL); > > + if (ret) > > + return ret; > > + > > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > > + LAN8841_PTP_TX_LATENCY_100M, > > + LAN8841_PTP_TX_LATENCY_100M_VAL); > > + if (ret) > > + return ret; > > + > > + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > > + LAN8841_PTP_RX_LATENCY_1000M, > > + LAN8841_PTP_RX_LATENCY_1000M_VAL); > > + if (ret) > > + return ret; > > + > > + return phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > > + LAN8841_PTP_TX_LATENCY_1000M, > > + LAN8841_PTP_TX_LATENCY_1000M_VAL); > > +} > > What affect does this have on systems which have already applied > adjustments in user space to correct for this? Will this cause > regressions for such systems? Yes. > I know Richard has rejected changes like this in the past. In this case I would need to extend the ethtool interface. The driver should provide recommended values and the user space can optionally read them and optionally write them to the HW. Get Recommended TimeSync Data Path Delays Command Name: ETHTOOL_G_TS_DELAYS_RECOMMENDED Description: This command retrieves the recommended TimeSync data path delays for transmit and receive paths, typically based on the interface type and link speed. This values are not stable. Get Currently Active TimeSync Data Path Delays Command Name: ETHTOOL_G_TS_DELAYS_ACTIVE Description: This command retrieves the currently active TimeSync data path delays that are being applied by the PHY. This is useful for real-time diagnostics and monitoring. Set Currently Active TimeSync Data Path Delays Command Name: ETHTOOL_S_TS_DELAYS_ACTIVE Description: This command sets the currently active TimeSync data path delays. This would allow the system administrator or the network management system to manually adjust the TimeSync delays to either align them with the recommended values or to tweak them for specific network conditions or compliance requirements. What do you think about this? Should the delay value be bound to the link mode or only to the speed? What if we have multiple components with delays? SoC/MAC specific delays, interface converters RGMII to xMII, etc? Should the ethtool interface provide summ of all delays in the chain, or we need to have access to each separately? Regards, Oleksij
> > What affect does this have on systems which have already applied > > adjustments in user space to correct for this? Will this cause > > regressions for such systems? > > Yes. > > > I know Richard has rejected changes like this in the past. > > In this case I would need to extend the ethtool interface. The driver > should provide recommended values and the user space can optionally > read them and optionally write them to the HW. I suggest you go read older messages from Richard. It was a discussion with Microchip about one of their PHYs. Andrew
On Wed, Apr 17, 2024 at 10:23:07PM +0200, Andrew Lunn wrote: > I suggest you go read older messages from Richard. It was a discussion > with Microchip about one of their PHYs. My 2 cents: User space has all of the hooks needed to configure corrections for a given setup. Hard coding corrections in device drivers is bound to fail, based on prior experience with Vendors not knowing or caring how their products actually work. Vendors will publish value X one year, then delete the info (to avoid embarrassment), then publish the new value Y, once customers have forgotten about X. So, prudent users will always calibrate their beloved systems, not trusting the Vendors to provide anything close to reasonable. Ergo, adding new magical correction in a kernel release causes regressions for prudent users. But, in the end, that doesn't matter, because prudent users are used to being abused by well-meaning yet misguided device driver authors. Prudent users are wise, and they will re-calibrate their systems before rolling out an updated kernel. After all, device driver authors leave them no other choice. Thanks, Richard
Hi Richard, On Thu, Apr 18, 2024 at 11:46:48PM -0700, Richard Cochran wrote: > On Wed, Apr 17, 2024 at 10:23:07PM +0200, Andrew Lunn wrote: > > I suggest you go read older messages from Richard. It was a discussion > > with Microchip about one of their PHYs. > > My 2 cents: > > User space has all of the hooks needed to configure corrections for a > given setup. > > Hard coding corrections in device drivers is bound to fail, based on > prior experience with Vendors not knowing or caring how their products > actually work. Vendors will publish value X one year, then delete the > info (to avoid embarrassment), then publish the new value Y, once > customers have forgotten about X. > > So, prudent users will always calibrate their beloved systems, not > trusting the Vendors to provide anything close to reasonable. > > Ergo, adding new magical correction in a kernel release causes > regressions for prudent users. > > But, in the end, that doesn't matter, because prudent users are used > to being abused by well-meaning yet misguided device driver authors. > > Prudent users are wise, and they will re-calibrate their systems > before rolling out an updated kernel. Ok, i see. Thank you for your feedback. Are the recommended FOSS projects managing calibration values per- linkmode/port/device in user space? What is recommended way for calibration? Using some recommended device? Regards, Oleksij
On Tue, Apr 23, 2024 at 09:07:59AM +0200, Oleksij Rempel wrote: > Are the recommended FOSS projects managing calibration values per- > linkmode/port/device in user space? No, I haven't seen this. I think the vendors should provide the numbers, like in the data sheet. This is more useful and flexible than letting vendors hard code the numbers into the source code of device drivers. > What is recommended way for calibration? Using some recommended device? You can try the "Calibration procedures" in IEEE 1588-2019, Annex N. Or if you have end to end PPS outputs (from the GM server and the client) then you can simply compare them with an oscilloscope and correct any static offset with the ptp4l "delayAsymmetry" configuration option. Or if you have auxiliary event inputs on both server and client, feed a pulse generator into both, compare time stamps, etc. Also linuxptp supports Meinberg's NetSync Monitor method. Also there are commercial vendors that rent/sell test equipment for PTP networks. So there are many possibilities. HTH, Richard
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index ddb50a0e2bc82..5831706e81623 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -3405,6 +3405,20 @@ static int lan8814_probe(struct phy_device *phydev) #define LAN8841_BTRX_POWER_DOWN_BTRX_CH_C BIT(5) #define LAN8841_BTRX_POWER_DOWN_BTRX_CH_D BIT(7) #define LAN8841_ADC_CHANNEL_MASK 198 +#define LAN8841_PTP_RX_LATENCY_10M 328 +#define LAN8841_PTP_TX_LATENCY_10M 329 +#define LAN8841_PTP_RX_LATENCY_100M 330 +#define LAN8841_PTP_TX_LATENCY_100M 331 +#define LAN8841_PTP_RX_LATENCY_1000M 332 +#define LAN8841_PTP_TX_LATENCY_1000M 333 + +#define LAN8841_PTP_RX_LATENCY_10M_VAL 5803 +#define LAN8841_PTP_TX_LATENCY_10M_VAL 3880 +#define LAN8841_PTP_RX_LATENCY_100M_VAL 443 +#define LAN8841_PTP_TX_LATENCY_100M_VAL 95 +#define LAN8841_PTP_RX_LATENCY_1000M_VAL 377 +#define LAN8841_PTP_TX_LATENCY_1000M_VAL 85 + #define LAN8841_PTP_RX_PARSE_L2_ADDR_EN 370 #define LAN8841_PTP_RX_PARSE_IP_ADDR_EN 371 #define LAN8841_PTP_RX_VERSION 374 @@ -3421,6 +3435,45 @@ static int lan8814_probe(struct phy_device *phydev) #define LAN8841_PTP_INSERT_TS_EN BIT(0) #define LAN8841_PTP_INSERT_TS_32BIT BIT(1) +static int lan8841_ptp_latency_init(struct phy_device *phydev) +{ + int ret; + + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, + LAN8841_PTP_RX_LATENCY_10M, + LAN8841_PTP_RX_LATENCY_10M_VAL); + if (ret) + return ret; + + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, + LAN8841_PTP_TX_LATENCY_10M, + LAN8841_PTP_TX_LATENCY_10M_VAL); + if (ret) + return ret; + + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, + LAN8841_PTP_RX_LATENCY_100M, + LAN8841_PTP_RX_LATENCY_100M_VAL); + if (ret) + return ret; + + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, + LAN8841_PTP_TX_LATENCY_100M, + LAN8841_PTP_TX_LATENCY_100M_VAL); + if (ret) + return ret; + + ret = phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, + LAN8841_PTP_RX_LATENCY_1000M, + LAN8841_PTP_RX_LATENCY_1000M_VAL); + if (ret) + return ret; + + return phy_write_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, + LAN8841_PTP_TX_LATENCY_1000M, + LAN8841_PTP_TX_LATENCY_1000M_VAL); +} + static int lan8841_config_init(struct phy_device *phydev) { int ret; @@ -3500,7 +3553,7 @@ static int lan8841_config_init(struct phy_device *phydev) LAN8841_MMD0_REGISTER_17_DROP_OPT(2) | LAN8841_MMD0_REGISTER_17_XMIT_TOG_TX_DIS); - return 0; + return lan8841_ptp_latency_init(phydev); } #define LAN8841_OUTPUT_CTRL 25
Set default PTP latency values to provide realistic path delay measurements and reflecting internal PHY latency asymetry. This values are based on ptp4l measurements for the path delay against identical PHY as link partner and latency asymmetry extracted from documented SOF Latency values of this PHY. Documented SOF Latency values are: TX 138ns/RX 430ns @ 1000Mbps TX 140ns/RX 615ns @ 100Mbps (fixed latency mode) TX 140ns/RX 488-524ns @ 100Mbps (variable latency mode) TX 654ns/227-2577ns @ 10Mbps Calculated asymmetry: 292ns @ 1000Mbps 238ns @ 100Mbps 1923ns @ 10Mbps Except of ptp4l based tests RGMII-PHY-PHY-RGMII path delay was measured to verify if values are in sane range. Following LAN8841 + LAN8841 RGMII delays are measured: 583ns @ 1000Mbps 1080ns @ 100Mbps 15200ns @ 10Mbps Without configuring compensation registers ptp4l reported following path delay results: ~467ns @ 1000Mbps ~544ns @ 100Mbps ~9688ns @ 10Mbps Magnetic + Cable + Magnetic delay in this setup is about 5ns. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/micrel.c | 55 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-)