[PATCH net-next 0/9] net: dsa: vsc73xx: fix MDIO bus access and PHY operations

[PATCH net-next 0/9] net: dsa: vsc73xx: fix MDIO bus access and PHY operations

[Pawel Dembicki]

The VSC73xx driver has issues with PHY configuration. This patch series
fixes most of them.

The first patch fixes the phylink capabilities, because the MAC in the
vsc73xx family doesn't handle 1000BASE HD mode.

The second patch synchronizes the register configuration routine with the
datasheet recommendations.

Patches 3-5 restore proper communication on the MDIO bus. Currently,
the write value isn't sent to the MDIO register, and without a mutex,
communication with the PHY can be interrupted. This causes the PHY to
receive improper configuration and autonegotiation could fail.

The sixth patch speeds up the internal MDIO bus to the maximum value
allowed by the datasheet.

The seventh patch removes the PHY reset blockade, as it is no longer
required.

After fixing the MDIO operations, autonegotiation became possible.
The eighth patch removes the blockade, which became unnecessary after
the MDIO operations fix. It also enables the MDI-X feature, which is
disabled by default in forced 100BASE-TX mode like other Vitesse PHYs.

The last patch implements the downshift feature and enables it by default.

Pawel Dembicki (9):
  net: dsa: vsc73xx: fix phylink capabilities
  net: dsa: vsc73xx: fix port MAC configuration in full duplex mode
  net: dsa: vsc73xx: pass value in phy_write operation
  net: dsa: vsc73xx: use defined values in phy operations
  net: dsa: vsc73xx: use mutex to mdio operations
  net: dsa: vsc73xx: speed up mdio bus to max allowed value
  net: dsa: vsc73xx: allow phy resetting
  net: phy: vitesse: repair vsc73xx autonegotiation
  net: phy: vitesse: implement downshift in vsc73xx phys

 drivers/net/dsa/vitesse-vsc73xx-core.c | 132 ++++++++++++++++++-------
 drivers/net/dsa/vitesse-vsc73xx.h      |   2 +
 drivers/net/phy/vitesse.c              | 125 +++++++++++++++++++++--
 3 files changed, 219 insertions(+), 40 deletions(-)

-- 
2.34.1

[PATCH net-next 1/9] net: dsa: vsc73xx: fix phylink capabilities

[Pawel Dembicki]

According datasheet, VSC73XX family switches supports symmetric and
asymmetric pause and 1000BASE in FD only.

This patch fix it.

Fixes: a026809c261b ("net: dsa: vsc73xx: add phylink capabilities")

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 07b704a1557e..43aeb578d608 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1491,7 +1491,8 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port,
 		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
 	}
 
-	config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000;
+	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
+				   MAC_10 | MAC_100 | MAC_1000FD;
 }
 
 static int
-- 
2.34.1

Re: [PATCH net-next 1/9] net: dsa: vsc73xx: fix phylink capabilities

[Andrew Lunn]

On Mon, Jul 29, 2024 at 11:06:07PM +0200, Pawel Dembicki wrote:
> According datasheet, VSC73XX family switches supports symmetric and
> asymmetric pause and 1000BASE in FD only.
> 
> This patch fix it.
> 
> Fixes: a026809c261b ("net: dsa: vsc73xx: add phylink capabilities")
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

No blank line between tags please.

   Andrew

Re: [PATCH net-next 1/9] net: dsa: vsc73xx: fix phylink capabilities

[Russell King (Oracle)]

On Mon, Jul 29, 2024 at 11:06:07PM +0200, Pawel Dembicki wrote:
> According datasheet, VSC73XX family switches supports symmetric and
> asymmetric pause and 1000BASE in FD only.

What about the configuration of the pause? I notice the mac_link_up()
method ignores tx_pause/rx_pause, and instead just does this:

        /* Flow control for the PHY facing ports:
         * Use a zero delay pause frame when pause condition is left
         * Obey pause control frames
         * When generating pause frames, use 0xff as pause value
         */
        vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_FCCONF,
                      VSC73XX_FCCONF_ZERO_PAUSE_EN |
                      VSC73XX_FCCONF_FLOW_CTRL_OBEY |
                      0xff);

which suggests only symmetric pause is supported by the driver _and_
it is always enabled irrespective of what was negotiated.

I think this needs to be fixed first, before changing the capabilities.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 2/9] net: dsa: vsc73xx: fix port MAC configuration in full duplex mode

[Pawel Dembicki]

According to the datasheet description ("Port Mode Procedure" in 5.6.2),
the VSC73XX_MAC_CFG_WEXC_DIS bit is configured only for half duplex mode.

