diff mbox series

[net-next,v4,02/15] net: dsa: vsc73xx: convert to PHYLINK

Message ID 20240213220331.239031-3-paweldembicki@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: vsc73xx: Make vsc73xx usable | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 989 this patch: 989
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 1006 this patch: 1006
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1006 this patch: 1006
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 200 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pawel Dembicki Feb. 13, 2024, 10:03 p.m. UTC 
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_mac_config
.phylink_mac_link_up
.phylink_mac_link_down

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v4:
  - update commit descripion
  - remove phylink_get_caps after rebase to current net-next/main
v3:
  - remove legacy_pre_march2020 after rebase
v2:
  - replace switch to if and get rid of macros in
    vsc73xx_phylink_mac_link_up function

 drivers/net/dsa/vitesse-vsc73xx-core.c | 166 +++++++++++--------------
 1 file changed, 72 insertions(+), 94 deletions(-)

Comments

Florian Fainelli Feb. 13, 2024, 11:19 p.m. UTC | #1 
On 2/13/24 14:03, 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_mac_config
> .phylink_mac_link_up
> .phylink_mac_link_down

The implementation of phylink_mac_link_down() strictly mimics what had 
been done by adjust_link() in the phydev->link == 0 case, but it really 
makes me wonder whether some bits do not logically belong to 
phylink_mac_link_up(), like "Accept packets again" for instance.

Are we certain there was not an assumption before that we would get 
adjust_link() called first with phydev->link = 0, and then phydev->link 
=1 and that this specific sequence would program things just the way we 
want?
Pawel Dembicki Feb. 14, 2024, 12:56 p.m. UTC | #2 
śr., 14 lut 2024 o 00:19 Florian Fainelli <f.fainelli@gmail.com> napisał(a):
>
> On 2/13/24 14:03, 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_mac_config
> > .phylink_mac_link_up
> > .phylink_mac_link_down
>
> The implementation of phylink_mac_link_down() strictly mimics what had
> been done by adjust_link() in the phydev->link == 0 case, but it really
> makes me wonder whether some bits do not logically belong to
> phylink_mac_link_up(), like "Accept packets again" for instance.
>
> Are we certain there was not an assumption before that we would get
> adjust_link() called first with phydev->link = 0, and then phydev->link
> =1 and that this specific sequence would program things just the way we
> want?

Yes, it was the simplest conversion possible, without any improvements.

Some part is implementation of datasheet (description of ARBEMPTY register):

        /* Discard packets */
        vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
                            VSC73XX_ARBDISC, BIT(port), BIT(port));

        /* Wait until queue is empty */
        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)
                dev_err(vsc->dev,
                        "timeout waiting for block arbiter\n");
        else if (err < 0)
                dev_err(vsc->dev, "error reading arbiter\n");

        /* Put this port into reset */
        vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
                      VSC73XX_MAC_CFG_RESET);


I agree that VSC73XX_ARBDISC should be moved to phylink_mac_link_up.
Other things could be optimised and it needs more care. (eg. This
implementation doesn't disable phy when the interface goes down.) I
plan to tweak it after the driver becomes usable. Please let me know
if it should be fixed in this patch.
Vladimir Oltean Feb. 15, 2024, 12:04 a.m. UTC | #3 
On Wed, Feb 14, 2024 at 01:56:10PM +0100, Paweł Dembicki wrote:
> śr., 14 lut 2024 o 00:19 Florian Fainelli <f.fainelli@gmail.com> napisał(a):
> >
> > On 2/13/24 14:03, 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_mac_config
> > > .phylink_mac_link_up
> > > .phylink_mac_link_down
> >
> > The implementation of phylink_mac_link_down() strictly mimics what had
> > been done by adjust_link() in the phydev->link == 0 case, but it really
> > makes me wonder whether some bits do not logically belong to
> > phylink_mac_link_up(), like "Accept packets again" for instance.
> >
> > Are we certain there was not an assumption before that we would get
> > adjust_link() called first with phydev->link = 0, and then phydev->link
> > =1 and that this specific sequence would program things just the way we
> > want?
> 
> Yes, it was the simplest conversion possible, without any improvements.
> 
> Some part is implementation of datasheet (description of ARBEMPTY register):
> 
>         /* Discard packets */
>         vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
>                             VSC73XX_ARBDISC, BIT(port), BIT(port));
> 
>         /* Wait until queue is empty */
>         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)
>                 dev_err(vsc->dev,
>                         "timeout waiting for block arbiter\n");
>         else if (err < 0)
>                 dev_err(vsc->dev, "error reading arbiter\n");
> 
>         /* Put this port into reset */
>         vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
>                       VSC73XX_MAC_CFG_RESET);
> 
> 
> I agree that VSC73XX_ARBDISC should be moved to phylink_mac_link_up.

FWIW, ocelot_phylink_mac_link_down() also calls ocelot_port_flush()
which is more or less the same procedure for a different piece of hw.

