| Message ID | 20230621191302.1405623-1-paweldembicki@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/6] net: dsa: vsc73xx: convert to PHYLINK | expand |
On Wed, Jun 21, 2023 at 9:13 PM Pawel Dembicki <paweldembicki@gmail.com> wrote: > This patch replaces the adjust_link api with the phylink apis that provide > equivalent functionality. > > The remaining functionality from the adjust_link is now covered in the > phylink_mac_link_* and phylink_mac_config. > > Removes: > .adjust_link > Adds: > .phylink_get_caps > .phylink_mac_link_down > .phylink_mac_link_up > .phylink_mac_link_down > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> Thanks for doing this! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote: > This patch replaces the adjust_link api with the phylink apis that provide > equivalent functionality. > > The remaining functionality from the adjust_link is now covered in the > phylink_mac_link_* and phylink_mac_config. > > Removes: > .adjust_link > Adds: > .phylink_get_caps > .phylink_mac_link_down > .phylink_mac_link_up > .phylink_mac_link_down > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> ... > +static void vsc73xx_phylink_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 vsc73xx *vsc = ds->priv; > + u32 val; > > + switch (speed) { > + case SPEED_1000: > /* Set up default for internal port or external RGMII */ > - if (phydev->interface == PHY_INTERFACE_MODE_RGMII) > + if (interface == PHY_INTERFACE_MODE_RGMII) > val = VSC73XX_MAC_CFG_1000M_F_RGMII; > else > val = VSC73XX_MAC_CFG_1000M_F_PHY; > - vsc73xx_adjust_enable_port(vsc, port, phydev, val); > - } else if (phydev->speed == SPEED_100) { > - if (phydev->duplex == DUPLEX_FULL) { > - val = VSC73XX_MAC_CFG_100_10M_F_PHY; > - dev_dbg(vsc->dev, > - "port %d: 100 Mbit full duplex mode\n", > - port); > - } else { > - val = VSC73XX_MAC_CFG_100_10M_H_PHY; > - dev_dbg(vsc->dev, > - "port %d: 100 Mbit half duplex mode\n", > - port); > - } > - vsc73xx_adjust_enable_port(vsc, port, phydev, val); > - } else if (phydev->speed == SPEED_10) { > - if (phydev->duplex == DUPLEX_FULL) { > + break; > + case SPEED_100: > + case SPEED_10: > + if (duplex == DUPLEX_FULL) > val = VSC73XX_MAC_CFG_100_10M_F_PHY; > - dev_dbg(vsc->dev, > - "port %d: 10 Mbit full duplex mode\n", > - port); > - } else { > + else > val = VSC73XX_MAC_CFG_100_10M_H_PHY; > - dev_dbg(vsc->dev, > - "port %d: 10 Mbit half duplex mode\n", > - port); > - } > - vsc73xx_adjust_enable_port(vsc, port, phydev, val); > - } else { > - dev_err(vsc->dev, > - "could not adjust link: unknown speed\n"); > + break; > } > > /* Enable port (forwarding) in the receieve mask */ > vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, > VSC73XX_RECVMASK, BIT(port), BIT(port)); > + vsc73xx_adjust_enable_port(vsc, port, val); Hi Pawel, GCC 12.3.0 [1] reports that val may now be uninitialised at this point, and in turn used uninitialised in vsc73xx_adjust_enable_port. In function 'vsc73xx_adjust_enable_port', inlined from 'vsc73xx_phylink_mac_link_up' at drivers/net/dsa/vitesse-vsc73xx-core.c:891:2: drivers/net/dsa/vitesse-vsc73xx-core.c:725:13: warning: 'val' may be used uninitialized [-Wmaybe-uninitialized] 725 | val |= VSC73XX_MAC_CFG_RESET; | ^ drivers/net/dsa/vitesse-vsc73xx-core.c: In function 'vsc73xx_phylink_mac_link_up': drivers/net/dsa/vitesse-vsc73xx-core.c:869:13: note: 'val' was declared here 869 | u32 val; | ^~~ ...
On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote: > + /* This driver does not make use of the speed, duplex, pause or the > + * advertisement in its mac_config, so it is safe to mark this driver > + * as non-legacy. > + */ > + config->legacy_pre_march2020 = false; Great stuff, thanks! > +static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port, > + unsigned int mode, > + const struct phylink_link_state *state) > +{ > + struct vsc73xx *vsc = ds->priv; Nit: normally have a blank line between function variable declarations and the rest of the function body. > /* Special handling of the CPU-facing port */ > if (port == CPU_PORT) { > /* Other ports are already initialized but not this one */ > @@ -775,104 +803,92 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port, > VSC73XX_ADVPORTM_ENA_GTX | > VSC73XX_ADVPORTM_DDR_MODE); > } > +} > > - /* This is the MAC confiuration that always need to happen > - * after a PHY or the CPU port comes up or down. > - */ > - if (!phydev->link) { > - int maxloop = 10; > +static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port, > + unsigned int mode, > + phy_interface_t interface) > +{ > + struct vsc73xx *vsc = ds->priv; > + u32 val; > > - dev_dbg(vsc->dev, "port %d: went down\n", > - port); > + int maxloop = VSC73XX_TABLE_ATTEMPTS; Reverse christmas-tree for variable declarations, and there shouldn't be a blank line between them. In any case, I don't think you need "maxloop" if you adopt my suggestion below. > > - /* Disable RX on this port */ > - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > - VSC73XX_MAC_CFG, > - VSC73XX_MAC_CFG_RX_EN, 0); > + dev_dbg(vsc->dev, "port %d: went down\n", > + port); > > - /* Discard packets */ > - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, > - VSC73XX_ARBDISC, BIT(port), BIT(port)); > + /* Disable RX on this port */ > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > + VSC73XX_MAC_CFG, > + VSC73XX_MAC_CFG_RX_EN, 0); > + > + /* Discard packets */ > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, > + VSC73XX_ARBDISC, BIT(port), BIT(port)); > > - /* Wait until queue is empty */ > + /* Wait until queue is empty */ > + vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0, > + VSC73XX_ARBEMPTY, &val); > + while (!(val & BIT(port))) { > + msleep(1); > vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0, > VSC73XX_ARBEMPTY, &val); > - while (!(val & BIT(port))) { > - msleep(1); > - vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0, > - VSC73XX_ARBEMPTY, &val); > - if (--maxloop == 0) { > - dev_err(vsc->dev, > - "timeout waiting for block arbiter\n"); > - /* Continue anyway */ > - break; > - } > + if (--maxloop == 0) { > + dev_err(vsc->dev, > + "timeout waiting for block arbiter\n"); > + /* Continue anyway */ > + break; > } > + } I know you're only moving this code, but I think it would be good to _first_ have a patch that fixes this polling function: int ret, err; ... ret = read_poll_timeout(vsc73xx_read, err, err < 0 || (val & BIT(port)), 1000, 10000, false, vsc, VSC73XX_BLOCK_ARBITER, 0, VSC73XX_ARBEMPTY, &val); if (ret != 0) dev_err(vsc->dev, "timeout waiting for block arbiter\n"); else if (err < 0) dev_err(vsc->dev, "error reading arbiter\n"); This avoids the issue that on the last iteration, the code reads the register, test it, find the condition that's being waiting for is false, _then_ waits and end up printing the error message - that last wait is rather useless, and as the arbiter state isn't checked after waiting, it could be that we had success during the last wait. > +static void vsc73xx_phylink_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 vsc73xx *vsc = ds->priv; > + u32 val; > > + switch (speed) { > + case SPEED_1000: > /* Set up default for internal port or external RGMII */ > - if (phydev->interface == PHY_INTERFACE_MODE_RGMII) > + if (interface == PHY_INTERFACE_MODE_RGMII) > val = VSC73XX_MAC_CFG_1000M_F_RGMII; > else > val = VSC73XX_MAC_CFG_1000M_F_PHY; > - vsc73xx_adjust_enable_port(vsc, port, phydev, val); > - } else if (phydev->speed == SPEED_100) { > - if (phydev->duplex == DUPLEX_FULL) { > - val = VSC73XX_MAC_CFG_100_10M_F_PHY; > - dev_dbg(vsc->dev, > - "port %d: 100 Mbit full duplex mode\n", > - port); > - } else { > - val = VSC73XX_MAC_CFG_100_10M_H_PHY; > - dev_dbg(vsc->dev, > - "port %d: 100 Mbit half duplex mode\n", > - port); > - } > - vsc73xx_adjust_enable_port(vsc, port, phydev, val); > - } else if (phydev->speed == SPEED_10) { > - if (phydev->duplex == DUPLEX_FULL) { > + break; > + case SPEED_100: > + case SPEED_10: > + if (duplex == DUPLEX_FULL) > val = VSC73XX_MAC_CFG_100_10M_F_PHY; > - dev_dbg(vsc->dev, > - "port %d: 10 Mbit full duplex mode\n", > - port); > - } else { > + else > val = VSC73XX_MAC_CFG_100_10M_H_PHY; > - dev_dbg(vsc->dev, > - "port %d: 10 Mbit half duplex mode\n", > - port); > - } > - vsc73xx_adjust_enable_port(vsc, port, phydev, val); > - } else { > - dev_err(vsc->dev, > - "could not adjust link: unknown speed\n"); > + break; > } Do the dev_dbg() add anything useful over what phylink prints when the link comes up? I don't think moving to a switch() statement for this is a good idea. Given that "val" may be uninitialised, I suspect the following may be a better solution: if (speed == SPEED_1000 || speed == SPEED_100 || speed == SPEED_10) { if (speed == SPEED_1000) { ... } else { ... } ... set VSC73XX_BLOCK_ANALYZER and call vsc73xx_adjust_enable_port ... } However, looking at the definitions of the various macros, it seems we can do a little better by not using the VSC73XX_MAC_CFG_*M_[FH]_* definitions: if (speed == SPEED_1000) { val = VSC73XX_MAC_CFG_GIGA_MODE | VSC73XX_MAC_CFG_TX_IPG_1000M; if (interface == PHY_INTERFACE_MODE_RGMII) val |= VSC73XX_MAC_CFG_CLK_SEL_1000M; else val |= VSC73XX_MAC_CFG_CLK_SEL_EXT; } else { val = VSC73XX_MAC_CFG_TX_IPG_100_10M | VSC73XX_MAC_CFG_CLK_SEL_EXT; } if (duplex == DUPLEX_FULL) val |= VSC73XX_MAC_CFG_FDX; Now, this reveals a question: when operating in RGMII mode, why do we need VSC73XX_MAC_CFG_CLK_SEL_1000M for 1G, and VSC73XX_MAC_CFG_CLK_SEL_EXT for 10M and 100M, whereas "PHY" mode always uses CLK_SEL_EXT ? Thanks.
czw., 22 cze 2023 o 13:55 Russell King (Oracle) <linux@armlinux.org.uk> napisał(a): > > On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote: > > + /* This driver does not make use of the speed, duplex, pause or the > > + * advertisement in its mac_config, so it is safe to mark this driver > > + * as non-legacy. > > + */ > > + config->legacy_pre_march2020 = false; > > Great stuff, thanks! > > > +static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port, > > + unsigned int mode, > > + const struct phylink_link_state *state) > > +{ > > + struct vsc73xx *vsc = ds->priv; > > Nit: normally have a blank line between function variable declarations > and the rest of the function body. > > > /* Special handling of the CPU-facing port */ > > if (port == CPU_PORT) { > > /* Other ports are already initialized but not this one */ > > @@ -775,104 +803,92 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port, > > VSC73XX_ADVPORTM_ENA_GTX | > > VSC73XX_ADVPORTM_DDR_MODE); > > } > > +} > > > > - /* This is the MAC confiuration that always need to happen > > - * after a PHY or the CPU port comes up or down. > > - */ > > - if (!phydev->link) { > > - int maxloop = 10; > > +static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port, > > + unsigned int mode, > > + phy_interface_t interface) > > +{ > > + struct vsc73xx *vsc = ds->priv; > > + u32 val; > > > > - dev_dbg(vsc->dev, "port %d: went down\n", > > - port); > > + int maxloop = VSC73XX_TABLE_ATTEMPTS; > > Reverse christmas-tree for variable declarations, and there shouldn't > be a blank line between them. In any case, I don't think you need > "maxloop" if you adopt my suggestion below. > > > > > - /* Disable RX on this port */ > > - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > - VSC73XX_MAC_CFG, > > - VSC73XX_MAC_CFG_RX_EN, 0); > > + dev_dbg(vsc->dev, "port %d: went down\n", > > + port); > > > > - /* Discard packets */ > > - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, > > - VSC73XX_ARBDISC, BIT(port), BIT(port)); > > + /* Disable RX on this port */ > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_MAC_CFG, > > + VSC73XX_MAC_CFG_RX_EN, 0); > > + > > + /* Discard packets */ > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, > > + VSC73XX_ARBDISC, BIT(port), BIT(port)); > > > > - /* Wait until queue is empty */ > > + /* Wait until queue is empty */ > > + vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0, > > + VSC73XX_ARBEMPTY, &val); > > + while (!(val & BIT(port))) { > > + msleep(1); > > vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0, > > VSC73XX_ARBEMPTY, &val); > > - while (!(val & BIT(port))) { > > - msleep(1); > > - vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0, > > - VSC73XX_ARBEMPTY, &val); > > - if (--maxloop == 0) { > > - dev_err(vsc->dev, > > - "timeout waiting for block arbiter\n"); > > - /* Continue anyway */ > > - break; > > - } > > + if (--maxloop == 0) { > > + dev_err(vsc->dev, > > + "timeout waiting for block arbiter\n"); > > + /* Continue anyway */ > > + break; > > } > > + } > > I know you're only moving this code, but I think it would be good to > _first_ have a patch that fixes this polling function: > > int ret, err; > ... > ret = read_poll_timeout(vsc73xx_read, err, > err < 0 || (val & BIT(port)), > 1000, 10000, false, > vsc, VSC73XX_BLOCK_ARBITER, 0, > VSC73XX_ARBEMPTY, &val); > if (ret != 0) > dev_err(vsc->dev, > "timeout waiting for block arbiter\n"); > else if (err < 0) > dev_err(vsc->dev, > "error reading arbiter\n"); > > This avoids the issue that on the last iteration, the code reads the > register, test it, find the condition that's being waiting for is > false, _then_ waits and end up printing the error message - that last > wait is rather useless, and as the arbiter state isn't checked after > waiting, it could be that we had success during the last wait. > Thank you for the tips. I will prepare additional commit in v2 series. > > +static void vsc73xx_phylink_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 vsc73xx *vsc = ds->priv; > > + u32 val; > > > > + switch (speed) { > > + case SPEED_1000: > > /* Set up default for internal port or external RGMII */ > > - if (phydev->interface == PHY_INTERFACE_MODE_RGMII) > > + if (interface == PHY_INTERFACE_MODE_RGMII) > > val = VSC73XX_MAC_CFG_1000M_F_RGMII; > > else > > val = VSC73XX_MAC_CFG_1000M_F_PHY; > > - vsc73xx_adjust_enable_port(vsc, port, phydev, val); > > - } else if (phydev->speed == SPEED_100) { > > - if (phydev->duplex == DUPLEX_FULL) { > > - val = VSC73XX_MAC_CFG_100_10M_F_PHY; > > - dev_dbg(vsc->dev, > > - "port %d: 100 Mbit full duplex mode\n", > > - port); > > - } else { > > - val = VSC73XX_MAC_CFG_100_10M_H_PHY; > > - dev_dbg(vsc->dev, > > - "port %d: 100 Mbit half duplex mode\n", > > - port); > > - } > > - vsc73xx_adjust_enable_port(vsc, port, phydev, val); > > - } else if (phydev->speed == SPEED_10) { > > - if (phydev->duplex == DUPLEX_FULL) { > > + break; > > + case SPEED_100: > > + case SPEED_10: > > + if (duplex == DUPLEX_FULL) > > val = VSC73XX_MAC_CFG_100_10M_F_PHY; > > - dev_dbg(vsc->dev, > > - "port %d: 10 Mbit full duplex mode\n", > > - port); > > - } else { > > + else > > val = VSC73XX_MAC_CFG_100_10M_H_PHY; > > - dev_dbg(vsc->dev, > > - "port %d: 10 Mbit half duplex mode\n", > > - port); > > - } > > - vsc73xx_adjust_enable_port(vsc, port, phydev, val); > > - } else { > > - dev_err(vsc->dev, > > - "could not adjust link: unknown speed\n"); > > + break; > > } > > Do the dev_dbg() add anything useful over what phylink prints when the > link comes up? > > I don't think moving to a switch() statement for this is a good idea. > Given that "val" may be uninitialised, I suspect the following may be > a better solution: > > if (speed == SPEED_1000 || speed == SPEED_100 || speed == SPEED_10) { > if (speed == SPEED_1000) { > ... > } else { > ... > } > > ... set VSC73XX_BLOCK_ANALYZER and call > vsc73xx_adjust_enable_port ... > } > > However, looking at the definitions of the various macros, it seems we > can do a little better by not using the VSC73XX_MAC_CFG_*M_[FH]_* > definitions: > > if (speed == SPEED_1000) { > val = VSC73XX_MAC_CFG_GIGA_MODE | > VSC73XX_MAC_CFG_TX_IPG_1000M; > > if (interface == PHY_INTERFACE_MODE_RGMII) > val |= VSC73XX_MAC_CFG_CLK_SEL_1000M; > else > val |= VSC73XX_MAC_CFG_CLK_SEL_EXT; > } else { > val = VSC73XX_MAC_CFG_TX_IPG_100_10M | > VSC73XX_MAC_CFG_CLK_SEL_EXT; > } > > if (duplex == DUPLEX_FULL) > val |= VSC73XX_MAC_CFG_FDX; > > Now, this reveals a question: when operating in RGMII mode, why do we > need VSC73XX_MAC_CFG_CLK_SEL_1000M for 1G, and > VSC73XX_MAC_CFG_CLK_SEL_EXT for 10M and 100M, whereas "PHY" mode always > uses CLK_SEL_EXT ? > VSC73XX_MAC_CFG_CLK_SEL_EXT should be used always when phy is used, no matter what speed is. VSC73XX_MAC_CFG_CLK_SEL_1000M in RGMII mode. It can be even more simplified: if (speed == SPEED_1000) val = VSC73XX_MAC_CFG_GIGA_MODE | VSC73XX_MAC_CFG_TX_IPG_1000M; else val = VSC73XX_MAC_CFG_TX_IPG_100_10M; if (interface == PHY_INTERFACE_MODE_RGMII) val |= VSC73XX_MAC_CFG_CLK_SEL_1000M; else val |= VSC73XX_MAC_CFG_CLK_SEL_EXT; if (duplex == DUPLEX_FULL) val |= VSC73XX_MAC_CFG_FDX; -- Pawel Dembicki
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c index ae55167ce0a6..e853b57b0bc8 100644 --- a/drivers/net/dsa/vitesse-vsc73xx-core.c +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c @@ -39,6 +39,7 @@ #define VSC73XX_BLOCK_SYSTEM 0x7 /* Only subblock 0 */ #define CPU_PORT 6 /* CPU port */ +#define VSC73XX_TABLE_ATTEMPTS 10 /* MAC Block registers */ #define VSC73XX_MAC_CFG 0x00 @@ -715,8 +716,7 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port) } static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc, - int port, struct phy_device *phydev, - u32 initval) + int port, u32 initval) { u32 val = initval; u8 seed; @@ -754,12 +754,40 @@ static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc, VSC73XX_MAC_CFG_TX_EN | VSC73XX_MAC_CFG_RX_EN); } -static void vsc73xx_adjust_link(struct dsa_switch *ds, int port, - struct phy_device *phydev) +static void vsc73xx_phylink_get_caps(struct dsa_switch *ds, int port, + struct phylink_config *config) { - struct vsc73xx *vsc = ds->priv; - u32 val; + /* This switch only supports full-duplex at 1Gbps */ + config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000FD | + MAC_ASYM_PAUSE | MAC_SYM_PAUSE; + if (port == CPU_PORT) { + __set_bit(PHY_INTERFACE_MODE_RGMII, + config->supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_GMII, + config->supported_interfaces); + } else { + __set_bit(PHY_INTERFACE_MODE_INTERNAL, + config->supported_interfaces); + /* Compatibility for phylib's default interface type when the + * phy-mode property is absent + */ + __set_bit(PHY_INTERFACE_MODE_GMII, + config->supported_interfaces); + } + + /* This driver does not make use of the speed, duplex, pause or the + * advertisement in its mac_config, so it is safe to mark this driver + * as non-legacy. + */ + config->legacy_pre_march2020 = false; +} + +static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port, + unsigned int mode, + const struct phylink_link_state *state) +{ + struct vsc73xx *vsc = ds->priv; /* Special handling of the CPU-facing port */ if (port == CPU_PORT) { /* Other ports are already initialized but not this one */ @@ -775,104 +803,92 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port, VSC73XX_ADVPORTM_ENA_GTX | VSC73XX_ADVPORTM_DDR_MODE); } +} - /* This is the MAC confiuration that always need to happen - * after a PHY or the CPU port comes up or down. - */ - if (!phydev->link) { - int maxloop = 10; +static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port, + unsigned int mode, + phy_interface_t interface) +{ + struct vsc73xx *vsc = ds->priv; + u32 val; - dev_dbg(vsc->dev, "port %d: went down\n", - port); + int maxloop = VSC73XX_TABLE_ATTEMPTS; - /* Disable RX on this port */ - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, - VSC73XX_MAC_CFG, - VSC73XX_MAC_CFG_RX_EN, 0); + dev_dbg(vsc->dev, "port %d: went down\n", + port); - /* Discard packets */ - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, - VSC73XX_ARBDISC, BIT(port), BIT(port)); + /* Disable RX on this port */ + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_MAC_CFG, + VSC73XX_MAC_CFG_RX_EN, 0); + + /* Discard packets */ + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, + VSC73XX_ARBDISC, BIT(port), BIT(port)); - /* Wait until queue is empty */ + /* Wait until queue is empty */ + vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0, + VSC73XX_ARBEMPTY, &val); + while (!(val & BIT(port))) { + msleep(1); vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0, VSC73XX_ARBEMPTY, &val); - while (!(val & BIT(port))) { - msleep(1); - vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0, - VSC73XX_ARBEMPTY, &val); - if (--maxloop == 0) { - dev_err(vsc->dev, - "timeout waiting for block arbiter\n"); - /* Continue anyway */ - break; - } + if (--maxloop == 0) { + dev_err(vsc->dev, + "timeout waiting for block arbiter\n"); + /* Continue anyway */ + break; } + } - /* Put this port into reset */ - vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, - VSC73XX_MAC_CFG_RESET); - - /* Accept packets again */ - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, - VSC73XX_ARBDISC, BIT(port), 0); + /* Put this port into reset */ + vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, + VSC73XX_MAC_CFG_RESET); - /* Allow backward dropping of frames from this port */ - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, - VSC73XX_SBACKWDROP, BIT(port), BIT(port)); + /* Accept packets again */ + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, + VSC73XX_ARBDISC, BIT(port), 0); - /* Receive mask (disable forwarding) */ - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, - VSC73XX_RECVMASK, BIT(port), 0); + /* Allow backward dropping of frames from this port */ + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0, + VSC73XX_SBACKWDROP, BIT(port), BIT(port)); - return; - } + /* Receive mask (disable forwarding) */ + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, + VSC73XX_RECVMASK, BIT(port), 0); +} - /* Figure out what speed was negotiated */ - if (phydev->speed == SPEED_1000) { - dev_dbg(vsc->dev, "port %d: 1000 Mbit mode full duplex\n", - port); +static void vsc73xx_phylink_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 vsc73xx *vsc = ds->priv; + u32 val; + switch (speed) { + case SPEED_1000: /* Set up default for internal port or external RGMII */ - if (phydev->interface == PHY_INTERFACE_MODE_RGMII) + if (interface == PHY_INTERFACE_MODE_RGMII) val = VSC73XX_MAC_CFG_1000M_F_RGMII; else val = VSC73XX_MAC_CFG_1000M_F_PHY; - vsc73xx_adjust_enable_port(vsc, port, phydev, val); - } else if (phydev->speed == SPEED_100) { - if (phydev->duplex == DUPLEX_FULL) { - val = VSC73XX_MAC_CFG_100_10M_F_PHY; - dev_dbg(vsc->dev, - "port %d: 100 Mbit full duplex mode\n", - port); - } else { - val = VSC73XX_MAC_CFG_100_10M_H_PHY; - dev_dbg(vsc->dev, - "port %d: 100 Mbit half duplex mode\n", - port); - } - vsc73xx_adjust_enable_port(vsc, port, phydev, val); - } else if (phydev->speed == SPEED_10) { - if (phydev->duplex == DUPLEX_FULL) { + break; + case SPEED_100: + case SPEED_10: + if (duplex == DUPLEX_FULL) val = VSC73XX_MAC_CFG_100_10M_F_PHY; - dev_dbg(vsc->dev, - "port %d: 10 Mbit full duplex mode\n", - port); - } else { + else val = VSC73XX_MAC_CFG_100_10M_H_PHY; - dev_dbg(vsc->dev, - "port %d: 10 Mbit half duplex mode\n", - port); - } - vsc73xx_adjust_enable_port(vsc, port, phydev, val); - } else { - dev_err(vsc->dev, - "could not adjust link: unknown speed\n"); + break; } /* Enable port (forwarding) in the receieve mask */ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_RECVMASK, BIT(port), BIT(port)); + vsc73xx_adjust_enable_port(vsc, port, val); } static int vsc73xx_port_enable(struct dsa_switch *ds, int port, @@ -1043,7 +1059,10 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = { .setup = vsc73xx_setup, .phy_read = vsc73xx_phy_read, .phy_write = vsc73xx_phy_write, - .adjust_link = vsc73xx_adjust_link, + .phylink_get_caps = vsc73xx_phylink_get_caps, + .phylink_mac_config = vsc73xx_phylink_mac_config, + .phylink_mac_link_down = vsc73xx_phylink_mac_link_down, + .phylink_mac_link_up = vsc73xx_phylink_mac_link_up, .get_strings = vsc73xx_get_strings, .get_ethtool_stats = vsc73xx_get_ethtool_stats, .get_sset_count = vsc73xx_get_sset_count,
This patch replaces the adjust_link api with the phylink apis that provide equivalent functionality. The remaining functionality from the adjust_link is now covered in the phylink_mac_link_* and phylink_mac_config. Removes: .adjust_link Adds: .phylink_get_caps .phylink_mac_link_down .phylink_mac_link_up .phylink_mac_link_down Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> --- drivers/net/dsa/vitesse-vsc73xx-core.c | 179 ++++++++++++++----------- 1 file changed, 99 insertions(+), 80 deletions(-)