[PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Luiz Angelo Daros de Luca]

The last patch cleans all the deprecated code while keeping the kernel
messages. However, if there is no "mdio" node but there is a node with
the old compatible stings "realtek,smi-mdio", it will show an error. It
should still work but it will use polling instead of interruptions.

My idea, if accepted, is to submit patches 1 and 2 now. After a
reasonable period, submit patch 3.

I don't have an SMI-connected device and I'm asking for testers. It
would be nice to test the first 2 patches with:
1) "mdio" without a compatible string. It should work without warnings.
2) "mdio" with a compatible string. It should work with a warning asking
to remove the compatible string
3) "xxx" node with compatible string. It should work with a warning
asking to rename "xxx" to "mdio" and remove the compatible string

In all those cases, the switch should still keep using interruptions.

After that, the last patch can be applied. The same tests can be
performed:
1) "mdio" without a compatible string. It should work without warnings.
2) "mdio" with a compatible string. It should work with a warning asking
to remove the compatible string
3) "xxx" node with compatible string. It should work with an error
asking to rename "xxx" to "mdio" and remove the compatible string. The
switch will use polling instead of interruptions.

This series might inspire other drivers as well. Currently, most dsa
driver implements a custom slave mii, normally only defining a
phy_{read,write} and loading properties from an "mdio" OF node. Since
fe7324b932, dsa generic code can do all that if the mdio node is named
"mdio".  I believe most drivers could simply drop their slave mii
implementations and add phy_{read,write} to the dsa_switch_ops. For
drivers that look for an "mdio-like" node using a compatible string, it
might need some type of transition to let vendors update their OF tree.

Regards,

Luiz

[PATCH net-next RFC 1/3] net: dsa: realtek: rename switch_ops to its usage, not users

[Luiz Angelo Daros de Luca]

The difference between ds_ops_smi and ds_ops_mdio is only the presence
of phy_{read,write} because the realtek-smi implements a custom slave
MII while realtek-mdio uses the standard slave mii created by DSA. It's
better to have a name that reflects why we need different switch_ops
instead of who is using it.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-mdio.c | 2 +-
 drivers/net/dsa/realtek/realtek-smi.c  | 2 +-
 drivers/net/dsa/realtek/realtek.h      | 4 ++--
 drivers/net/dsa/realtek/rtl8365mb.c    | 8 ++++----
 drivers/net/dsa/realtek/rtl8366rb.c    | 8 ++++----
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index c58f49d558d2..90624a5102b5 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -222,7 +222,7 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 	priv->ds->dev = dev;
 	priv->ds->num_ports = priv->num_ports;
 	priv->ds->priv = priv;
