diff mbox series

[RFC,net-next,v2,11/12] net: dsa: Separate C22 and C45 MDIO bus transaction methods

Message ID 20221227-v6-2-rc1-c45-seperation-v2-11-ddb37710e5a7@walle.cc (mailing list archive)
State New, archived
Headers show
Series net: mdio: Start separating C22 and C45 | expand

Commit Message

Michael Walle Dec. 27, 2022, 11:07 p.m. UTC 
From: Andrew Lunn <andrew@lunn.ch>

By adding _c45 function pointers to the dsa_switch_op structure, the
dsa core can register an MDIO bus with C45 accessors.

The dsa-loop driver could in theory provide such accessors, since it
just passed requests to the MDIO bus it is on, but it seems unlikely
to be useful at the moment. It can however be added later.

mt7530 does support C45, but its uses a mix of registering its MDIO
bus and using the DSA core provided bus. This makes the change a bit
more complex.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Michael Walle <michael@walle.cc>
---
v2:
 - [al] Remove conditional c45, since all switches support c45
 - [al] Remove dsa core changes, they are not needed
 - [al] Add comment that DSA provided MDIO bus is C22 only.
---
 drivers/net/dsa/mt7530.c | 87 ++++++++++++++++++++++++------------------------
 drivers/net/dsa/mt7530.h | 15 ++++++---
 include/net/dsa.h        |  2 +-
 3 files changed, 56 insertions(+), 48 deletions(-)

Comments

Vladimir Oltean Jan. 3, 2023, 3:31 p.m. UTC | #1 
On Wed, Dec 28, 2022 at 12:07:27AM +0100, Michael Walle wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> By adding _c45 function pointers to the dsa_switch_op structure, the
> dsa core can register an MDIO bus with C45 accessors.
> 
> The dsa-loop driver could in theory provide such accessors, since it
> just passed requests to the MDIO bus it is on, but it seems unlikely
> to be useful at the moment. It can however be added later.
> 
> mt7530 does support C45, but its uses a mix of registering its MDIO
> bus and using the DSA core provided bus. This makes the change a bit
> more complex.

"using the DSA core provided bus" is a misrepresentation AFAICS.
Rather said, "providing its private MDIO bus to the DSA core too".

> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> v2:
>  - [al] Remove conditional c45, since all switches support c45
>  - [al] Remove dsa core changes, they are not needed
>  - [al] Add comment that DSA provided MDIO bus is C22 only.
> ---
>  drivers/net/dsa/mt7530.c | 87 ++++++++++++++++++++++++------------------------
>  drivers/net/dsa/mt7530.h | 15 ++++++---
>  include/net/dsa.h        |  2 +-
>  3 files changed, 56 insertions(+), 48 deletions(-)

This patch is not very coherent after the changes in v2.

There are really 2 distinct pieces:

1. a comment in include/net/dsa.h, which provides a justification for
why dsa_switch_ops :: {phy_read(), phy_write()} weren't split into
{phy_read(), phy_write()} and {phy_read_c45() and phy_write_c45()}.

2. a conversion of the mt7530 MDIO bus driver.