By re-reading the commit message of eb4733d7cffc ("net: dsa: felix:
implement port flushing on .phylink_mac_link_down"), I can find a good
reason to flush the port on link down and not on link up. With flow
control enabled, packets would remain in the queue system until there's
link again if not flushed there, otherwise.

Paweł, maybe it is simply the case that you should move the procedure
from the datasheet into a more clearly named sub-function?

> Other things could be optimised and it needs more care. (eg. This
> implementation doesn't disable phy when the interface goes down.) I
> plan to tweak it after the driver becomes usable. Please let me know
> if it should be fixed in this patch.

What do you mean by disabling the PHY when the interface goes down,
exactly? Down as in administratively down, aka "ip link set swp0 down",
not when the link drops?

That's a thing for the PHY driver to handle, by implementing .suspend()
and .resume(), I guess?

What driver do the internal PHYs use?
Pawel Dembicki Feb. 20, 2024, 12:28 p.m. UTC | #4 
czw., 15 lut 2024 o 01:04 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Wed, Feb 14, 2024 at 01:56:10PM +0100, Paweł Dembicki wrote:
> > śr., 14 lut 2024 o 00:19 Florian Fainelli <f.fainelli@gmail.com> napisał(a):
> > >
> > > On 2/13/24 14:03, 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_mac_config
> > > > .phylink_mac_link_up
> > > > .phylink_mac_link_down
> > >
> > > The implementation of phylink_mac_link_down() strictly mimics what had
> > > been done by adjust_link() in the phydev->link == 0 case, but it really
> > > makes me wonder whether some bits do not logically belong to
> > > phylink_mac_link_up(), like "Accept packets again" for instance.
> > >
> > > Are we certain there was not an assumption before that we would get
> > > adjust_link() called first with phydev->link = 0, and then phydev->link
> > > =1 and that this specific sequence would program things just the way we
> > > want?
> >
> > Yes, it was the simplest conversion possible, without any improvements.
> >
> > Some part is implementation of datasheet (description of ARBEMPTY register):
> >
> >         /* Discard packets */
> >         vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> >                             VSC73XX_ARBDISC, BIT(port), BIT(port));
> >
> >         /* Wait until queue is empty */
> >         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)
> >                 dev_err(vsc->dev,
> >                         "timeout waiting for block arbiter\n");
> >         else if (err < 0)
> >                 dev_err(vsc->dev, "error reading arbiter\n");
> >
> >         /* Put this port into reset */
> >         vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
> >                       VSC73XX_MAC_CFG_RESET);
> >
> >
> > I agree that VSC73XX_ARBDISC should be moved to phylink_mac_link_up.
>
> FWIW, ocelot_phylink_mac_link_down() also calls ocelot_port_flush()
> which is more or less the same procedure for a different piece of hw.
>
> By re-reading the commit message of eb4733d7cffc ("net: dsa: felix:
> implement port flushing on .phylink_mac_link_down"), I can find a good
> reason to flush the port on link down and not on link up. With flow
> control enabled, packets would remain in the queue system until there's
> link again if not flushed there, otherwise.
>
> Paweł, maybe it is simply the case that you should move the procedure
> from the datasheet into a more clearly named sub-function?
>

I will try to do it more clearly.

> > Other things could be optimised and it needs more care. (eg. This
> > implementation doesn't disable phy when the interface goes down.) I
> > plan to tweak it after the driver becomes usable. Please let me know
> > if it should be fixed in this patch.
>
> What do you mean by disabling the PHY when the interface goes down,
> exactly? Down as in administratively down, aka "ip link set swp0 down",
> not when the link drops?
>

I should be more precise.

> That's a thing for the PHY driver to handle, by implementing .suspend()
> and .resume(), I guess?
>

Yes, exactly.

> What driver do the internal PHYs use?

It is a PHY Vitesse VSC7385 from vitesse.c.
diff mbox series

Patch

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 8b2219404601..70dd3f96dafb 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -714,8 +714,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;
@@ -753,12 +752,11 @@  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_mac_config(struct dsa_switch *ds, int port,
+				       unsigned int mode,
+				       const struct phylink_link_state *state)
 {
 	struct vsc73xx *vsc = ds->priv;
-	u32 val;
-
 	/* Special handling of the CPU-facing port */
 	if (port == CPU_PORT) {
 		/* Other ports are already initialized but not this one */
@@ -774,101 +772,79 @@  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 ret, err;
-
-		dev_dbg(vsc->dev, "port %d: went down\n",
-			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 */
-		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)
-			dev_err(vsc->dev,
-				"timeout waiting for block arbiter\n");
-		else if (err < 0)
-			dev_err(vsc->dev, "error reading arbiter\n");
-
-		/* 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);
-
-		/* Allow backward dropping of frames from this port */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
-				    VSC73XX_SBACKWDROP, BIT(port), BIT(port));
-
-		/* Receive mask (disable forwarding) */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
-				    VSC73XX_RECVMASK, BIT(port), 0);
+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;
+	int ret, err;
+	u32 val;
 
-		return;
-	}
+	/* Disable RX on this port */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+			    VSC73XX_MAC_CFG,
+			    VSC73XX_MAC_CFG_RX_EN, 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);
-
-		/* Set up default for internal port or external RGMII */
-		if (phydev->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) {
-			val = VSC73XX_MAC_CFG_100_10M_F_PHY;
-			dev_dbg(vsc->dev,
-				"port %d: 10 Mbit full duplex mode\n",
-				port);
-		} 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 {
+	/* Discard packets */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+			    VSC73XX_ARBDISC, BIT(port), BIT(port));
+
+	/* Wait until queue is empty */
+	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)
 		dev_err(vsc->dev,
-			"could not adjust link: unknown speed\n");
-	}
+			"timeout waiting for block arbiter\n");
+	else if (err < 0)
+		dev_err(vsc->dev, "error reading arbiter\n");
+
+	/* 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);
+
+	/* Allow backward dropping of frames from this port */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+			    VSC73XX_SBACKWDROP, BIT(port), BIT(port));
+
+	/* Receive mask (disable forwarding) */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+			    VSC73XX_RECVMASK, BIT(port), 0);
+}
+
+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;
+
+	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;
 
 	/* 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,
@@ -1055,7 +1031,9 @@  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_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,