The WEXC_DIS bit is responsible for MAC behavior after an excessive
collision. Let's set it as described in the datasheet.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 43aeb578d608..9bd186af8941 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1019,6 +1019,11 @@ static void vsc73xx_mac_link_up(struct phylink_config *config,
 
 	if (duplex == DUPLEX_FULL)
 		val |= VSC73XX_MAC_CFG_FDX;
+	else
+		/* In datasheet description ("Port Mode Procedure" in 5.6.2)
+		 * this bit is configured only for half duplex.
+		 */
+		val |= VSC73XX_MAC_CFG_WEXC_DIS;
 
 	/* This routine is described in the datasheet (below ARBDISC register
 	 * description)
@@ -1029,7 +1034,6 @@ static void vsc73xx_mac_link_up(struct phylink_config *config,
 	get_random_bytes(&seed, 1);
 	val |= seed << VSC73XX_MAC_CFG_SEED_OFFSET;
 	val |= VSC73XX_MAC_CFG_SEED_LOAD;
-	val |= VSC73XX_MAC_CFG_WEXC_DIS;
 
 	/* Those bits are responsible for MTU only. Kernel takes care about MTU,
 	 * let's enable +8 bytes frame length unconditionally.
-- 
2.34.1

[PATCH net-next 3/9] net: dsa: vsc73xx: pass value in phy_write operation

[Pawel Dembicki]

In the 'vsc73xx_phy_write' function, the register value is missing,
and the phy write operation always sends zeros.

This commit passes the value variable into the proper register.

Fixes: 975ae7c69d51 ("net: phy: vitesse: Add support for VSC73xx")

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 9bd186af8941..e5466396669d 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -574,7 +574,7 @@ static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
 		return 0;
 	}
 
-	cmd = (phy << 21) | (regnum << 16);
+	cmd = (phy << 21) | (regnum << 16) | val;
 	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_MII, 0, 1, cmd);
 	if (ret)
 		return ret;
-- 
2.34.1

[PATCH net-next 4/9] net: dsa: vsc73xx: use defined values in phy operations

[Pawel Dembicki]

This commit changes magic numbers in phy operations.
Some shifted registers was replaced with bitfield macros.

No functional changes done.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 45 ++++++++++++++++++++------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index e5466396669d..5eb37dee2261 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -21,6 +21,7 @@
 #include <linux/of.h>
 #include <linux/of_mdio.h>
 #include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/if_bridge.h>
 #include <linux/if_vlan.h>
 #include <linux/etherdevice.h>
@@ -40,6 +41,10 @@
 #define VSC73XX_BLOCK_ARBITER	0x5 /* Only subblock 0 */
 #define VSC73XX_BLOCK_SYSTEM	0x7 /* Only subblock 0 */
 
+/* MII Block subblock */
+#define VSC73XX_BLOCK_MII_INTERNAL	0x0 /* Internal MDIO subblock */
+#define VSC73XX_BLOCK_MII_EXTERNAL	0x1 /* External MDIO subblock */
+
 #define CPU_PORT	6 /* CPU port */
 
 /* MAC Block registers */
@@ -221,9 +226,22 @@
 #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE	3
 
 /* MII block 3 registers */
-#define VSC73XX_MII_STAT	0x0
-#define VSC73XX_MII_CMD		0x1
-#define VSC73XX_MII_DATA	0x2
+#define VSC73XX_MII_STAT		0x0
+#define VSC73XX_MII_CMD			0x1
+#define VSC73XX_MII_DATA		0x2
+
+#define VSC73XX_MII_STAT_BUSY		BIT(3)
+#define VSC73XX_MII_STAT_READ		BIT(2)
+#define VSC73XX_MII_STAT_WRITE		BIT(1)
+
+#define VSC73XX_MII_CMD_SCAN		BIT(27)
+#define VSC73XX_MII_CMD_OPERATION	BIT(26)
+#define VSC73XX_MII_CMD_PHY_ADDR	GENMASK(25, 21)
+#define VSC73XX_MII_CMD_PHY_REG		GENMASK(20, 16)
+#define VSC73XX_MII_CMD_WRITE_DATA	GENMASK(15, 0)
+
+#define VSC73XX_MII_DATA_FAILURE	BIT(16)
+#define VSC73XX_MII_DATA_READ_DATA	GENMASK(15, 0)
 
 /* Arbiter block 5 registers */
 #define VSC73XX_ARBEMPTY		0x0c
@@ -535,20 +553,24 @@ static int vsc73xx_phy_read(struct dsa_switch *ds, int phy, int regnum)
 	int ret;
 
 	/* Setting bit 26 means "read" */