I would expect these to be distinct patches.

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 96086289aa9b..732c7bc261a9 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -858,7 +858,7 @@ struct dsa_switch_ops {
>  	u32	(*get_phy_flags)(struct dsa_switch *ds, int port);
>  
>  	/*
> -	 * Access to the switch's PHY registers.
> +	 * Access to the switch's PHY registers. C22 only.
>  	 */
>  	int	(*phy_read)(struct dsa_switch *ds, int port, int regnum);
>  	int	(*phy_write)(struct dsa_switch *ds, int port,

Let me try to untangle for you what these operations really do.
When they are present, DSA will allocate ds->slave_mii_bus on behalf of
the driver, and use these methods for MDIO access of internal PHYs.

The purpose of ds->slave_mii_bus is to offer a non-OF based
phy_connect() for old-style device trees where there is no phy-handle
between the user port fwnode and the internal PHY fwnode (normally
because the ethernet-phy isn't described in the device tree at all).

Like this:

	ethernet-switch {
		ethernet-ports {
			port@1 {
				reg = <1>;
			};
		};
	};

So ds->slave_mii_bus is useful with or without the ds->ops->phy_read()
and ds->ops->phy_write() pointers, which is for example why mt7530
allocates its own MDIO bus with its own private methods (so it doesn't
populate ds->ops->phy_read()), but it also populates ds->slave_mii_bus
with its own bus.

Since clause 45 PHYs are identified by the "ethernet-phy-ieee802.3-c45"
compatible string (otherwise they are C22), then a PHY which is not
described in the device tree can only be C22. So this is why
ds->slave_mii_bus only deals with clause 22 methods, and the true reason
behind the comment above.

But actually this premise is no longer true since Luiz' commit
fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), which introduced the
strange concept of an "OF-aware helper for internal PHYs which are not
described in the device tree". After his patch, it is possible to have
something like this:

	ethernet-switch {
		ethernet-ports {
			port@1 {
				reg = <1>;
			};
		};

		mdio {
			ethernet-phy@1 {
				compatible = "ethernet-phy-ieee802.3-c45"
				reg = <1>;
			};
		};
	};

As you can see, this is a clause 45 internal PHY which lacks a
phy-handle, so its bus must be put in ds->slave_mii_bus in order for
dsa_slave_phy_connect() to see it without that phy-handle (based on the
port number matching with the PHY number). After Luiz' patch, this kind
of device tree is possible, and it invalidates the assumption about
ds->slave_mii_bus only driving C22 PHYs.

> 
> -- 
> 2.30.2
Andrew Lunn Jan. 3, 2023, 3:48 p.m. UTC | #2 
> Since clause 45 PHYs are identified by the "ethernet-phy-ieee802.3-c45"
> compatible string (otherwise they are C22), then a PHY which is not
> described in the device tree can only be C22. So this is why
> ds->slave_mii_bus only deals with clause 22 methods, and the true reason
> behind the comment above.
> 
> But actually this premise is no longer true since Luiz' commit
> fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), which introduced the
> strange concept of an "OF-aware helper for internal PHYs which are not
> described in the device tree". After his patch, it is possible to have
> something like this:
> 
> 	ethernet-switch {
> 		ethernet-ports {
> 			port@1 {
> 				reg = <1>;
> 			};
> 		};
> 
> 		mdio {
> 			ethernet-phy@1 {
> 				compatible = "ethernet-phy-ieee802.3-c45"
> 				reg = <1>;
> 			};
> 		};
> 	};
> 
> As you can see, this is a clause 45 internal PHY which lacks a
> phy-handle, so its bus must be put in ds->slave_mii_bus in order for
> dsa_slave_phy_connect() to see it without that phy-handle (based on the
> port number matching with the PHY number). After Luiz' patch, this kind
> of device tree is possible, and it invalidates the assumption about
> ds->slave_mii_bus only driving C22 PHYs.

My memory is hazy, but i think at the time i wrote these patches,
there was no DSA driver which made use of ds->slave_mii_bus with
C45. So i took the short cut of only supporting C22.

Those DSA drivers which do support C45 all register their bus directly
with the MDIO core.

So Luiz patches may allow a C45 bus, but are there any drivers today
actually using it?

	 Andrew
Vladimir Oltean Jan. 3, 2023, 3:56 p.m. UTC | #3 
On Tue, Jan 03, 2023 at 04:48:59PM +0100, Andrew Lunn wrote:
> > Since clause 45 PHYs are identified by the "ethernet-phy-ieee802.3-c45"
> > compatible string (otherwise they are C22), then a PHY which is not
> > described in the device tree can only be C22. So this is why
> > ds->slave_mii_bus only deals with clause 22 methods, and the true reason
> > behind the comment above.
> > 
> > But actually this premise is no longer true since Luiz' commit
> > fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), which introduced the
> > strange concept of an "OF-aware helper for internal PHYs which are not
> > described in the device tree". After his patch, it is possible to have
> > something like this:
> > 
> > 	ethernet-switch {
> > 		ethernet-ports {
> > 			port@1 {
> > 				reg = <1>;
> > 			};
> > 		};
> > 
> > 		mdio {
> > 			ethernet-phy@1 {
> > 				compatible = "ethernet-phy-ieee802.3-c45"
> > 				reg = <1>;
> > 			};
> > 		};
> > 	};
> > 
> > As you can see, this is a clause 45 internal PHY which lacks a
> > phy-handle, so its bus must be put in ds->slave_mii_bus in order for
> > dsa_slave_phy_connect() to see it without that phy-handle (based on the
> > port number matching with the PHY number). After Luiz' patch, this kind
> > of device tree is possible, and it invalidates the assumption about
> > ds->slave_mii_bus only driving C22 PHYs.
> 
> My memory is hazy, but i think at the time i wrote these patches,
> there was no DSA driver which made use of ds->slave_mii_bus with
> C45. So i took the short cut of only supporting C22.