-	priv->ds->ops = var->ds_ops_mdio;
+	priv->ds->ops = var->ds_ops;
 
 	ret = dsa_register_switch(priv->ds);
 	if (ret) {
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 45992f79ec8d..3eb9d67fd2de 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -498,7 +498,7 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	priv->ds->num_ports = priv->num_ports;
 	priv->ds->priv = priv;
 
-	priv->ds->ops = var->ds_ops_smi;
+	priv->ds->ops = var->ds_ops_custom_slavemii;
 	ret = dsa_register_switch(priv->ds);
 	if (ret) {
 		dev_err_probe(dev, ret, "unable to register switch\n");
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 4fa7c6ba874a..004a9ae91ccf 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -116,8 +116,8 @@ struct realtek_ops {
 };
 
 struct realtek_variant {
-	const struct dsa_switch_ops *ds_ops_smi;
-	const struct dsa_switch_ops *ds_ops_mdio;
+	const struct dsa_switch_ops *ds_ops_custom_slavemii;
+	const struct dsa_switch_ops *ds_ops;
 	const struct realtek_ops *ops;
 	unsigned int clk_delay;
 	u8 cmd_read;
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index da31d8b839ac..35fd32c4d340 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -2086,7 +2086,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 	return 0;
 }
 
-static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
+static const struct dsa_switch_ops rtl8365mb_switch_ops_custom_slavemii = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
 	.change_tag_protocol = rtl8365mb_change_tag_protocol,
 	.setup = rtl8365mb_setup,
@@ -2105,7 +2105,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
 	.get_stats64 = rtl8365mb_get_stats64,
 };
 
-static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
+static const struct dsa_switch_ops rtl8365mb_switch_ops = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
 	.change_tag_protocol = rtl8365mb_change_tag_protocol,
 	.setup = rtl8365mb_setup,
@@ -2133,8 +2133,8 @@ static const struct realtek_ops rtl8365mb_ops = {
 };
 
 const struct realtek_variant rtl8365mb_variant = {
-	.ds_ops_smi = &rtl8365mb_switch_ops_smi,
-	.ds_ops_mdio = &rtl8365mb_switch_ops_mdio,
+	.ds_ops_custom_slavemii = &rtl8365mb_switch_ops_custom_slavemii,
+	.ds_ops = &rtl8365mb_switch_ops,
 	.ops = &rtl8365mb_ops,
 	.clk_delay = 10,
 	.cmd_read = 0xb9,
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 25f88022b9e4..7c3de3be3a53 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1793,7 +1793,7 @@ static int rtl8366rb_detect(struct realtek_priv *priv)
 	return 0;
 }
 
-static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = {
+static const struct dsa_switch_ops rtl8366rb_switch_ops_custom_slavemii = {
 	.get_tag_protocol = rtl8366_get_tag_protocol,
 	.setup = rtl8366rb_setup,
 	.phylink_mac_link_up = rtl8366rb_mac_link_up,
@@ -1816,7 +1816,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = {
 	.port_max_mtu = rtl8366rb_max_mtu,
 };
 
-static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = {
+static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.get_tag_protocol = rtl8366_get_tag_protocol,
 	.setup = rtl8366rb_setup,
 	.phy_read = rtl8366rb_dsa_phy_read,
@@ -1858,8 +1858,8 @@ static const struct realtek_ops rtl8366rb_ops = {
 };
 
 const struct realtek_variant rtl8366rb_variant = {
-	.ds_ops_smi = &rtl8366rb_switch_ops_smi,
-	.ds_ops_mdio = &rtl8366rb_switch_ops_mdio,
+	.ds_ops_custom_slavemii = &rtl8366rb_switch_ops_custom_slavemii,
+	.ds_ops = &rtl8366rb_switch_ops,
 	.ops = &rtl8366rb_ops,
 	.clk_delay = 10,
 	.cmd_read = 0xa9,
-- 
2.36.1

[PATCH net-next RFC 2/3] net: dsa: realtek: deprecate custom slave mii

[Luiz Angelo Daros de Luca]

The custom slave mii was required only to parse the OF mdio node.
However, since fe7324b932, the standard slave mii created by dsa already
looks for an "mdio" node.

The realtek-smi was using a compatible string ("realtek,smi-mdio") to
find the slave mdio node. Although device-tree bindings and examples all
use "mdio". If the name does not match "mdio", the driver will still use
the custom mii slave. The driver will also ask to remove the compatible
string if it exists in the "mdio" node.

After a grace period, we can remove:
- realtek_variant.ds_ops_custom_slavemii
- realtek_ops.phy_{read,write}
- realtek_ops.setup_interface
- {rtl8365mb,rtl8366rb}_phy_{read,write}
- realtek_smi_setup_mdio

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-smi.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 3eb9d67fd2de..c3668a9208ac 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -379,6 +379,10 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
+	dev_warn(priv->dev,
+		 "Rename '%s' to 'mdio' and remove the compatible string\n",
+		  mdio_np->full_name);
+
 	priv->slave_mii_bus = devm_mdiobus_alloc(priv->dev);
 	if (!priv->slave_mii_bus) {
 		ret = -ENOMEM;
@@ -412,10 +416,10 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 static int realtek_smi_probe(struct platform_device *pdev)
 {
 	const struct realtek_variant *var;
+	struct device_node *np, *mdio_np;
 	struct device *dev = &pdev->dev;
 	struct realtek_priv *priv;
 	struct regmap_config rc;
-	struct device_node *np;
 	int ret;
 
 	var = of_device_get_match_data(dev);
@@ -452,7 +456,6 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	priv->cmd_write = var->cmd_write;
 	priv->ops = var->ops;
 
-	priv->setup_interface = realtek_smi_setup_mdio;
 	priv->write_reg_noack = realtek_smi_write_reg_noack;
 
 	dev_set_drvdata(dev, priv);
@@ -497,8 +500,20 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	priv->ds->dev = dev;
 	priv->ds->num_ports = priv->num_ports;
 	priv->ds->priv = priv;
+	priv->ds->ops = var->ds_ops;
+
+	mdio_np = of_get_child_by_name(np, "mdio");
+	if (mdio_np) {
+		if (of_device_is_compatible(mdio_np, "realtek,smi-mdio"))
+			dev_warn(dev, "Remove deprecated prop '%s' from '%s'",
+				 "compatible = \"realtek,smi-mdio\"",
+				 mdio_np->full_name);
+		of_node_put(mdio_np);
+	} else {
+		priv->ds->ops = var->ds_ops_custom_slavemii;
+		priv->setup_interface = realtek_smi_setup_mdio;
+	}
 
-	priv->ds->ops = var->ds_ops_custom_slavemii;
 	ret = dsa_register_switch(priv->ds);
 	if (ret) {
 		dev_err_probe(dev, ret, "unable to register switch\n");
-- 
2.36.1

[PATCH net-next RFC 3/3] net: dsa: realtek: remove deprecated custom slave mii

[Luiz Angelo Daros de Luca]

Drop the code path using the custom slave mii. It still warns if there
is a compatible string in "mdio" node. However, if that compatible
string is in a node not named "mdio", it will show an error. In that
case, it might still work using polling instead of interruptions.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-smi.c | 71 +++------------------------
 drivers/net/dsa/realtek/realtek.h     |  5 --
 drivers/net/dsa/realtek/rtl8365mb.c   | 30 -----------
 drivers/net/dsa/realtek/rtl8366rb.c   | 34 -------------
 4 files changed, 8 insertions(+), 132 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index c3668a9208ac..5de0b1563e44 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -352,67 +352,6 @@ static const struct regmap_config realtek_smi_nolock_regmap_config = {
 	.disable_locking = true,
 };
 
-static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum)
-{
-	struct realtek_priv *priv = bus->priv;
-
-	return priv->ops->phy_read(priv, addr, regnum);
-}
-
-static int realtek_smi_mdio_write(struct mii_bus *bus, int addr, int regnum,
-				  u16 val)
-{
-	struct realtek_priv *priv = bus->priv;
-
-	return priv->ops->phy_write(priv, addr, regnum, val);
-}
-
-static int realtek_smi_setup_mdio(struct dsa_switch *ds)
-{
-	struct realtek_priv *priv =  ds->priv;
-	struct device_node *mdio_np;
-	int ret;
-
-	mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
-	if (!mdio_np) {
-		dev_err(priv->dev, "no MDIO bus node\n");
-		return -ENODEV;
-	}
-
-	dev_warn(priv->dev,
-		 "Rename '%s' to 'mdio' and remove the compatible string\n",
-		  mdio_np->full_name);
-
-	priv->slave_mii_bus = devm_mdiobus_alloc(priv->dev);
-	if (!priv->slave_mii_bus) {
-		ret = -ENOMEM;
-		goto err_put_node;
-	}
-	priv->slave_mii_bus->priv = priv;
-	priv->slave_mii_bus->name = "SMI slave MII";
-	priv->slave_mii_bus->read = realtek_smi_mdio_read;
-	priv->slave_mii_bus->write = realtek_smi_mdio_write;
-	snprintf(priv->slave_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d",
-		 ds->index);
-	priv->slave_mii_bus->dev.of_node = mdio_np;
-	priv->slave_mii_bus->parent = priv->dev;
-	ds->slave_mii_bus = priv->slave_mii_bus;
-
-	ret = devm_of_mdiobus_register(priv->dev, priv->slave_mii_bus, mdio_np);
-	if (ret) {
-		dev_err(priv->dev, "unable to register MDIO bus %s\n",
-			priv->slave_mii_bus->id);
-		goto err_put_node;
-	}
-
-	return 0;
-
-err_put_node:
-	of_node_put(mdio_np);
-
-	return ret;
-}
-
 static int realtek_smi_probe(struct platform_device *pdev)
 {
 	const struct realtek_variant *var;
@@ -510,8 +449,14 @@ static int realtek_smi_probe(struct platform_device *pdev)
 				 mdio_np->full_name);
 		of_node_put(mdio_np);
 	} else {
-		priv->ds->ops = var->ds_ops_custom_slavemii;
-		priv->setup_interface = realtek_smi_setup_mdio;
+		mdio_np = of_get_compatible_child(priv->dev->of_node,
+						  "realtek,smi-mdio");
+		if (mdio_np) {
+			dev_err(priv->dev,
+				"Rename '%s' to 'mdio' and remove the compatible string\n",
+				mdio_np->full_name);
+			of_node_put(mdio_np);
+		}
 	}
 
 	ret = dsa_register_switch(priv->ds);
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 004a9ae91ccf..972a87be7e39 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -73,7 +73,6 @@ struct realtek_priv {
 	struct rtl8366_mib_counter *mib_counters;
 
 	const struct realtek_ops *ops;
-	int			(*setup_interface)(struct dsa_switch *ds);
 	int			(*write_reg_noack)(void *ctx, u32 addr, u32 data);
 
 	int			vlan_enabled;
@@ -110,13 +109,9 @@ struct realtek_ops {
 	int	(*enable_vlan)(struct realtek_priv *priv, bool enable);
 	int	(*enable_vlan4k)(struct realtek_priv *priv, bool enable);
 	int	(*enable_port)(struct realtek_priv *priv, int port, bool enable);
-	int	(*phy_read)(struct realtek_priv *priv, int phy, int regnum);
-	int	(*phy_write)(struct realtek_priv *priv, int phy, int regnum,
-			     u16 val);
 };
 
 struct realtek_variant {
-	const struct dsa_switch_ops *ds_ops_custom_slavemii;
 	const struct dsa_switch_ops *ds_ops;
 	const struct realtek_ops *ops;
 	unsigned int clk_delay;
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 35fd32c4d340..bca4019f4fca 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -1987,14 +1987,6 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 	if (ret)
 		goto out_teardown_irq;
 
-	if (priv->setup_interface) {
-		ret = priv->setup_interface(ds);
-		if (ret) {
-			dev_err(priv->dev, "could not set up MDIO bus\n");
-			goto out_teardown_irq;
-		}
-	}
-
 	/* Start statistics counter polling */
 	rtl8365mb_stats_setup(priv);
 
@@ -2086,25 +2078,6 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 	return 0;
 }
 
-static const struct dsa_switch_ops rtl8365mb_switch_ops_custom_slavemii = {
-	.get_tag_protocol = rtl8365mb_get_tag_protocol,
-	.change_tag_protocol = rtl8365mb_change_tag_protocol,
-	.setup = rtl8365mb_setup,
-	.teardown = rtl8365mb_teardown,
-	.phylink_get_caps = rtl8365mb_phylink_get_caps,
-	.phylink_mac_config = rtl8365mb_phylink_mac_config,
-	.phylink_mac_link_down = rtl8365mb_phylink_mac_link_down,
-	.phylink_mac_link_up = rtl8365mb_phylink_mac_link_up,
-	.port_stp_state_set = rtl8365mb_port_stp_state_set,
-	.get_strings = rtl8365mb_get_strings,
-	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
-	.get_sset_count = rtl8365mb_get_sset_count,
-	.get_eth_phy_stats = rtl8365mb_get_phy_stats,
-	.get_eth_mac_stats = rtl8365mb_get_mac_stats,
-	.get_eth_ctrl_stats = rtl8365mb_get_ctrl_stats,
-	.get_stats64 = rtl8365mb_get_stats64,
-};
-
 static const struct dsa_switch_ops rtl8365mb_switch_ops = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
 	.change_tag_protocol = rtl8365mb_change_tag_protocol,
@@ -2128,12 +2101,9 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops = {
 
 static const struct realtek_ops rtl8365mb_ops = {
 	.detect = rtl8365mb_detect,
-	.phy_read = rtl8365mb_phy_read,
-	.phy_write = rtl8365mb_phy_write,
 };
 
 const struct realtek_variant rtl8365mb_variant = {
-	.ds_ops_custom_slavemii = &rtl8365mb_switch_ops_custom_slavemii,
 	.ds_ops = &rtl8365mb_switch_ops,
 	.ops = &rtl8365mb_ops,
 	.clk_delay = 10,
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 7c3de3be3a53..d2f4ccaaf54f 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1030,14 +1030,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	if (ret)
 		dev_info(priv->dev, "no interrupt support\n");
 
-	if (priv->setup_interface) {
-		ret = priv->setup_interface(ds);
-		if (ret) {
-			dev_err(priv->dev, "could not set up MDIO bus\n");
-			return -ENODEV;
-		}
-	}
-
 	return 0;
 }
 
@@ -1793,29 +1785,6 @@ static int rtl8366rb_detect(struct realtek_priv *priv)
 	return 0;
 }
 
-static const struct dsa_switch_ops rtl8366rb_switch_ops_custom_slavemii = {
-	.get_tag_protocol = rtl8366_get_tag_protocol,
-	.setup = rtl8366rb_setup,
-	.phylink_mac_link_up = rtl8366rb_mac_link_up,
-	.phylink_mac_link_down = rtl8366rb_mac_link_down,
-	.get_strings = rtl8366_get_strings,
-	.get_ethtool_stats = rtl8366_get_ethtool_stats,
-	.get_sset_count = rtl8366_get_sset_count,
-	.port_bridge_join = rtl8366rb_port_bridge_join,
-	.port_bridge_leave = rtl8366rb_port_bridge_leave,
-	.port_vlan_filtering = rtl8366rb_vlan_filtering,
-	.port_vlan_add = rtl8366_vlan_add,
-	.port_vlan_del = rtl8366_vlan_del,
-	.port_enable = rtl8366rb_port_enable,
-	.port_disable = rtl8366rb_port_disable,
-	.port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags,
-	.port_bridge_flags = rtl8366rb_port_bridge_flags,
-	.port_stp_state_set = rtl8366rb_port_stp_state_set,
-	.port_fast_age = rtl8366rb_port_fast_age,
-	.port_change_mtu = rtl8366rb_change_mtu,
-	.port_max_mtu = rtl8366rb_max_mtu,
-};
-
 static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.get_tag_protocol = rtl8366_get_tag_protocol,
 	.setup = rtl8366rb_setup,
@@ -1853,12 +1822,9 @@ static const struct realtek_ops rtl8366rb_ops = {
 	.is_vlan_valid	= rtl8366rb_is_vlan_valid,
 	.enable_vlan	= rtl8366rb_enable_vlan,
 	.enable_vlan4k	= rtl8366rb_enable_vlan4k,
-	.phy_read	= rtl8366rb_phy_read,
-	.phy_write	= rtl8366rb_phy_write,
 };
 
 const struct realtek_variant rtl8366rb_variant = {
-	.ds_ops_custom_slavemii = &rtl8366rb_switch_ops_custom_slavemii,
 	.ds_ops = &rtl8366rb_switch_ops,
 	.ops = &rtl8366rb_ops,
 	.clk_delay = 10,
-- 
2.36.1

Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Arınç ÜNAL]

On 29.06.2022 06:54, Luiz Angelo Daros de Luca wrote:
> The last patch cleans all the deprecated code while keeping the kernel
> messages. However, if there is no "mdio" node but there is a node with
> the old compatible stings "realtek,smi-mdio", it will show an error. It
> should still work but it will use polling instead of interruptions.
> 
> My idea, if accepted, is to submit patches 1 and 2 now. After a
> reasonable period, submit patch 3.
> 
> I don't have an SMI-connected device and I'm asking for testers. It
> would be nice to test the first 2 patches with:

I'd love to test this on an Asus RT-AC88U which has got the 
smi-connected RTL8365MB switch but modifying the OpenWrt SDK to build 
for latest kernels is a really painful process. I know it's not related 
to this patch series but, does anyone know a more efficient way of 
building the kernel with rootfs with sufficent userspace tools? Like, am 
I supposed to use Buildroot, Yocto?

Arınç

Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Luiz Angelo Daros de Luca]

This RFC patch series cleans realtek-smi custom slave mii bus. Since
fe7324b932, dsa generic code provides everything needed for
realtek-smi driver. For extra caution, this series should be applied
in two steps: the first 2 patches introduce the new code path that
uses dsa generic code. It will show a warning message if the tree
contains deprecated references. It will still fall back to the old
code path if an "mdio"
is not found.

>
> The last patch cleans all the deprecated code while keeping the kernel
> messages. However, if there is no "mdio" node but there is a node with
> the old compatible stings "realtek,smi-mdio", it will show an error. It
> should still work but it will use polling instead of interruptions.
>
> My idea, if accepted, is to submit patches 1 and 2 now. After a
> reasonable period, submit patch 3.
>
> I don't have an SMI-connected device and I'm asking for testers. It
> would be nice to test the first 2 patches with:
> 1) "mdio" without a compatible string. It should work without warnings.
> 2) "mdio" with a compatible string. It should work with a warning asking
> to remove the compatible string
> 3) "xxx" node with compatible string. It should work with a warning
> asking to rename "xxx" to "mdio" and remove the compatible string
>
> In all those cases, the switch should still keep using interruptions.
>
> After that, the last patch can be applied. The same tests can be
> performed:
> 1) "mdio" without a compatible string. It should work without warnings.
> 2) "mdio" with a compatible string. It should work with a warning asking
> to remove the compatible string
> 3) "xxx" node with compatible string. It should work with an error
> asking to rename "xxx" to "mdio" and remove the compatible string. The
> switch will use polling instead of interruptions.
>
> This series might inspire other drivers as well. Currently, most dsa
> driver implements a custom slave mii, normally only defining a
> phy_{read,write} and loading properties from an "mdio" OF node. Since
> fe7324b932, dsa generic code can do all that if the mdio node is named
> "mdio".  I believe most drivers could simply drop their slave mii
> implementations and add phy_{read,write} to the dsa_switch_ops. For
> drivers that look for an "mdio-like" node using a compatible string, it
> might need some type of transition to let vendors update their OF tree.
>
> Regards,
>
> Luiz
>