-	cmd = BIT(26) | (phy << 21) | (regnum << 16);
-	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_MII, 0, 1, cmd);
+	cmd = VSC73XX_MII_CMD_OPERATION |
+	      FIELD_PREP(VSC73XX_MII_CMD_PHY_ADDR, phy) |
+	      FIELD_PREP(VSC73XX_MII_CMD_PHY_REG, regnum);
+	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
+			    VSC73XX_MII_CMD, cmd);
 	if (ret)
 		return ret;
 	msleep(2);
-	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_MII, 0, 2, &val);
+	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
+			   VSC73XX_MII_DATA, &val);
 	if (ret)
 		return ret;
-	if (val & BIT(16)) {
+	if (val & VSC73XX_MII_DATA_FAILURE) {
 		dev_err(vsc->dev, "reading reg %02x from phy%d failed\n",
 			regnum, phy);
 		return -EIO;
 	}
-	val &= 0xFFFFU;
+	val &= VSC73XX_MII_DATA_READ_DATA;
 
 	dev_dbg(vsc->dev, "read reg %02x from phy%d = %04x\n",
 		regnum, phy, val);
@@ -574,8 +596,11 @@ static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
 		return 0;
 	}
 
-	cmd = (phy << 21) | (regnum << 16) | val;
-	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_MII, 0, 1, cmd);
+	cmd = FIELD_PREP(VSC73XX_MII_CMD_PHY_ADDR, phy) |
+	      FIELD_PREP(VSC73XX_MII_CMD_PHY_REG, regnum) |
+	      FIELD_PREP(VSC73XX_MII_CMD_WRITE_DATA, val);
+	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
+			    VSC73XX_MII_CMD, cmd);
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH net-next 5/9] net: dsa: vsc73xx: use mutex to mdio operations

[Pawel Dembicki]

vsc73xx needs mutex during mdio bus access to avoid races. Without it,
phys are misconfigured and bus operations aren't work as expected.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 59 ++++++++++++++++++++------
 drivers/net/dsa/vitesse-vsc73xx.h      |  2 +
 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 5eb37dee2261..40c64ef7e729 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -545,6 +545,18 @@ static int vsc73xx_detect(struct vsc73xx *vsc)
 	return 0;
 }
 
+static int vsc73xx_mdio_busy_check(struct vsc73xx *vsc)
+{
+	int val, ret;
+
+	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
+			   VSC73XX_MII_STAT, &val);
+	if (ret)
+		return ret;
+
+	return (val & VSC73XX_MII_STAT_BUSY) ? -EBUSY : 0;
+}
+
 static int vsc73xx_phy_read(struct dsa_switch *ds, int phy, int regnum)
 {
 	struct vsc73xx *vsc = ds->priv;
@@ -552,6 +564,12 @@ static int vsc73xx_phy_read(struct dsa_switch *ds, int phy, int regnum)
 	u32 val;
 	int ret;
 
+	mutex_lock(&vsc->mdio_lock);
+
+	ret = vsc73xx_mdio_busy_check(vsc);
+	if (ret)
+		goto err;
+
 	/* Setting bit 26 means "read" */
 	cmd = VSC73XX_MII_CMD_OPERATION |
 	      FIELD_PREP(VSC73XX_MII_CMD_PHY_ADDR, phy) |
@@ -559,23 +577,27 @@ static int vsc73xx_phy_read(struct dsa_switch *ds, int phy, int regnum)
 	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
 			    VSC73XX_MII_CMD, cmd);
 	if (ret)
-		return ret;
-	msleep(2);
+		goto err;
+	usleep_range(100, 200);
 	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
 			   VSC73XX_MII_DATA, &val);
 	if (ret)
-		return ret;
+		goto err;
+
 	if (val & VSC73XX_MII_DATA_FAILURE) {
 		dev_err(vsc->dev, "reading reg %02x from phy%d failed\n",
 			regnum, phy);
-		return -EIO;
+		ret = -EIO;
+		goto err;
 	}
-	val &= VSC73XX_MII_DATA_READ_DATA;
+	ret = val & VSC73XX_MII_DATA_READ_DATA;
 
 	dev_dbg(vsc->dev, "read reg %02x from phy%d = %04x\n",
-		regnum, phy, val);
+		regnum, phy, ret);
 
-	return val;
+err:
+	mutex_unlock(&vsc->mdio_lock);
+	return ret;
 }
 
 static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
