* [PATCH RFC net 0/2] lantiq: GSWIP: two more fixes @ 2021-04-06 20:35 Martin Blumenstingl 2021-04-06 20:35 ` [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling Martin Blumenstingl 2021-04-06 20:35 ` [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits Martin Blumenstingl 0 siblings, 2 replies; 13+ messages in thread From: Martin Blumenstingl @ 2021-04-06 20:35 UTC (permalink / raw) To: hauke, andrew, f.fainelli, vivien.didelot, olteanv, netdev Cc: davem, kuba, linux, linux-kernel, Martin Blumenstingl 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(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling 2021-04-06 20:35 [PATCH RFC net 0/2] lantiq: GSWIP: two more fixes Martin Blumenstingl @ 2021-04-06 20:35 ` Martin Blumenstingl 2021-04-07 0:25 ` Andrew Lunn 2021-04-06 20:35 ` [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits Martin Blumenstingl 1 sibling, 1 reply; 13+ messages in thread From: Martin Blumenstingl @ 2021-04-06 20:35 UTC (permalink / raw) To: hauke, andrew, f.fainelli, vivien.didelot, olteanv, netdev Cc: davem, kuba, linux, linux-kernel, Martin Blumenstingl, stable 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> --- drivers/net/dsa/lantiq_gswip.c | 191 ++++++++++++++++++++++++++++----- 1 file changed, 165 insertions(+), 26 deletions(-) diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c index 809dfa3be6bb..47ea3a8c90a4 100644 --- a/drivers/net/dsa/lantiq_gswip.c +++ b/drivers/net/dsa/lantiq_gswip.c @@ -190,6 +190,23 @@ #define GSWIP_PCE_DEFPVID(p) (0x486 + ((p) * 0xA)) #define GSWIP_MAC_FLEN 0x8C5 +#define GSWIP_MAC_CTRL_0p(p) (0x903 + ((p) * 0xC)) +#define GSWIP_MAC_CTRL_0_PADEN BIT(8) +#define GSWIP_MAC_CTRL_0_FCS_EN BIT(7) +#define GSWIP_MAC_CTRL_0_FCON_MASK 0x0070 +#define GSWIP_MAC_CTRL_0_FCON_AUTO 0x0000 +#define GSWIP_MAC_CTRL_0_FCON_RX 0x0010 +#define GSWIP_MAC_CTRL_0_FCON_TX 0x0020 +#define GSWIP_MAC_CTRL_0_FCON_RXTX 0x0030 +#define GSWIP_MAC_CTRL_0_FCON_NONE 0x0040 +#define GSWIP_MAC_CTRL_0_FDUP_MASK 0x000C +#define GSWIP_MAC_CTRL_0_FDUP_AUTO 0x0000 +#define GSWIP_MAC_CTRL_0_FDUP_EN 0x0004 +#define GSWIP_MAC_CTRL_0_FDUP_DIS 0x000C +#define GSWIP_MAC_CTRL_0_GMII_MASK 0x0003 +#define GSWIP_MAC_CTRL_0_GMII_AUTO 0x0000 +#define GSWIP_MAC_CTRL_0_GMII_MII 0x0001 +#define GSWIP_MAC_CTRL_0_GMII_RGMII 0x0002 #define GSWIP_MAC_CTRL_2p(p) (0x905 + ((p) * 0xC)) #define GSWIP_MAC_CTRL_2_MLEN BIT(3) /* Maximum Untagged Frame Lnegth */ @@ -653,16 +670,13 @@ static int gswip_port_enable(struct dsa_switch *ds, int port, GSWIP_SDMA_PCTRLp(port)); if (!dsa_is_cpu_port(ds, port)) { - u32 macconf = GSWIP_MDIO_PHY_LINK_AUTO | - GSWIP_MDIO_PHY_SPEED_AUTO | - GSWIP_MDIO_PHY_FDUP_AUTO | - GSWIP_MDIO_PHY_FCONTX_AUTO | - GSWIP_MDIO_PHY_FCONRX_AUTO | - (phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK); - - gswip_mdio_w(priv, macconf, GSWIP_MDIO_PHYp(port)); - /* Activate MDIO auto polling */ - gswip_mdio_mask(priv, 0, BIT(port), GSWIP_MDIO_MDC_CFG0); + u32 mdio_phy = 0; + + if (phydev) + mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK; + + gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy, + GSWIP_MDIO_PHYp(port)); } return 0; @@ -675,14 +689,6 @@ static void gswip_port_disable(struct dsa_switch *ds, int port) if (!dsa_is_user_port(ds, port)) return; - if (!dsa_is_cpu_port(ds, port)) { - gswip_mdio_mask(priv, GSWIP_MDIO_PHY_LINK_DOWN, - GSWIP_MDIO_PHY_LINK_MASK, - GSWIP_MDIO_PHYp(port)); - /* Deactivate MDIO auto polling */ - gswip_mdio_mask(priv, BIT(port), 0, GSWIP_MDIO_MDC_CFG0); - } - gswip_switch_mask(priv, GSWIP_FDMA_PCTRL_EN, 0, GSWIP_FDMA_PCTRLp(port)); gswip_switch_mask(priv, GSWIP_SDMA_PCTRL_EN, 0, @@ -794,20 +800,31 @@ static int gswip_setup(struct dsa_switch *ds) gswip_switch_w(priv, BIT(cpu_port), GSWIP_PCE_PMAP2); gswip_switch_w(priv, BIT(cpu_port), GSWIP_PCE_PMAP3); - /* disable PHY auto polling */ + /* Deactivate MDIO PHY auto polling. Some PHYs as the AR8030 have an + * interoperability problem with this auto polling mechanism because + * their status registers think that the link is in a different state + * than it actually is. For the AR8030 it has the BMSR_ESTATEN bit set + * as well as ESTATUS_1000_TFULL and ESTATUS_1000_XFULL. This makes the + * auto polling state machine consider the link being negotiated with + * 1Gbit/s. Since the PHY itself is a Fast Ethernet RMII PHY this leads + * to the switch port being completely dead (RX and TX are both not + * working). + * Also with various other PHY / port combinations (PHY11G GPHY, PHY22F + * GPHY, external RGMII PEF7071/7072) any traffic would stop. Sometimes + * it would work fine for a few minutes to hours and then stop, on + * other device it would no traffic could be sent or received at all. + * Testing shows that when PHY auto polling is disabled these problems + * go away. + */ gswip_mdio_w(priv, 0x0, GSWIP_MDIO_MDC_CFG0); + /* Configure the MDIO Clock 2.5 MHz */ gswip_mdio_mask(priv, 0xff, 0x09, GSWIP_MDIO_MDC_CFG1); - for (i = 0; i < priv->hw_info->max_ports; i++) { - /* Disable the xMII link */ + /* Disable the xMII link */ + for (i = 0; i < priv->hw_info->max_ports; i++) gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, i); - /* Automatically select the xMII interface clock */ - gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_RATE_MASK, - GSWIP_MII_CFG_RATE_AUTO, i); - } - /* enable special tag insertion on cpu port */ gswip_switch_mask(priv, 0, GSWIP_FDMA_PCTRL_STEN, GSWIP_FDMA_PCTRLp(cpu_port)); @@ -1455,6 +1472,112 @@ static void gswip_phylink_validate(struct dsa_switch *ds, int port, return; } +static void gswip_port_set_link(struct gswip_priv *priv, int port, bool link) +{ + u32 mdio_phy; + + if (link) + mdio_phy = GSWIP_MDIO_PHY_LINK_UP; + else + mdio_phy = GSWIP_MDIO_PHY_LINK_DOWN; + + gswip_mdio_mask(priv, GSWIP_MDIO_PHY_LINK_MASK, mdio_phy, + GSWIP_MDIO_PHYp(port)); +} + +static void gswip_port_set_speed(struct gswip_priv *priv, int port, int speed, + phy_interface_t interface) +{ + u32 mdio_phy = 0, mii_cfg = 0, mac_ctrl_0 = 0; + + switch (speed) { + case SPEED_10: + mdio_phy = GSWIP_MDIO_PHY_SPEED_M10; + + if (interface == PHY_INTERFACE_MODE_RMII) + mii_cfg = GSWIP_MII_CFG_RATE_M50; + else + mii_cfg = GSWIP_MII_CFG_RATE_M2P5; + + mac_ctrl_0 = GSWIP_MAC_CTRL_0_GMII_MII; + break; + + case SPEED_100: + mdio_phy = GSWIP_MDIO_PHY_SPEED_M100; + + if (interface == PHY_INTERFACE_MODE_RMII) + mii_cfg = GSWIP_MII_CFG_RATE_M50; + else + mii_cfg = GSWIP_MII_CFG_RATE_M25; + + mac_ctrl_0 = GSWIP_MAC_CTRL_0_GMII_MII; + break; + + case SPEED_1000: + mdio_phy = GSWIP_MDIO_PHY_SPEED_G1; + + mii_cfg = GSWIP_MII_CFG_RATE_M125; + + mac_ctrl_0 = GSWIP_MAC_CTRL_0_GMII_RGMII; + break; + } + + gswip_mdio_mask(priv, GSWIP_MDIO_PHY_SPEED_MASK, mdio_phy, + GSWIP_MDIO_PHYp(port)); + gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_RATE_MASK, mii_cfg, port); + gswip_switch_mask(priv, GSWIP_MAC_CTRL_0_GMII_MASK, mac_ctrl_0, + GSWIP_MAC_CTRL_0p(port)); +} + +static void gswip_port_set_duplex(struct gswip_priv *priv, int port, int duplex) +{ + u32 mac_ctrl_0, mdio_phy; + + if (duplex == DUPLEX_FULL) { + mac_ctrl_0 = GSWIP_MAC_CTRL_0_FDUP_EN; + mdio_phy = GSWIP_MDIO_PHY_FDUP_EN; + } else { + mac_ctrl_0 = GSWIP_MAC_CTRL_0_FDUP_DIS; + mdio_phy = GSWIP_MDIO_PHY_FDUP_DIS; + } + + gswip_switch_mask(priv, GSWIP_MAC_CTRL_0_FDUP_MASK, mac_ctrl_0, + GSWIP_MAC_CTRL_0p(port)); + gswip_mdio_mask(priv, GSWIP_MDIO_PHY_FDUP_MASK, mdio_phy, + GSWIP_MDIO_PHYp(port)); +} + +static void gswip_port_set_pause(struct gswip_priv *priv, int port, + bool tx_pause, bool rx_pause) +{ + u32 mac_ctrl_0, mdio_phy; + + if (tx_pause && rx_pause) { + mac_ctrl_0 = GSWIP_MAC_CTRL_0_FCON_RXTX; + mdio_phy = GSWIP_MDIO_PHY_FCONTX_EN | + GSWIP_MDIO_PHY_FCONRX_EN; + } else if (tx_pause) { + mac_ctrl_0 = GSWIP_MAC_CTRL_0_FCON_TX; + mdio_phy = GSWIP_MDIO_PHY_FCONTX_EN | + GSWIP_MDIO_PHY_FCONRX_DIS; + } else if (rx_pause) { + mac_ctrl_0 = GSWIP_MAC_CTRL_0_FCON_RX; + mdio_phy = GSWIP_MDIO_PHY_FCONTX_DIS | + GSWIP_MDIO_PHY_FCONRX_EN; + } else { + mac_ctrl_0 = GSWIP_MAC_CTRL_0_FCON_NONE; + mdio_phy = GSWIP_MDIO_PHY_FCONTX_DIS | + GSWIP_MDIO_PHY_FCONRX_DIS; + } + + gswip_switch_mask(priv, GSWIP_MAC_CTRL_0_FCON_MASK, + mac_ctrl_0, GSWIP_MAC_CTRL_0p(port)); + gswip_mdio_mask(priv, + GSWIP_MDIO_PHY_FCONTX_MASK | + GSWIP_MDIO_PHY_FCONRX_MASK, + mdio_phy, GSWIP_MDIO_PHYp(port)); +} + static void gswip_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, const struct phylink_link_state *state) @@ -1488,6 +1611,12 @@ static void gswip_phylink_mac_config(struct dsa_switch *ds, int port, } gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_MODE_MASK, miicfg, port); + gswip_port_set_link(priv, port, state->link); + gswip_port_set_speed(priv, port, state->speed, state->interface); + gswip_port_set_duplex(priv, port, state->duplex); + gswip_port_set_pause(priv, port, !!(state->pause & MLO_PAUSE_TX), + !!(state->pause & MLO_PAUSE_RX)); + switch (state->interface) { case PHY_INTERFACE_MODE_RGMII_ID: gswip_mii_mask_pcdu(priv, GSWIP_MII_PCDU_TXDLY_MASK | @@ -1511,6 +1640,9 @@ static void gswip_phylink_mac_link_down(struct dsa_switch *ds, int port, struct gswip_priv *priv = ds->priv; gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, port); + + if (!dsa_is_cpu_port(ds, port)) + gswip_port_set_link(priv, port, false); } static void gswip_phylink_mac_link_up(struct dsa_switch *ds, int port, @@ -1522,6 +1654,13 @@ static void gswip_phylink_mac_link_up(struct dsa_switch *ds, int port, { struct gswip_priv *priv = ds->priv; + if (!dsa_is_cpu_port(ds, port)) { + gswip_port_set_link(priv, port, true); + gswip_port_set_speed(priv, port, speed, interface); + gswip_port_set_duplex(priv, port, duplex); + gswip_port_set_pause(priv, port, tx_pause, rx_pause); + } + gswip_mii_mask_cfg(priv, 0, GSWIP_MII_CFG_EN, port); } -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling 2021-04-06 20:35 ` [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling Martin Blumenstingl @ 2021-04-07 0:25 ` Andrew Lunn 2021-04-07 18:56 ` Martin Blumenstingl 0 siblings, 1 reply; 13+ messages in thread From: Andrew Lunn @ 2021-04-07 0:25 UTC (permalink / raw) To: Martin Blumenstingl Cc: hauke, f.fainelli, vivien.didelot, olteanv, netdev, davem, kuba, linux, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling 2021-04-07 0:25 ` Andrew Lunn @ 2021-04-07 18:56 ` Martin Blumenstingl 2021-04-07 19:44 ` Andrew Lunn 0 siblings, 1 reply; 13+ messages in thread From: Martin Blumenstingl @ 2021-04-07 18:56 UTC (permalink / raw) To: Andrew Lunn Cc: Hauke Mehrtens, f.fainelli, vivien.didelot, olteanv, netdev, davem, kuba, linux, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling 2021-04-07 18:56 ` Martin Blumenstingl @ 2021-04-07 19:44 ` Andrew Lunn 2021-04-07 20:28 ` Martin Blumenstingl 0 siblings, 1 reply; 13+ messages in thread From: Andrew Lunn @ 2021-04-07 19:44 UTC (permalink / raw) To: Martin Blumenstingl Cc: Hauke Mehrtens, f.fainelli, vivien.didelot, olteanv, netdev, davem, kuba, linux, linux-kernel, stable > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling 2021-04-07 19:44 ` Andrew Lunn @ 2021-04-07 20:28 ` Martin Blumenstingl 2021-04-07 23:26 ` Andrew Lunn 0 siblings, 1 reply; 13+ messages in thread From: Martin Blumenstingl @ 2021-04-07 20:28 UTC (permalink / raw) To: Andrew Lunn Cc: Hauke Mehrtens, f.fainelli, vivien.didelot, olteanv, netdev, davem, kuba, linux, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling 2021-04-07 20:28 ` Martin Blumenstingl @ 2021-04-07 23:26 ` Andrew Lunn 0 siblings, 0 replies; 13+ messages in thread From: Andrew Lunn @ 2021-04-07 23:26 UTC (permalink / raw) To: Martin Blumenstingl Cc: Hauke Mehrtens, f.fainelli, vivien.didelot, olteanv, netdev, davem, kuba, linux, linux-kernel, stable > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits 2021-04-06 20:35 [PATCH RFC net 0/2] lantiq: GSWIP: two more fixes Martin Blumenstingl 2021-04-06 20:35 ` [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling Martin Blumenstingl @ 2021-04-06 20:35 ` Martin Blumenstingl 2021-04-06 21:09 ` Hauke Mehrtens 2021-04-07 0:32 ` Andrew Lunn 1 sibling, 2 replies; 13+ messages in thread From: Martin Blumenstingl @ 2021-04-06 20:35 UTC (permalink / raw) To: hauke, andrew, f.fainelli, vivien.didelot, olteanv, netdev Cc: davem, kuba, linux, linux-kernel, Martin Blumenstingl, stable There are a few more bits in the GSWIP_MII_CFG register for which we did rely on the boot-loader (or the hardware defaults) to set them up properly. For some external RMII PHYs we need to select the GSWIP_MII_CFG_RMII_CLK bit and also we should un-set it for non-RMII PHYs. The GSWIP IP also supports in-band auto-negotiation for RGMII PHYs. Set or unset the corresponding bit depending on the auto-negotiation mode. Clear the xMII isolation bit when set at initialization time if it was previously set by the bootloader. Not doing so could lead to no traffic (neither RX nor TX) on a port with this bit set. While here, also add the GSWIP_MII_CFG_RESET bit. We don't need to manage it because this bit is self-clearning when set. We still add it here to get a better overview of the GSWIP_MII_CFG register. Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200") Cc: stable@vger.kernel.org Suggested-by: Hauke Mehrtens <hauke@hauke-m.de> Acked-by: Hauke Mehrtens <hauke@hauke-m.de> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/net/dsa/lantiq_gswip.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c index 47ea3a8c90a4..f330035ed85b 100644 --- a/drivers/net/dsa/lantiq_gswip.c +++ b/drivers/net/dsa/lantiq_gswip.c @@ -93,8 +93,12 @@ /* GSWIP MII Registers */ #define GSWIP_MII_CFGp(p) (0x2 * (p)) +#define GSWIP_MII_CFG_RESET BIT(15) #define GSWIP_MII_CFG_EN BIT(14) +#define GSWIP_MII_CFG_ISOLATE BIT(13) #define GSWIP_MII_CFG_LDCLKDIS BIT(12) +#define GSWIP_MII_CFG_RGMII_IBS BIT(8) +#define GSWIP_MII_CFG_RMII_CLK BIT(7) #define GSWIP_MII_CFG_MODE_MIIP 0x0 #define GSWIP_MII_CFG_MODE_MIIM 0x1 #define GSWIP_MII_CFG_MODE_RMIIP 0x2 @@ -821,9 +825,11 @@ static int gswip_setup(struct dsa_switch *ds) /* Configure the MDIO Clock 2.5 MHz */ gswip_mdio_mask(priv, 0xff, 0x09, GSWIP_MDIO_MDC_CFG1); - /* Disable the xMII link */ + /* Disable the xMII interface and clear it's isolation bit */ for (i = 0; i < priv->hw_info->max_ports; i++) - gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, i); + gswip_mii_mask_cfg(priv, + GSWIP_MII_CFG_EN | GSWIP_MII_CFG_ISOLATE, + 0, i); /* enable special tag insertion on cpu port */ gswip_switch_mask(priv, 0, GSWIP_FDMA_PCTRL_STEN, @@ -1597,19 +1603,29 @@ static void gswip_phylink_mac_config(struct dsa_switch *ds, int port, break; case PHY_INTERFACE_MODE_RMII: miicfg |= GSWIP_MII_CFG_MODE_RMIIM; + + /* Configure the RMII clock as output: */ + miicfg |= GSWIP_MII_CFG_RMII_CLK; break; case PHY_INTERFACE_MODE_RGMII: case PHY_INTERFACE_MODE_RGMII_ID: case PHY_INTERFACE_MODE_RGMII_RXID: case PHY_INTERFACE_MODE_RGMII_TXID: miicfg |= GSWIP_MII_CFG_MODE_RGMII; + + if (phylink_autoneg_inband(mode)) + miicfg |= GSWIP_MII_CFG_RGMII_IBS; break; default: dev_err(ds->dev, "Unsupported interface: %d\n", state->interface); return; } - gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_MODE_MASK, miicfg, port); + + gswip_mii_mask_cfg(priv, + GSWIP_MII_CFG_MODE_MASK | GSWIP_MII_CFG_RMII_CLK | + GSWIP_MII_CFG_RGMII_IBS | GSWIP_MII_CFG_LDCLKDIS, + miicfg, port); gswip_port_set_link(priv, port, state->link); gswip_port_set_speed(priv, port, state->speed, state->interface); -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits 2021-04-06 20:35 ` [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits Martin Blumenstingl @ 2021-04-06 21:09 ` Hauke Mehrtens 2021-04-07 0:32 ` Andrew Lunn 1 sibling, 0 replies; 13+ messages in thread From: Hauke Mehrtens @ 2021-04-06 21:09 UTC (permalink / raw) To: Martin Blumenstingl, andrew, f.fainelli, vivien.didelot, olteanv, netdev Cc: davem, kuba, linux, linux-kernel, stable On 4/6/21 10:35 PM, Martin Blumenstingl wrote: > There are a few more bits in the GSWIP_MII_CFG register for which we > did rely on the boot-loader (or the hardware defaults) to set them up > properly. > > For some external RMII PHYs we need to select the GSWIP_MII_CFG_RMII_CLK > bit and also we should un-set it for non-RMII PHYs. The GSWIP_MII_CFG_RMII_CLK option is ignored in other modes. > The GSWIP IP also > supports in-band auto-negotiation for RGMII PHYs. Set or unset the > corresponding bit depending on the auto-negotiation mode. > > Clear the xMII isolation bit when set at initialization time if it was > previously set by the bootloader. Not doing so could lead to no traffic > (neither RX nor TX) on a port with this bit set. > > While here, also add the GSWIP_MII_CFG_RESET bit. We don't need to > manage it because this bit is self-clearning when set. We still add it > here to get a better overview of the GSWIP_MII_CFG register. > > Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200") > Cc: stable@vger.kernel.org > Suggested-by: Hauke Mehrtens <hauke@hauke-m.de> > Acked-by: Hauke Mehrtens <hauke@hauke-m.de> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/net/dsa/lantiq_gswip.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c > index 47ea3a8c90a4..f330035ed85b 100644 > --- a/drivers/net/dsa/lantiq_gswip.c > +++ b/drivers/net/dsa/lantiq_gswip.c > @@ -93,8 +93,12 @@ > > /* GSWIP MII Registers */ > #define GSWIP_MII_CFGp(p) (0x2 * (p)) > +#define GSWIP_MII_CFG_RESET BIT(15) > #define GSWIP_MII_CFG_EN BIT(14) > +#define GSWIP_MII_CFG_ISOLATE BIT(13) > #define GSWIP_MII_CFG_LDCLKDIS BIT(12) > +#define GSWIP_MII_CFG_RGMII_IBS BIT(8) > +#define GSWIP_MII_CFG_RMII_CLK BIT(7) > #define GSWIP_MII_CFG_MODE_MIIP 0x0 > #define GSWIP_MII_CFG_MODE_MIIM 0x1 > #define GSWIP_MII_CFG_MODE_RMIIP 0x2 > @@ -821,9 +825,11 @@ static int gswip_setup(struct dsa_switch *ds) > /* Configure the MDIO Clock 2.5 MHz */ > gswip_mdio_mask(priv, 0xff, 0x09, GSWIP_MDIO_MDC_CFG1); > > - /* Disable the xMII link */ > + /* Disable the xMII interface and clear it's isolation bit */ > for (i = 0; i < priv->hw_info->max_ports; i++) > - gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, i); > + gswip_mii_mask_cfg(priv, > + GSWIP_MII_CFG_EN | GSWIP_MII_CFG_ISOLATE, > + 0, i); > > /* enable special tag insertion on cpu port */ > gswip_switch_mask(priv, 0, GSWIP_FDMA_PCTRL_STEN, > @@ -1597,19 +1603,29 @@ static void gswip_phylink_mac_config(struct dsa_switch *ds, int port, > break; > case PHY_INTERFACE_MODE_RMII: > miicfg |= GSWIP_MII_CFG_MODE_RMIIM; > + > + /* Configure the RMII clock as output: */ > + miicfg |= GSWIP_MII_CFG_RMII_CLK; > break; > case PHY_INTERFACE_MODE_RGMII: > case PHY_INTERFACE_MODE_RGMII_ID: > case PHY_INTERFACE_MODE_RGMII_RXID: > case PHY_INTERFACE_MODE_RGMII_TXID: > miicfg |= GSWIP_MII_CFG_MODE_RGMII; > + > + if (phylink_autoneg_inband(mode)) > + miicfg |= GSWIP_MII_CFG_RGMII_IBS; > break; > default: > dev_err(ds->dev, > "Unsupported interface: %d\n", state->interface); > return; > } > - gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_MODE_MASK, miicfg, port); > + > + gswip_mii_mask_cfg(priv, > + GSWIP_MII_CFG_MODE_MASK | GSWIP_MII_CFG_RMII_CLK | > + GSWIP_MII_CFG_RGMII_IBS | GSWIP_MII_CFG_LDCLKDIS, > + miicfg, port); > > gswip_port_set_link(priv, port, state->link); > gswip_port_set_speed(priv, port, state->speed, state->interface); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits 2021-04-06 20:35 ` [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits Martin Blumenstingl 2021-04-06 21:09 ` Hauke Mehrtens @ 2021-04-07 0:32 ` Andrew Lunn 2021-04-07 16:47 ` Florian Fainelli 2021-04-07 20:04 ` Hauke Mehrtens 1 sibling, 2 replies; 13+ messages in thread From: Andrew Lunn @ 2021-04-07 0:32 UTC (permalink / raw) To: Martin Blumenstingl Cc: hauke, f.fainelli, vivien.didelot, olteanv, netdev, davem, kuba, linux, linux-kernel, stable > case PHY_INTERFACE_MODE_RGMII: > case PHY_INTERFACE_MODE_RGMII_ID: > case PHY_INTERFACE_MODE_RGMII_RXID: > case PHY_INTERFACE_MODE_RGMII_TXID: > miicfg |= GSWIP_MII_CFG_MODE_RGMII; > + > + if (phylink_autoneg_inband(mode)) > + miicfg |= GSWIP_MII_CFG_RGMII_IBS; Is there any other MAC driver doing this? Are there any boards actually enabling it? Since it is so odd, if there is nothing using it, i would be tempted to leave this out. Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits 2021-04-07 0:32 ` Andrew Lunn @ 2021-04-07 16:47 ` Florian Fainelli 2021-04-07 18:49 ` Martin Blumenstingl 2021-04-07 20:04 ` Hauke Mehrtens 1 sibling, 1 reply; 13+ messages in thread From: Florian Fainelli @ 2021-04-07 16:47 UTC (permalink / raw) To: Andrew Lunn, Martin Blumenstingl Cc: hauke, vivien.didelot, olteanv, netdev, davem, kuba, linux, linux-kernel, stable On 4/6/2021 5:32 PM, Andrew Lunn wrote: >> case PHY_INTERFACE_MODE_RGMII: >> case PHY_INTERFACE_MODE_RGMII_ID: >> case PHY_INTERFACE_MODE_RGMII_RXID: >> case PHY_INTERFACE_MODE_RGMII_TXID: >> miicfg |= GSWIP_MII_CFG_MODE_RGMII; >> + >> + if (phylink_autoneg_inband(mode)) >> + miicfg |= GSWIP_MII_CFG_RGMII_IBS; > > Is there any other MAC driver doing this? Are there any boards > actually enabling it? Since it is so odd, if there is nothing using > it, i would be tempted to leave this out. Some PHYs (Broadcom namely) support suppressing the RGMII in-band signaling towards the MAC, so if the MAC relies on that signaling to configure itself based on what the PHY reports this may not work. -- Florian ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits 2021-04-07 16:47 ` Florian Fainelli @ 2021-04-07 18:49 ` Martin Blumenstingl 0 siblings, 0 replies; 13+ messages in thread From: Martin Blumenstingl @ 2021-04-07 18:49 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn Cc: Hauke Mehrtens, vivien.didelot, olteanv, netdev, davem, kuba, linux, linux-kernel, stable Hello, On Wed, Apr 7, 2021 at 6:47 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 4/6/2021 5:32 PM, Andrew Lunn wrote: > >> case PHY_INTERFACE_MODE_RGMII: > >> case PHY_INTERFACE_MODE_RGMII_ID: > >> case PHY_INTERFACE_MODE_RGMII_RXID: > >> case PHY_INTERFACE_MODE_RGMII_TXID: > >> miicfg |= GSWIP_MII_CFG_MODE_RGMII; > >> + > >> + if (phylink_autoneg_inband(mode)) > >> + miicfg |= GSWIP_MII_CFG_RGMII_IBS; > > > > Is there any other MAC driver doing this? Are there any boards > > actually enabling it? Since it is so odd, if there is nothing using > > it, i would be tempted to leave this out. > > Some PHYs (Broadcom namely) support suppressing the RGMII in-band > signaling towards the MAC, so if the MAC relies on that signaling to > configure itself based on what the PHY reports this may not work. point taken. in v2 we'll not set GSWIP_MII_CFG_RGMII_IBS unless there's someone who can actually test this. so far I don't know any hardware with Lantiq SoC that uses it Best regards, Martin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits 2021-04-07 0:32 ` Andrew Lunn 2021-04-07 16:47 ` Florian Fainelli @ 2021-04-07 20:04 ` Hauke Mehrtens 1 sibling, 0 replies; 13+ messages in thread From: Hauke Mehrtens @ 2021-04-07 20:04 UTC (permalink / raw) To: Andrew Lunn, Martin Blumenstingl Cc: f.fainelli, vivien.didelot, olteanv, netdev, davem, kuba, linux, linux-kernel, stable On 4/7/21 2:32 AM, Andrew Lunn wrote: >> case PHY_INTERFACE_MODE_RGMII: >> case PHY_INTERFACE_MODE_RGMII_ID: >> case PHY_INTERFACE_MODE_RGMII_RXID: >> case PHY_INTERFACE_MODE_RGMII_TXID: >> miicfg |= GSWIP_MII_CFG_MODE_RGMII; >> + >> + if (phylink_autoneg_inband(mode)) >> + miicfg |= GSWIP_MII_CFG_RGMII_IBS; > > Is there any other MAC driver doing this? Are there any boards > actually enabling it? Since it is so odd, if there is nothing using > it, i would be tempted to leave this out. We saw this option in the switch documentation and activated it to prepare for such systems, but I do not have any board which uses this and I am also not aware that this is used anywhere. Hauke ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-04-07 23:26 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-06 20:35 [PATCH RFC net 0/2] lantiq: GSWIP: two more fixes Martin Blumenstingl 2021-04-06 20:35 ` [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling Martin Blumenstingl 2021-04-07 0:25 ` Andrew Lunn 2021-04-07 18:56 ` Martin Blumenstingl 2021-04-07 19:44 ` Andrew Lunn 2021-04-07 20:28 ` Martin Blumenstingl 2021-04-07 23:26 ` Andrew Lunn 2021-04-06 20:35 ` [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits Martin Blumenstingl 2021-04-06 21:09 ` Hauke Mehrtens 2021-04-07 0:32 ` Andrew Lunn 2021-04-07 16:47 ` Florian Fainelli 2021-04-07 18:49 ` Martin Blumenstingl 2021-04-07 20:04 ` Hauke Mehrtens
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).