I might have forgotten to add a new line after the subject. It ate the
first paragraph. I'm top-posting it.

Regards,

Luiz

Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Luiz Angelo Daros de Luca]

Em qua., 29 de jun. de 2022 às 09:30, Arınç ÜNAL
<arinc.unal@arinc9.com> escreveu:
>
> On 29.06.2022 06:54, Luiz Angelo Daros de Luca wrote:
> > The last patch cleans all the deprecated code while keeping the kernel
> > messages. However, if there is no "mdio" node but there is a node with
> > the old compatible stings "realtek,smi-mdio", it will show an error. It
> > should still work but it will use polling instead of interruptions.
> >
> > My idea, if accepted, is to submit patches 1 and 2 now. After a
> > reasonable period, submit patch 3.
> >
> > I don't have an SMI-connected device and I'm asking for testers. It
> > would be nice to test the first 2 patches with:
>
> I'd love to test this on an Asus RT-AC88U which has got the
> smi-connected RTL8365MB switch but modifying the OpenWrt SDK to build
> for latest kernels is a really painful process. I know it's not related
> to this patch series but, does anyone know a more efficient way of
> building the kernel with rootfs with sufficent userspace tools? Like, am
> I supposed to use Buildroot, Yocto?

Hello Arinç,

You can backport those patches to mostly any device already using
rtl8365mb. The code it changes is mostly the same since the files
migrated to the realtek directory. However, you do need to backport
fe7324b932 (which is also easily applicable). I believe only the last
patch will conflict as some new functions were added to the
dsa_switch_ops that is being removed.

