diff mbox series

[net-next,v3,2/8] net: dsa: vsc73xx: convert to PHYLINK

Message ID 20230912122201.3752918-3-paweldembicki@gmail.com (mailing list archive)
State Changes Requested
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/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: 1340 this patch: 1340
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 1363 this patch: 1363
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: 1363 this patch: 1363
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 224 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pawel Dembicki Sept. 12, 2023, 12:21 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_get_caps
.phylink_mac_link_down
.phylink_mac_link_up
.phylink_mac_link_down

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
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 | 190 +++++++++++++------------
 1 file changed, 96 insertions(+), 94 deletions(-)

Comments

Russell King (Oracle) Sept. 12, 2023, 4:49 p.m. UTC | #1 
On Tue, Sep 12, 2023 at 02:21:56PM +0200, Pawel Dembicki wrote:
> +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;

I know the original code tested against PHY_INTERFACE_MODE_RGMII, but
is this correct, or should it be:

	if (phy_interface_is_rgmii(interface))

since the various RGMII* modes are used to determine the delay on the
PHY side.

Even so, I don't think that is a matter for this patch, but a future
(or maybe a preceeding patch) to address.

Other than that, I think it looks okay.

Thanks.
Vladimir Oltean Sept. 26, 2023, 11:03 p.m. UTC | #2 
On Tue, Sep 12, 2023 at 05:49:36PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 12, 2023 at 02:21:56PM +0200, Pawel Dembicki wrote:
> > +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;
> 
> I know the original code tested against PHY_INTERFACE_MODE_RGMII, but
> is this correct, or should it be:
> 
> 	if (phy_interface_is_rgmii(interface))
> 
> since the various RGMII* modes are used to determine the delay on the
> PHY side.
> 
> Even so, I don't think that is a matter for this patch, but a future
> (or maybe a preceeding patch) to address.
> 
> Other than that, I think it looks okay.
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

I also agree with adding one more patch to this which converts to
phy_interface_is_rgmii(). Paweł: there was a recent discussion about
the (ir)relevance of the specific rgmii phy-mode in fixed-link here.
https://lore.kernel.org/netdev/ZNpEaMJjmDqhK1dW@shell.armlinux.org.uk/
Pawel Dembicki Oct. 3, 2023, 8:45 p.m. UTC | #3 
śr., 27 wrz 2023 o 01:03 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Tue, Sep 12, 2023 at 05:49:36PM +0100, Russell King (Oracle) wrote:
> > On Tue, Sep 12, 2023 at 02:21:56PM +0200, Pawel Dembicki wrote:
> > > +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;
> >
> > I know the original code tested against PHY_INTERFACE_MODE_RGMII, but
> > is this correct, or should it be:
> >
> >       if (phy_interface_is_rgmii(interface))
> >
> > since the various RGMII* modes are used to determine the delay on the
> > PHY side.
> >
> > Even so, I don't think that is a matter for this patch, but a future
> > (or maybe a preceeding patch) to address.
> >
> > Other than that, I think it looks okay.
> >
> > Thanks.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
>
> I also agree with adding one more patch to this which converts to
> phy_interface_is_rgmii(). Paweł: there was a recent discussion about
> the (ir)relevance of the specific rgmii phy-mode in fixed-link here.
> https://lore.kernel.org/netdev/ZNpEaMJjmDqhK1dW@shell.armlinux.org.uk/

I plan to make rgmii delays configurable from the device tree. Should I?
a. switch to phy_interface_is_rgmii in the current patch?
b. add another patch in this series?
c. wait with change to phy_interface_is_rgmii for patch with rgmii
delays configuration?
Vladimir Oltean Oct. 3, 2023, 9:14 p.m. UTC | #4 
Hi Paweł,

On Tue, Oct 03, 2023 at 10:45:45PM +0200, Paweł Dembicki wrote:
> I plan to make rgmii delays configurable from the device tree. Should I?
> a. switch to phy_interface_is_rgmii in the current patch?
> b. add another patch in this series?
> c. wait with change to phy_interface_is_rgmii for patch with rgmii
> delays configuration?

If you want to configure the RGMII delays in the vsc73xx MAC, you should
look at the "rx-internal-delay-ps" and "tx-internal-delay-ps" properties
in the MAC OF node, rather than at the phy-mode. The phy-mode is for the
internal delays from the PHY.

In any case, you should accept any phy_interface_is_rgmii() regardless
of whether internal delays are configurable in the MAC. And yes, parsing
those MAC OF properties should be a separate change.

If the series exceeds 15 patches, I would consider splitting it per
topic and submitting separate series (link management would be one,
tag_8021q/bridging/VLAN would be another). If they don't conflict and
can be applied independently, you could also send the 2 series
simultaneously.
Andrew Lunn Oct. 3, 2023, 9:32 p.m. UTC | #5 
> I plan to make rgmii delays configurable from the device tree. Should I?
> a. switch to phy_interface_is_rgmii in the current patch?
> b. add another patch in this series?
> c. wait with change to phy_interface_is_rgmii for patch with rgmii
> delays configuration?

Do you actually need this feature? Does the PHY you are using already
support fine tuning of the delays?

	Andrew
Pawel Dembicki Oct. 3, 2023, 9:50 p.m. UTC | #6 
wt., 3 paź 2023 o 23:32 Andrew Lunn <andrew@lunn.ch> napisał(a):
>
> > I plan to make rgmii delays configurable from the device tree. Should I?
> > a. switch to phy_interface_is_rgmii in the current patch?
> > b. add another patch in this series?
> > c. wait with change to phy_interface_is_rgmii for patch with rgmii
> > delays configuration?
>
> Do you actually need this feature? Does the PHY you are using already
> support fine tuning of the delays?
>

After Vladimir answer I know that it should be a separate change.
I need it for MAC <-> switch connection, the rgmii port connected to
the cpu. At this moment, rgmii delays are hardcoded in vsc73xx_setup
and it should be tunable for the p2020rdb board. I plan to work on it
after the driver becomes usable.
diff mbox series

Patch

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index b117c0c18e36..39d3d78f4bc3 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,34 @@  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);
+	}
+}
+
+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 */
@@ -774,101 +795,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,
@@ -1039,7 +1038,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,