| Message ID | 20210406203508.476122-1-martin.blumenstingl@googlemail.com |
|---|---|
| Headers | show |
| Series | lantiq: GSWIP: two more fixes | expand |
On Tue, Apr 06, 2021 at 10:35:07PM +0200, Martin Blumenstingl wrote: > PHY auto polling on the GSWIP hardware can be used so link changes > (speed, link up/down, etc.) can be detected automatically. Internally > GSWIP reads the PHY's registers for this functionality. Based on this > automatic detection GSWIP can also automatically re-configure it's port > settings. Unfortunately this auto polling (and configuration) mechanism > seems to cause various issues observed by different people on different > devices: > - FritzBox 7360v2: the two Gbit/s ports (connected to the two internal > PHY11G instances) are working fine but the two Fast Ethernet ports > (using an AR8030 RMII PHY) are completely dead (neither RX nor TX are > received). It turns out that the AR8030 PHY sets the BMSR_ESTATEN bit > as well as the ESTATUS_1000_TFULL and ESTATUS_1000_XFULL bits. This > makes the PHY auto polling state machine (rightfully?) think that the > established link speed (when the other side is Gbit/s capable) is > 1Gbit/s. > - None of the Ethernet ports on the Zyxel P-2812HNU-F1 (two are > connected to the internal PHY11G GPHYs while the other three are > external RGMII PHYs) are working. Neither RX nor TX traffic was > observed. It is not clear which part of the PHY auto polling state- > machine caused this. > - FritzBox 7412 (only one LAN port which is connected to one of the > internal GPHYs running in PHY22F / Fast Ethernet mode) was seeing > random disconnects (link down events could be seen). Sometimes all > traffic would stop after such disconnect. It is not clear which part > of the PHY auto polling state-machine cauased this. > - TP-Link TD-W9980 (two ports are connected to the internal GPHYs > running in PHY11G / Gbit/s mode, the other two are external RGMII > PHYs) was affected by similar issues as the FritzBox 7412 just without > the "link down" events > > Switch to software based configuration instead of PHY auto polling (and > letting the GSWIP hardware configure the ports automatically) for the > following link parameters: > - link up/down > - link speed > - full/half duplex > - flow control (RX / TX pause) > > After a big round of manual testing by various people (who helped test > this on OpenWrt) it turns out that this fixes all reported issues. > > Additionally it can be considered more future proof because any > "quirk" which is implemented for a PHY on the driver side can now be > used with the GSWIP hardware as well because Linux is in control of the > link parameters. > > As a nice side-effect this also solves a problem where fixed-links were > not supported previously because we were relying on the PHY auto polling > mechanism, which cannot work for fixed-links as there's no PHY from > where it can read the registers. Configuring the link settings on the > GSWIP ports means that we now use the settings from device-tree also for > ports with fixed-links. > > Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200") > Fixes: 3e6fdeb28f4c33 ("net: dsa: lantiq_gswip: Let GSWIP automatically set the xMII clock") > Cc: stable@vger.kernel.org > Acked-by: Hauke Mehrtens <hauke@hauke-m.de> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Having the MAC polling the PHY is pretty much always a bad idea. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi Andrew, On Wed, Apr 7, 2021 at 2:25 AM Andrew Lunn <andrew@lunn.ch> wrote: [...] > Having the MAC polling the PHY is pretty much always a bad idea. > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> thanks for reviewing this! For my own curiosity: is there a "recommended" way where to configure link up/down, speed, duplex and flow control? currently I have the logic in both, .phylink_mac_config and .phylink_mac_link_up. Thank you! Martin
> For my own curiosity: is there a "recommended" way where to configure > link up/down, speed, duplex and flow control? currently I have the > logic in both, .phylink_mac_config and .phylink_mac_link_up. You probably want to read the documentation in include/linux/phylink.h Andrew
On Wed, Apr 7, 2021 at 9:44 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > For my own curiosity: is there a "recommended" way where to configure > > link up/down, speed, duplex and flow control? currently I have the > > logic in both, .phylink_mac_config and .phylink_mac_link_up. > > You probably want to read the documentation in > > include/linux/phylink.h it turns out that I should have scrolled down in that file. there's a perfect explanation in it about the various functions, just not at the top. thanks for the hint! For my own reference: [...] @state->link [...] are never guaranteed to be correct, and so any mac_config() implementation must never reference these fields. I am referencing state->link so I will fix that in v2 [...] drivers may use @state->speed, @state->duplex and @state->pause to configure the MAC, but this is deprecated; such drivers should be converted to use mac_link_up so I will also drop these also from the gswip_phylink_mac_config implementation If dropping the modifications to gswip_phylink_mac_config is my only change: do you want me to keep or drop your Reviewed-by in v2? Best regards, Martin
> If dropping the modifications to gswip_phylink_mac_config is my only change: > do you want me to keep or drop your Reviewed-by in v2? You can keep it. Andrew
Hello, after my last patch got accepted and is now in net as commit 3e6fdeb28f4c33 ("net: dsa: lantiq_gswip: Let GSWIP automatically set the xMII clock") [0] some more people from the OpenWrt community (many thanks to everyone involved) helped test the GSWIP driver: [1] It turns out that the previous fix does not work for all boards. There's no regression, but it doesn't fix as many problems as I thought. This is why two more fixes are needed: - the first one solves many (four known but probably there are a few extra hidden ones) reported bugs with the GSWIP where no traffic would flow. Not all circumstances are fully understood but testing shows that switching away from PHY auto polling solves all of them - while investigating the different problems which are addressed by the first patch some small issues with the existing code were found. These are addressed by the second patch Why am I sending this as RFC then? Because I am not sure where to place the link configuration bits in the first patch (as I don't understand why phylink_mac_config and also phylink_mac_link_up both have the required parameters to configure the link, just in differnet formats): - in gswip_phylink_mac_config - in gswip_phylink_mac_link_up - in both Any feedback is very welcome on this topic! I have already gotten Hauke's Acked-by off-list. He's Cc'ed so he can speak up if he changes his opinion or finds some issue with the patches still. Best regards, Martin [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3e6fdeb28f4c331acbd27bdb0effc4befd4ef8e8 [1] https://github.com/openwrt/openwrt/pull/3085 Martin Blumenstingl (2): net: dsa: lantiq_gswip: Don't use PHY auto polling net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits drivers/net/dsa/lantiq_gswip.c | 211 ++++++++++++++++++++++++++++----- 1 file changed, 183 insertions(+), 28 deletions(-)