Regards,

Luiz

Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Alvin Šipraga]

Hi Luiz,

On Wed, Jun 29, 2022 at 01:43:45PM -0300, Luiz Angelo Daros de Luca wrote:
> This RFC patch series cleans realtek-smi custom slave mii bus. Since
> fe7324b932, dsa generic code provides everything needed for
> realtek-smi driver. For extra caution, this series should be applied
> in two steps: the first 2 patches introduce the new code path that
> uses dsa generic code. It will show a warning message if the tree
> contains deprecated references. It will still fall back to the old
> code path if an "mdio"
> is not found.

In principle I like your changes, but I'm not sure if what you are doing
is allowed, since DT is ABI. The fact that you have to split this into
two steps, with the first step warning about old "incompatible" DTs
(your point 3 below) before the second step breaks that compatibility,
suggests that you are aware that you could be breaking old DTs.

I'm not going to argue with you if you say "but the node with compatible
realtek,smi-mdio was also called mdio in the bindings, so it shouldn't
break old DTs", which is a valid point. But if that is your rationale,
then there's no need to split the series at all, right?

If you want to avoid that debate, what you could do instead is add a
const char *slave_mii_compatible; member to struct dsa_switch, and try
searching in dsa_switch_setup() for a child node with that compatible if
the lookup of a node named "mdio" fails. I don't know if this would help
you do the same thing with other drivers.

