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

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

[Pawel Dembicki]

This series are extracted patches from net-next series [0].

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

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

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

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

After fixing the MDIO operations, autonegotiation became possible.
The last 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.

[0] https://patchwork.kernel.org/project/netdevbpf/list/?series=874739&state=%2A&archive=both

Pawel Dembicki (6):
  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: check busy flag in MDIO operations
  net: dsa: vsc73xx: allow phy resetting
  net: phy: vitesse: repair vsc73xx autonegotiation

 drivers/net/dsa/vitesse-vsc73xx-core.c | 93 +++++++++++++++++++-------
 drivers/net/phy/vitesse.c              | 25 +++++--
 2 files changed, 89 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH net 1/6] 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>
---
This patch came from net-next series[0].
Changes since net-next:
  - rebased to netdev/main only

[0] https://patchwork.kernel.org/project/netdevbpf/patch/20240729210615.279952-6-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 d9d3e30fd47a..f548ed4cb23f 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -957,6 +957,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)
@@ -967,7 +972,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 2/6] 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>
---
This patch came from net-next series[0].
Changes since net-next:
  - rebased to netdev/main only

[0] https://patchwork.kernel.org/project/netdevbpf/patch/20240729210615.279952-6-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 f548ed4cb23f..4b300c293dec 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 3/6] 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>
---
This patch came from net-next series[0].
Changes since net-next:
  - rebased to netdev/main only

[0] https://patchwork.kernel.org/project/netdevbpf/patch/20240729210615.279952-6-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 4b300c293dec..b6c46a3da9a5 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 4/6] net: dsa: vsc73xx: check busy flag in MDIO operations

[Pawel Dembicki]

The VSC73xx has a busy flag used during MDIO operations. It is raised
when MDIO read/write operations are in progress. Without it, PHYs are
misconfigured and bus operations do not work as expected.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

---
This patch came from net-next series[0].
Changes since net-next:
  - removed mutex
  - used method poll.h to poll busy value in 'vsc73xx_mdio_busy_check'
  - use 'vsc73xx_mdio_busy_check' for control if mdio is ready

[0] https://patchwork.kernel.org/project/netdevbpf/patch/20240729210615.279952-6-paweldembicki@gmail.com/
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 33 ++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index b6c46a3da9a5..42b4f312c418 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -317,6 +317,7 @@
 #define IS_739X(a) (IS_7395(a) || IS_7398(a))
 
 #define VSC73XX_POLL_SLEEP_US		1000
+#define VSC73XX_MDIO_POLL_SLEEP_US	5
 #define VSC73XX_POLL_TIMEOUT_US		10000
 
 struct vsc73xx_counter {
@@ -545,6 +546,22 @@ static int vsc73xx_detect(struct vsc73xx *vsc)
 	return 0;
 }
 
+static int vsc73xx_mdio_busy_check(struct vsc73xx *vsc)
+{
+	int ret, err;
+	u32 val;
+
+	ret = read_poll_timeout(vsc73xx_read, err,
+				err < 0 || !(val & VSC73XX_MII_STAT_BUSY),
+				VSC73XX_MDIO_POLL_SLEEP_US,
+				VSC73XX_POLL_TIMEOUT_US, false, vsc,
+				VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
+				VSC73XX_MII_STAT, &val);
+	if (ret)
+		return ret;
+	return err;
+}
+
 static int vsc73xx_phy_read(struct dsa_switch *ds, int phy, int regnum)
 {
 	struct vsc73xx *vsc = ds->priv;
@@ -552,6 +569,10 @@ static int vsc73xx_phy_read(struct dsa_switch *ds, int phy, int regnum)
 	u32 val;
 	int ret;
 
+	ret = vsc73xx_mdio_busy_check(vsc);
+	if (ret)
+		return ret;
+
 	/* Setting bit 26 means "read" */
 	cmd = VSC73XX_MII_CMD_OPERATION |
 	      FIELD_PREP(VSC73XX_MII_CMD_PHY_ADDR, phy) |
@@ -560,7 +581,11 @@ static int vsc73xx_phy_read(struct dsa_switch *ds, int phy, int regnum)
 			    VSC73XX_MII_CMD, cmd);
 	if (ret)
 		return ret;