Actually I believe that in v1 you did extend ds->ops with C45 methods,
but it's me who told you to remove them:
https://patchwork.kernel.org/project/netdevbpf/patch/20220508153049.427227-10-andrew@lunn.ch/#24852813

> 
> Those DSA drivers which do support C45 all register their bus directly
> with the MDIO core.

And rightfully so. IMHO, letting DSA allocate ds->slave_mii_bus out of
driver writer sheer convenience (a secondary purpose) should be deprecated,
unless the reason for using ds->slave_mii_bus is the lack of a phy-handle
(the primary purpose). It becomes that more confusing to have to extend
dsa_switch_ops with 2 more methods which serve the secondary purpose but
not the primary one.

> So Luiz patches may allow a C45 bus, but are there any drivers today
> actually using it?

I sent a private email to Luiz a few minutes ago asking him to confirm.
Michael Walle Jan. 16, 2023, 7:51 a.m. UTC | #4 
Hi,

Am 2023-01-03 16:56, schrieb Vladimir Oltean:
>> So Luiz patches may allow a C45 bus, but are there any drivers today
>> actually using it?
> 
> I sent a private email to Luiz a few minutes ago asking him to confirm.

Any news here? Do we need the c45 methods?

-michael
Vladimir Oltean Jan. 16, 2023, 7:28 p.m. UTC | #5 
On Mon, Jan 16, 2023 at 08:51:55AM +0100, Michael Walle wrote:
> Hi,
> 
> Am 2023-01-03 16:56, schrieb Vladimir Oltean:
> > > So Luiz patches may allow a C45 bus, but are there any drivers today
> > > actually using it?
> > 
> > I sent a private email to Luiz a few minutes ago asking him to confirm.
> 
> Any news here? Do we need the c45 methods?

No news it seems. I am going to default to assuming that no, a ds->slave_mii_bus
created by dsa_switch_setup() does not need C45 accessors.

This patch should be modified to only touch the mt7530 driver, and
adjust the commit message accordingly. I see no connection to what the
DSA core does.
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 908fa89444c9..616b21c90d05 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -608,17 +608,29 @@  mt7530_mib_reset(struct dsa_switch *ds)
 	mt7530_write(priv, MT7530_MIB_CCR, CCR_MIB_ACTIVATE);
 }
 
-static int mt7530_phy_read(struct mt7530_priv *priv, int port, int regnum)
+static int mt7530_phy_read_c22(struct mt7530_priv *priv, int port, int regnum)
 {
 	return mdiobus_read_nested(priv->bus, port, regnum);
 }
 
-static int mt7530_phy_write(struct mt7530_priv *priv, int port, int regnum,
-			    u16 val)
+static int mt7530_phy_write_c22(struct mt7530_priv *priv, int port, int regnum,
+				u16 val)
 {
 	return mdiobus_write_nested(priv->bus, port, regnum, val);
 }
 
+static int mt7530_phy_read_c45(struct mt7530_priv *priv, int port,
+			       int devad, int regnum)
+{
+	return mdiobus_c45_read_nested(priv->bus, port, devad, regnum);
+}
+
+static int mt7530_phy_write_c45(struct mt7530_priv *priv, int port, int devad,
+				int regnum, u16 val)
+{
+	return mdiobus_c45_write_nested(priv->bus, port, devad, regnum, val);
+}
+
 static int
 mt7531_ind_c45_phy_read(struct mt7530_priv *priv, int port, int devad,
 			int regnum)