Btw, I think the first patch in the series is kind of pointless. You can
just do the rename of ds_ops_mdio to ds_ops in the last patch, adding
your justification in the commit message: "while we're at it, rename
ds_ops_mdio etc...".

Kind regards,
Alvin

> 
> >
> > The last patch cleans all the deprecated code while keeping the kernel
> > messages. However, if there is no "mdio" node but there is a node with
> > the old compatible stings "realtek,smi-mdio", it will show an error. It
> > should still work but it will use polling instead of interruptions.
> >
> > My idea, if accepted, is to submit patches 1 and 2 now. After a
> > reasonable period, submit patch 3.
> >
> > I don't have an SMI-connected device and I'm asking for testers. It
> > would be nice to test the first 2 patches with:
> > 1) "mdio" without a compatible string. It should work without warnings.
> > 2) "mdio" with a compatible string. It should work with a warning asking
> > to remove the compatible string
> > 3) "xxx" node with compatible string. It should work with a warning
> > asking to rename "xxx" to "mdio" and remove the compatible string
> >
> > In all those cases, the switch should still keep using interruptions.
> >
> > After that, the last patch can be applied. The same tests can be
> > performed:
> > 1) "mdio" without a compatible string. It should work without warnings.
> > 2) "mdio" with a compatible string. It should work with a warning asking
> > to remove the compatible string
> > 3) "xxx" node with compatible string. It should work with an error
> > asking to rename "xxx" to "mdio" and remove the compatible string. The
> > switch will use polling instead of interruptions.
> >
> > This series might inspire other drivers as well. Currently, most dsa
> > driver implements a custom slave mii, normally only defining a
> > phy_{read,write} and loading properties from an "mdio" OF node. Since
> > fe7324b932, dsa generic code can do all that if the mdio node is named
> > "mdio".  I believe most drivers could simply drop their slave mii
> > implementations and add phy_{read,write} to the dsa_switch_ops. For
> > drivers that look for an "mdio-like" node using a compatible string, it
> > might need some type of transition to let vendors update their OF tree.
> >
> > Regards,
> >
> > Luiz
> >
> 
> I might have forgotten to add a new line after the subject. It ate the
> first paragraph. I'm top-posting it.
> 
> Regards,
> 
> Luiz

Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Alvin Šipraga]

