| Message ID | 20220725202957.2460420-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: realtek: rtl8366rb: Configure ports properly | expand |
Hi Linus, On Mon, Jul 25, 2022 at 10:29:57PM +0200, Linus Walleij wrote: > Instead of just hammering the CPU port up at 1GBit at > .phylink_mac_link_up calls for that specific port, support > configuring any port: this works like a charm. Could you clarify what is intended to be functionally improved with this change, exactly? According to your phylink_get_caps() implementation, I see that all ports are internal, so presumably the CPU ports too (and the user ports are connected to internal PHYs). Is it just to act upon the phylink parameters rather than assuming the CPU port is at gigabit? Can you actually set the CPU port at lower rates? As for the internal PHY ports, do they need their link speed to be forced at 10/100, or did those previously work at those lower speeds, just left unforced? > > Drop the code to enable/disable the port in the > .phylink_mac_link_up/.phylink_mac_link_down callbacks: > this is handled perfectly well by the callbacks to > .port_enable/.port_disable. > > Tested on the D-Link DIR-685. > > Cc: Alvin Šipraga <alsi@bang-olufsen.dk> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/net/dsa/realtek/rtl8366rb.c | 155 ++++++++++++++++++++-------- > 1 file changed, 111 insertions(+), 44 deletions(-) > > diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c > index 25f88022b9e4..6ef8449d3e7a 100644 > --- a/drivers/net/dsa/realtek/rtl8366rb.c > +++ b/drivers/net/dsa/realtek/rtl8366rb.c > @@ -95,12 +95,6 @@ > #define RTL8366RB_PAACR_RX_PAUSE BIT(6) > #define RTL8366RB_PAACR_AN BIT(7) > > -#define RTL8366RB_PAACR_CPU_PORT (RTL8366RB_PAACR_SPEED_1000M | \ > - RTL8366RB_PAACR_FULL_DUPLEX | \ > - RTL8366RB_PAACR_LINK_UP | \ > - RTL8366RB_PAACR_TX_PAUSE | \ > - RTL8366RB_PAACR_RX_PAUSE) > - > /* bits 0..7 = port 0, bits 8..15 = port 1 */ > #define RTL8366RB_PSTAT0 0x0014 > /* bits 0..7 = port 2, bits 8..15 = port 3 */ > @@ -1049,63 +1043,134 @@ static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds, > return DSA_TAG_PROTO_RTL4_A; > } > > -static void > -rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode, > - phy_interface_t interface, struct phy_device *phydev, > - int speed, int duplex, bool tx_pause, bool rx_pause) > +static void rtl8366rb_link_get_caps(struct dsa_switch *ds, int port, > + struct phylink_config *config) > { > - struct realtek_priv *priv = ds->priv; > - int ret; > + /* The SYM and ASYM pause is RX and TX pause */ No, SYM and ASYM pause are not RX and TX pause, but rather they are advertisement bits. After autoneg completes, the 4 SYM and ASYM pause advertisement bits of you and your link partner get resolved independently by you and your link partner according to the table described in linkmode_resolve_pause(), and the result of that resolution is what RX and TX pause are. > + config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | > + MAC_10 | MAC_100 | MAC_1000; > > - if (port != priv->cpu_port) > - return; > + /* These are all internal, no external interfaces supported */ > + __set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces); > > - dev_dbg(priv->dev, "MAC link up on CPU port (%d)\n", port); > + /* GMII is the default interface mode for phylib, so > + * we have to support it for ports with integrated PHY. > + */ > + __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces); > +} > > - /* Force the fixed CPU port into 1Gbit mode, no autonegotiation */ > - ret = regmap_update_bits(priv->map, RTL8366RB_MAC_FORCE_CTRL_REG, > - BIT(port), BIT(port)); > - if (ret) { > - dev_err(priv->dev, "failed to force 1Gbit on CPU port\n"); > - return; > +static int rtl8366rb_config_link(struct realtek_priv *priv, int port, bool link, > + int speed, int duplex, bool tx_pause, bool rx_pause) > +{ > + u32 paacr; > + u32 portreg; > + u32 portmask; > + u32 portshift; > + int ret; > + > + switch (port) { > + case 0: > + portreg = RTL8366RB_PAACR0; > + portshift = 0; > + break; > + case 1: > + portreg = RTL8366RB_PAACR0; > + portshift = 8; > + break; > + case 2: > + portreg = RTL8366RB_PAACR1; > + portshift = 0; > + break; > + case 3: > + portreg = RTL8366RB_PAACR1; > + portshift = 8; > + break; > + case 4: > + portreg = RTL8366RB_PAACR2; > + portshift = 0; > + break; > + case 5: > + portreg = RTL8366RB_PAACR2; > + portshift = 8; > + break; > + default: > + dev_err(priv->dev, "illegal port %d\n", port); > + return -EINVAL; > } > > - ret = regmap_update_bits(priv->map, RTL8366RB_PAACR2, > - 0xFF00U, > - RTL8366RB_PAACR_CPU_PORT << 8); > - if (ret) { > - dev_err(priv->dev, "failed to set PAACR on CPU port\n"); > - return; > + portmask = GENMASK(portshift + 7, portshift); > + > + if (link) { > + switch (speed) { > + case SPEED_1000: > + paacr = RTL8366RB_PAACR_SPEED_1000M; > + dev_dbg(priv->dev, "set port %d to 1Gbit\n", port); > + break; > + case SPEED_100: > + paacr = RTL8366RB_PAACR_SPEED_100M; > + dev_dbg(priv->dev, "set port %d to 100Mbit\n", port); > + break; > + case SPEED_10: > + paacr = RTL8366RB_PAACR_SPEED_10M; > + dev_dbg(priv->dev, "set port %d to 10Mbit\n", port); > + break; > + default: > + dev_err(priv->dev, "illegal speed request on port %d\n", port); > + return -EINVAL; > + } > + > + if (duplex == DUPLEX_FULL) > + paacr |= RTL8366RB_PAACR_FULL_DUPLEX; > + dev_dbg(priv->dev, "set port %d to %s duplex\n", port, > + (duplex == DUPLEX_FULL) ? "full" : "half"); > + > + if (tx_pause) > + paacr |= RTL8366RB_PAACR_TX_PAUSE; > + > + if (rx_pause) > + paacr |= RTL8366RB_PAACR_RX_PAUSE; > + > + /* We are in the link up function so force it up */ > + paacr |= RTL8366RB_PAACR_LINK_UP; > + } else { > + /* If link goes down just zero the register including link up */ > + paacr = 0; > } > > - /* Enable the CPU port */ > - ret = regmap_update_bits(priv->map, RTL8366RB_PECR, BIT(port), > - 0); > + ret = regmap_update_bits(priv->map, portreg, portmask, paacr << portshift); > if (ret) { > - dev_err(priv->dev, "failed to enable the CPU port\n"); > - return; > + dev_err(priv->dev, "failed to set PAACR on port %d\n", port); > + return ret; > } > + dev_dbg(priv->dev, "Updated port %d reg %08x, mask %08x, shift %d with value %08x\n", > + port, portreg, portmask, portshift, paacr); > + > + return 0; > } > > static void > -rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, > - phy_interface_t interface) > +rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode, > + phy_interface_t interface, struct phy_device *phydev, > + int speed, int duplex, bool tx_pause, bool rx_pause) > { > struct realtek_priv *priv = ds->priv; > int ret; > > - if (port != priv->cpu_port) > - return; > + ret = rtl8366rb_config_link(priv, port, true, speed, duplex, tx_pause, rx_pause); > + if (ret) > + dev_err(priv->dev, "error configuring link on port %d\n", port); > +} > > - dev_dbg(priv->dev, "MAC link down on CPU port (%d)\n", port); > +static void > +rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, > + phy_interface_t interface) > +{ > + struct realtek_priv *priv = ds->priv; > + int ret; > > - /* Disable the CPU port */ > - ret = regmap_update_bits(priv->map, RTL8366RB_PECR, BIT(port), > - BIT(port)); > - if (ret) { > - dev_err(priv->dev, "failed to disable the CPU port\n"); > - return; > - } > + ret = rtl8366rb_config_link(priv, port, false, 0, 0, false, false); > + if (ret) > + dev_err(priv->dev, "error configuring link on port %d\n", port); > } > > static void rb8366rb_set_port_led(struct realtek_priv *priv, > @@ -1796,6 +1861,7 @@ static int rtl8366rb_detect(struct realtek_priv *priv) > static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = { > .get_tag_protocol = rtl8366_get_tag_protocol, > .setup = rtl8366rb_setup, > + .phylink_get_caps = rtl8366rb_link_get_caps, > .phylink_mac_link_up = rtl8366rb_mac_link_up, > .phylink_mac_link_down = rtl8366rb_mac_link_down, > .get_strings = rtl8366_get_strings, > @@ -1821,6 +1887,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = { > .setup = rtl8366rb_setup, > .phy_read = rtl8366rb_dsa_phy_read, > .phy_write = rtl8366rb_dsa_phy_write, > + .phylink_get_caps = rtl8366rb_link_get_caps, > .phylink_mac_link_up = rtl8366rb_mac_link_up, > .phylink_mac_link_down = rtl8366rb_mac_link_down, > .get_strings = rtl8366_get_strings, > -- > 2.36.1 >
On Tue, Jul 26, 2022 at 1:02 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Mon, Jul 25, 2022 at 10:29:57PM +0200, Linus Walleij wrote: > > Instead of just hammering the CPU port up at 1GBit at > > .phylink_mac_link_up calls for that specific port, support > > configuring any port: this works like a charm. > > Could you clarify what is intended to be functionally improved with this > change, exactly? I can try, but as usual I am probably confused :) > According to your phylink_get_caps() implementation, I see that all > ports are internal, so presumably the CPU ports too (and the user ports > are connected to internal PHYs). Correct, if by internal you mean there is no external, discrete PHY component. They all route out to the physical sockets, maybe with some small analog filter inbetween. > Is it just to act upon the phylink parameters rather than assuming the > CPU port is at gigabit? Can you actually set the CPU port at lower rates? I think you can, actually. The Realtek vendor mess does support it. Hm I should test to gear it down to 100Mbit and 10Mbit and see what happens. > As for the internal PHY ports, do they need their link speed to be > forced at 10/100, or did those previously work at those lower speeds, > just left unforced? They were left in "power-on"-state. Which I *guess* is autonegotiate. But haven't really tested. It leaves me a bit uneasy since these registers are never explicit set up to autonegotiate. Maybe I should do a separate patch to just set them explicitly in autonegotiation mode? I have a small 10MBit router, I will try to connect it and see what happens, if anything happens and can be detected. > > -static void > > -rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode, > > - phy_interface_t interface, struct phy_device *phydev, > > - int speed, int duplex, bool tx_pause, bool rx_pause) > > +static void rtl8366rb_link_get_caps(struct dsa_switch *ds, int port, > > + struct phylink_config *config) > > { > > - struct realtek_priv *priv = ds->priv; > > - int ret; > > + /* The SYM and ASYM pause is RX and TX pause */ > > No, SYM and ASYM pause are not RX and TX pause, but rather they are > advertisement bits. After autoneg completes, the 4 SYM and ASYM pause > advertisement bits of you and your link partner get resolved independently > by you and your link partner according to the table described in > linkmode_resolve_pause(), and the result of that resolution is what RX > and TX pause are. I stand corrected. I'll reword or drop this. Yours, Linus Walleij
Hi Linus, On Tue, Jul 26, 2022 at 04:20:18PM +0200, Linus Walleij wrote: > > According to your phylink_get_caps() implementation, I see that all > > ports are internal, so presumably the CPU ports too (and the user ports > > are connected to internal PHYs). > > Correct, if by internal you mean there is no external, discrete PHY > component. They all route out to the physical sockets, maybe with > some small analog filter inbetween. Yes, I mean that if there are RJ-45 ports, they are all driven by PHYs which are internal to the switch chip, and if there is no internal PHY, those ports are also internal to the chip, like in the case of the CPU port. > > Is it just to act upon the phylink parameters rather than assuming the > > CPU port is at gigabit? Can you actually set the CPU port at lower rates? > > I think you can, actually. The Realtek vendor mess does support it. > > Hm I should test to gear it down to 100Mbit and 10Mbit and see > what happens. So the change wasn't intended for the CPU port, this is good to know. > > As for the internal PHY ports, do they need their link speed to be > > forced at 10/100, or did those previously work at those lower speeds, > > just left unforced? > > They were left in "power-on"-state. Which I *guess* is > autonegotiate. But haven't really tested. > > It leaves me a bit uneasy since these registers are never explicit > set up to autonegotiate. Maybe I should do a separate patch > to just set them explicitly in autonegotiation mode? I am confused as to what you are describing when you are using the "autonegotiation" word. "Port" is too generic, every user port will have a MAC and a PHY. The PHY is what deals with autonegotiation; also, the PHY is what the DSA driver does *not* control (it uses either a dedicated or a generic driver from drivers/net/phy). Between the internal MAC and the internal PHY, what's going on isn't called autonegotiation, but doesn't have a specific name either, as far as I know. Rather, because the 2 components are part of the same die, the hardware designers may have added logic that automatically adapts the speed in the MAC according to the speed that the PHY negotiated for (or was forced at). Your change (or at least the way in which I understand it) essentially always forces the internal MAC to the same speed as the PHY. This is not wrong per se, but I'm not clear if it's necessary either, considering that there might be hardware logic to do this automatically. > I have a small 10MBit router, I will try to connect it and see what > happens, if anything happens and can be detected. You don't need a dedicated 10BASE-T device to test this, you can run "ethtool -s eth0 advertise 0x2" on any gigabit-capable device, and this will limit the advertisement in its PHY to just 10BASE-T.
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c index 25f88022b9e4..6ef8449d3e7a 100644 --- a/drivers/net/dsa/realtek/rtl8366rb.c +++ b/drivers/net/dsa/realtek/rtl8366rb.c @@ -95,12 +95,6 @@ #define RTL8366RB_PAACR_RX_PAUSE BIT(6) #define RTL8366RB_PAACR_AN BIT(7) -#define RTL8366RB_PAACR_CPU_PORT (RTL8366RB_PAACR_SPEED_1000M | \ - RTL8366RB_PAACR_FULL_DUPLEX | \ - RTL8366RB_PAACR_LINK_UP | \ - RTL8366RB_PAACR_TX_PAUSE | \ - RTL8366RB_PAACR_RX_PAUSE) - /* bits 0..7 = port 0, bits 8..15 = port 1 */ #define RTL8366RB_PSTAT0 0x0014 /* bits 0..7 = port 2, bits 8..15 = port 3 */ @@ -1049,63 +1043,134 @@ static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds, return DSA_TAG_PROTO_RTL4_A; } -static void -rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode, - phy_interface_t interface, struct phy_device *phydev, - int speed, int duplex, bool tx_pause, bool rx_pause) +static void rtl8366rb_link_get_caps(struct dsa_switch *ds, int port, + struct phylink_config *config) { - struct realtek_priv *priv = ds->priv; - int ret; + /* The SYM and ASYM pause is RX and TX pause */ + config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | + MAC_10 | MAC_100 | MAC_1000; - if (port != priv->cpu_port) - return; + /* These are all internal, no external interfaces supported */ + __set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces); - dev_dbg(priv->dev, "MAC link up on CPU port (%d)\n", port); + /* GMII is the default interface mode for phylib, so + * we have to support it for ports with integrated PHY. + */ + __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces); +} - /* Force the fixed CPU port into 1Gbit mode, no autonegotiation */ - ret = regmap_update_bits(priv->map, RTL8366RB_MAC_FORCE_CTRL_REG, - BIT(port), BIT(port)); - if (ret) { - dev_err(priv->dev, "failed to force 1Gbit on CPU port\n"); - return; +static int rtl8366rb_config_link(struct realtek_priv *priv, int port, bool link, + int speed, int duplex, bool tx_pause, bool rx_pause) +{ + u32 paacr; + u32 portreg; + u32 portmask; + u32 portshift; + int ret; + + switch (port) { + case 0: + portreg = RTL8366RB_PAACR0; + portshift = 0; + break; + case 1: + portreg = RTL8366RB_PAACR0; + portshift = 8; + break; + case 2: + portreg = RTL8366RB_PAACR1; + portshift = 0; + break; + case 3: + portreg = RTL8366RB_PAACR1; + portshift = 8; + break; + case 4: + portreg = RTL8366RB_PAACR2; + portshift = 0; + break; + case 5: + portreg = RTL8366RB_PAACR2; + portshift = 8; + break; + default: + dev_err(priv->dev, "illegal port %d\n", port); + return -EINVAL; } - ret = regmap_update_bits(priv->map, RTL8366RB_PAACR2, - 0xFF00U, - RTL8366RB_PAACR_CPU_PORT << 8); - if (ret) { - dev_err(priv->dev, "failed to set PAACR on CPU port\n"); - return; + portmask = GENMASK(portshift + 7, portshift); + + if (link) { + switch (speed) { + case SPEED_1000: + paacr = RTL8366RB_PAACR_SPEED_1000M; + dev_dbg(priv->dev, "set port %d to 1Gbit\n", port); + break; + case SPEED_100: + paacr = RTL8366RB_PAACR_SPEED_100M; + dev_dbg(priv->dev, "set port %d to 100Mbit\n", port); + break; + case SPEED_10: + paacr = RTL8366RB_PAACR_SPEED_10M; + dev_dbg(priv->dev, "set port %d to 10Mbit\n", port); + break; + default: + dev_err(priv->dev, "illegal speed request on port %d\n", port); + return -EINVAL; + } + + if (duplex == DUPLEX_FULL) + paacr |= RTL8366RB_PAACR_FULL_DUPLEX; + dev_dbg(priv->dev, "set port %d to %s duplex\n", port, + (duplex == DUPLEX_FULL) ? "full" : "half"); + + if (tx_pause) + paacr |= RTL8366RB_PAACR_TX_PAUSE; + + if (rx_pause) + paacr |= RTL8366RB_PAACR_RX_PAUSE; + + /* We are in the link up function so force it up */ + paacr |= RTL8366RB_PAACR_LINK_UP; + } else { + /* If link goes down just zero the register including link up */ + paacr = 0; } - /* Enable the CPU port */ - ret = regmap_update_bits(priv->map, RTL8366RB_PECR, BIT(port), - 0); + ret = regmap_update_bits(priv->map, portreg, portmask, paacr << portshift); if (ret) { - dev_err(priv->dev, "failed to enable the CPU port\n"); - return; + dev_err(priv->dev, "failed to set PAACR on port %d\n", port); + return ret; } + dev_dbg(priv->dev, "Updated port %d reg %08x, mask %08x, shift %d with value %08x\n", + port, portreg, portmask, portshift, paacr); + + return 0; } static void -rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, - phy_interface_t interface) +rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode, + phy_interface_t interface, struct phy_device *phydev, + int speed, int duplex, bool tx_pause, bool rx_pause) { struct realtek_priv *priv = ds->priv; int ret; - if (port != priv->cpu_port) - return; + ret = rtl8366rb_config_link(priv, port, true, speed, duplex, tx_pause, rx_pause); + if (ret) + dev_err(priv->dev, "error configuring link on port %d\n", port); +} - dev_dbg(priv->dev, "MAC link down on CPU port (%d)\n", port); +static void +rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, + phy_interface_t interface) +{ + struct realtek_priv *priv = ds->priv; + int ret; - /* Disable the CPU port */ - ret = regmap_update_bits(priv->map, RTL8366RB_PECR, BIT(port), - BIT(port)); - if (ret) { - dev_err(priv->dev, "failed to disable the CPU port\n"); - return; - } + ret = rtl8366rb_config_link(priv, port, false, 0, 0, false, false); + if (ret) + dev_err(priv->dev, "error configuring link on port %d\n", port); } static void rb8366rb_set_port_led(struct realtek_priv *priv, @@ -1796,6 +1861,7 @@ static int rtl8366rb_detect(struct realtek_priv *priv) static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = { .get_tag_protocol = rtl8366_get_tag_protocol, .setup = rtl8366rb_setup, + .phylink_get_caps = rtl8366rb_link_get_caps, .phylink_mac_link_up = rtl8366rb_mac_link_up, .phylink_mac_link_down = rtl8366rb_mac_link_down, .get_strings = rtl8366_get_strings, @@ -1821,6 +1887,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = { .setup = rtl8366rb_setup, .phy_read = rtl8366rb_dsa_phy_read, .phy_write = rtl8366rb_dsa_phy_write, + .phylink_get_caps = rtl8366rb_link_get_caps, .phylink_mac_link_up = rtl8366rb_mac_link_up, .phylink_mac_link_down = rtl8366rb_mac_link_down, .get_strings = rtl8366_get_strings,
Instead of just hammering the CPU port up at 1GBit at .phylink_mac_link_up calls for that specific port, support configuring any port: this works like a charm. Drop the code to enable/disable the port in the .phylink_mac_link_up/.phylink_mac_link_down callbacks: this is handled perfectly well by the callbacks to .port_enable/.port_disable. Tested on the D-Link DIR-685. Cc: Alvin Šipraga <alsi@bang-olufsen.dk> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/net/dsa/realtek/rtl8366rb.c | 155 ++++++++++++++++++++-------- 1 file changed, 111 insertions(+), 44 deletions(-)