@@ -670,7 +682,7 @@  mt7531_ind_c45_phy_read(struct mt7530_priv *priv, int port, int devad,
 
 static int
 mt7531_ind_c45_phy_write(struct mt7530_priv *priv, int port, int devad,
-			 int regnum, u32 data)
+			 int regnum, u16 data)
 {
 	struct mii_bus *bus = priv->bus;
 	struct mt7530_dummy_poll p;
@@ -793,55 +805,36 @@  mt7531_ind_c22_phy_write(struct mt7530_priv *priv, int port, int regnum,
 }
 
 static int
-mt7531_ind_phy_read(struct mt7530_priv *priv, int port, int regnum)
+mt753x_phy_read_c22(struct mii_bus *bus, int port, int regnum)
 {
-	int devad;
-	int ret;
-
-	if (regnum & MII_ADDR_C45) {
-		devad = (regnum >> MII_DEVADDR_C45_SHIFT) & 0x1f;
-		ret = mt7531_ind_c45_phy_read(priv, port, devad,
-					      regnum & MII_REGADDR_C45_MASK);
-	} else {
-		ret = mt7531_ind_c22_phy_read(priv, port, regnum);
-	}
+	struct mt7530_priv *priv = bus->priv;
 
-	return ret;
+	return priv->info->phy_read_c22(priv, port, regnum);
 }
 
 static int
-mt7531_ind_phy_write(struct mt7530_priv *priv, int port, int regnum,
-		     u16 data)
+mt753x_phy_read_c45(struct mii_bus *bus, int port, int devad, int regnum)
 {
-	int devad;
-	int ret;
-
-	if (regnum & MII_ADDR_C45) {
-		devad = (regnum >> MII_DEVADDR_C45_SHIFT) & 0x1f;
-		ret = mt7531_ind_c45_phy_write(priv, port, devad,
-					       regnum & MII_REGADDR_C45_MASK,
-					       data);
-	} else {
-		ret = mt7531_ind_c22_phy_write(priv, port, regnum, data);
-	}
+	struct mt7530_priv *priv = bus->priv;
 
-	return ret;
+	return priv->info->phy_read_c45(priv, port, devad, regnum);
 }
 
 static int
-mt753x_phy_read(struct mii_bus *bus, int port, int regnum)
+mt753x_phy_write_c22(struct mii_bus *bus, int port, int regnum, u16 val)
 {
 	struct mt7530_priv *priv = bus->priv;
 
-	return priv->info->phy_read(priv, port, regnum);
+	return priv->info->phy_write_c22(priv, port, regnum, val);
 }
 
 static int
-mt753x_phy_write(struct mii_bus *bus, int port, int regnum, u16 val)
+mt753x_phy_write_c45(struct mii_bus *bus, int port, int devad, int regnum,
+		     u16 val)
 {
 	struct mt7530_priv *priv = bus->priv;
 
-	return priv->info->phy_write(priv, port, regnum, val);
+	return priv->info->phy_write_c45(priv, port, devad, regnum, val);
 }
 
 static void
@@ -2086,8 +2079,10 @@  mt7530_setup_mdio(struct mt7530_priv *priv)
 	bus->priv = priv;
 	bus->name = KBUILD_MODNAME "-mii";
 	snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
-	bus->read = mt753x_phy_read;
-	bus->write = mt753x_phy_write;
+	bus->read = mt753x_phy_read_c22;
+	bus->write = mt753x_phy_write_c22;
+	bus->read_c45 = mt753x_phy_read_c45;
+	bus->write_c45 = mt753x_phy_write_c45;
 	bus->parent = dev;
 	bus->phy_mask = ~ds->phys_mii_mask;
 
@@ -3182,8 +3177,10 @@  static const struct mt753x_info mt753x_table[] = {
 		.id = ID_MT7621,
 		.pcs_ops = &mt7530_pcs_ops,
 		.sw_setup = mt7530_setup,
-		.phy_read = mt7530_phy_read,
-		.phy_write = mt7530_phy_write,
+		.phy_read_c22 = mt7530_phy_read_c22,
+		.phy_write_c22 = mt7530_phy_write_c22,
+		.phy_read_c45 = mt7530_phy_read_c45,
+		.phy_write_c45 = mt7530_phy_write_c45,
 		.pad_setup = mt7530_pad_clk_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
@@ -3192,8 +3189,10 @@  static const struct mt753x_info mt753x_table[] = {
 		.id = ID_MT7530,
 		.pcs_ops = &mt7530_pcs_ops,
 		.sw_setup = mt7530_setup,
-		.phy_read = mt7530_phy_read,
-		.phy_write = mt7530_phy_write,
+		.phy_read_c22 = mt7530_phy_read_c22,
+		.phy_write_c22 = mt7530_phy_write_c22,
+		.phy_read_c45 = mt7530_phy_read_c45,
+		.phy_write_c45 = mt7530_phy_write_c45,
 		.pad_setup = mt7530_pad_clk_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
@@ -3202,8 +3201,10 @@  static const struct mt753x_info mt753x_table[] = {
 		.id = ID_MT7531,
 		.pcs_ops = &mt7531_pcs_ops,
 		.sw_setup = mt7531_setup,
-		.phy_read = mt7531_ind_phy_read,
-		.phy_write = mt7531_ind_phy_write,
+		.phy_read_c22 = mt7531_ind_c22_phy_read,
+		.phy_write_c22 = mt7531_ind_c22_phy_write,
+		.phy_read_c45 = mt7531_ind_c45_phy_read,
+		.phy_write_c45 = mt7531_ind_c45_phy_write,
 		.pad_setup = mt7531_pad_setup,
 		.cpu_port_config = mt7531_cpu_port_config,
 		.mac_port_get_caps = mt7531_mac_port_get_caps,
@@ -3263,7 +3264,7 @@  mt7530_probe(struct mdio_device *mdiodev)
 	 * properly.
 	 */
 	if (!priv->info->sw_setup || !priv->info->pad_setup ||
-	    !priv->info->phy_read || !priv->info->phy_write ||
+	    !priv->info->phy_read_c22 || !priv->info->phy_write_c22 ||
 	    !priv->info->mac_port_get_caps ||
 	    !priv->info->mac_port_config)
 		return -EINVAL;
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index e8d966435350..6b2fc6290ea8 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -750,8 +750,10 @@  struct mt753x_pcs {
 /* struct mt753x_info -	This is the main data structure for holding the specific
  *			part for each supported device
  * @sw_setup:		Holding the handler to a device initialization
- * @phy_read:		Holding the way reading PHY port
- * @phy_write:		Holding the way writing PHY port
+ * @phy_read_c22:	Holding the way reading PHY port using C22
+ * @phy_write_c22:	Holding the way writing PHY port using C22
+ * @phy_read_c45:	Holding the way reading PHY port using C45
+ * @phy_write_c45:	Holding the way writing PHY port using C45
  * @pad_setup:		Holding the way setting up the bus pad for a certain
  *			MAC port
  * @phy_mode_supported:	Check if the PHY type is being supported on a certain
@@ -767,8 +769,13 @@  struct mt753x_info {
 	const struct phylink_pcs_ops *pcs_ops;
 
 	int (*sw_setup)(struct dsa_switch *ds);
-	int (*phy_read)(struct mt7530_priv *priv, int port, int regnum);
-	int (*phy_write)(struct mt7530_priv *priv, int port, int regnum, u16 val);
+	int (*phy_read_c22)(struct mt7530_priv *priv, int port, int regnum);
+	int (*phy_write_c22)(struct mt7530_priv *priv, int port, int regnum,
+			     u16 val);
+	int (*phy_read_c45)(struct mt7530_priv *priv, int port, int devad,
+			    int regnum);
+	int (*phy_write_c45)(struct mt7530_priv *priv, int port, int devad,
+			     int regnum, u16 val);
 	int (*pad_setup)(struct dsa_switch *ds, phy_interface_t interface);
 	int (*cpu_port_config)(struct dsa_switch *ds, int port);
 	void (*mac_port_get_caps)(struct dsa_switch *ds, int port,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 96086289aa9b..732c7bc261a9 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -858,7 +858,7 @@  struct dsa_switch_ops {
 	u32	(*get_phy_flags)(struct dsa_switch *ds, int port);
 
 	/*
-	 * Access to the switch's PHY registers.
+	 * Access to the switch's PHY registers. C22 only.
 	 */
 	int	(*phy_read)(struct dsa_switch *ds, int port, int regnum);
 	int	(*phy_write)(struct dsa_switch *ds, int port,