On Wed, Jun 29, 2022 at 06:14:56PM +0000, Alvin Šipraga wrote:
> If you want to avoid that debate, what you could do instead is add a
> const char *slave_mii_compatible; member to struct dsa_switch, and try
> searching in dsa_switch_setup() for a child node with that compatible if
> the lookup of a node named "mdio" fails. I don't know if this would help
> you do the same thing with other drivers.

Forgot to make it clear, but here I meant that a DSA driver could then
set ds->slave_mii_compatible = "realtek,smi-mdio" in its probe function
before calling dsa_register_switch(ds).

Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Luiz Angelo Daros de Luca]

> Hi Luiz,
>
> On Wed, Jun 29, 2022 at 01:43:45PM -0300, Luiz Angelo Daros de Luca wrote:
> > This RFC patch series cleans realtek-smi custom slave mii bus. Since
> > fe7324b932, dsa generic code provides everything needed for
> > realtek-smi driver. For extra caution, this series should be applied
> > in two steps: the first 2 patches introduce the new code path that
> > uses dsa generic code. It will show a warning message if the tree
> > contains deprecated references. It will still fall back to the old
> > code path if an "mdio"
> > is not found.
>
> In principle I like your changes, but I'm not sure if what you are doing
> is allowed, since DT is ABI. The fact that you have to split this into
> two steps, with the first step warning about old "incompatible" DTs
> (your point 3 below) before the second step breaks that compatibility,
> suggests that you are aware that you could be breaking old DTs.

Thanks Alvin for your review. Yes, that is a good question for the ML.
I don't know at what level we can break compatibility (DT and driver).
That's why it is a RFC.

> I'm not going to argue with you if you say "but the node with compatible
> realtek,smi-mdio was also called mdio in the bindings, so it shouldn't
> break old DTs", which is a valid point. But if that is your rationale,
> then there's no need to split the series at all, right?

The DT requires "realtek,smi-mdio" but also mentions the "mdio" name,
not a generic name as "mdioX". If we agree that the name "mdio" is
already required by the DT bindings, it is the driver implementation
that is not compliant. Even if we are not violating the DT bindings,
we are changing the driver behavior. That's why I suggested the
transition process. I do believe that it would be very, very rare to
name that mdio as anything other than "mdio" and even the driver
itself is too fresh to be widespread. In a non-RFC series, I would
also drop the "realtek,smi-mdio" compatible string from the bindings
(as it is back compatible).

> If you want to avoid that debate, what you could do instead is add a
> const char *slave_mii_compatible; member to struct dsa_switch, and try
> searching in dsa_switch_setup() for a child node with that compatible if
> the lookup of a node named "mdio" fails. I don't know if this would help
> you do the same thing with other drivers.

The DSA change to accept "mdio" was an improvement to avoid adding a
custom slave mdio when you already have a single mdio and just need to
point to a DT node. Adding compatible strings for that situation does
not make much sense as a compatible string is not necessary when you
are already restricting your case to a single mdio. For more complex
setups, you still need to create your own slave mdio implementation.
Some drivers already depend on the "mdio" name and this series is also
a suggestion for them to try their drivers dropping their custom slave
mdio implementations.

> Btw, I think the first patch in the series is kind of pointless. You can
> just do the rename of ds_ops_mdio to ds_ops in the last patch, adding
> your justification in the commit message: "while we're at it, rename
> ds_ops_mdio etc...".