-	msleep(2);
+
+	ret = vsc73xx_mdio_busy_check(vsc);
+	if (ret)
+		return ret;
+
 	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_MII, VSC73XX_BLOCK_MII_INTERNAL,
 			   VSC73XX_MII_DATA, &val);
 	if (ret)
@@ -583,7 +608,11 @@ 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;
+
+	ret = vsc73xx_mdio_busy_check(vsc);
+	if (ret)
+		return ret;
 
 	/* It was found through tedious experiments that this router
 	 * chip really hates to have it's PHYs reset. They
-- 
2.34.1

[PATCH net 5/6] 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>
---
This patch came from net-next series[0].
Changes since net-next:
  - rebased to netdev/main only

[0] https://patchwork.kernel.org/project/netdevbpf/patch/20240729210615.279952-6-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 42b4f312c418..5f63c56db905 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -614,17 +614,6 @@ static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	if (ret)
 		return ret;
 
-	/* 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 6/6] 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>
---
This patch came from net-next series[0].
Changes since net-next:
  - rebased to netdev/main only

[0] https://patchwork.kernel.org/project/netdevbpf/patch/20240729210615.279952-6-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

Re: [PATCH net 5/6] net: dsa: vsc73xx: allow phy resetting

[Russell King (Oracle)]

On Fri, Aug 02, 2024 at 10:04:02AM +0200, Pawel Dembicki wrote:
> 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.

This commit message needs to explain more clearly why a soft reset is no
longer a problem. For example, which patch in this series makes it now
safe to do?

-- 
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 6/6] net: phy: vitesse: repair vsc73xx autonegotiation

[Russell King (Oracle)]

On Fri, Aug 02, 2024 at 10:04:03AM +0200, Pawel Dembicki wrote:
> 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.

Why not implement proper MDI(-X) configuration support so that the user
can configure it as desired?

-- 
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 6/6] net: phy: vitesse: repair vsc73xx autonegotiation

[Paweł Dembicki]

pt., 2 sie 2024 o 15:03 Russell King (Oracle) <linux@armlinux.org.uk>
napisał(a):
>
> On Fri, Aug 02, 2024 at 10:04:03AM +0200, Pawel Dembicki wrote:
> > 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.
>
> Why not implement proper MDI(-X) configuration support so that the user
> can configure it as desired?
>

This approach is a copy of an idea from other PHYs in the 'vitesse' driver.
I can implement MDI(-X) configuration and status.
But the question is: Should I do it in this patch series or send a
separate patch to net-next after this series gets merged?

Re: [PATCH net 1/6] net: dsa: vsc73xx: fix port MAC configuration in full duplex mode

[Linus Walleij]

On Fri, Aug 2, 2024 at 10:04 AM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> 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>

Good catch!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net 2/6] net: dsa: vsc73xx: pass value in phy_write operation

[Linus Walleij]

On Fri, Aug 2, 2024 at 10:04 AM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> 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>

It makes perfect sense and I have no idea how I managed
to write this bad code or why it worked so far.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net 2/6] net: dsa: vsc73xx: pass value in phy_write operation

[Linus Walleij]

On Fri, Aug 2, 2024 at 10:04 AM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> 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")

Actually that should be:
Fixes: 05bd97fc559d ("net: dsa: Add Vitesse VSC73xx DSA router driver")

Yours,
Linus Walleij

Re: [PATCH net 3/6] net: dsa: vsc73xx: use defined values in phy operations

[Linus Walleij]

On Fri, Aug 2, 2024 at 10:04 AM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> 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>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net 4/6] net: dsa: vsc73xx: check busy flag in MDIO operations

[Linus Walleij]

On Fri, Aug 2, 2024 at 10:04 AM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> The VSC73xx has a busy flag used during MDIO operations. It is raised
> when MDIO read/write operations are in progress. Without it, PHYs are
> misconfigured and bus operations do not work as expected.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

Haven't seen the issue but it looks reasonable.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net 5/6] net: dsa: vsc73xx: allow phy resetting

[Linus Walleij]

On Fri, Aug 2, 2024 at 10:04 AM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> 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>

Like Russell says, it needs an explanation.

I think it worked because since the phy write operations were
not properly implemented, the PHY relied on power-on
or firmware defaults before, so things just happened to work
on some systems. We were just lucky things worked if we didn't
reset the PHY.

With some explanation like that:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net 6/6] net: phy: vitesse: repair vsc73xx autonegotiation

[Linus Walleij]

On Fri, Aug 2, 2024 at 10:04 AM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> 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>

Looks good to me, as Russell points out there are further improvements
we can make but this patch stands on its own as well.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net 2/6] net: dsa: vsc73xx: pass value in phy_write operation

[Florian Fainelli]

On 8/2/24 01:03, Pawel Dembicki wrote:
> 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net 1/6] net: dsa: vsc73xx: fix port MAC configuration in full duplex mode

[Florian Fainelli]

On 8/2/24 01:03, Pawel Dembicki wrote:
> 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net 4/6] net: dsa: vsc73xx: check busy flag in MDIO operations

[Florian Fainelli]

On 8/2/24 01:04, Pawel Dembicki wrote:
> The VSC73xx has a busy flag used during MDIO operations. It is raised
> when MDIO read/write operations are in progress. Without it, PHYs are
> misconfigured and bus operations do not work as expected.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net 5/6] net: dsa: vsc73xx: allow phy resetting

[Florian Fainelli]

On 8/2/24 01:04, Pawel Dembicki wrote:
> 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>

Same comment as Linus and Russell, with a better explanation:

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net 3/6] net: dsa: vsc73xx: use defined values in phy operations

[Florian Fainelli]

On 8/2/24 01:04, Pawel Dembicki wrote:
> 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net 3/6] net: dsa: vsc73xx: use defined values in phy operations

[Vladimir Oltean]

On Fri, Aug 02, 2024 at 10:04:00AM +0200, Pawel Dembicki wrote:
> 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>
> ---

Your patch helps. It makes it clearer that the hardware could be driven
by the drivers/net/mdio/mdio-mscc-miim.c driver. No?

Otherwise, I wonder if the triage you've done between bug fixes for
'net' and cleanup for 'net-next' is enough.

Re: [PATCH net 1/6] net: dsa: vsc73xx: fix port MAC configuration in full duplex mode

[Andrew Lunn]

On Fri, Aug 02, 2024 at 10:03:58AM +0200, Pawel Dembicki wrote:
> 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>
> ---
> This patch came from net-next series[0].
> Changes since net-next:
>   - rebased to netdev/main only

Since this is targeting net, a fixes tag would still be good. If
nothing else it prevents somebody trying to backport it to linux
2.6.39 :-)

       Andrew

Re: [PATCH net 3/6] net: dsa: vsc73xx: use defined values in phy operations

[Paweł Dembicki]

sob., 3 sie 2024 o 00:15 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Fri, Aug 02, 2024 at 10:04:00AM +0200, Pawel Dembicki wrote:
> > 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>
> > ---
>
> Your patch helps. It makes it clearer that the hardware could be driven
> by the drivers/net/mdio/mdio-mscc-miim.c driver. No?

Older VItesse hardware implementation is similar. vsc73xx has slightly
different registers but it could be resolved.
However I'm not sure if it is possible to have one implementation for
platform and spi driver. I can't prepare support for spi connection
without a device for tests.

To be honest, I would prefer to merge the fixes to the existing
implementation at this point.

> Otherwise, I wonder if the triage you've done between bug fixes for
> 'net' and cleanup for 'net-next' is enough.

This patch isn't a fix. It helps to reduce magic numbers in the next
patch. I could send it to net-next and add missing definitions to the
"busy_check" patch.