@@ -583,7 +605,13 @@ static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
 {
 	struct vsc73xx *vsc = ds->priv;
 	u32 cmd;
-	int ret;
+	int ret = 0;
+
+	mutex_lock(&vsc->mdio_lock);
+
+	ret = vsc73xx_mdio_busy_check(vsc);
+	if (ret)
+		goto err;
 
 	/* It was found through tedious experiments that this router
 	 * chip really hates to have it's PHYs reset. They
@@ -601,12 +629,13 @@ static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	      FIELD_PREP(VSC73XX_MII_CMD_WRITE_DATA, val);
 	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
 			    VSC73XX_MII_CMD, cmd);
-	if (ret)
-		return ret;
-
-	dev_dbg(vsc->dev, "write %04x to reg %02x in phy%d\n",
-		val, regnum, phy);
-	return 0;
+	if (!ret)
+		dev_dbg(vsc->dev, "write %04x to reg %02x in phy%d\n",
+			val, regnum, phy);
+	usleep_range(100, 200);
+err:
+	mutex_unlock(&vsc->mdio_lock);
+	return ret;
 }
 
 static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
@@ -1973,6 +2002,8 @@ int vsc73xx_probe(struct vsc73xx *vsc)
 		return -ENODEV;
 	}
 
+	mutex_init(&vsc->mdio_lock);
+
 	eth_random_addr(vsc->addr);
 	dev_info(vsc->dev,
 		 "MAC for control frames: %02X:%02X:%02X:%02X:%02X:%02X\n",
diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
index 3ca579acc798..fc0fa8f5f57c 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -45,6 +45,7 @@ struct vsc73xx_portinfo {
  * @vlans: List of configured vlans. Contains port mask and untagged status of
  *	every vlan configured in port vlan operation. It doesn't cover tag_8021q
  *	vlans.
+ * @mdio_lock: Mutex used in mdio operations
  */
 struct vsc73xx {
 	struct device			*dev;
@@ -57,6 +58,7 @@ struct vsc73xx {
 	void				*priv;
 	struct vsc73xx_portinfo		portinfo[VSC73XX_MAX_NUM_PORTS];
 	struct list_head		vlans;
+	struct mutex			mdio_lock;
 };
 
 /**
-- 
2.34.1

Re: [PATCH net-next 5/9] net: dsa: vsc73xx: use mutex to mdio operations

[Andrew Lunn]

On Mon, Jul 29, 2024 at 11:06:11PM +0200, Pawel Dembicki wrote:
> vsc73xx needs mutex during mdio bus access to avoid races. Without it,
> phys are misconfigured and bus operations aren't work as expected.

This is adding much more than a mutex.

The mdio core already has a mutex, so there should not be multiple
parallel operations going on. So i don't think the mutex itself is the
fix. It is more likely to be a change in the timing. Which in itself
is not good. Maybe use your new vsc73xx_mdio_busy_check() with one of
the helpers in include/linux/iopoll.h

    Andrew

---
pw-bot: cr

[PATCH net-next 6/9] net: dsa: vsc73xx: speed up mdio bus to max allowed value

[Pawel Dembicki]

According the datasheet, vsc73xx family max internal mdio bus speed is
20MHz. It also allow to disable preamble.

This commit sets mdio clock prescaler to minimal value and crop preamble
to speed up mdio operations.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 40c64ef7e729..9e88eda6f4dd 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -229,6 +229,7 @@
 #define VSC73XX_MII_STAT		0x0
 #define VSC73XX_MII_CMD			0x1
 #define VSC73XX_MII_DATA		0x2
+#define VSC73XX_MII_MPRES		0x3
 
 #define VSC73XX_MII_STAT_BUSY		BIT(3)
 #define VSC73XX_MII_STAT_READ		BIT(2)
@@ -243,6 +244,10 @@
 #define VSC73XX_MII_DATA_FAILURE	BIT(16)
 #define VSC73XX_MII_DATA_READ_DATA	GENMASK(15, 0)
 
+#define VSC73XX_MII_MPRES_NOPREAMBLE	BIT(6)
+#define VSC73XX_MII_MPRES_PRESCALEVAL	GENMASK(5, 0)
+#define VSC73XX_MII_PRESCALEVAL_MIN	3 /* min allowed mdio clock prescaler */
+
 /* Arbiter block 5 registers */
 #define VSC73XX_ARBEMPTY		0x0c
 #define VSC73XX_ARBDISC			0x0e
@@ -578,7 +583,7 @@ static int vsc73xx_phy_read(struct dsa_switch *ds, int phy, int regnum)
 			    VSC73XX_MII_CMD, cmd);
 	if (ret)
 		goto err;
-	usleep_range(100, 200);
+	usleep_range(4, 100);
 	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
 			   VSC73XX_MII_DATA, &val);
 	if (ret)