As a RFC, I'm trying to split each change in such a way they can be
merged individually. I believe that the new names make it clearer why
we have two structures. Even if the idea behind this series did not
get accepted, that first patch might be useful for someone reading the
driver for the first time.

Regards,

>
> Kind regards,
> Alvin
>
> >
> > >
> > > The last patch cleans all the deprecated code while keeping the kernel
> > > messages. However, if there is no "mdio" node but there is a node with
> > > the old compatible stings "realtek,smi-mdio", it will show an error. It
> > > should still work but it will use polling instead of interruptions.
> > >
> > > My idea, if accepted, is to submit patches 1 and 2 now. After a
> > > reasonable period, submit patch 3.
> > >
> > > I don't have an SMI-connected device and I'm asking for testers. It
> > > would be nice to test the first 2 patches with:
> > > 1) "mdio" without a compatible string. It should work without warnings.
> > > 2) "mdio" with a compatible string. It should work with a warning asking
> > > to remove the compatible string
> > > 3) "xxx" node with compatible string. It should work with a warning
> > > asking to rename "xxx" to "mdio" and remove the compatible string
> > >
> > > In all those cases, the switch should still keep using interruptions.
> > >
> > > After that, the last patch can be applied. The same tests can be
> > > performed:
> > > 1) "mdio" without a compatible string. It should work without warnings.
> > > 2) "mdio" with a compatible string. It should work with a warning asking
> > > to remove the compatible string
> > > 3) "xxx" node with compatible string. It should work with an error
> > > asking to rename "xxx" to "mdio" and remove the compatible string. The
> > > switch will use polling instead of interruptions.
> > >
> > > This series might inspire other drivers as well. Currently, most dsa
> > > driver implements a custom slave mii, normally only defining a
> > > phy_{read,write} and loading properties from an "mdio" OF node. Since
> > > fe7324b932, dsa generic code can do all that if the mdio node is named
> > > "mdio".  I believe most drivers could simply drop their slave mii
> > > implementations and add phy_{read,write} to the dsa_switch_ops. For
> > > drivers that look for an "mdio-like" node using a compatible string, it
> > > might need some type of transition to let vendors update their OF tree.
> > >
> > > Regards,
> > >
> > > Luiz
> > >
> >
> > I might have forgotten to add a new line after the subject. It ate the
> > first paragraph. I'm top-posting it.
> >
> > Regards,
> >
> > Luiz

Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Alvin Šipraga]

On Thu, Jun 30, 2022 at 02:05:39PM -0300, Luiz Angelo Daros de Luca wrote:
> > If you want to avoid that debate, what you could do instead is add a
> > const char *slave_mii_compatible; member to struct dsa_switch, and try
> > searching in dsa_switch_setup() for a child node with that compatible if
> > the lookup of a node named "mdio" fails. I don't know if this would help
> > you do the same thing with other drivers.
> 
> The DSA change to accept "mdio" was an improvement to avoid adding a
> custom slave mdio when you already have a single mdio and just need to
> point to a DT node. Adding compatible strings for that situation does
> not make much sense as a compatible string is not necessary when you
> are already restricting your case to a single mdio. For more complex
> setups, you still need to create your own slave mdio implementation.
> Some drivers already depend on the "mdio" name and this series is also
> a suggestion for them to try their drivers dropping their custom slave
> mdio implementations.

It was just a suggestion for how to achieve what you want (less code in
the driver) without breaking backward compatibility. Obviously newly
submitted drivers shouldn't use the interface for the reasons you just
gave.

Whether it is a good idea to add such an interface to the DSA core,
idk. Personally I think it's fair enough, but I also don't mind the
status quo. The code you're removing is not exactly doing any harm.

I think my suggestion only makes sense if you can show that other
drivers can benefit as well.

Kind regards,
Alvin

Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Vladimir Oltean]

On Thu, Jun 30, 2022 at 02:05:39PM -0300, Luiz Angelo Daros de Luca wrote:
> > Hi Luiz,
> >
> > On Wed, Jun 29, 2022 at 01:43:45PM -0300, Luiz Angelo Daros de Luca wrote:
> > > This RFC patch series cleans realtek-smi custom slave mii bus. Since
> > > fe7324b932, dsa generic code provides everything needed for
> > > realtek-smi driver. For extra caution, this series should be applied
> > > in two steps: the first 2 patches introduce the new code path that
> > > uses dsa generic code. It will show a warning message if the tree
> > > contains deprecated references. It will still fall back to the old
> > > code path if an "mdio"
> > > is not found.
> >
> > In principle I like your changes, but I'm not sure if what you are doing
> > is allowed, since DT is ABI. The fact that you have to split this into
> > two steps, with the first step warning about old "incompatible" DTs
> > (your point 3 below) before the second step breaks that compatibility,
> > suggests that you are aware that you could be breaking old DTs.
> 
> Thanks Alvin for your review. Yes, that is a good question for the ML.
> I don't know at what level we can break compatibility (DT and driver).
> That's why it is a RFC.

DT bindings are only extended in backwards-compatible ways. Only in the
case where you can prove that there is no DT user of a certain binding,
and that none should appear either, is when you can consider breaking
the backward compatibility. The idea here is that old DT blobs may live
forever and be provided by fixed firmware such as U-Boot, you can't
really force anyone to update them.

