| Message ID | 20220401094805.3343464-1-horatiu.vultur@microchip.com |
|---|---|
| Headers | show |
| Series | net: phy: micrel: Remove latencies support lan8814 | expand |
On Fri, Apr 01, 2022 at 11:48:04AM +0200, Horatiu Vultur wrote: > Based on the discussions here[1], the PHY driver is the wrong place > to set the latencies, therefore remove them. > > [1] https://lkml.org/lkml/2022/3/4/325 > > Fixes: ece19502834d84 ("net: phy: micrel: 1588 support for LAN8814 phy") > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> Thanks for the revert. Reviewed-by: Andrew Lunn <andrew@lunn.ch> > -static struct kszphy_latencies lan8814_latencies = { > - .rx_10 = 0x22AA, > - .tx_10 = 0x2E4A, > - .rx_100 = 0x092A, > - .tx_100 = 0x02C1, > - .rx_1000 = 0x01AD, > - .tx_1000 = 0x00C9, > -}; What are the reset defaults of these? I'm just wondering if we should explicitly set them to 0, so we don't get into a mess where some vendor bootloader sets values but mainline bootloader does not, breaking a configuration where the userspace daemon does the correct? Andrew
On Fri, Apr 01, 2022 at 11:48:05AM +0200, Horatiu Vultur wrote: > When the PHY and the MAC are capable of doing timestamping, the PHY has > priority. Therefore the DT option lan8814,ignore-ts was added such that > the PHY will not expose a PHC so then the timestamping was done in the > MAC. This is not the correct approach of doing it, therefore remove > this. > > Fixes: ece19502834d84 ("net: phy: micrel: 1588 support for LAN8814 phy") > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
The 04/01/2022 14:47, Andrew Lunn wrote: > > On Fri, Apr 01, 2022 at 11:48:04AM +0200, Horatiu Vultur wrote: > > Based on the discussions here[1], the PHY driver is the wrong place > > to set the latencies, therefore remove them. > > > > [1] https://lkml.org/lkml/2022/3/4/325 > > > > Fixes: ece19502834d84 ("net: phy: micrel: 1588 support for LAN8814 phy") > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > Thanks for the revert. > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > -static struct kszphy_latencies lan8814_latencies = { > > - .rx_10 = 0x22AA, > > - .tx_10 = 0x2E4A, > > - .rx_100 = 0x092A, > > - .tx_100 = 0x02C1, > > - .rx_1000 = 0x01AD, > > - .tx_1000 = 0x00C9, > > -}; > > What are the reset defaults of these? Those are actually the reset values. > I'm just wondering if we should > explicitly set them to 0, so we don't get into a mess where some > vendor bootloader sets values but mainline bootloader does not, > breaking a configuration where the userspace daemon does the correct? It would be fine for me to set them to 0. But then definitely we need a way to set these latencies from userspace. > > Andrew
On 01.04.2022 15:34, Horatiu Vultur wrote: >The 04/01/2022 14:47, Andrew Lunn wrote: >> >> On Fri, Apr 01, 2022 at 11:48:04AM +0200, Horatiu Vultur wrote: >> > Based on the discussions here[1], the PHY driver is the wrong place >> > to set the latencies, therefore remove them. >> > >> > [1] https://lkml.org/lkml/2022/3/4/325 >> > >> > Fixes: ece19502834d84 ("net: phy: micrel: 1588 support for LAN8814 phy") >> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> >> >> Thanks for the revert. >> >> Reviewed-by: Andrew Lunn <andrew@lunn.ch> >> >> > -static struct kszphy_latencies lan8814_latencies = { >> > - .rx_10 = 0x22AA, >> > - .tx_10 = 0x2E4A, >> > - .rx_100 = 0x092A, >> > - .tx_100 = 0x02C1, >> > - .rx_1000 = 0x01AD, >> > - .tx_1000 = 0x00C9, >> > -}; >> >> What are the reset defaults of these? > >Those are actually the reset values. > >> I'm just wondering if we should >> explicitly set them to 0, so we don't get into a mess where some >> vendor bootloader sets values but mainline bootloader does not, >> breaking a configuration where the userspace daemon does the correct? > >It would be fine for me to set them to 0. But then definitely we need a >way to set these latencies from userspace. I would like to keep the default values. With default values, you can get PTP working (accuracy is not great - but it is much better than if set to zero). There is no risk of bootloaders to pre-load other values, as the kernel will reset the PHY, and after reset we will be back to these numbers. /Allan
On Fri, Apr 01, 2022 at 03:34:54PM +0200, Horatiu Vultur wrote: > The 04/01/2022 14:47, Andrew Lunn wrote: > > > > On Fri, Apr 01, 2022 at 11:48:04AM +0200, Horatiu Vultur wrote: > > > Based on the discussions here[1], the PHY driver is the wrong place > > > to set the latencies, therefore remove them. > > > > > > [1] https://lkml.org/lkml/2022/3/4/325 > > > > > > Fixes: ece19502834d84 ("net: phy: micrel: 1588 support for LAN8814 phy") > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > > > Thanks for the revert. > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > > -static struct kszphy_latencies lan8814_latencies = { > > > - .rx_10 = 0x22AA, > > > - .tx_10 = 0x2E4A, > > > - .rx_100 = 0x092A, > > > - .tx_100 = 0x02C1, > > > - .rx_1000 = 0x01AD, > > > - .tx_1000 = 0x00C9, > > > -}; > > > > What are the reset defaults of these? > > Those are actually the reset values. Does the driver do a reset, so we can assume the hardware is actually using these? Andrew
> I would like to keep the default values. With default values, you can > get PTP working (accuracy is not great - but it is much better than > if set to zero). > > There is no risk of bootloaders to pre-load other values, as the kernel > will reset the PHY, and after reset we will be back to these numbers. O.K, that is what i wanted to know. It should be reasonable safe to assume these values, and userspace daemons can apply whatever correction they want, assuming this is what the hardware is doing. Andrew
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 --- 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] [1] https://lkml.org/lkml/2022/4/1/231 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(-)