| Message ID | 20220401110522.3418258-1-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: phy: micrel: Remove latencies support lan8814 | expand |
On Fri, Apr 01, 2022 at 01:05:19PM +0200, Horatiu Vultur wrote: > Remove the latencies support both from the PHY driver and from the DT. > The IP already has some default latencies values which can be used to get > decent results. It has the following values(defined in ns): > rx-1000mbit: 429 > tx-1000mbit: 201 > rx-100mbit: 2346 > tx-100mbit: 705 So one alternative option here is that ptp4l looks at /sys/class/net/<ifname>/phydev/phy_id to identify the PHY, listens to netlink messages to determine the link speed and then applies the correction itself in user space. That gives you a pretty generic solution, works for any existing PHY and pretty much any existing kernel version. And if you want board specific values you can override them in the ptp4l configuration file. Andrew
On 01.04.2022 14:57, Andrew Lunn wrote: >On Fri, Apr 01, 2022 at 01:05:19PM +0200, Horatiu Vultur wrote: >> Remove the latencies support both from the PHY driver and from the DT. >> The IP already has some default latencies values which can be used to get >> decent results. It has the following values(defined in ns): >> rx-1000mbit: 429 >> tx-1000mbit: 201 >> rx-100mbit: 2346 >> tx-100mbit: 705 > >So one alternative option here is that ptp4l looks at > >/sys/class/net/<ifname>/phydev/phy_id > >to identify the PHY, listens to netlink messages to determine the link >speed and then applies the correction itself in user space. That gives >you a pretty generic solution, works for any existing PHY and pretty >much any existing kernel version. And if you want board specific >values you can override them in the ptp4l configuration file. I think it is good to have both options. If you want PTP4L to compensate in user-space, do not call the tunable, if you want to HW to compensate, call the tunable (this is useful both for users using ptp4l and other ptpimplementations). If system behaves strange, it is easy to see what delays has been applied. We are planning on creating a small proejct, which go through all PHYs in the current system. It shall check a config file to see if the user has configured interface specific numbers, then apply them, other wise see if we have default numbers based on the PHY-ID (like you describe). Idea is to run this at boot as a one-off. It will apply the adjustments on all speeds, all capable PHYs regardless of they PHY-Timestamping is used or not. BTW: If there is a desire, we can add a flag to this tool which can set all the delay-values to zero. Just for the record: It is not that I do not like PTP4L - it is by far the best PTP implementation I have seen. But I'm keen on having a solution that also works for non-ptp4l users. /Allan
On Fri, Apr 01, 2022 at 04:11:20PM +0200, Allan W. Nielsen wrote: > On 01.04.2022 14:57, Andrew Lunn wrote: > > On Fri, Apr 01, 2022 at 01:05:19PM +0200, Horatiu Vultur wrote: > > > Remove the latencies support both from the PHY driver and from the DT. > > > The IP already has some default latencies values which can be used to get > > > decent results. It has the following values(defined in ns): > > > rx-1000mbit: 429 > > > tx-1000mbit: 201 > > > rx-100mbit: 2346 > > > tx-100mbit: 705 > > > > So one alternative option here is that ptp4l looks at > > > > /sys/class/net/<ifname>/phydev/phy_id > > > > to identify the PHY, listens to netlink messages to determine the link > > speed and then applies the correction itself in user space. That gives > > you a pretty generic solution, works for any existing PHY and pretty > > much any existing kernel version. And if you want board specific > > values you can override them in the ptp4l configuration file. > > I think it is good to have both options. If you want PTP4L to compensate > in user-space, do not call the tunable, if you want to HW to compensate, > call the tunable (this is useful both for users using ptp4l and other > ptpimplementations). > > If system behaves strange, it is easy to see what delays has been > applied. I don't actually think that is true. How wound i know that > > > rx-1000mbit: 429 > > > tx-1000mbit: 201 > > > rx-100mbit: 2346 > > > tx-100mbit: 705 are the default values? I cannot just look at them and obviously see they are not the default values. I would need to learn what the default values are of every PHY in linux which allows the PHY to perform a correction. Are you also saying that ptp4l needs to read the values from the driver, calculate the differ from the defaults, and then apply that difference to the correction specified in the configuration file it will apply in userspace? Does the PTP API enforce mutual exclusion for a device? Can there be multiple applications running on an interface, some which assume the hardware is configured to perform corrections and some which will apply the correction in user space? Richard? Andrew
On Fri, Apr 01, 2022 at 02:57:46PM +0200, Andrew Lunn wrote: > On Fri, Apr 01, 2022 at 01:05:19PM +0200, Horatiu Vultur wrote: > > Remove the latencies support both from the PHY driver and from the DT. > > The IP already has some default latencies values which can be used to get > > decent results. It has the following values(defined in ns): > > rx-1000mbit: 429 > > tx-1000mbit: 201 > > rx-100mbit: 2346 > > tx-100mbit: 705 > > So one alternative option here is that ptp4l looks at > > /sys/class/net/<ifname>/phydev/phy_id That doesn't work for Clause 45 PHYs, only Clause 22 PHYs. If we want userspace to know which PHY it is, we need a proper interface that exports all the 31 Clause 45 IDs (each mmd's registers 2/3) as well, as well as the Clause 45 package ID (registers 14/15).
On Fri, Apr 01, 2022 at 04:39:10PM +0200, Andrew Lunn wrote: > Are you also saying that ptp4l needs to read the values from the > driver, calculate the differ from the defaults, and then apply that > difference to the correction specified in the configuration file it > will apply in userspace? Personally I wouldn't bother with that. At the end of day, users who care about sub-microsecond performance will need to calibrate their particular setup. The output of the calibration will be the system delay asymmetry correction. That number will be applied in ONE place, namely the user space PTP stack. Breaking it up into little bits is just extra work for no benefit. That is why I'm against any of this driver nonsense. The only purpose of putting values in to the driver is to unpleasantly surprise the end users after kernel upgrade. If this driver defaults + run time query/setting stuff goes mainline, I'll never use it. > Does the PTP API enforce mutual exclusion for a device? Can there be > multiple applications running on an interface, some which assume the > hardware is configured to perform corrections and some which will > apply the correction in user space? There is no mutual exclusion at the kernel API. The main hindrance is the SIOCSHWTSTAMP ioctl which is at the device level. The setting applies system wide and is a root caps operation. There is the "Get" variant that allows co-operation but does not enforce it. Thanks, Richard
Hello: This series was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Fri, 1 Apr 2022 13:05:19 +0200 you wrote: > Remove the latencies support both from the PHY driver and from the DT. > The IP already has some default latencies values which can be used to get > decent results. It has the following values(defined in ns): > rx-1000mbit: 429 > tx-1000mbit: 201 > rx-100mbit: 2346 > tx-100mbit: 705 > > [...] Here is the summary with links: - [net,v2,1/3] dt-bindings: net: micrel: Revert latency support and timestamping check https://git.kernel.org/netdev/net/c/b117c88df0e3 - [net,v2,2/3] net: phy: micrel: Remove latency from driver https://git.kernel.org/netdev/net/c/b814403a8cd8 - [net,v2,3/3] net: phy: micrel: Remove DT option lan8814,ignore-ts https://git.kernel.org/netdev/net/c/76e9ccd68943 You are awesome, thank you!
Remove the latencies support both from the PHY driver and from the DT. The IP already has some default latencies values which can be used to get decent results. It has the following values(defined in ns): rx-1000mbit: 429 tx-1000mbit: 201 rx-100mbit: 2346 tx-100mbit: 705 v0->v1: - fix the split of the patches, there was a compiling error between patch 2 and patch 3. --- But to get better results the following values needs to be set: rx-1000mbit: 459 tx-1000mbit: 171 rx-100mbit: 1706 tx-100mbit: 1345 We are proposing to use ethtool to set these latencies, the RFC can be found here[1] Horatiu Vultur (3): dt-bindings: net: micrel: Revert latency support and timestamping check net: phy: micrel: Remove latency from driver net: phy: micrel: Remove DT option lan8814,ignore-ts .../devicetree/bindings/net/micrel.txt | 17 --- drivers/net/phy/micrel.c | 106 +----------------- 2 files changed, 2 insertions(+), 121 deletions(-)