> > I'm not going to argue with you if you say "but the node with compatible
> > realtek,smi-mdio was also called mdio in the bindings, so it shouldn't
> > break old DTs", which is a valid point. But if that is your rationale,
> > then there's no need to split the series at all, right?
> 
> The DT requires "realtek,smi-mdio" but also mentions the "mdio" name,
> not a generic name as "mdioX". If we agree that the name "mdio" is
> already required by the DT bindings, it is the driver implementation
> that is not compliant. Even if we are not violating the DT bindings,
> we are changing the driver behavior. That's why I suggested the
> transition process. I do believe that it would be very, very rare to
> name that mdio as anything other than "mdio" and even the driver
> itself is too fresh to be widespread. In a non-RFC series, I would
> also drop the "realtek,smi-mdio" compatible string from the bindings
> (as it is back compatible).

Technically the MDIO node should be named "mdio", at least that's what
Documentation/devicetree/bindings/net/mdio.yaml says, that it should be
"mdio" or "mdio@something". And since the #address-cells of the DSA
switch OF node itself (parent of MDIO node) is 0, it can't really be
"mdio@something", so that leaves us with "mdio".

However I don't think there is any validation for the node name.

> > If you want to avoid that debate, what you could do instead is add a
> > const char *slave_mii_compatible; member to struct dsa_switch, and try
> > searching in dsa_switch_setup() for a child node with that compatible if
> > the lookup of a node named "mdio" fails. I don't know if this would help
> > you do the same thing with other drivers.
> 
> The DSA change to accept "mdio" was an improvement to avoid adding a
> custom slave mdio when you already have a single mdio and just need to
> point to a DT node. Adding compatible strings for that situation does
> not make much sense as a compatible string is not necessary when you
> are already restricting your case to a single mdio. For more complex
> setups, you still need to create your own slave mdio implementation.
> Some drivers already depend on the "mdio" name and this series is also
> a suggestion for them to try their drivers dropping their custom slave
> mdio implementations.

I think we are going down a slippery slope here. DSA providing generic
code to register an MDIO bus is not something I'd consider core
functionality, and I don't consider any duplication of concerns or logic
if drivers register their own MDIO buses.

Here you are effectively proposing to gratuitously break the DT binding,
the obvious question is, what is there to even gain from this?

And why do you even need to remove the compatible string from the MDIO
node, can't you just ignore it, does it bother you in any way?

Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

[Linus Walleij]

On Wed, Jul 6, 2022 at 5:29 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Thu, Jun 30, 2022 at 02:05:39PM -0300, Luiz Angelo Daros de Luca wrote:

> > > In principle I like your changes, but I'm not sure if what you are doing
> > > is allowed, since DT is ABI. The fact that you have to split this into
> > > two steps, with the first step warning about old "incompatible" DTs
> > > (your point 3 below) before the second step breaks that compatibility,
> > > suggests that you are aware that you could be breaking old DTs.
> >
> > Thanks Alvin for your review. Yes, that is a good question for the ML.
> > I don't know at what level we can break compatibility (DT and driver).
> > That's why it is a RFC.
>
> DT bindings are only extended in backwards-compatible ways. Only in the
> case where you can prove that there is no DT user of a certain binding,
> and that none should appear either, is when you can consider breaking
> the backward compatibility. The idea here is that old DT blobs may live
> forever and be provided by fixed firmware such as U-Boot, you can't
> really force anyone to update them.

We break it when it makes sense.

The central question is to ascertain if there are actually binary DTBs
deployed with these bindings, in mass-market products, and these are
not upgraded in tandem with the kernel.

A mistake (IMO) in the early days of DT was to assume that
it was used with Open Firmware (OF) which is like ACPI, a kind of BIOS.
Most users of DT do not use OF, the only thing we ever see relating
to it is the of_* prefix.

People actually using open firmware would embed the DTB with the
open firmware and flash it into a (desktop) computer as a blob, pretty
much like how the ACPI BIOS works now.

It turns out the majority of contemporary users of DT don't use DTBs
like this at all: instead they compile the kernel and the DTB, then flash
both into the platform at the same time. There is even the FIT format for
U-Boot which is a package of both kernel and DT and whatnot.

Actually very few people flash their DTB in such a way that it cannot get
upgraded, and in fact most flash both at the same time, after building
both from source. In that case it doesn't matter if we break compatibility.

While we strive to keep DT schemas strict and compatible (it is a good
ambition) I would reverse the burden of proof for backward compatibility:
if it can not be proven that irrevocable DTBs have been deployed, and
that kernels may get upgraded independently of the DTB, using this
specific binding, then it is fine to change it in incompatible ways if
we need to.

It could also be that the DT bindings started to get used
with another operating system. But these things have to be demonstrated,
they are the rare cases and should not be the assumption, as if a
DT binding is immediately used in a myriad of places the second
we merge it to Torvald's tree. Such adoption in the real world happens
much later.

If the only specimens are inside a company that has not yet released
any products we can certainly change it. What we don't want is the
general public running into these incompatibilities.

Notice that as we discuss this, I see some people being requested to
reflash their (ACPI) bioses rather than put fixes in the kernel for
erroneous ACPI DSDT:s. Not for end users, but for people working
with prototypes still in development. "Go fix your DSDT BIOS tables".

Yours,
Linus Walleij