[net-next,1/6] net: dsa: vsc73xx: convert to PHYLINK
Commit Message
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(-)
Comments
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
@@ -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,