@@ -632,7 +637,7 @@ static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	if (!ret)
 		dev_dbg(vsc->dev, "write %04x to reg %02x in phy%d\n",
 			val, regnum, phy);
-	usleep_range(100, 200);
+	usleep_range(4, 100);
 err:
 	mutex_unlock(&vsc->mdio_lock);
 	return ret;
@@ -802,7 +807,7 @@ static int vsc73xx_configure_rgmii_port_delay(struct dsa_switch *ds)
 static int vsc73xx_setup(struct dsa_switch *ds)
 {
 	struct vsc73xx *vsc = ds->priv;
-	int i, ret;
+	int i, ret, val;
 
 	dev_info(vsc->dev, "set up the switch\n");
 
@@ -875,6 +880,15 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 
 	mdelay(50);
 
+	/* Disable preamble and use maximum allowed clock for the internal
+	 * mdio bus, used for communication with internal PHYs only.
+	 */
+	val = VSC73XX_MII_MPRES_NOPREAMBLE |
+	      FIELD_PREP(VSC73XX_MII_MPRES_PRESCALEVAL,
+			 VSC73XX_MII_PRESCALEVAL_MIN);
+	vsc73xx_write(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
+		      VSC73XX_MII_MPRES, val);
+
 	/* Release reset from the internal PHYs */
 	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
 		      VSC73XX_GLORESET_PHY_RESET);
-- 
2.34.1

Re: [PATCH net-next 6/9] net: dsa: vsc73xx: speed up mdio bus to max allowed value

[Andrew Lunn]

On Mon, Jul 29, 2024 at 11:06:12PM +0200, Pawel Dembicki wrote:
> According the datasheet, vsc73xx family max internal mdio bus speed is
> 20MHz. It also allow to disable preamble.
> 
> This commit sets mdio clock prescaler to minimal value and crop preamble
> to speed up mdio operations.

Just checking...

This has no effect on the external MDIO bus, correct. It has its own
set of registers for the divider and to crop the preamble.

    Andrew

Re: [PATCH net-next 6/9] net: dsa: vsc73xx: speed up mdio bus to max allowed value

[Paweł Dembicki]

wt., 30 lip 2024 o 01:10 Andrew Lunn <andrew@lunn.ch> napisał(a):
>
> On Mon, Jul 29, 2024 at 11:06:12PM +0200, Pawel Dembicki wrote:
> > According the datasheet, vsc73xx family max internal mdio bus speed is
> > 20MHz. It also allow to disable preamble.
> >
> > This commit sets mdio clock prescaler to minimal value and crop preamble
> > to speed up mdio operations.
>
> Just checking...
>
> This has no effect on the external MDIO bus, correct. It has its own
> set of registers for the divider and to crop the preamble.

Yes. It's configured in a completely different subblock. Internal and
external mdio buses have symmetrical register set. It can be
configured separately.

-- 
Best regards,
Pawel Dembicki

Re: [PATCH net-next 6/9] net: dsa: vsc73xx: speed up mdio bus to max allowed value

[Andrew Lunn]

> Yes. It's configured in a completely different subblock. Internal and
> external mdio buses have symmetrical register set. It can be
> configured separately.

O.K. Great.

For the external block, there are well defined DT properties for the
bus speed, and suppressing the pre-amble. They should be used. For the
internal block, i don't see the need, you can hard coded them, they
either work, or they don't.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 7/9] net: dsa: vsc73xx: allow phy resetting

[Pawel Dembicki]

Now, phy reset isn't a problem for vsc73xx switches.
'soft_reset' can be done normally.

This commit removes the reset blockade in the 'vsc73xx_phy_write'
function.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 9e88eda6f4dd..df36118644f2 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -618,17 +618,6 @@ static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	if (ret)
 		goto err;
 
-	/* It was found through tedious experiments that this router
-	 * chip really hates to have it's PHYs reset. They
-	 * never recover if that happens: autonegotiation stops
-	 * working after a reset. Just filter out this command.
-	 * (Resetting the whole chip is OK.)
-	 */
-	if (regnum == 0 && (val & BIT(15))) {
-		dev_info(vsc->dev, "reset PHY - disallowed\n");
-		return 0;
-	}
-
 	cmd = FIELD_PREP(VSC73XX_MII_CMD_PHY_ADDR, phy) |
 	      FIELD_PREP(VSC73XX_MII_CMD_PHY_REG, regnum) |
 	      FIELD_PREP(VSC73XX_MII_CMD_WRITE_DATA, val);
-- 
2.34.1

[PATCH net-next 8/9] net: phy: vitesse: repair vsc73xx autonegotiation

[Pawel Dembicki]

When the vsc73xx mdio bus work properly, the generic autonegotiation
configuration works well.

Vsc73xx have auto MDI-X disabled by default in forced mode. This commit
enables it.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/phy/vitesse.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 897b979ec03c..19b7bf189be5 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -60,6 +60,11 @@
 /* Vitesse Extended Page Access Register */
 #define MII_VSC82X4_EXT_PAGE_ACCESS	0x1f
 
+/* VSC73XX PHY_BYPASS_CTRL register*/
+#define MII_VSC73XX_PHY_BYPASS_CTRL		MII_DCOUNTER
+#define MII_PBC_FORCED_SPEED_AUTO_MDIX_DIS	BIT(7)
+#define MII_VSC73XX_PBC_AUTO_NP_EXCHANGE_DIS	BIT(1)
+
 /* Vitesse VSC8601 Extended PHY Control Register 1 */
 #define MII_VSC8601_EPHY_CTL		0x17
 #define MII_VSC8601_EPHY_CTL_RGMII_SKEW	(1 << 8)
@@ -239,12 +244,20 @@ static int vsc739x_config_init(struct phy_device *phydev)
 
 static int vsc73xx_config_aneg(struct phy_device *phydev)
 {
-	/* The VSC73xx switches does not like to be instructed to
-	 * do autonegotiation in any way, it prefers that you just go
-	 * with the power-on/reset defaults. Writing some registers will
-	 * just make autonegotiation permanently fail.
-	 */
-	return 0;
+	int ret;
+
+	/* Enable Auto MDI-X in forced 10/100 mode */
+	if (phydev->autoneg != AUTONEG_ENABLE && phydev->speed <= SPEED_100) {
+		ret = genphy_setup_forced(phydev);
+
+		if (ret < 0) /* error */
+			return ret;
+
+		return phy_clear_bits(phydev, MII_VSC73XX_PHY_BYPASS_CTRL,
+				      MII_PBC_FORCED_SPEED_AUTO_MDIX_DIS);
+	}
+
+	return genphy_config_aneg(phydev);
 }
 
 /* This adds a skew for both TX and RX clocks, so the skew should only be
-- 
2.34.1

[PATCH net-next 9/9] net: phy: vitesse: implement downshift in vsc73xx phys

[Pawel Dembicki]

This commit implements downshift feature in vsc73xx family phys.

By default downshift was enabled with maximum tries.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/phy/vitesse.c | 100 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 19b7bf189be5..9228c6652627 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -10,8 +10,10 @@
 #include <linux/mii.h>
 #include <linux/ethtool.h>
 #include <linux/phy.h>
+#include <linux/bitfield.h>
 
 /* Vitesse Extended Page Magic Register(s) */
+#define MII_VSC73XX_EXT_PAGE_1E		0x01
 #define MII_VSC82X4_EXT_PAGE_16E	0x10
 #define MII_VSC82X4_EXT_PAGE_17E	0x11
 #define MII_VSC82X4_EXT_PAGE_18E	0x12
@@ -60,6 +62,15 @@
 /* Vitesse Extended Page Access Register */
 #define MII_VSC82X4_EXT_PAGE_ACCESS	0x1f
 
+/* Vitesse VSC73XX Extended Control Register */
+#define MII_VSC73XX_PHY_CTRL_EXT3		0x14
+
+#define MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN	BIT(4)
+#define MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT	GENMASK(3, 2)
+#define MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_STA	BIT(1)
+#define MII_VSC73XX_DOWNSHIFT_MAX		5
+#define MII_VSC73XX_DOWNSHIFT_INVAL		1
+
 /* VSC73XX PHY_BYPASS_CTRL register*/
 #define MII_VSC73XX_PHY_BYPASS_CTRL		MII_DCOUNTER
 #define MII_PBC_FORCED_SPEED_AUTO_MDIX_DIS	BIT(7)
@@ -133,6 +144,84 @@ static int vsc73xx_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, VSC73XX_EXT_PAGE_ACCESS, page);
 }
 
+static int vsc73xx_get_downshift(struct phy_device *phydev, u8 *data)
+{
+	int page, val, cnt, enable;
+
+	page = phy_select_page(phydev, MII_VSC73XX_EXT_PAGE_1E);
+	if (page < 0)
+		return page;
+
+	val = __phy_read(phydev, MII_VSC73XX_PHY_CTRL_EXT3);
+	if (val < 0)
+		goto restore_page;
+
+	enable = FIELD_GET(MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN, val);
+	cnt = FIELD_GET(MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT, val) + 2;
+
+	*data = enable ? cnt : DOWNSHIFT_DEV_DISABLE;
+
+restore_page:
+	phy_restore_page(phydev, page, val);
+	return 0;
+}
+
+static int vsc73xx_set_downshift(struct phy_device *phydev, u8 cnt)
+{
+	int page, ret, val;
+
+	if (cnt > MII_VSC73XX_DOWNSHIFT_MAX)
+		return -E2BIG;
+	else if (cnt == MII_VSC73XX_DOWNSHIFT_INVAL)
+		return -EINVAL;
+
+	page = phy_select_page(phydev, MII_VSC73XX_EXT_PAGE_1E);
+	if (page < 0)
+		return page;
+
+	if (!cnt) {
+		ret = __phy_clear_bits(phydev, MII_VSC73XX_PHY_CTRL_EXT3,
+				       MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN);
+	} else {
+		val = MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN |
+		      FIELD_PREP(MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT,
+				 cnt - 2);
+
+		ret = __phy_modify(phydev, MII_VSC73XX_PHY_CTRL_EXT3,
+				   MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN |
+				   MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT,
+				   val);
+	}
+
+	ret = phy_restore_page(phydev, page, ret);
+	if (ret < 0)
+		return ret;
+
+	return genphy_soft_reset(phydev);
+}
+
+static int vsc73xx_get_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return vsc73xx_get_downshift(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int vsc73xx_set_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return vsc73xx_set_downshift(phydev, *(const u8 *)data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static void vsc73xx_config_init(struct phy_device *phydev)
 {
 	/* Receiver init */
@@ -142,6 +231,9 @@ static void vsc73xx_config_init(struct phy_device *phydev)
 
 	/* Config LEDs 0x61 */
 	phy_modify(phydev, MII_TPISTATUS, 0xff00, 0x0061);
+
+	/* Enable downshift by default */
+	vsc73xx_set_downshift(phydev, MII_VSC73XX_DOWNSHIFT_MAX);
 }
 
 static int vsc738x_config_init(struct phy_device *phydev)
@@ -460,6 +552,8 @@ static struct phy_driver vsc82xx_driver[] = {
 	.config_aneg    = vsc73xx_config_aneg,
 	.read_page      = vsc73xx_read_page,
 	.write_page     = vsc73xx_write_page,
+	.get_tunable    = vsc73xx_get_tunable,
+	.set_tunable    = vsc73xx_set_tunable,
 }, {
 	.phy_id         = PHY_ID_VSC7388,
 	.name           = "Vitesse VSC7388",
@@ -469,6 +563,8 @@ static struct phy_driver vsc82xx_driver[] = {
 	.config_aneg    = vsc73xx_config_aneg,
 	.read_page      = vsc73xx_read_page,
 	.write_page     = vsc73xx_write_page,
+	.get_tunable    = vsc73xx_get_tunable,
+	.set_tunable    = vsc73xx_set_tunable,
 }, {
 	.phy_id         = PHY_ID_VSC7395,
 	.name           = "Vitesse VSC7395",
@@ -478,6 +574,8 @@ static struct phy_driver vsc82xx_driver[] = {
 	.config_aneg    = vsc73xx_config_aneg,
 	.read_page      = vsc73xx_read_page,
 	.write_page     = vsc73xx_write_page,
+	.get_tunable    = vsc73xx_get_tunable,
+	.set_tunable    = vsc73xx_set_tunable,
 }, {
 	.phy_id         = PHY_ID_VSC7398,
 	.name           = "Vitesse VSC7398",
@@ -487,6 +585,8 @@ static struct phy_driver vsc82xx_driver[] = {
 	.config_aneg    = vsc73xx_config_aneg,
 	.read_page      = vsc73xx_read_page,
 	.write_page     = vsc73xx_write_page,
+	.get_tunable    = vsc73xx_get_tunable,
+	.set_tunable    = vsc73xx_set_tunable,
 }, {
 	.phy_id         = PHY_ID_VSC8662,
 	.name           = "Vitesse VSC8662",
-- 
2.34.1

Re: [PATCH net-next 9/9] net: phy: vitesse: implement downshift in vsc73xx phys

[Russell King (Oracle)]

On Mon, Jul 29, 2024 at 11:06:15PM +0200, Pawel Dembicki wrote:
> +static int vsc73xx_get_downshift(struct phy_device *phydev, u8 *data)
> +{
> +	int page, val, cnt, enable;
> +
> +	page = phy_select_page(phydev, MII_VSC73XX_EXT_PAGE_1E);
> +	if (page < 0)
> +		return page;
> +
> +	val = __phy_read(phydev, MII_VSC73XX_PHY_CTRL_EXT3);
> +	if (val < 0)
> +		goto restore_page;
> +
> +	enable = FIELD_GET(MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN, val);
> +	cnt = FIELD_GET(MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT, val) + 2;
> +
> +	*data = enable ? cnt : DOWNSHIFT_DEV_DISABLE;
> +
> +restore_page:
> +	phy_restore_page(phydev, page, val);
> +	return 0;

If you're going to use phy_select_page()..phy_restore_page(), please
read the documentation. If the __phy_read() fails, then you're
returning zero from this function, but leaving *data uninitialised.
Surely not what you want.

In any case, this complexity is unnecessary.

static int vsc73xx_get_downshift(struct phy_device *phydev, u8 *data)
{
	int val, enable, cnt;

	val = phy_read_paged(phydev, MII_VSC73XX_EXT_PAGE_1E,
			     MII_VSC73XX_PHY_CTRL_EXT3);
	if (val < 0)
		return val;

	enable = FIELD_GET(MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN, val);
	cnt = FIELD_GET(MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT, val) + 2;

	*data = enable ? cnt : DOWNSHIFT_DEV_DISABLE;

	return 0;
}

is all that it needs.

> +}
> +
> +static int vsc73xx_set_downshift(struct phy_device *phydev, u8 cnt)
> +{
> +	int page, ret, val;
> +
> +	if (cnt > MII_VSC73XX_DOWNSHIFT_MAX)
> +		return -E2BIG;
> +	else if (cnt == MII_VSC73XX_DOWNSHIFT_INVAL)
> +		return -EINVAL;
> +
> +	page = phy_select_page(phydev, MII_VSC73XX_EXT_PAGE_1E);
> +	if (page < 0)
> +		return page;
> +
> +	if (!cnt) {
> +		ret = __phy_clear_bits(phydev, MII_VSC73XX_PHY_CTRL_EXT3,
> +				       MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN);
> +	} else {
> +		val = MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN |
> +		      FIELD_PREP(MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT,
> +				 cnt - 2);
> +
> +		ret = __phy_modify(phydev, MII_VSC73XX_PHY_CTRL_EXT3,
> +				   MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN |
> +				   MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT,
> +				   val);
> +	}
> +
> +	ret = phy_restore_page(phydev, page, ret);
> +	if (ret < 0)
> +		return ret;

This is also needlessly over-complex.

	u16 mask, val;

	...

	mask = MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN;

	if (!cnt) {
		val = 0;
	} else {
		mask |= MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT;
		val = MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_EN |
		      FIELD_PREP(MII_VSC73XX_PHY_CTRL_EXT3_DOWNSHIFT_CNT,
				 cnt - 2);
	}

	ret = phy_modify_paged(phydev, MII_VSC73XX_EXT_PAGE_1E,
			       MII_VSC73XX_PHY_CTRL_EXT3, mask, val);
	if (ret < 0)
		return ret;

Would be equivalent to your code above, only simpler.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/9] net: dsa: vsc73xx: fix MDIO bus access and PHY operations

[Andrew Lunn]

On Mon, Jul 29, 2024 at 11:06:06PM +0200, Pawel Dembicki wrote:
> The VSC73xx driver has issues with PHY configuration. This patch series
> fixes most of them.
> 
> The first patch fixes the phylink capabilities, because the MAC in the
> vsc73xx family doesn't handle 1000BASE HD mode.
> 
> The second patch synchronizes the register configuration routine with the
> datasheet recommendations.
> 
> Patches 3-5 restore proper communication on the MDIO bus. Currently,
> the write value isn't sent to the MDIO register, and without a mutex,
> communication with the PHY can be interrupted. This causes the PHY to
> receive improper configuration and autonegotiation could fail.
> 
> The sixth patch speeds up the internal MDIO bus to the maximum value
> allowed by the datasheet.
> 
> The seventh patch removes the PHY reset blockade, as it is no longer
> required.
> 
> After fixing the MDIO operations, autonegotiation became possible.
> The eighth patch removes the blockade, which became unnecessary after
> the MDIO operations fix. It also enables the MDI-X feature, which is
> disabled by default in forced 100BASE-TX mode like other Vitesse PHYs.
> 
> The last patch implements the downshift feature and enables it by default.

Please separate fixes from new development. Fixed should target net,
while new features should be for net-next.

      Andrew