[PATCH net-next v4 00/11] net: dsa: realtek: variants to drivers, interfaces to a common module

[PATCH net-next v4 00/11] net: dsa: realtek: variants to drivers, interfaces to a common module

[Luiz Angelo Daros de Luca]

The current driver consists of two interface modules (SMI and MDIO) and
two family/variant modules (RTL8365MB and RTL8366RB). The SMI and MDIO
modules serve as the platform and MDIO drivers, respectively, calling
functions from the variant modules. In this setup, one interface module
can be loaded independently of the other, but both variants must be
loaded (if not disabled at build time) for any type of interface. This
approach doesn't scale well, especially with the addition of more switch
variants (e.g., RTL8366B), leading to loaded but unused modules.
Additionally, this also seems upside down, as the specific driver code
normally depends on the more generic functions and not the other way
around.

Each variant module was converted into real drivers, serving as both a
platform driver (for switches connected using the SMI interface) and an
MDIO driver (for MDIO-connected switches). The relationship between the
variant and interface modules is reversed, with the variant module now
calling both interface functions (if not disabled at build time). While
in most devices only one interface is likely used, the interface code is
significantly smaller than a variant module, consuming fewer resources
than the previous code. With variant modules now functioning as real
drivers, compatible strings are published only in a single variant
module, preventing conflicts.

The patch series introduces a new common module for functions shared by
both variants. This module also absorbs the two previous interface
modules, as they would always be loaded anyway.

The series relocates the user MII driver from realtek-smi to rtl83xx. It
is now used by MDIO-connected switches instead of the generic DSA
driver. There's a change in how this driver locates the MDIO node. It
now only searches for a child node named "mdio".

The dsa_switch in realtek_priv->ds is now embedded in the struct. It is
always in use and avoids dynamic memory allocation.

Testing has been performed with an RTL8367S (rtl8365mb) using MDIO
interface and an RTL8366RB (rtl8366) with SMI interface.

Luiz

---

Changes:

v3-v4:
1) Changed Makefile to use ifdef instead of dynamic variable names.
2) Added comments for all exported symbols.
3) Migrated exported symbols to REALTEK_DSA namespace.
4) renamed realtek_common to rtl83xx.
5) put the mdio node just after registration and not in driver remove.
6) rtl83xx_probe now receives a struct with regmap read/write functions
   and build regmap_config dynamically.
7) pulled into a new patch the realtek_priv change from "common
   realtek-dsa module".
8) pulled into a new patch the user_mii_bus setup changes from "migrate
   user_mii_bus setup to realtek-dsa".
9) removed the revert "net: dsa: OF-ware slave_mii_bus" patch from the
   series.

v2-v3:
1) Look for the MDIO bus searching for a child node named "mdio" instead
   of the compatible string.
2) Removed the check for a phy-handle in ports. ds->user_mii_bus will
   not be used anymore.
3) Dropped comments for realtek_common_{probe,register_switch}.
4) Fixed a compile error in "net: dsa: OF-ware slave_mii_bus".
5) Used the wrapper realtek_smi_driver_register instead of
   platform_driver_register.

v1-v2:
1)  Renamed realtek_common module to realtek-dsa.
2)  Removed the warning when the MDIO node is not named "mdio."
3)  ds->user_mii_bus is only assigned if all user ports do not have a
    phy-handle.
4)  of_node_put is now back to the driver remove method.
5)  Renamed realtek_common_probe_{pre,post} to
    realtek_common_{probe,register_switch}.
6)  Added some comments for realtek_common_{probe,register_switch}.
7)  Using dev_err_probe whenever possible.
8)  Embedded priv->ds into realtek_priv, removing its dynamic
    allocation.
9)  Fixed realtek-common.h macros.
10) Save and check the return value in functions, even when it is the
    last one.
11) Added the #if expression as a comment to #else and #endif in header
    files.
12) Unregister the platform and the MDIO driver in the reverse order
    they are registered.
13) Unregister the first driver if the second one failed to register.
14) Added the revert patch for "net: dsa: OF-ware slave_mii_bus."

Luiz Angelo Daros de Luca (11):
  net: dsa: realtek: drop cleanup from realtek_ops
  net: dsa: realtek: introduce REALTEK_DSA namespace
  net: dsa: realtek: convert variants into real drivers
  net: dsa: realtek: keep variant reference in realtek_priv
  net: dsa: realtek: common rtl83xx module
  net: dsa: realtek: merge rtl83xx and interface modules into
    realtek-dsa
  net: dsa: realtek: get internal MDIO node by name
  net: dsa: realtek: clean user_mii_bus setup
  net: dsa: realtek: migrate user_mii_bus setup to realtek-dsa
  net: dsa: realtek: use the same mii bus driver for both interfaces
  net: dsa: realtek: embed dsa_switch into realtek_priv

 drivers/net/dsa/realtek/Kconfig        |  20 +-
 drivers/net/dsa/realtek/Makefile       |  13 +-
 drivers/net/dsa/realtek/realtek-mdio.c | 211 +++++--------------
 drivers/net/dsa/realtek/realtek-mdio.h |  48 +++++
 drivers/net/dsa/realtek/realtek-smi.c  | 277 +++++--------------------
 drivers/net/dsa/realtek/realtek-smi.h  |  48 +++++
 drivers/net/dsa/realtek/realtek.h      |  12 +-
 drivers/net/dsa/realtek/rtl8365mb.c    | 126 ++++++-----
 drivers/net/dsa/realtek/rtl8366-core.c |  22 +-
 drivers/net/dsa/realtek/rtl8366rb.c    | 119 ++++++-----
 drivers/net/dsa/realtek/rtl83xx.c      | 267 ++++++++++++++++++++++++
 drivers/net/dsa/realtek/rtl83xx.h      |  22 ++
 12 files changed, 667 insertions(+), 518 deletions(-)
 create mode 100644 drivers/net/dsa/realtek/realtek-mdio.h
 create mode 100644 drivers/net/dsa/realtek/realtek-smi.h
 create mode 100644 drivers/net/dsa/realtek/rtl83xx.c
 create mode 100644 drivers/net/dsa/realtek/rtl83xx.h

-- 
2.43.0

[PATCH net-next v4 01/11] net: dsa: realtek: drop cleanup from realtek_ops

[Luiz Angelo Daros de Luca]

It was never used and never referenced.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/realtek/realtek.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 790488e9c667..e9ee778665b2 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -91,7 +91,6 @@ struct realtek_ops {
 	int	(*detect)(struct realtek_priv *priv);
 	int	(*reset_chip)(struct realtek_priv *priv);
 	int	(*setup)(struct realtek_priv *priv);
-	void	(*cleanup)(struct realtek_priv *priv);
 	int	(*get_mib_counter)(struct realtek_priv *priv,
 				   int port,
 				   struct rtl8366_mib_counter *mib,
-- 
2.43.0

[PATCH net-next v4 02/11] net: dsa: realtek: introduce REALTEK_DSA namespace

[Luiz Angelo Daros de Luca]

Create a namespace to group the exported symbols.

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/rtl8365mb.c    |  2 ++
 drivers/net/dsa/realtek/rtl8366-core.c | 22 +++++++++++-----------
 drivers/net/dsa/realtek/rtl8366rb.c    |  2 ++
 5 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 292e6d087e8b..df214b2f60d1 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -288,3 +288,5 @@ mdio_module_driver(realtek_mdio_driver);
 MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
 MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(REALTEK_DSA);
+
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 755546ed8db6..f628b54de538 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -565,3 +565,5 @@ module_platform_driver(realtek_smi_driver);
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(REALTEK_DSA);
+
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index b072045eb154..462d5a9a280e 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -2177,3 +2177,5 @@ EXPORT_SYMBOL_GPL(rtl8365mb_variant);
 MODULE_AUTHOR("Alvin Šipraga <alsi@bang-olufsen.dk>");
 MODULE_DESCRIPTION("Driver for RTL8365MB-VC ethernet switch");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(REALTEK_DSA);
+
diff --git a/drivers/net/dsa/realtek/rtl8366-core.c b/drivers/net/dsa/realtek/rtl8366-core.c
index 59f98d2c8769..7c6520ba3a26 100644
--- a/drivers/net/dsa/realtek/rtl8366-core.c
+++ b/drivers/net/dsa/realtek/rtl8366-core.c
@@ -34,7 +34,7 @@ int rtl8366_mc_is_used(struct realtek_priv *priv, int mc_index, int *used)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rtl8366_mc_is_used);
+EXPORT_SYMBOL_NS_GPL(rtl8366_mc_is_used, REALTEK_DSA);
 
 /**
  * rtl8366_obtain_mc() - retrieve or allocate a VLAN member configuration
@@ -187,7 +187,7 @@ int rtl8366_set_vlan(struct realtek_priv *priv, int vid, u32 member,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(rtl8366_set_vlan);
+EXPORT_SYMBOL_NS_GPL(rtl8366_set_vlan, REALTEK_DSA);
 
 int rtl8366_set_pvid(struct realtek_priv *priv, unsigned int port,
 		     unsigned int vid)
@@ -217,7 +217,7 @@ int rtl8366_set_pvid(struct realtek_priv *priv, unsigned int port,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rtl8366_set_pvid);
+EXPORT_SYMBOL_NS_GPL(rtl8366_set_pvid, REALTEK_DSA);
 
 int rtl8366_enable_vlan4k(struct realtek_priv *priv, bool enable)
 {
@@ -243,7 +243,7 @@ int rtl8366_enable_vlan4k(struct realtek_priv *priv, bool enable)
 	priv->vlan4k_enabled = enable;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rtl8366_enable_vlan4k);
+EXPORT_SYMBOL_NS_GPL(rtl8366_enable_vlan4k, REALTEK_DSA);
 
 int rtl8366_enable_vlan(struct realtek_priv *priv, bool enable)
 {
@@ -265,7 +265,7 @@ int rtl8366_enable_vlan(struct realtek_priv *priv, bool enable)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(rtl8366_enable_vlan);
+EXPORT_SYMBOL_NS_GPL(rtl8366_enable_vlan, REALTEK_DSA);
 
 int rtl8366_reset_vlan(struct realtek_priv *priv)
 {
@@ -290,7 +290,7 @@ int rtl8366_reset_vlan(struct realtek_priv *priv)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
+EXPORT_SYMBOL_NS_GPL(rtl8366_reset_vlan, REALTEK_DSA);
 
 int rtl8366_vlan_add(struct dsa_switch *ds, int port,
 		     const struct switchdev_obj_port_vlan *vlan,
@@ -345,7 +345,7 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rtl8366_vlan_add);
+EXPORT_SYMBOL_NS_GPL(rtl8366_vlan_add, REALTEK_DSA);
 
 int rtl8366_vlan_del(struct dsa_switch *ds, int port,
 		     const struct switchdev_obj_port_vlan *vlan)
@@ -389,7 +389,7 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rtl8366_vlan_del);
+EXPORT_SYMBOL_NS_GPL(rtl8366_vlan_del, REALTEK_DSA);
 
 void rtl8366_get_strings(struct dsa_switch *ds, int port, u32 stringset,
 			 uint8_t *data)
@@ -403,7 +403,7 @@ void rtl8366_get_strings(struct dsa_switch *ds, int port, u32 stringset,
 	for (i = 0; i < priv->num_mib_counters; i++)
 		ethtool_puts(&data, priv->mib_counters[i].name);
 }
-EXPORT_SYMBOL_GPL(rtl8366_get_strings);
+EXPORT_SYMBOL_NS_GPL(rtl8366_get_strings, REALTEK_DSA);
 
 int rtl8366_get_sset_count(struct dsa_switch *ds, int port, int sset)
 {
@@ -417,7 +417,7 @@ int rtl8366_get_sset_count(struct dsa_switch *ds, int port, int sset)
 
 	return priv->num_mib_counters;
 }
-EXPORT_SYMBOL_GPL(rtl8366_get_sset_count);
+EXPORT_SYMBOL_NS_GPL(rtl8366_get_sset_count, REALTEK_DSA);
 
 void rtl8366_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data)
 {
@@ -441,4 +441,4 @@ void rtl8366_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data)
 		data[i] = mibvalue;
 	}
 }
-EXPORT_SYMBOL_GPL(rtl8366_get_ethtool_stats);
+EXPORT_SYMBOL_NS_GPL(rtl8366_get_ethtool_stats, REALTEK_DSA);
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index e3b6a470ca67..baeab5445d99 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1938,3 +1938,5 @@ EXPORT_SYMBOL_GPL(rtl8366rb_variant);
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Driver for RTL8366RB ethernet switch");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(REALTEK_DSA);
+
-- 
2.43.0

[PATCH net-next v4 03/11] net: dsa: realtek: convert variants into real drivers

[Luiz Angelo Daros de Luca]

Previously, the interface modules realtek-smi and realtek-mdio served as
a platform and an MDIO driver, respectively. Each interface module
redundantly specified the same compatible strings for both variants and
referenced symbols from the variants.

Now, each variant module has been transformed into a unified driver
serving both as a platform and an MDIO driver. This modification
reverses the relationship between the interface and variant modules,
with the variant module now utilizing symbols from the interface
modules.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/Kconfig        | 20 ++-----
 drivers/net/dsa/realtek/realtek-mdio.c | 71 ++++++++++++++----------
 drivers/net/dsa/realtek/realtek-mdio.h | 48 ++++++++++++++++
 drivers/net/dsa/realtek/realtek-smi.c  | 76 +++++++++++++++-----------
 drivers/net/dsa/realtek/realtek-smi.h  | 48 ++++++++++++++++
 drivers/net/dsa/realtek/rtl8365mb.c    | 54 +++++++++++++++++-
 drivers/net/dsa/realtek/rtl8366rb.c    | 54 +++++++++++++++++-
 7 files changed, 294 insertions(+), 77 deletions(-)
 create mode 100644 drivers/net/dsa/realtek/realtek-mdio.h
 create mode 100644 drivers/net/dsa/realtek/realtek-smi.h

diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
index 060165a85fb7..9d182fde11b4 100644
--- a/drivers/net/dsa/realtek/Kconfig
+++ b/drivers/net/dsa/realtek/Kconfig
@@ -16,37 +16,29 @@ menuconfig NET_DSA_REALTEK
 if NET_DSA_REALTEK
 
 config NET_DSA_REALTEK_MDIO
-	tristate "Realtek MDIO interface driver"
+	tristate "Realtek MDIO interface support"
 	depends on OF
-	depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
-	depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
-	depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
 	help
 	  Select to enable support for registering switches configured
 	  through MDIO.
 
 config NET_DSA_REALTEK_SMI
-	tristate "Realtek SMI interface driver"
+	tristate "Realtek SMI interface support"
 	depends on OF
-	depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
-	depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
-	depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
 	help
 	  Select to enable support for registering switches connected
 	  through SMI.
 
 config NET_DSA_REALTEK_RTL8365MB
-	tristate "Realtek RTL8365MB switch subdriver"
-	imply NET_DSA_REALTEK_SMI
-	imply NET_DSA_REALTEK_MDIO
+	tristate "Realtek RTL8365MB switch driver"
+	depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
 	select NET_DSA_TAG_RTL8_4
 	help
 	  Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
 
 config NET_DSA_REALTEK_RTL8366RB
-	tristate "Realtek RTL8366RB switch subdriver"
-	imply NET_DSA_REALTEK_SMI
-	imply NET_DSA_REALTEK_MDIO
+	tristate "Realtek RTL8366RB switch driver"
+	depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
 	select NET_DSA_TAG_RTL4_A
 	help
 	  Select to enable support for Realtek RTL8366RB.
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index df214b2f60d1..22a63f41e3f2 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -25,6 +25,7 @@
 #include <linux/regmap.h>
 
 #include "realtek.h"
+#include "realtek-mdio.h"
 
 /* Read/write via mdiobus */
 #define REALTEK_MDIO_CTRL0_REG		31
@@ -140,7 +141,20 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
 	.disable_locking = true,
 };
 
-static int realtek_mdio_probe(struct mdio_device *mdiodev)
+/**
+ * realtek_mdio_probe() - Probe a platform device for an MDIO-connected switch
+ * @pdev: platform_device to probe on.
+ *
+ * This function should be used as the .probe in an mdio_driver. It
+ * initializes realtek_priv and read data from the device-tree node. The switch
+ * is hard resetted if a method is provided. It checks the switch chip ID and,
+ * finally, a DSA switch is registered.
+ *
+ * Context: Any context. Takes and releases priv->map_lock.
+ * Return: Returns 0 on success, a negative error on failure.
+ *
+ */
+int realtek_mdio_probe(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv;
 	struct device *dev = &mdiodev->dev;
@@ -235,8 +249,21 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(realtek_mdio_probe, REALTEK_DSA);
 
-static void realtek_mdio_remove(struct mdio_device *mdiodev)
+/**
+ * realtek_mdio_remove() - Remove the driver of an MDIO-connected switch
+ * @pdev: platform_device to probe on.
+ *
+ * This function should be used as the .remove_new in an mdio_driver. First
+ * it unregisters the DSA switch and cleans internal data. If a method is
+ * provided, the hard reset is asserted to avoid traffic leakage.
+ *
+ * Context: Any context.
+ * Return: Nothing.
+ *
+ */
+void realtek_mdio_remove(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
 
@@ -249,8 +276,20 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
 	if (priv->reset)
 		gpiod_set_value(priv->reset, 1);
 }
+EXPORT_SYMBOL_NS_GPL(realtek_mdio_remove, REALTEK_DSA);
 
-static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
+/**
+ * realtek_mdio_shutdown() - Shutdown the driver of a MDIO-connected switch
+ * @pdev: platform_device to probe on.
+ *
+ * This function should be used as the .shutdown in an mdio_driver. It shuts
+ * down the DSA switch and cleans the platform driver data.
+ *
+ * Context: Any context.
+ * Return: Nothing.
+ *
+ */
+void realtek_mdio_shutdown(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
 
@@ -261,32 +300,8 @@ static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
 
 	dev_set_drvdata(&mdiodev->dev, NULL);
 }
-
-static const struct of_device_id realtek_mdio_of_match[] = {
-#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB)
-	{ .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, },
-#endif
-#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
-	{ .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, },
-#endif
-	{ /* sentinel */ },
-};
-MODULE_DEVICE_TABLE(of, realtek_mdio_of_match);
-
-static struct mdio_driver realtek_mdio_driver = {
-	.mdiodrv.driver = {
-		.name = "realtek-mdio",
-		.of_match_table = realtek_mdio_of_match,
-	},
-	.probe  = realtek_mdio_probe,
-	.remove = realtek_mdio_remove,
-	.shutdown = realtek_mdio_shutdown,
-};
-
-mdio_module_driver(realtek_mdio_driver);
+EXPORT_SYMBOL_NS_GPL(realtek_mdio_shutdown, REALTEK_DSA);
 
 MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
 MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
 MODULE_LICENSE("GPL");
-MODULE_IMPORT_NS(REALTEK_DSA);
-
diff --git a/drivers/net/dsa/realtek/realtek-mdio.h b/drivers/net/dsa/realtek/realtek-mdio.h
new file mode 100644
index 000000000000..ee70f6a5b8ff
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-mdio.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _REALTEK_MDIO_H
+#define _REALTEK_MDIO_H
+
+#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO)
+
+static inline int realtek_mdio_driver_register(struct mdio_driver *drv)
+{
+	return mdio_driver_register(drv);
+}
+
+static inline void realtek_mdio_driver_unregister(struct mdio_driver *drv)
+{
+	mdio_driver_unregister(drv);
+}
+
+int realtek_mdio_probe(struct mdio_device *mdiodev);
+void realtek_mdio_remove(struct mdio_device *mdiodev);
+void realtek_mdio_shutdown(struct mdio_device *mdiodev);
+
+#else /* IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO) */
+
+static inline int realtek_mdio_driver_register(struct mdio_driver *drv)
+{
+	return 0;
+}
+
+static inline void realtek_mdio_driver_unregister(struct mdio_driver *drv)
+{
+}
+
+static inline int realtek_mdio_probe(struct mdio_device *mdiodev)
+{
+	return -ENOENT;
+}
+
+static inline void realtek_mdio_remove(struct mdio_device *mdiodev)
+{
+}
+
+static inline void realtek_mdio_shutdown(struct mdio_device *mdiodev)
+{
+}
+
+#endif /* IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO) */
+
+#endif /* _REALTEK_MDIO_H */
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index f628b54de538..618547befa13 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -40,6 +40,7 @@
 #include <linux/if_bridge.h>
 
 #include "realtek.h"
+#include "realtek-smi.h"
 
 #define REALTEK_SMI_ACK_RETRY_COUNT		5
 
@@ -408,7 +409,20 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 	return ret;
 }
 
-static int realtek_smi_probe(struct platform_device *pdev)
+/**
+ * realtek_smi_probe() - Probe a platform device for an SMI-connected switch
+ * @pdev: platform_device to probe on.
+ *
+ * This function should be used as the .probe in a platform_driver. It
+ * initializes realtek_priv and read data from the device-tree node. The switch
+ * is hard resetted if a method is provided. It checks the switch chip ID and,
+ * finally, a DSA switch is registered.
+ *
+ * Context: Any context. Takes and releases priv->map_lock.
+ * Return: Returns 0 on success, a negative error on failure.
+ *
+ */
+int realtek_smi_probe(struct platform_device *pdev)
 {
 	const struct realtek_variant *var;
 	struct device *dev = &pdev->dev;
@@ -505,8 +519,21 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	}
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(realtek_smi_probe, REALTEK_DSA);
 
-static void realtek_smi_remove(struct platform_device *pdev)
+/**
+ * realtek_smi_remove() - Remove the driver of a SMI-connected switch
+ * @pdev: platform_device to probe on.
+ *
+ * This function should be used as the .remove_new in a platform_driver. First
+ * it unregisters the DSA switch and cleans internal data. If a method is
+ * provided, the hard reset is asserted to avoid traffic leakage.
+ *
+ * Context: Any context.
+ * Return: Nothing.
+ *
+ */
+void realtek_smi_remove(struct platform_device *pdev)
 {
 	struct realtek_priv *priv = platform_get_drvdata(pdev);
 
@@ -521,8 +548,20 @@ static void realtek_smi_remove(struct platform_device *pdev)
 	if (priv->reset)
 		gpiod_set_value(priv->reset, 1);
 }
+EXPORT_SYMBOL_NS_GPL(realtek_smi_remove, REALTEK_DSA);
 
-static void realtek_smi_shutdown(struct platform_device *pdev)
+/**
+ * realtek_smi_shutdown() - Shutdown the driver of a SMI-connected switch
+ * @pdev: platform_device to probe on.
+ *
+ * This function should be used as the .shutdown in a platform_driver. It shuts
+ * down the DSA switch and cleans the platform driver data.
+ *
+ * Context: Any context.
+ * Return: Nothing.
+ *
+ */
+void realtek_smi_shutdown(struct platform_device *pdev)
 {
 	struct realtek_priv *priv = platform_get_drvdata(pdev);
 
@@ -533,37 +572,8 @@ static void realtek_smi_shutdown(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 }
-
-static const struct of_device_id realtek_smi_of_match[] = {
-#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB)
-	{
-		.compatible = "realtek,rtl8366rb",
-		.data = &rtl8366rb_variant,
-	},
-#endif
-#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
-	{
-		.compatible = "realtek,rtl8365mb",
-		.data = &rtl8365mb_variant,
-	},
-#endif
-	{ /* sentinel */ },
-};
-MODULE_DEVICE_TABLE(of, realtek_smi_of_match);
-
-static struct platform_driver realtek_smi_driver = {
-	.driver = {
-		.name = "realtek-smi",
-		.of_match_table = realtek_smi_of_match,
-	},
-	.probe  = realtek_smi_probe,
-	.remove_new = realtek_smi_remove,
-	.shutdown = realtek_smi_shutdown,
-};
-module_platform_driver(realtek_smi_driver);
+EXPORT_SYMBOL_NS_GPL(realtek_smi_shutdown, REALTEK_DSA);
 
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
 MODULE_LICENSE("GPL");
-MODULE_IMPORT_NS(REALTEK_DSA);
-
diff --git a/drivers/net/dsa/realtek/realtek-smi.h b/drivers/net/dsa/realtek/realtek-smi.h
new file mode 100644
index 000000000000..ea49a2edd3c8
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-smi.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _REALTEK_SMI_H
+#define _REALTEK_SMI_H
+
+#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI)
+
+static inline int realtek_smi_driver_register(struct platform_driver *drv)
+{
+	return platform_driver_register(drv);
+}
+
+static inline void realtek_smi_driver_unregister(struct platform_driver *drv)
+{
+	platform_driver_unregister(drv);
+}
+
+int realtek_smi_probe(struct platform_device *pdev);
+void realtek_smi_remove(struct platform_device *pdev);
+void realtek_smi_shutdown(struct platform_device *pdev);
+
+#else /* IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI) */
+
+static inline int realtek_smi_driver_register(struct platform_driver *drv)
+{
+	return 0;
+}
+
+static inline void realtek_smi_driver_unregister(struct platform_driver *drv)
+{
+}
+
+static inline int realtek_smi_probe(struct platform_device *pdev)
+{
+	return -ENOENT;
+}
+
+static inline void realtek_smi_remove(struct platform_device *pdev)
+{
+}
+
+static inline void realtek_smi_shutdown(struct platform_device *pdev)
+{
+}
+
+#endif /* IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI) */
+
+#endif  /* _REALTEK_SMI_H */
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 462d5a9a280e..bd76bcf5fa44 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -101,6 +101,8 @@
 #include <linux/if_vlan.h>
 
 #include "realtek.h"
+#include "realtek-smi.h"
+#include "realtek-mdio.h"
 
 /* Family-specific data and limits */
 #define RTL8365MB_PHYADDRMAX		7
@@ -2172,7 +2174,57 @@ const struct realtek_variant rtl8365mb_variant = {
 	.cmd_write = 0xb8,
 	.chip_data_sz = sizeof(struct rtl8365mb),
 };
-EXPORT_SYMBOL_GPL(rtl8365mb_variant);
+
+static const struct of_device_id rtl8365mb_of_match[] = {
+	{ .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rtl8365mb_of_match);
+
+static struct platform_driver rtl8365mb_smi_driver = {
+	.driver = {
+		.name = "rtl8365mb-smi",
+		.of_match_table = rtl8365mb_of_match,
+	},
+	.probe  = realtek_smi_probe,
+	.remove_new = realtek_smi_remove,
+	.shutdown = realtek_smi_shutdown,
+};
+
+static struct mdio_driver rtl8365mb_mdio_driver = {
+	.mdiodrv.driver = {
+		.name = "rtl8365mb-mdio",
+		.of_match_table = rtl8365mb_of_match,
+	},
+	.probe  = realtek_mdio_probe,
+	.remove = realtek_mdio_remove,
+	.shutdown = realtek_mdio_shutdown,
+};
+
+static int rtl8365mb_init(void)
+{
+	int ret;
+
+	ret = realtek_mdio_driver_register(&rtl8365mb_mdio_driver);
+	if (ret)
+		return ret;
+
+	ret = realtek_smi_driver_register(&rtl8365mb_smi_driver);
+	if (ret) {
+		realtek_mdio_driver_unregister(&rtl8365mb_mdio_driver);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(rtl8365mb_init);
+
+static void __exit rtl8365mb_exit(void)
+{
+	realtek_smi_driver_unregister(&rtl8365mb_smi_driver);
+	realtek_mdio_driver_unregister(&rtl8365mb_mdio_driver);
+}
+module_exit(rtl8365mb_exit);
 
 MODULE_AUTHOR("Alvin Šipraga <alsi@bang-olufsen.dk>");
 MODULE_DESCRIPTION("Driver for RTL8365MB-VC ethernet switch");
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index baeab5445d99..74e14b73a1ec 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -23,6 +23,8 @@
 #include <linux/regmap.h>
 
 #include "realtek.h"
+#include "realtek-smi.h"
+#include "realtek-mdio.h"
 
 #define RTL8366RB_PORT_NUM_CPU		5
 #define RTL8366RB_NUM_PORTS		6
@@ -1933,7 +1935,57 @@ const struct realtek_variant rtl8366rb_variant = {
 	.cmd_write = 0xa8,
 	.chip_data_sz = sizeof(struct rtl8366rb),
 };
-EXPORT_SYMBOL_GPL(rtl8366rb_variant);
+
+static const struct of_device_id rtl8366rb_of_match[] = {
+	{ .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rtl8366rb_of_match);
+
+static struct platform_driver rtl8366rb_smi_driver = {
+	.driver = {
+		.name = "rtl8366rb-smi",
+		.of_match_table = rtl8366rb_of_match,
+	},
+	.probe  = realtek_smi_probe,
+	.remove_new = realtek_smi_remove,
+	.shutdown = realtek_smi_shutdown,
+};
+
+static struct mdio_driver rtl8366rb_mdio_driver = {
+	.mdiodrv.driver = {
+		.name = "rtl8366rb-mdio",
+		.of_match_table = rtl8366rb_of_match,
+	},
+	.probe  = realtek_mdio_probe,
+	.remove = realtek_mdio_remove,
+	.shutdown = realtek_mdio_shutdown,
+};
+
+static int rtl8366rb_init(void)
+{
+	int ret;
+
+	ret = realtek_mdio_driver_register(&rtl8366rb_mdio_driver);
+	if (ret)
+		return ret;
+
+	ret = realtek_smi_driver_register(&rtl8366rb_smi_driver);
+	if (ret) {
+		realtek_mdio_driver_unregister(&rtl8366rb_mdio_driver);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(rtl8366rb_init);
+
+static void __exit rtl8366rb_exit(void)
+{
+	realtek_smi_driver_unregister(&rtl8366rb_smi_driver);
+	realtek_mdio_driver_unregister(&rtl8366rb_mdio_driver);
+}
+module_exit(rtl8366rb_exit);
 
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Driver for RTL8366RB ethernet switch");
-- 
2.43.0

[PATCH net-next v4 04/11] net: dsa: realtek: keep variant reference in realtek_priv

[Luiz Angelo Daros de Luca]

Instead of copying values from the variant, we can keep a reference in
realtek_priv.

This is a preliminary change for sharing code betwen interfaces. It will
allow to move most of the probe into a common module while still allow
code specific to each interface to read variant fields.

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

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 22a63f41e3f2..57bd5d8814c2 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -197,9 +197,7 @@ int realtek_mdio_probe(struct mdio_device *mdiodev)
 	priv->dev = &mdiodev->dev;
 	priv->chip_data = (void *)priv + sizeof(*priv);
 
-	priv->clk_delay = var->clk_delay;
-	priv->cmd_read = var->cmd_read;
-	priv->cmd_write = var->cmd_write;
+	priv->variant = var;
 	priv->ops = var->ops;
 
 	priv->write_reg_noack = realtek_mdio_write;
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 618547befa13..274dd96b099c 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -46,7 +46,7 @@
 
 static inline void realtek_smi_clk_delay(struct realtek_priv *priv)
 {
-	ndelay(priv->clk_delay);
+	ndelay(priv->variant->clk_delay);
 }
 
 static void realtek_smi_start(struct realtek_priv *priv)
@@ -209,7 +209,7 @@ static int realtek_smi_read_reg(struct realtek_priv *priv, u32 addr, u32 *data)
 	realtek_smi_start(priv);
 
 	/* Send READ command */
-	ret = realtek_smi_write_byte(priv, priv->cmd_read);
+	ret = realtek_smi_write_byte(priv, priv->variant->cmd_read);
 	if (ret)
 		goto out;
 
@@ -250,7 +250,7 @@ static int realtek_smi_write_reg(struct realtek_priv *priv,
 	realtek_smi_start(priv);
 
 	/* Send WRITE command */
-	ret = realtek_smi_write_byte(priv, priv->cmd_write);
+	ret = realtek_smi_write_byte(priv, priv->variant->cmd_write);
 	if (ret)
 		goto out;
 
@@ -460,9 +460,7 @@ int realtek_smi_probe(struct platform_device *pdev)
 
 	/* Link forward and backward */
 	priv->dev = dev;
-	priv->clk_delay = var->clk_delay;
-	priv->cmd_read = var->cmd_read;
-	priv->cmd_write = var->cmd_write;
+	priv->variant = var;
 	priv->ops = var->ops;
 
 	priv->setup_interface = realtek_smi_setup_mdio;
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index e9ee778665b2..0c51b5132c61 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -58,9 +58,6 @@ struct realtek_priv {
 	struct mii_bus		*bus;
 	int			mdio_addr;
 
-	unsigned int		clk_delay;
-	u8			cmd_read;
-	u8			cmd_write;
 	spinlock_t		lock; /* Locks around command writes */
 	struct dsa_switch	*ds;
 	struct irq_domain	*irqdomain;
@@ -79,6 +76,8 @@ struct realtek_priv {
 	int			vlan_enabled;
 	int			vlan4k_enabled;
 
+	const struct realtek_variant *variant;
+
 	char			buf[4096];
 	void			*chip_data; /* Per-chip extra variant data */
 };
-- 
2.43.0

[PATCH net-next v4 05/11] net: dsa: realtek: common rtl83xx module

[Luiz Angelo Daros de Luca]

Some code can be shared between both interface modules (MDIO and SMI)
and among variants. These interface functions migrated to a common
module:

- rtl83xx_lock
- rtl83xx_unlock
- rtl83xx_probe
- rtl83xx_register_switch
- rtl83xx_remove

The reset during probe was moved to the end of the common probe. This way,
we avoid a reset if anything else fails.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/Makefile       |   2 +
 drivers/net/dsa/realtek/realtek-mdio.c | 145 +++---------------
 drivers/net/dsa/realtek/realtek-smi.c  | 150 +++---------------
 drivers/net/dsa/realtek/realtek.h      |   1 +
 drivers/net/dsa/realtek/rtl8365mb.c    |   9 +-
 drivers/net/dsa/realtek/rtl8366rb.c    |   9 +-
 drivers/net/dsa/realtek/rtl83xx.c      | 201 +++++++++++++++++++++++++
 drivers/net/dsa/realtek/rtl83xx.h      |  21 +++
 8 files changed, 279 insertions(+), 259 deletions(-)
 create mode 100644 drivers/net/dsa/realtek/rtl83xx.c
 create mode 100644 drivers/net/dsa/realtek/rtl83xx.h

diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
index 0aab57252a7c..67b5ee1c43a9 100644
--- a/drivers/net/dsa/realtek/Makefile
+++ b/drivers/net/dsa/realtek/Makefile
@@ -1,4 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_NET_DSA_REALTEK)		+= realtek-dsa.o
+realtek-dsa-objs			:= rtl83xx.o
 obj-$(CONFIG_NET_DSA_REALTEK_MDIO) 	+= realtek-mdio.o
 obj-$(CONFIG_NET_DSA_REALTEK_SMI) 	+= realtek-smi.o
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 57bd5d8814c2..26b8371ecc87 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -26,6 +26,7 @@
 
 #include "realtek.h"
 #include "realtek-mdio.h"
+#include "rtl83xx.h"
 
 /* Read/write via mdiobus */
 #define REALTEK_MDIO_CTRL0_REG		31
@@ -100,55 +101,19 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
 	return ret;
 }
 
-static void realtek_mdio_lock(void *ctx)
-{
-	struct realtek_priv *priv = ctx;
-
-	mutex_lock(&priv->map_lock);
-}
-
-static void realtek_mdio_unlock(void *ctx)
-{
-	struct realtek_priv *priv = ctx;
-
-	mutex_unlock(&priv->map_lock);
-}
-
-static const struct regmap_config realtek_mdio_regmap_config = {
-	.reg_bits = 10, /* A4..A0 R4..R0 */
-	.val_bits = 16,
-	.reg_stride = 1,
-	/* PHY regs are at 0x8000 */
-	.max_register = 0xffff,
-	.reg_format_endian = REGMAP_ENDIAN_BIG,
-	.reg_read = realtek_mdio_read,
-	.reg_write = realtek_mdio_write,
-	.cache_type = REGCACHE_NONE,
-	.lock = realtek_mdio_lock,
-	.unlock = realtek_mdio_unlock,
-};
-
-static const struct regmap_config realtek_mdio_nolock_regmap_config = {
-	.reg_bits = 10, /* A4..A0 R4..R0 */
-	.val_bits = 16,
-	.reg_stride = 1,
-	/* PHY regs are at 0x8000 */
-	.max_register = 0xffff,
-	.reg_format_endian = REGMAP_ENDIAN_BIG,
+static const struct realtek_interface_info realtek_mdio_info = {
 	.reg_read = realtek_mdio_read,
 	.reg_write = realtek_mdio_write,
-	.cache_type = REGCACHE_NONE,
-	.disable_locking = true,
 };
 
 /**
  * realtek_mdio_probe() - Probe a platform device for an MDIO-connected switch
  * @pdev: platform_device to probe on.
  *
- * This function should be used as the .probe in an mdio_driver. It
- * initializes realtek_priv and read data from the device-tree node. The switch
- * is hard resetted if a method is provided. It checks the switch chip ID and,
- * finally, a DSA switch is registered.
+ * This function should be used as the .probe in an mdio_driver. After
+ * calling the common probe function for both interfaces, it initializes the
+ * values specific for MDIO-connected devices. Finally, it calls a common
+ * function to register the DSA switch.
  *
  * Context: Any context. Takes and releases priv->map_lock.
  * Return: Returns 0 on success, a negative error on failure.
@@ -156,94 +121,22 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
  */
 int realtek_mdio_probe(struct mdio_device *mdiodev)
 {
-	struct realtek_priv *priv;
 	struct device *dev = &mdiodev->dev;
-	const struct realtek_variant *var;
-	struct regmap_config rc;
-	struct device_node *np;
+	struct realtek_priv *priv;
 	int ret;
 
-	var = of_device_get_match_data(dev);
-	if (!var)
-		return -EINVAL;
-
-	priv = devm_kzalloc(&mdiodev->dev,
-			    size_add(sizeof(*priv), var->chip_data_sz),
-			    GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	mutex_init(&priv->map_lock);
-
-	rc = realtek_mdio_regmap_config;
-	rc.lock_arg = priv;
-	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
-	if (IS_ERR(priv->map)) {
-		ret = PTR_ERR(priv->map);
-		dev_err(dev, "regmap init failed: %d\n", ret);
-		return ret;
-	}
-
-	rc = realtek_mdio_nolock_regmap_config;
-	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
-	if (IS_ERR(priv->map_nolock)) {
-		ret = PTR_ERR(priv->map_nolock);
-		dev_err(dev, "regmap init failed: %d\n", ret);
-		return ret;
-	}
+	priv = rtl83xx_probe(dev, &realtek_mdio_info);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
 
-	priv->mdio_addr = mdiodev->addr;
 	priv->bus = mdiodev->bus;
-	priv->dev = &mdiodev->dev;
-	priv->chip_data = (void *)priv + sizeof(*priv);
-
-	priv->variant = var;
-	priv->ops = var->ops;
-
+	priv->mdio_addr = mdiodev->addr;
 	priv->write_reg_noack = realtek_mdio_write;
+	priv->ds_ops = priv->variant->ds_ops_mdio;
 
-	np = dev->of_node;
-
-	dev_set_drvdata(dev, priv);
-
-	/* TODO: if power is software controlled, set up any regulators here */
-	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
-
-	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(priv->reset)) {
-		dev_err(dev, "failed to get RESET GPIO\n");
-		return PTR_ERR(priv->reset);
-	}
-
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
-		dev_dbg(dev, "asserted RESET\n");
-		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
-		msleep(REALTEK_HW_START_DELAY);
-		dev_dbg(dev, "deasserted RESET\n");
-	}
-
-	ret = priv->ops->detect(priv);
-	if (ret) {
-		dev_err(dev, "unable to detect switch\n");
-		return ret;
-	}
-
-	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
-	if (!priv->ds)
-		return -ENOMEM;
-
-	priv->ds->dev = dev;
-	priv->ds->num_ports = priv->num_ports;
-	priv->ds->priv = priv;
-	priv->ds->ops = var->ds_ops_mdio;
-
-	ret = dsa_register_switch(priv->ds);
-	if (ret) {
-		dev_err(priv->dev, "unable to register switch ret = %d\n", ret);
+	ret = rtl83xx_register_switch(priv);
+	if (ret)
 		return ret;
-	}
 
 	return 0;
 }
@@ -254,8 +147,8 @@ EXPORT_SYMBOL_NS_GPL(realtek_mdio_probe, REALTEK_DSA);
  * @pdev: platform_device to probe on.
  *
  * This function should be used as the .remove_new in an mdio_driver. First
- * it unregisters the DSA switch and cleans internal data. If a method is
- * provided, the hard reset is asserted to avoid traffic leakage.
+ * it unregisters the DSA switch and cleans internal data. Finally, it calls
+ * the common remove function.
  *
  * Context: Any context.
  * Return: Nothing.
@@ -270,9 +163,7 @@ void realtek_mdio_remove(struct mdio_device *mdiodev)
 
 	dsa_unregister_switch(priv->ds);
 
-	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	rtl83xx_remove(priv);
 }
 EXPORT_SYMBOL_NS_GPL(realtek_mdio_remove, REALTEK_DSA);
 
@@ -303,3 +194,5 @@ EXPORT_SYMBOL_NS_GPL(realtek_mdio_shutdown, REALTEK_DSA);
 MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
 MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(REALTEK_DSA);
+
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 274dd96b099c..840b1a835d07 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -41,6 +41,7 @@
 
 #include "realtek.h"
 #include "realtek-smi.h"
+#include "rtl83xx.h"
 
 #define REALTEK_SMI_ACK_RETRY_COUNT		5
 
@@ -311,47 +312,6 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val)
 	return realtek_smi_read_reg(priv, reg, val);
 }
 
-static void realtek_smi_lock(void *ctx)
-{
-	struct realtek_priv *priv = ctx;
-
-	mutex_lock(&priv->map_lock);
-}
-
-static void realtek_smi_unlock(void *ctx)
-{
-	struct realtek_priv *priv = ctx;
-
-	mutex_unlock(&priv->map_lock);
-}
-
-static const struct regmap_config realtek_smi_regmap_config = {
-	.reg_bits = 10, /* A4..A0 R4..R0 */
-	.val_bits = 16,
-	.reg_stride = 1,
-	/* PHY regs are at 0x8000 */
-	.max_register = 0xffff,
-	.reg_format_endian = REGMAP_ENDIAN_BIG,
-	.reg_read = realtek_smi_read,
-	.reg_write = realtek_smi_write,
-	.cache_type = REGCACHE_NONE,
-	.lock = realtek_smi_lock,
-	.unlock = realtek_smi_unlock,
-};
-
-static const struct regmap_config realtek_smi_nolock_regmap_config = {
-	.reg_bits = 10, /* A4..A0 R4..R0 */
-	.val_bits = 16,
-	.reg_stride = 1,
-	/* PHY regs are at 0x8000 */
-	.max_register = 0xffff,
-	.reg_format_endian = REGMAP_ENDIAN_BIG,
-	.reg_read = realtek_smi_read,
-	.reg_write = realtek_smi_write,
-	.cache_type = REGCACHE_NONE,
-	.disable_locking = true,
-};
-
 static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum)
 {
 	struct realtek_priv *priv = bus->priv;
@@ -409,14 +369,19 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 	return ret;
 }
 
+static const struct realtek_interface_info realtek_smi_info = {
+	.reg_read = realtek_smi_read,
+	.reg_write = realtek_smi_write,
+};
+
 /**
  * realtek_smi_probe() - Probe a platform device for an SMI-connected switch
  * @pdev: platform_device to probe on.
  *
- * This function should be used as the .probe in a platform_driver. It
- * initializes realtek_priv and read data from the device-tree node. The switch
- * is hard resetted if a method is provided. It checks the switch chip ID and,
- * finally, a DSA switch is registered.
+ * This function should be used as the .probe in a platform_driver. After
+ * calling the common probe function for both interfaces, it initializes the
+ * values specific for SMI-connected devices. Finally, it calls a common
+ * function to register the DSA switch.
  *
  * Context: Any context. Takes and releases priv->map_lock.
  * Return: Returns 0 on success, a negative error on failure.
@@ -424,97 +389,31 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
  */
 int realtek_smi_probe(struct platform_device *pdev)
 {
-	const struct realtek_variant *var;
 	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);
-	np = dev->of_node;
-
-	priv = devm_kzalloc(dev, sizeof(*priv) + var->chip_data_sz, GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-	priv->chip_data = (void *)priv + sizeof(*priv);
-
-	mutex_init(&priv->map_lock);
-
-	rc = realtek_smi_regmap_config;
-	rc.lock_arg = priv;
-	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
-	if (IS_ERR(priv->map)) {
-		ret = PTR_ERR(priv->map);
-		dev_err(dev, "regmap init failed: %d\n", ret);
-		return ret;
-	}
-
-	rc = realtek_smi_nolock_regmap_config;
-	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
-	if (IS_ERR(priv->map_nolock)) {
-		ret = PTR_ERR(priv->map_nolock);
-		dev_err(dev, "regmap init failed: %d\n", ret);
-		return ret;
-	}
-
-	/* Link forward and backward */
-	priv->dev = dev;
-	priv->variant = var;
-	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);
-	spin_lock_init(&priv->lock);
-
-	/* TODO: if power is software controlled, set up any regulators here */
-
-	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(priv->reset)) {
-		dev_err(dev, "failed to get RESET GPIO\n");
-		return PTR_ERR(priv->reset);
-	}
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
-		dev_dbg(dev, "asserted RESET\n");
-		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
-		msleep(REALTEK_HW_START_DELAY);
-		dev_dbg(dev, "deasserted RESET\n");
-	}
+	priv = rtl83xx_probe(dev, &realtek_smi_info);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
 
 	/* Fetch MDIO pins */
 	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->mdc))
 		return PTR_ERR(priv->mdc);
+
 	priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->mdio))
 		return PTR_ERR(priv->mdio);
 
-	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
+	priv->write_reg_noack = realtek_smi_write_reg_noack;
+	priv->setup_interface = realtek_smi_setup_mdio;
+	priv->ds_ops = priv->variant->ds_ops_smi;
 
-	ret = priv->ops->detect(priv);
-	if (ret) {
-		dev_err(dev, "unable to detect switch\n");
+	ret = rtl83xx_register_switch(priv);
+	if (ret)
 		return ret;
-	}
-
-	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
-	if (!priv->ds)
-		return -ENOMEM;
 
-	priv->ds->dev = dev;
-	priv->ds->num_ports = priv->num_ports;
-	priv->ds->priv = priv;
-
-	priv->ds->ops = var->ds_ops_smi;
-	ret = dsa_register_switch(priv->ds);
-	if (ret) {
-		dev_err_probe(dev, ret, "unable to register switch\n");
-		return ret;
-	}
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(realtek_smi_probe, REALTEK_DSA);
@@ -524,8 +423,8 @@ EXPORT_SYMBOL_NS_GPL(realtek_smi_probe, REALTEK_DSA);
  * @pdev: platform_device to probe on.
  *
  * This function should be used as the .remove_new in a platform_driver. First
- * it unregisters the DSA switch and cleans internal data. If a method is
- * provided, the hard reset is asserted to avoid traffic leakage.
+ * it unregisters the DSA switch and cleans internal data. Finally, it calls
+ * the common remove function.
  *
  * Context: Any context.
  * Return: Nothing.
@@ -539,12 +438,11 @@ void realtek_smi_remove(struct platform_device *pdev)
 		return;
 
 	dsa_unregister_switch(priv->ds);
+
 	if (priv->user_mii_bus)
 		of_node_put(priv->user_mii_bus->dev.of_node);
 
-	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	rtl83xx_remove(priv);
 }
 EXPORT_SYMBOL_NS_GPL(realtek_smi_remove, REALTEK_DSA);
 
@@ -575,3 +473,5 @@ EXPORT_SYMBOL_NS_GPL(realtek_smi_shutdown, REALTEK_DSA);
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(REALTEK_DSA);
+
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 0c51b5132c61..fbd0616c1df3 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -60,6 +60,7 @@ struct realtek_priv {
 
 	spinlock_t		lock; /* Locks around command writes */
 	struct dsa_switch	*ds;
+	const struct dsa_switch_ops *ds_ops;
 	struct irq_domain	*irqdomain;
 	bool			leds_disabled;
 
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index bd76bcf5fa44..97a41ba73718 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -103,6 +103,7 @@
 #include "realtek.h"
 #include "realtek-smi.h"
 #include "realtek-mdio.h"
+#include "rtl83xx.h"
 
 /* Family-specific data and limits */
 #define RTL8365MB_PHYADDRMAX		7
@@ -691,7 +692,7 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy,
 	u32 val;
 	int ret;
 
-	mutex_lock(&priv->map_lock);
+	rtl83xx_lock(priv);
 
 	ret = rtl8365mb_phy_poll_busy(priv);
 	if (ret)
@@ -724,7 +725,7 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy,
 	*data = val & 0xFFFF;
 
 out:
-	mutex_unlock(&priv->map_lock);
+	rtl83xx_unlock(priv);
 
 	return ret;
 }
@@ -735,7 +736,7 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy,
 	u32 val;
 	int ret;
 
-	mutex_lock(&priv->map_lock);
+	rtl83xx_lock(priv);
 
 	ret = rtl8365mb_phy_poll_busy(priv);
 	if (ret)
@@ -766,7 +767,7 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy,
 		goto out;
 
 out:
-	mutex_unlock(&priv->map_lock);
+	rtl83xx_unlock(priv);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 74e14b73a1ec..ec7a55d70bad 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -25,6 +25,7 @@
 #include "realtek.h"
 #include "realtek-smi.h"
 #include "realtek-mdio.h"
+#include "rtl83xx.h"
 
 #define RTL8366RB_PORT_NUM_CPU		5
 #define RTL8366RB_NUM_PORTS		6
@@ -1720,7 +1721,7 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
 	if (phy > RTL8366RB_PHY_NO_MAX)
 		return -EINVAL;
 
-	mutex_lock(&priv->map_lock);
+	rtl83xx_lock(priv);
 
 	ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG,
 			   RTL8366RB_PHY_CTRL_READ);
@@ -1748,7 +1749,7 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
 		phy, regnum, reg, val);
 
 out:
-	mutex_unlock(&priv->map_lock);
+	rtl83xx_unlock(priv);
 
 	return ret;
 }
@@ -1762,7 +1763,7 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 	if (phy > RTL8366RB_PHY_NO_MAX)
 		return -EINVAL;
 
-	mutex_lock(&priv->map_lock);
+	rtl83xx_lock(priv);
 
 	ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG,
 			   RTL8366RB_PHY_CTRL_WRITE);
@@ -1779,7 +1780,7 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 		goto out;
 
 out:
-	mutex_unlock(&priv->map_lock);
+	rtl83xx_unlock(priv);
 
 	return ret;
 }
diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
new file mode 100644
index 000000000000..57d185226b03
--- /dev/null
+++ b/drivers/net/dsa/realtek/rtl83xx.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/module.h>
+
+#include "realtek.h"
+#include "rtl83xx.h"
+
+/**
+ * rtl83xx_lock() - Locks the mutex used by regmaps
+ * @ctx: realtek_priv pointer
+ *
+ * This function is passed to regmap to be used as the lock function.
+ * It is also used externally to block regmap before executing multiple
+ * operations that must happen in sequence (which will use
+ * realtek_priv.map_nolock instead).
+ *
+ * Context: Any context. Holds priv->map_lock lock.
+ * Return: nothing
+ */
+void rtl83xx_lock(void *ctx)
+{
+	struct realtek_priv *priv = ctx;
+
+	mutex_lock(&priv->map_lock);
+}
+EXPORT_SYMBOL_NS_GPL(rtl83xx_lock, REALTEK_DSA);
+
+/**
+ * rtl83xx_unlock() - Unlocks the mutex used by regmaps
+ * @ctx: realtek_priv pointer
+ *
+ * This function unlocks the lock acquired by rtl83xx_lock.
+ *
+ * Context: Any context. Releases priv->map_lock lock
+ * Return: nothing
+ */
+void rtl83xx_unlock(void *ctx)
+{
+	struct realtek_priv *priv = ctx;
+
+	mutex_unlock(&priv->map_lock);
+}
+EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
+
+/**
+ * rtl83xx_probe() - probe a Realtek switch
+ * @dev: the device being probed
+ * @interface_info: reg read/write methods for a specific management interface.
+ *
+ * This function initializes realtek_priv and read data from the device-tree
+ * node. The switch is hard resetted if a method is provided.
+ *
+ * Context: Any context.
+ * Return: Pointer to the realtek_priv or ERR_PTR() in case of failure.
+ *
+ * The realtek_priv pointer does not need to be freed as it is controlled by
+ * devres.
+ *
+ */
+struct realtek_priv *
+rtl83xx_probe(struct device *dev,
+	      const struct realtek_interface_info *interface_info)
+{
+	const struct realtek_variant *var;
+	struct realtek_priv *priv;
+	struct regmap_config rc = {
+			.reg_bits = 10, /* A4..A0 R4..R0 */
+			.val_bits = 16,
+			.reg_stride = 1,
+			.max_register = 0xffff,
+			.reg_format_endian = REGMAP_ENDIAN_BIG,
+			.reg_read = interface_info->reg_read,
+			.reg_write = interface_info->reg_write,
+			.cache_type = REGCACHE_NONE,
+			.lock = rtl83xx_lock,
+			.unlock = rtl83xx_unlock,
+	};
+	int ret;
+
+	var = of_device_get_match_data(dev);
+	if (!var)
+		return ERR_PTR(-EINVAL);
+
+	priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz),
+			    GFP_KERNEL);
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_init(&priv->map_lock);
+
+	rc.lock_arg = priv;
+	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
+	if (IS_ERR(priv->map)) {
+		ret = PTR_ERR(priv->map);
+		dev_err(dev, "regmap init failed: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	rc.disable_locking = true;
+	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
+	if (IS_ERR(priv->map_nolock)) {
+		ret = PTR_ERR(priv->map_nolock);
+		dev_err(dev, "regmap init failed: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	/* Link forward and backward */
+	priv->dev = dev;
+	priv->variant = var;
+	priv->ops = var->ops;
+	priv->chip_data = (void *)priv + sizeof(*priv);
+
+	dev_set_drvdata(dev, priv);
+	spin_lock_init(&priv->lock);
+
+	priv->leds_disabled = of_property_read_bool(dev->of_node,
+						    "realtek,disable-leds");
+
+	/* TODO: if power is software controlled, set up any regulators here */
+
+	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->reset)) {
+		dev_err(dev, "failed to get RESET GPIO\n");
+		return ERR_CAST(priv->reset);
+	}
+	if (priv->reset) {
+		gpiod_set_value(priv->reset, 1);
+		dev_dbg(dev, "asserted RESET\n");
+		msleep(REALTEK_HW_STOP_DELAY);
+		gpiod_set_value(priv->reset, 0);
+		msleep(REALTEK_HW_START_DELAY);
+		dev_dbg(dev, "deasserted RESET\n");
+	}
+
+	return priv;
+}
+EXPORT_SYMBOL_NS_GPL(rtl83xx_probe, REALTEK_DSA);
+
+/**
+ * rtl83xx_register_switch() - detects and register a switch
+ * @priv: realtek_priv pointer
+ *
+ * This function first checks the switch chip ID and register a DSA
+ * switch.
+ *
+ * Context: Any context. Takes and releases priv->map_lock.
+ * Return: 0 on success, negative value for failure.
+ */
+int rtl83xx_register_switch(struct realtek_priv *priv)
+{
+	int ret;
+
+	ret = priv->ops->detect(priv);
+	if (ret) {
+		dev_err_probe(priv->dev, ret, "unable to detect switch\n");
+		return ret;
+	}
+
+	priv->ds = devm_kzalloc(priv->dev, sizeof(*priv->ds), GFP_KERNEL);
+	if (!priv->ds)
+		return -ENOMEM;
+
+	priv->ds->priv = priv;
+	priv->ds->dev = priv->dev;
+	priv->ds->ops = priv->ds_ops;
+	priv->ds->num_ports = priv->num_ports;
+
+	ret = dsa_register_switch(priv->ds);
+	if (ret) {
+		dev_err_probe(priv->dev, ret, "unable to register switch\n");
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(rtl83xx_register_switch, REALTEK_DSA);
+
+/**
+ * rtl83xx_remove() - Cleanup a realtek switch driver
+ * @ctx: realtek_priv pointer
+ *
+ * If a method is provided, this function asserts the hard reset of the switch
+ * in order to avoid leaking traffic when the driver is gone.
+ *
+ * Context: Any context.
+ * Return: nothing
+ */
+void rtl83xx_remove(struct realtek_priv *priv)
+{
+	if (!priv)
+		return;
+
+	/* leave the device reset asserted */
+	if (priv->reset)
+		gpiod_set_value(priv->reset, 1);
+}
+EXPORT_SYMBOL_NS_GPL(rtl83xx_remove, REALTEK_DSA);
+
+MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
+MODULE_DESCRIPTION("Realtek DSA switches common module");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
new file mode 100644
index 000000000000..9eb8197a58fa
--- /dev/null
+++ b/drivers/net/dsa/realtek/rtl83xx.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _RTL83XX_H
+#define _RTL83XX_H
+
+#include <linux/regmap.h>
+
+struct realtek_interface_info {
+	int (*reg_read)(void *ctx, u32 reg, u32 *val);
+	int (*reg_write)(void *ctx, u32 reg, u32 val);
+};
+
+void rtl83xx_lock(void *ctx);
+void rtl83xx_unlock(void *ctx);
+struct realtek_priv *
+rtl83xx_probe(struct device *dev,
+	      const struct realtek_interface_info *interface_info);
+int rtl83xx_register_switch(struct realtek_priv *priv);
+void rtl83xx_remove(struct realtek_priv *priv);
+
+#endif /* _RTL83XX_H */
-- 
2.43.0

[PATCH net-next v4 06/11] net: dsa: realtek: merge rtl83xx and interface modules into realtek-dsa

[Luiz Angelo Daros de Luca]

Since rtl83xx and realtek-{smi,mdio} are always loaded together,
we can optimize resource usage by consolidating them into a single
module.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/Kconfig        |  4 ++--
 drivers/net/dsa/realtek/Makefile       | 11 +++++++++--
 drivers/net/dsa/realtek/realtek-mdio.c |  5 -----
 drivers/net/dsa/realtek/realtek-smi.c  |  5 -----
 drivers/net/dsa/realtek/rtl83xx.c      |  1 +
 5 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
index 9d182fde11b4..6989972eebc3 100644
--- a/drivers/net/dsa/realtek/Kconfig
+++ b/drivers/net/dsa/realtek/Kconfig
@@ -16,14 +16,14 @@ menuconfig NET_DSA_REALTEK
 if NET_DSA_REALTEK
 
 config NET_DSA_REALTEK_MDIO
-	tristate "Realtek MDIO interface support"
+	bool "Realtek MDIO interface support"
 	depends on OF
 	help
 	  Select to enable support for registering switches configured
 	  through MDIO.
 
 config NET_DSA_REALTEK_SMI
-	tristate "Realtek SMI interface support"
+	bool "Realtek SMI interface support"
 	depends on OF
 	help
 	  Select to enable support for registering switches connected
diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
index 67b5ee1c43a9..6ed6b4598d2e 100644
--- a/drivers/net/dsa/realtek/Makefile
+++ b/drivers/net/dsa/realtek/Makefile
@@ -1,8 +1,15 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_NET_DSA_REALTEK)		+= realtek-dsa.o
 realtek-dsa-objs			:= rtl83xx.o
-obj-$(CONFIG_NET_DSA_REALTEK_MDIO) 	+= realtek-mdio.o
-obj-$(CONFIG_NET_DSA_REALTEK_SMI) 	+= realtek-smi.o
+
+ifdef CONFIG_NET_DSA_REALTEK_MDIO
+realtek-dsa-objs += realtek-mdio.o
+endif
+
+ifdef CONFIG_NET_DSA_REALTEK_SMI
+realtek-dsa-objs += realtek-smi.o
+endif
+
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
 rtl8366-objs 				:= rtl8366-core.o rtl8366rb.o
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 26b8371ecc87..0171185ec665 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -191,8 +191,3 @@ void realtek_mdio_shutdown(struct mdio_device *mdiodev)
 }
 EXPORT_SYMBOL_NS_GPL(realtek_mdio_shutdown, REALTEK_DSA);
 
-MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
-MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
-MODULE_LICENSE("GPL");
-MODULE_IMPORT_NS(REALTEK_DSA);
-
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 840b1a835d07..5533b79d67f5 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -470,8 +470,3 @@ void realtek_smi_shutdown(struct platform_device *pdev)
 }
 EXPORT_SYMBOL_NS_GPL(realtek_smi_shutdown, REALTEK_DSA);
 
-MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
-MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
-MODULE_LICENSE("GPL");
-MODULE_IMPORT_NS(REALTEK_DSA);
-
diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
index 57d185226b03..3d07c5662fa4 100644
--- a/drivers/net/dsa/realtek/rtl83xx.c
+++ b/drivers/net/dsa/realtek/rtl83xx.c
@@ -197,5 +197,6 @@ void rtl83xx_remove(struct realtek_priv *priv)
 EXPORT_SYMBOL_NS_GPL(rtl83xx_remove, REALTEK_DSA);
 
 MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
+MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Realtek DSA switches common module");
 MODULE_LICENSE("GPL");
-- 
2.43.0

[PATCH net-next v4 07/11] net: dsa: realtek: get internal MDIO node by name

[Luiz Angelo Daros de Luca]

The binding docs requires for SMI-connected devices that the switch
must have a child node named "mdio" and with a compatible string of
"realtek,smi-mdio". Meanwile, for MDIO-connected switches, the binding
docs only requires a child node named "mdio".

This patch changes the driver to use the common denominator for both
interfaces, looking for the MDIO node by name, ignoring the compatible
string.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/realtek/realtek-smi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 5533b79d67f5..0ccb2a6059a6 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -333,7 +333,7 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 	struct device_node *mdio_np;
 	int ret;
 
-	mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
+	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
 	if (!mdio_np) {
 		dev_err(priv->dev, "no MDIO bus node\n");
 		return -ENODEV;
-- 
2.43.0

[PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Luiz Angelo Daros de Luca]

The line assigning dev.of_node in mdio_bus has been removed since the
subsequent of_mdiobus_register will always overwrite it.

ds->user_mii_bus is not assigned anymore[1]. It should work as before as
long as the switch ports have a valid phy-handle property.

Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
to mdiobus"), we can put the "mdio" node just after the MDIO bus
registration. The switch unregistration was moved into
realtek_common_remove() as both interfaces now use the same code path.

[1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u

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

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 0171185ec665..c75b4550802c 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -158,11 +158,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
 
-	if (!priv)
-		return;
-
-	dsa_unregister_switch(priv->ds);
-
 	rtl83xx_remove(priv);
 }
 EXPORT_SYMBOL_NS_GPL(realtek_mdio_remove, REALTEK_DSA);
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 0ccb2a6059a6..a89813e527d2 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -331,7 +331,7 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 {
 	struct realtek_priv *priv =  ds->priv;
 	struct device_node *mdio_np;
-	int ret;
+	int ret = 0;
 
 	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
 	if (!mdio_np) {
@@ -344,15 +344,14 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 		ret = -ENOMEM;
 		goto err_put_node;
 	}
+
 	priv->user_mii_bus->priv = priv;
 	priv->user_mii_bus->name = "SMI user MII";
 	priv->user_mii_bus->read = realtek_smi_mdio_read;
 	priv->user_mii_bus->write = realtek_smi_mdio_write;
 	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d",
 		 ds->index);
-	priv->user_mii_bus->dev.of_node = mdio_np;
 	priv->user_mii_bus->parent = priv->dev;
-	ds->user_mii_bus = priv->user_mii_bus;
 
 	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
 	if (ret) {
@@ -361,8 +360,6 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 		goto err_put_node;
 	}
 
-	return 0;
-
 err_put_node:
 	of_node_put(mdio_np);
 
@@ -434,14 +431,6 @@ void realtek_smi_remove(struct platform_device *pdev)
 {
 	struct realtek_priv *priv = platform_get_drvdata(pdev);
 
-	if (!priv)
-		return;
-
-	dsa_unregister_switch(priv->ds);
-
-	if (priv->user_mii_bus)
-		of_node_put(priv->user_mii_bus->dev.of_node);
-
 	rtl83xx_remove(priv);
 }
 EXPORT_SYMBOL_NS_GPL(realtek_smi_remove, REALTEK_DSA);
diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
index 3d07c5662fa4..53bacbacc82e 100644
--- a/drivers/net/dsa/realtek/rtl83xx.c
+++ b/drivers/net/dsa/realtek/rtl83xx.c
@@ -190,6 +190,8 @@ void rtl83xx_remove(struct realtek_priv *priv)
 	if (!priv)
 		return;
 
+	dsa_unregister_switch(priv->ds);
+
 	/* leave the device reset asserted */
 	if (priv->reset)
 		gpiod_set_value(priv->reset, 1);
-- 
2.43.0

[PATCH net-next v4 09/11] net: dsa: realtek: migrate user_mii_bus setup to realtek-dsa

[Luiz Angelo Daros de Luca]

In the user MDIO driver, despite numerous references to SMI, including
its compatible string, there's nothing inherently specific about the SMI
interface in the user MDIO bus. Consequently, the code has been migrated
to the rtl83xx module. All references to SMI have been eliminated.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-smi.c | 57 +----------------------
 drivers/net/dsa/realtek/rtl83xx.c     | 67 +++++++++++++++++++++++++++
 drivers/net/dsa/realtek/rtl83xx.h     |  1 +
 3 files changed, 69 insertions(+), 56 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index a89813e527d2..70f3967e56e8 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -31,7 +31,6 @@
 #include <linux/spinlock.h>
 #include <linux/skbuff.h>
 #include <linux/of.h>
-#include <linux/of_mdio.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/platform_device.h>
@@ -312,60 +311,6 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val)
 	return realtek_smi_read_reg(priv, reg, val);
 }
 
-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 = 0;
-
-	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
-	if (!mdio_np) {
-		dev_err(priv->dev, "no MDIO bus node\n");
-		return -ENODEV;
-	}
-
-	priv->user_mii_bus = devm_mdiobus_alloc(priv->dev);
-	if (!priv->user_mii_bus) {
-		ret = -ENOMEM;
-		goto err_put_node;
-	}
-
-	priv->user_mii_bus->priv = priv;
-	priv->user_mii_bus->name = "SMI user MII";
-	priv->user_mii_bus->read = realtek_smi_mdio_read;
-	priv->user_mii_bus->write = realtek_smi_mdio_write;
-	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d",
-		 ds->index);
-	priv->user_mii_bus->parent = priv->dev;
-
-	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
-	if (ret) {
-		dev_err(priv->dev, "unable to register MDIO bus %s\n",
-			priv->user_mii_bus->id);
-		goto err_put_node;
-	}
-
-err_put_node:
-	of_node_put(mdio_np);
-
-	return ret;
-}
-
 static const struct realtek_interface_info realtek_smi_info = {
 	.reg_read = realtek_smi_read,
 	.reg_write = realtek_smi_write,
@@ -404,7 +349,7 @@ int realtek_smi_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->mdio);
 
 	priv->write_reg_noack = realtek_smi_write_reg_noack;
-	priv->setup_interface = realtek_smi_setup_mdio;
+	priv->setup_interface = rtl83xx_setup_user_mdio;
 	priv->ds_ops = priv->variant->ds_ops_smi;
 
 	ret = rtl83xx_register_switch(priv);
diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
index 53bacbacc82e..525d8c014136 100644
--- a/drivers/net/dsa/realtek/rtl83xx.c
+++ b/drivers/net/dsa/realtek/rtl83xx.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 
 #include <linux/module.h>
+#include <linux/of_mdio.h>
 
 #include "realtek.h"
 #include "rtl83xx.h"
@@ -42,6 +43,72 @@ void rtl83xx_unlock(void *ctx)
 }
 EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
 
+static int rtl83xx_user_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 rtl83xx_user_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);
+}
+
+/**
+ * rtl83xx_setup_user_mdio() - register the user mii bus driver
+ * @ds: DSA switch associated with this user_mii_bus
+ *
+ * This function first gets and mdio node under the dev OF node, aborting
+ * if missing. That mdio node describing an mdio bus is used to register a
+ * new mdio bus.
+ *
+ * Context: Any context.
+ * Return: 0 on success, negative value for failure.
+ */
+int rtl83xx_setup_user_mdio(struct dsa_switch *ds)
+{
+	struct realtek_priv *priv =  ds->priv;
+	struct device_node *mdio_np;
+	int ret = 0;
+
+	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
+	if (!mdio_np) {
+		dev_err(priv->dev, "no MDIO bus node\n");
+		return -ENODEV;
+	}
+
+	priv->user_mii_bus = devm_mdiobus_alloc(priv->dev);
+	if (!priv->user_mii_bus) {
+		ret = -ENOMEM;
+		goto err_put_node;
+	}
+
+	priv->user_mii_bus->priv = priv;
+	priv->user_mii_bus->name = "Realtek user MII";
+	priv->user_mii_bus->read = rtl83xx_user_mdio_read;
+	priv->user_mii_bus->write = rtl83xx_user_mdio_write;
+	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "Realtek-%d",
+		 ds->index);
+	priv->user_mii_bus->parent = priv->dev;
+
+	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
+	if (ret) {
+		dev_err(priv->dev, "unable to register MDIO bus %s\n",
+			priv->user_mii_bus->id);
+		goto err_put_node;
+	}
+
+err_put_node:
+	of_node_put(mdio_np);
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(rtl83xx_setup_user_mdio, REALTEK_DSA);
+
 /**
  * rtl83xx_probe() - probe a Realtek switch
  * @dev: the device being probed
diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
index 9eb8197a58fa..b5d464bb850d 100644
--- a/drivers/net/dsa/realtek/rtl83xx.h
+++ b/drivers/net/dsa/realtek/rtl83xx.h
@@ -12,6 +12,7 @@ struct realtek_interface_info {
 
 void rtl83xx_lock(void *ctx);
 void rtl83xx_unlock(void *ctx);
+int rtl83xx_setup_user_mdio(struct dsa_switch *ds);
 struct realtek_priv *
 rtl83xx_probe(struct device *dev,
 	      const struct realtek_interface_info *interface_info);
-- 
2.43.0

[PATCH net-next v4 10/11] net: dsa: realtek: use the same mii bus driver for both interfaces

[Luiz Angelo Daros de Luca]

The realtek-mdio will now use this driver instead of the generic DSA
driver ("dsa user smi"), which should not be used with OF[1].

With a single ds_ops for both interfaces, the ds_ops in realtek_priv is
no longer necessary. Now, the realtek_variant.ds_ops can be used
directly.

The realtek_priv.setup_interface() has been removed as we can directly
call the new common function.

[1] https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-mdio.c |  1 -
 drivers/net/dsa/realtek/realtek-smi.c  |  2 -
 drivers/net/dsa/realtek/realtek.h      |  5 +--
 drivers/net/dsa/realtek/rtl8365mb.c    | 49 +++---------------------
 drivers/net/dsa/realtek/rtl8366rb.c    | 52 +++-----------------------
 drivers/net/dsa/realtek/rtl83xx.c      |  2 +-
 6 files changed, 14 insertions(+), 97 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index c75b4550802c..6415408c337d 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -132,7 +132,6 @@ int realtek_mdio_probe(struct mdio_device *mdiodev)
 	priv->bus = mdiodev->bus;
 	priv->mdio_addr = mdiodev->addr;
 	priv->write_reg_noack = realtek_mdio_write;
-	priv->ds_ops = priv->variant->ds_ops_mdio;
 
 	ret = rtl83xx_register_switch(priv);
 	if (ret)
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 70f3967e56e8..b9523ebc7413 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -349,8 +349,6 @@ int realtek_smi_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->mdio);
 
 	priv->write_reg_noack = realtek_smi_write_reg_noack;
-	priv->setup_interface = rtl83xx_setup_user_mdio;
-	priv->ds_ops = priv->variant->ds_ops_smi;
 
 	ret = rtl83xx_register_switch(priv);
 	if (ret)
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index fbd0616c1df3..7af6dcc1bb24 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -60,7 +60,6 @@ struct realtek_priv {
 
 	spinlock_t		lock; /* Locks around command writes */
 	struct dsa_switch	*ds;
-	const struct dsa_switch_ops *ds_ops;
 	struct irq_domain	*irqdomain;
 	bool			leds_disabled;
 
@@ -71,7 +70,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;
@@ -115,8 +113,7 @@ 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;
 	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 97a41ba73718..d7d3ae4746f6 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -828,17 +828,6 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 	return 0;
 }
 
-static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum)
-{
-	return rtl8365mb_phy_read(ds->priv, phy, regnum);
-}
-
-static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum,
-				   u16 val)
-{
-	return rtl8365mb_phy_write(ds->priv, phy, regnum, val);
-}
-
 static const struct rtl8365mb_extint *
 rtl8365mb_get_port_extint(struct realtek_priv *priv, int port)
 {
@@ -2017,12 +2006,10 @@ 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;
-		}
+	ret = rtl83xx_setup_user_mdio(ds);
+	if (ret) {
+		dev_err(priv->dev, "could not set up MDIO bus\n");
+		goto out_teardown_irq;
 	}
 
 	/* Start statistics counter polling */
@@ -2116,28 +2103,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 	return 0;
 }
 
-static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
-	.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,
-	.port_change_mtu = rtl8365mb_port_change_mtu,
-	.port_max_mtu = rtl8365mb_port_max_mtu,
-};
-
-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,
@@ -2146,8 +2112,6 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
 	.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,
-	.phy_read = rtl8365mb_dsa_phy_read,
-	.phy_write = rtl8365mb_dsa_phy_write,
 	.port_stp_state_set = rtl8365mb_port_stp_state_set,
 	.get_strings = rtl8365mb_get_strings,
 	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
@@ -2167,8 +2131,7 @@ 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 = &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 ec7a55d70bad..5084ad151f0f 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1033,12 +1033,10 @@ 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;
-		}
+	ret = rtl83xx_setup_user_mdio(ds);
+	if (ret) {
+		dev_err(priv->dev, "could not set up MDIO bus\n");
+		return -ENODEV;
 	}
 
 	return 0;
@@ -1785,17 +1783,6 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 	return ret;
 }
 
-static int rtl8366rb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum)
-{
-	return rtl8366rb_phy_read(ds->priv, phy, regnum);
-}
-
-static int rtl8366rb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum,
-				   u16 val)
-{
-	return rtl8366rb_phy_write(ds->priv, phy, regnum, val);
-}
-
 static int rtl8366rb_reset_chip(struct realtek_priv *priv)
 {
 	int timeout = 10;
@@ -1861,35 +1848,9 @@ static int rtl8366rb_detect(struct realtek_priv *priv)
 	return 0;
 }
 
-static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = {
-	.get_tag_protocol = rtl8366_get_tag_protocol,
-	.setup = rtl8366rb_setup,
-	.phylink_get_caps = rtl8366rb_phylink_get_caps,
-	.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_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,
-	.phy_write = rtl8366rb_dsa_phy_write,
 	.phylink_get_caps = rtl8366rb_phylink_get_caps,
 	.phylink_mac_link_up = rtl8366rb_mac_link_up,
 	.phylink_mac_link_down = rtl8366rb_mac_link_down,
@@ -1928,8 +1889,7 @@ 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 = &rtl8366rb_switch_ops,
 	.ops = &rtl8366rb_ops,
 	.clk_delay = 10,
 	.cmd_read = 0xa9,
diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
index 525d8c014136..2f39472a44d2 100644
--- a/drivers/net/dsa/realtek/rtl83xx.c
+++ b/drivers/net/dsa/realtek/rtl83xx.c
@@ -229,7 +229,7 @@ int rtl83xx_register_switch(struct realtek_priv *priv)
 
 	priv->ds->priv = priv;
 	priv->ds->dev = priv->dev;
-	priv->ds->ops = priv->ds_ops;
+	priv->ds->ops = priv->variant->ds_ops;
 	priv->ds->num_ports = priv->num_ports;
 
 	ret = dsa_register_switch(priv->ds);
-- 
2.43.0

[PATCH net-next v4 11/11] net: dsa: realtek: embed dsa_switch into realtek_priv

[Luiz Angelo Daros de Luca]

To eliminate the need for a second memory allocation for dsa_switch, it
has been embedded within realtek_priv.

Suggested-by: Alvin Šipraga <alsi@bang-olufsen.dk>
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      |  2 +-
 drivers/net/dsa/realtek/rtl8365mb.c    | 12 ++++++------
 drivers/net/dsa/realtek/rtl8366rb.c    |  2 +-
 drivers/net/dsa/realtek/rtl83xx.c      | 16 ++++++----------
 6 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 6415408c337d..cb3bc6219c6c 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -179,7 +179,7 @@ void realtek_mdio_shutdown(struct mdio_device *mdiodev)
 	if (!priv)
 		return;
 
-	dsa_switch_shutdown(priv->ds);
+	dsa_switch_shutdown(&priv->ds);
 
 	dev_set_drvdata(&mdiodev->dev, NULL);
 }
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index b9523ebc7413..eac9ce5d6ae0 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -396,7 +396,7 @@ void realtek_smi_shutdown(struct platform_device *pdev)
 	if (!priv)
 		return;
 
-	dsa_switch_shutdown(priv->ds);
+	dsa_switch_shutdown(&priv->ds);
 
 	platform_set_drvdata(pdev, NULL);
 }
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 7af6dcc1bb24..0217b8032c01 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -59,7 +59,7 @@ struct realtek_priv {
 	int			mdio_addr;
 
 	spinlock_t		lock; /* Locks around command writes */
-	struct dsa_switch	*ds;
+	struct dsa_switch	ds;
 	struct irq_domain	*irqdomain;
 	bool			leds_disabled;
 
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index d7d3ae4746f6..c9dbe0240ab7 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -880,7 +880,7 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
 	if (!extint)
 		return -ENODEV;
 
-	dp = dsa_to_port(priv->ds, port);
+	dp = dsa_to_port(&priv->ds, port);
 	dn = dp->dn;
 
 	/* Set the RGMII TX/RX delay
@@ -1543,7 +1543,7 @@ static void rtl8365mb_stats_setup(struct realtek_priv *priv)
 	for (i = 0; i < priv->num_ports; i++) {
 		struct rtl8365mb_port *p = &mb->ports[i];
 
-		if (dsa_is_unused_port(priv->ds, i))
+		if (dsa_is_unused_port(&priv->ds, i))
 			continue;
 
 		/* Per-port spinlock to protect the stats64 data */
@@ -1564,7 +1564,7 @@ static void rtl8365mb_stats_teardown(struct realtek_priv *priv)
 	for (i = 0; i < priv->num_ports; i++) {
 		struct rtl8365mb_port *p = &mb->ports[i];
 
-		if (dsa_is_unused_port(priv->ds, i))
+		if (dsa_is_unused_port(&priv->ds, i))
 			continue;
 
 		cancel_delayed_work_sync(&p->mib_work);
@@ -1963,7 +1963,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 		dev_info(priv->dev, "no interrupt support\n");
 
 	/* Configure CPU tagging */
-	dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
+	dsa_switch_for_each_cpu_port(cpu_dp, &priv->ds) {
 		cpu->mask |= BIT(cpu_dp->index);
 
 		if (cpu->trap_port == RTL8365MB_MAX_NUM_PORTS)
@@ -1978,7 +1978,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 	for (i = 0; i < priv->num_ports; i++) {
 		struct rtl8365mb_port *p = &mb->ports[i];
 
-		if (dsa_is_unused_port(priv->ds, i))
+		if (dsa_is_unused_port(&priv->ds, i))
 			continue;
 
 		/* Forward only to the CPU */
@@ -1995,7 +1995,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 		 * ports will still forward frames to the CPU despite being
 		 * administratively down by default.
 		 */
-		rtl8365mb_port_stp_state_set(priv->ds, i, BR_STATE_DISABLED);
+		rtl8365mb_port_stp_state_set(&priv->ds, i, BR_STATE_DISABLED);
 
 		/* Set up per-port private data */
 		p->priv = priv;
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 5084ad151f0f..30ae30cdbea5 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1675,7 +1675,7 @@ static int rtl8366rb_set_mc_index(struct realtek_priv *priv, int port, int index
 	 * not drop any untagged or C-tagged frames. Make sure to update the
 	 * filtering setting.
 	 */
-	if (dsa_port_is_vlan_filtering(dsa_to_port(priv->ds, port)))
+	if (dsa_port_is_vlan_filtering(dsa_to_port(&priv->ds, port)))
 		ret = rtl8366rb_drop_untagged(priv, port, !pvid_enabled);
 
 	return ret;
diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
index 2f39472a44d2..ac8c632263c6 100644
--- a/drivers/net/dsa/realtek/rtl83xx.c
+++ b/drivers/net/dsa/realtek/rtl83xx.c
@@ -223,16 +223,12 @@ int rtl83xx_register_switch(struct realtek_priv *priv)
 		return ret;
 	}
 
-	priv->ds = devm_kzalloc(priv->dev, sizeof(*priv->ds), GFP_KERNEL);
-	if (!priv->ds)
-		return -ENOMEM;
+	priv->ds.priv = priv;
+	priv->ds.dev = priv->dev;
+	priv->ds.ops = priv->variant->ds_ops;
+	priv->ds.num_ports = priv->num_ports;
 
-	priv->ds->priv = priv;
-	priv->ds->dev = priv->dev;
-	priv->ds->ops = priv->variant->ds_ops;
-	priv->ds->num_ports = priv->num_ports;
-
-	ret = dsa_register_switch(priv->ds);
+	ret = dsa_register_switch(&priv->ds);
 	if (ret) {
 		dev_err_probe(priv->dev, ret, "unable to register switch\n");
 		return ret;
@@ -257,7 +253,7 @@ void rtl83xx_remove(struct realtek_priv *priv)
 	if (!priv)
 		return;
 
-	dsa_unregister_switch(priv->ds);
+	dsa_unregister_switch(&priv->ds);
 
 	/* leave the device reset asserted */
 	if (priv->reset)
-- 
2.43.0

Re: [PATCH net-next v4 03/11] net: dsa: realtek: convert variants into real drivers

[Jakub Kicinski]

On Tue, 23 Jan 2024 18:55:55 -0300 Luiz Angelo Daros de Luca wrote:
> +/**
> + * realtek_mdio_remove() - Remove the driver of an MDIO-connected switch
> + * @pdev: platform_device to probe on.
> + *
> + * This function should be used as the .remove_new in an mdio_driver. First
> + * it unregisters the DSA switch and cleans internal data. If a method is
> + * provided, the hard reset is asserted to avoid traffic leakage.
> + *
> + * Context: Any context.
> + * Return: Nothing.
> + *
> + */
> +void realtek_mdio_remove(struct mdio_device *mdiodev)

kerne-doc says:

drivers/net/dsa/realtek/realtek-mdio.c:159: warning: Function parameter or struct member 'mdiodev' not described in 'realtek_mdio_probe'
drivers/net/dsa/realtek/realtek-mdio.c:159: warning: Excess function parameter 'pdev' description in 'realtek_mdio_probe'
drivers/net/dsa/realtek/realtek-mdio.c:268: warning: Function parameter or struct member 'mdiodev' not described in 'realtek_mdio_remove'
drivers/net/dsa/realtek/realtek-mdio.c:268: warning: Excess function parameter 'pdev' description in 'realtek_mdio_remove'
drivers/net/dsa/realtek/realtek-mdio.c:294: warning: Function parameter or struct member 'mdiodev' not described in 'realtek_mdio_shutdown'
drivers/net/dsa/realtek/realtek-mdio.c:294: warning: Excess function parameter 'pdev' description in 'realtek_mdio_shutdown'
-- 
pw-bot: cr

Re: [PATCH net-next v4 02/11] net: dsa: realtek: introduce REALTEK_DSA namespace

[Vladimir Oltean]

On Tue, Jan 23, 2024 at 06:55:54PM -0300, Luiz Angelo Daros de Luca wrote:
> Create a namespace to group the exported symbols.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v4 03/11] net: dsa: realtek: convert variants into real drivers

[Vladimir Oltean]

On Tue, Jan 23, 2024 at 06:55:55PM -0300, Luiz Angelo Daros de Luca wrote:
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index df214b2f60d1..22a63f41e3f2 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -249,8 +276,20 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
>  	if (priv->reset)
>  		gpiod_set_value(priv->reset, 1);
>  }
> +EXPORT_SYMBOL_NS_GPL(realtek_mdio_remove, REALTEK_DSA);
>  
> -static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> +/**
> + * realtek_mdio_shutdown() - Shutdown the driver of a MDIO-connected switch
> + * @pdev: platform_device to probe on.
> + *
> + * This function should be used as the .shutdown in an mdio_driver. It shuts
> + * down the DSA switch and cleans the platform driver data.

, to prevent realtek_mdio_remove() from running afterwards, which is
possible if the parent bus implements its own .shutdown() as .remove().

> + *
> + * Context: Any context.
> + * Return: Nothing.
> + *
> + */
> +void realtek_mdio_shutdown(struct mdio_device *mdiodev)
>  {
>  	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
>  
> @@ -521,8 +548,20 @@ static void realtek_smi_remove(struct platform_device *pdev)
>  	if (priv->reset)
>  		gpiod_set_value(priv->reset, 1);
>  }
> +EXPORT_SYMBOL_NS_GPL(realtek_smi_remove, REALTEK_DSA);
>  
> -static void realtek_smi_shutdown(struct platform_device *pdev)
> +/**
> + * realtek_smi_shutdown() - Shutdown the driver of a SMI-connected switch
> + * @pdev: platform_device to probe on.
> + *
> + * This function should be used as the .shutdown in a platform_driver. It shuts
> + * down the DSA switch and cleans the platform driver data.

Likewise.

> + *
> + * Context: Any context.
> + * Return: Nothing.
> + *

I'm not sure if the blank line at the end of the comment is necessary.

> + */
> +void realtek_smi_shutdown(struct platform_device *pdev)
>  {
>  	struct realtek_priv *priv = platform_get_drvdata(pdev);
>  

Re: [PATCH net-next v4 04/11] net: dsa: realtek: keep variant reference in realtek_priv

[Vladimir Oltean]

On Tue, Jan 23, 2024 at 06:55:56PM -0300, Luiz Angelo Daros de Luca wrote:
> Instead of copying values from the variant, we can keep a reference in
> realtek_priv.
> 
> This is a preliminary change for sharing code betwen interfaces. It will
> allow to move most of the probe into a common module while still allow
> code specific to each interface to read variant fields.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v4 05/11] net: dsa: realtek: common rtl83xx module

[Vladimir Oltean]

On Tue, Jan 23, 2024 at 06:55:57PM -0300, Luiz Angelo Daros de Luca wrote:
> Some code can be shared between both interface modules (MDIO and SMI)
> and among variants. These interface functions migrated to a common
> module:
>
> - rtl83xx_lock
> - rtl83xx_unlock
> - rtl83xx_probe
> - rtl83xx_register_switch

For API uniformity, I would also introduce rtl83xx_unregister_switch()
to make it clear that there is a teardown step associated with this.

> - rtl83xx_remove

No rtl83xx_shutdown()?

You could centralize the comments from the mdio and smi interfaces into
the rtl83xx common layer.

>
> The reset during probe was moved to the end of the common probe. This way,
> we avoid a reset if anything else fails.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 57bd5d8814c2..26b8371ecc87 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -303,3 +194,5 @@ EXPORT_SYMBOL_NS_GPL(realtek_mdio_shutdown, REALTEK_DSA);
>  MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
>  MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
>  MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(REALTEK_DSA);
> +

Stray blank line at the end of the file.

> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 274dd96b099c..840b1a835d07 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -575,3 +473,5 @@ EXPORT_SYMBOL_NS_GPL(realtek_smi_shutdown, REALTEK_DSA);
>  MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
>  MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
>  MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(REALTEK_DSA);
> +

Likewise.

> diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> new file mode 100644
> index 000000000000..57d185226b03
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/rtl83xx.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/module.h>
> +
> +#include "realtek.h"
> +#include "rtl83xx.h"
> +
> +/**
> + * rtl83xx_lock() - Locks the mutex used by regmaps
> + * @ctx: realtek_priv pointer
> + *
> + * This function is passed to regmap to be used as the lock function.
> + * It is also used externally to block regmap before executing multiple
> + * operations that must happen in sequence (which will use
> + * realtek_priv.map_nolock instead).
> + *
> + * Context: Any context. Holds priv->map_lock lock.

Not "any context", but "sleepable context". You cannot acquire a mutex
in atomic context. Actually this applies across the board in this patch
set. The entry points into DSA also take mutex_lock(&dsa2_mutex), so
this also applies to its callers' context..

> + * Return: nothing
> + */
> +void rtl83xx_lock(void *ctx)
> +{
> +	struct realtek_priv *priv = ctx;
> +
> +	mutex_lock(&priv->map_lock);
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_lock, REALTEK_DSA);
> +
> +/**
> + * rtl83xx_unlock() - Unlocks the mutex used by regmaps
> + * @ctx: realtek_priv pointer
> + *
> + * This function unlocks the lock acquired by rtl83xx_lock.
> + *
> + * Context: Any context. Releases priv->map_lock lock
> + * Return: nothing
> + */
> +void rtl83xx_unlock(void *ctx)
> +{
> +	struct realtek_priv *priv = ctx;
> +
> +	mutex_unlock(&priv->map_lock);
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
> +
> +/**
> + * rtl83xx_probe() - probe a Realtek switch
> + * @dev: the device being probed
> + * @interface_info: reg read/write methods for a specific management interface.

Leave this description more open-ended, otherwise it will have to be
modified when struct realtek_interface_info gains more fields.

> + *
> + * This function initializes realtek_priv and read data from the device-tree

s/read/reads/
s/device-tree/device tree/

> + * node. The switch is hard resetted if a method is provided.
> + *
> + * Context: Any context.
> + * Return: Pointer to the realtek_priv or ERR_PTR() in case of failure.
> + *
> + * The realtek_priv pointer does not need to be freed as it is controlled by
> + * devres.
> + *
> + */
> +struct realtek_priv *
> +rtl83xx_probe(struct device *dev,
> +	      const struct realtek_interface_info *interface_info)
> +{
> +	const struct realtek_variant *var;
> +	struct realtek_priv *priv;
> +	struct regmap_config rc = {
> +			.reg_bits = 10, /* A4..A0 R4..R0 */
> +			.val_bits = 16,
> +			.reg_stride = 1,
> +			.max_register = 0xffff,
> +			.reg_format_endian = REGMAP_ENDIAN_BIG,
> +			.reg_read = interface_info->reg_read,
> +			.reg_write = interface_info->reg_write,
> +			.cache_type = REGCACHE_NONE,
> +			.lock = rtl83xx_lock,
> +			.unlock = rtl83xx_unlock,

Too many levels of indentation.

> +	};
> +	int ret;
> +
> +	var = of_device_get_match_data(dev);
> +	if (!var)
> +		return ERR_PTR(-EINVAL);
> +
> +	priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_init(&priv->map_lock);
> +
> +	rc.lock_arg = priv;
> +	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> +	if (IS_ERR(priv->map)) {
> +		ret = PTR_ERR(priv->map);
> +		dev_err(dev, "regmap init failed: %d\n", ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	rc.disable_locking = true;
> +	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
> +	if (IS_ERR(priv->map_nolock)) {
> +		ret = PTR_ERR(priv->map_nolock);
> +		dev_err(dev, "regmap init failed: %d\n", ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	/* Link forward and backward */
> +	priv->dev = dev;
> +	priv->variant = var;
> +	priv->ops = var->ops;
> +	priv->chip_data = (void *)priv + sizeof(*priv);
> +
> +	dev_set_drvdata(dev, priv);
> +	spin_lock_init(&priv->lock);
> +
> +	priv->leds_disabled = of_property_read_bool(dev->of_node,
> +						    "realtek,disable-leds");
> +
> +	/* TODO: if power is software controlled, set up any regulators here */
> +
> +	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->reset)) {
> +		dev_err(dev, "failed to get RESET GPIO\n");
> +		return ERR_CAST(priv->reset);
> +	}
> +	if (priv->reset) {
> +		gpiod_set_value(priv->reset, 1);
> +		dev_dbg(dev, "asserted RESET\n");
> +		msleep(REALTEK_HW_STOP_DELAY);
> +		gpiod_set_value(priv->reset, 0);
> +		msleep(REALTEK_HW_START_DELAY);
> +		dev_dbg(dev, "deasserted RESET\n");
> +	}
> +
> +	return priv;
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_probe, REALTEK_DSA);
> +
> +/**
> + * rtl83xx_register_switch() - detects and register a switch
> + * @priv: realtek_priv pointer
> + *
> + * This function first checks the switch chip ID and register a DSA
> + * switch.
> + *
> + * Context: Any context. Takes and releases priv->map_lock.
> + * Return: 0 on success, negative value for failure.
> + */
> +int rtl83xx_register_switch(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	ret = priv->ops->detect(priv);
> +	if (ret) {
> +		dev_err_probe(priv->dev, ret, "unable to detect switch\n");
> +		return ret;
> +	}
> +
> +	priv->ds = devm_kzalloc(priv->dev, sizeof(*priv->ds), GFP_KERNEL);
> +	if (!priv->ds)
> +		return -ENOMEM;
> +
> +	priv->ds->priv = priv;
> +	priv->ds->dev = priv->dev;
> +	priv->ds->ops = priv->ds_ops;
> +	priv->ds->num_ports = priv->num_ports;
> +
> +	ret = dsa_register_switch(priv->ds);
> +	if (ret) {
> +		dev_err_probe(priv->dev, ret, "unable to register switch\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_register_switch, REALTEK_DSA);
> +
> +/**
> + * rtl83xx_remove() - Cleanup a realtek switch driver
> + * @ctx: realtek_priv pointer

s/ctx/priv/

> + *
> + * If a method is provided, this function asserts the hard reset of the switch
> + * in order to avoid leaking traffic when the driver is gone.
> + *
> + * Context: Any context.
> + * Return: nothing
> + */
> +void rtl83xx_remove(struct realtek_priv *priv)
> +{
> +	if (!priv)
> +		return;
> +
> +	/* leave the device reset asserted */
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, 1);
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_remove, REALTEK_DSA);
> +
> +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
> +MODULE_DESCRIPTION("Realtek DSA switches common module");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
> new file mode 100644
> index 000000000000..9eb8197a58fa
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/rtl83xx.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _RTL83XX_H
> +#define _RTL83XX_H
> +
> +#include <linux/regmap.h>

I don't think anything from this header needs regmap.h?
For testing what is needed, you can create a dummy.c file which includes
only rtl83xx.h, and add just the necessary headers so that dummy.c
builds cleanly.

> +
> +struct realtek_interface_info {
> +	int (*reg_read)(void *ctx, u32 reg, u32 *val);
> +	int (*reg_write)(void *ctx, u32 reg, u32 val);
> +};
> +
> +void rtl83xx_lock(void *ctx);
> +void rtl83xx_unlock(void *ctx);
> +struct realtek_priv *
> +rtl83xx_probe(struct device *dev,
> +	      const struct realtek_interface_info *interface_info);
> +int rtl83xx_register_switch(struct realtek_priv *priv);
> +void rtl83xx_remove(struct realtek_priv *priv);
> +
> +#endif /* _RTL83XX_H */
> --
> 2.43.0
>

Re: [PATCH net-next v4 06/11] net: dsa: realtek: merge rtl83xx and interface modules into realtek-dsa

[Vladimir Oltean]

On Tue, Jan 23, 2024 at 06:55:58PM -0300, Luiz Angelo Daros de Luca wrote:
> Since rtl83xx and realtek-{smi,mdio} are always loaded together,
> we can optimize resource usage by consolidating them into a single
> module.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Vladimir Oltean]

On Tue, Jan 23, 2024 at 06:56:00PM -0300, Luiz Angelo Daros de Luca wrote:
> The line assigning dev.of_node in mdio_bus has been removed since the
> subsequent of_mdiobus_register will always overwrite it.

Please use present tense and imperative mood. "Remove the line assigning
dev.of_node, because ...".

> 
> ds->user_mii_bus is not assigned anymore[1].

"As discussed in [1], allow the DSA core to be simplified, by not
assigning ds->user_mii_bus when the MDIO bus is described in OF, as it
is unnecessary."

> It should work as before as long as the switch ports have a valid
> phy-handle property.
> 
> Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> to mdiobus"), we can put the "mdio" node just after the MDIO bus
> registration.

> The switch unregistration was moved into realtek_common_remove() as
> both interfaces now use the same code path.

Hopefully you can sort this part out in a separate patch that's
unrelated to the user_mii_bus cleanup, ideally in "net: dsa: realtek:
common rtl83xx module".

> 
> [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/realtek-mdio.c |  5 -----
>  drivers/net/dsa/realtek/realtek-smi.c  | 15 ++-------------
>  drivers/net/dsa/realtek/rtl83xx.c      |  2 ++
>  3 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 0171185ec665..c75b4550802c 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -158,11 +158,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev)
>  {
>  	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
>  
> -	if (!priv)
> -		return;
> -

The way I would structure these guards is I would keep them here, and
not in rtl83xx_remove() and rtl83xx_shutdown(). Then I would make sure
that rtl83xx_remove() is the exact opposite of just rtl83xx_probe(), and
rtl83xx_unregister_switch() is the exact opposite of just rtl83xx_register_switch().

And I would fix the error handling path of realtek_smi_probe() and
realtek_mdio_probe() to call rtl83xx_remove() when rtl83xx_register_switch()
fails.

> -	dsa_unregister_switch(priv->ds);
> -
>  	rtl83xx_remove(priv);
>  }
>  EXPORT_SYMBOL_NS_GPL(realtek_mdio_remove, REALTEK_DSA);
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 0ccb2a6059a6..a89813e527d2 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -331,7 +331,7 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  {
>  	struct realtek_priv *priv =  ds->priv;
>  	struct device_node *mdio_np;
> -	int ret;
> +	int ret = 0;
>  
>  	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
>  	if (!mdio_np) {
> @@ -344,15 +344,14 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  		ret = -ENOMEM;
>  		goto err_put_node;
>  	}
> +
>  	priv->user_mii_bus->priv = priv;
>  	priv->user_mii_bus->name = "SMI user MII";
>  	priv->user_mii_bus->read = realtek_smi_mdio_read;
>  	priv->user_mii_bus->write = realtek_smi_mdio_write;
>  	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d",
>  		 ds->index);
> -	priv->user_mii_bus->dev.of_node = mdio_np;
>  	priv->user_mii_bus->parent = priv->dev;
> -	ds->user_mii_bus = priv->user_mii_bus;
>  
>  	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
>  	if (ret) {
> @@ -361,8 +360,6 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  		goto err_put_node;
>  	}
>  
> -	return 0;
> -
>  err_put_node:
>  	of_node_put(mdio_np);
>  
> @@ -434,14 +431,6 @@ void realtek_smi_remove(struct platform_device *pdev)
>  {
>  	struct realtek_priv *priv = platform_get_drvdata(pdev);
>  
> -	if (!priv)
> -		return;
> -
> -	dsa_unregister_switch(priv->ds);
> -
> -	if (priv->user_mii_bus)
> -		of_node_put(priv->user_mii_bus->dev.of_node);
> -
>  	rtl83xx_remove(priv);
>  }
>  EXPORT_SYMBOL_NS_GPL(realtek_smi_remove, REALTEK_DSA);
> diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> index 3d07c5662fa4..53bacbacc82e 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.c
> +++ b/drivers/net/dsa/realtek/rtl83xx.c
> @@ -190,6 +190,8 @@ void rtl83xx_remove(struct realtek_priv *priv)
>  	if (!priv)
>  		return;
>  
> +	dsa_unregister_switch(priv->ds);
> +
>  	/* leave the device reset asserted */
>  	if (priv->reset)
>  		gpiod_set_value(priv->reset, 1);
> -- 
> 2.43.0
> 

Re: [PATCH net-next v4 09/11] net: dsa: realtek: migrate user_mii_bus setup to realtek-dsa

[Vladimir Oltean]

On Tue, Jan 23, 2024 at 06:56:01PM -0300, Luiz Angelo Daros de Luca wrote:
> In the user MDIO driver, despite numerous references to SMI, including
> its compatible string, there's nothing inherently specific about the SMI
> interface in the user MDIO bus. Consequently, the code has been migrated
> to the rtl83xx module. All references to SMI have been eliminated.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
> diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> index 53bacbacc82e..525d8c014136 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.c
> +++ b/drivers/net/dsa/realtek/rtl83xx.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
>  #include <linux/module.h>
> +#include <linux/of_mdio.h>
>  
>  #include "realtek.h"
>  #include "rtl83xx.h"
> @@ -42,6 +43,72 @@ void rtl83xx_unlock(void *ctx)
>  }
>  EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
>  
> +static int rtl83xx_user_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 rtl83xx_user_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);
> +}

Do we actually need to go through this extra indirection, or can the
priv->ops->phy_read/write() prototypes be made to take just struct
mii_bus * as their first argument?

> +
> +/**
> + * rtl83xx_setup_user_mdio() - register the user mii bus driver
> + * @ds: DSA switch associated with this user_mii_bus
> + *
> + * This function first gets and mdio node under the dev OF node, aborting
> + * if missing. That mdio node describing an mdio bus is used to register a
> + * new mdio bus.
> + *
> + * Context: Any context.
> + * Return: 0 on success, negative value for failure.
> + */
> +int rtl83xx_setup_user_mdio(struct dsa_switch *ds)
> +{
> +	struct realtek_priv *priv =  ds->priv;

Please remove the extra space here in " =  ds->priv".

> +	struct device_node *mdio_np;
> +	int ret = 0;
> +
> +	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
> +	if (!mdio_np) {
> +		dev_err(priv->dev, "no MDIO bus node\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->user_mii_bus = devm_mdiobus_alloc(priv->dev);
> +	if (!priv->user_mii_bus) {
> +		ret = -ENOMEM;
> +		goto err_put_node;
> +	}
> +
> +	priv->user_mii_bus->priv = priv;
> +	priv->user_mii_bus->name = "Realtek user MII";
> +	priv->user_mii_bus->read = rtl83xx_user_mdio_read;
> +	priv->user_mii_bus->write = rtl83xx_user_mdio_write;
> +	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "Realtek-%d",
> +		 ds->index);

There isn't much consistency here, but maybe something derived from
dev_name(dev) or %pOF would make it clearer that it describes a switch's
internal MDIO bus, rather than just any Realtek thing?

> +	priv->user_mii_bus->parent = priv->dev;
> +
> +	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
> +	if (ret) {
> +		dev_err(priv->dev, "unable to register MDIO bus %s\n",
> +			priv->user_mii_bus->id);
> +		goto err_put_node;
> +	}

Maybe this function would look a bit less cluttered with a temporary
struct mii_bus *bus variable, and priv->user_mii_bus gets assigned to
"bus" at the end (on success), and another struct device *dev = priv->dev.

> +
> +err_put_node:
> +	of_node_put(mdio_np);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_setup_user_mdio, REALTEK_DSA);
> +
>  /**
>   * rtl83xx_probe() - probe a Realtek switch
>   * @dev: the device being probed
> diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
> index 9eb8197a58fa..b5d464bb850d 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.h
> +++ b/drivers/net/dsa/realtek/rtl83xx.h
> @@ -12,6 +12,7 @@ struct realtek_interface_info {
>  
>  void rtl83xx_lock(void *ctx);
>  void rtl83xx_unlock(void *ctx);
> +int rtl83xx_setup_user_mdio(struct dsa_switch *ds);
>  struct realtek_priv *
>  rtl83xx_probe(struct device *dev,
>  	      const struct realtek_interface_info *interface_info);
> -- 
> 2.43.0
> 

Otherwise, this is in principle ok and what I wanted to see.

Re: [PATCH net-next v4 05/11] net: dsa: realtek: common rtl83xx module

[kernel test robot]

Hi Luiz,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Luiz-Angelo-Daros-de-Luca/net-dsa-realtek-drop-cleanup-from-realtek_ops/20240124-060710
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240123215606.26716-6-luizluca%40gmail.com
patch subject: [PATCH net-next v4 05/11] net: dsa: realtek: common rtl83xx module
config: i386-randconfig-004-20240127 (https://download.01.org/0day-ci/archive/20240127/202401270719.OZMv8I26-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240127/202401270719.OZMv8I26-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401270719.OZMv8I26-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/dsa/realtek/rtl83xx.c:189: warning: Function parameter or struct member 'priv' not described in 'rtl83xx_remove'
>> drivers/net/dsa/realtek/rtl83xx.c:189: warning: Excess function parameter 'ctx' description in 'rtl83xx_remove'


vim +189 drivers/net/dsa/realtek/rtl83xx.c

   177	
   178	/**
   179	 * rtl83xx_remove() - Cleanup a realtek switch driver
   180	 * @ctx: realtek_priv pointer
   181	 *
   182	 * If a method is provided, this function asserts the hard reset of the switch
   183	 * in order to avoid leaking traffic when the driver is gone.
   184	 *
   185	 * Context: Any context.
   186	 * Return: nothing
   187	 */
   188	void rtl83xx_remove(struct realtek_priv *priv)
 > 189	{
   190		if (!priv)
   191			return;
   192	
   193		/* leave the device reset asserted */
   194		if (priv->reset)
   195			gpiod_set_value(priv->reset, 1);
   196	}
   197	EXPORT_SYMBOL_NS_GPL(rtl83xx_remove, REALTEK_DSA);
   198	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v4 03/11] net: dsa: realtek: convert variants into real drivers

[Luiz Angelo Daros de Luca]

> On Tue, Jan 23, 2024 at 06:55:55PM -0300, Luiz Angelo Daros de Luca wrote:
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index df214b2f60d1..22a63f41e3f2 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -249,8 +276,20 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
> >       if (priv->reset)
> >               gpiod_set_value(priv->reset, 1);
> >  }
> > +EXPORT_SYMBOL_NS_GPL(realtek_mdio_remove, REALTEK_DSA);
> >
> > -static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> > +/**
> > + * realtek_mdio_shutdown() - Shutdown the driver of a MDIO-connected switch
> > + * @pdev: platform_device to probe on.
> > + *
> > + * This function should be used as the .shutdown in an mdio_driver. It shuts
> > + * down the DSA switch and cleans the platform driver data.
>
> , to prevent realtek_mdio_remove() from running afterwards, which is
> possible if the parent bus implements its own .shutdown() as .remove().

I didn't think that both could be called in sequence. I learned
something today. Thanks.

>
> > + *
> > + * Context: Any context.
> > + * Return: Nothing.
> > + *
> > + */
> > +void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> >  {
> >       struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
> >
> > @@ -521,8 +548,20 @@ static void realtek_smi_remove(struct platform_device *pdev)
> >       if (priv->reset)
> >               gpiod_set_value(priv->reset, 1);
> >  }
> > +EXPORT_SYMBOL_NS_GPL(realtek_smi_remove, REALTEK_DSA);
> >
> > -static void realtek_smi_shutdown(struct platform_device *pdev)
> > +/**
> > + * realtek_smi_shutdown() - Shutdown the driver of a SMI-connected switch
> > + * @pdev: platform_device to probe on.
> > + *
> > + * This function should be used as the .shutdown in a platform_driver. It shuts
> > + * down the DSA switch and cleans the platform driver data.
>
> Likewise.
>
> > + *
> > + * Context: Any context.
> > + * Return: Nothing.
> > + *
>
> I'm not sure if the blank line at the end of the comment is necessary.

I'll remove them.

Regards,

Luiz

Re: [PATCH net-next v4 05/11] net: dsa: realtek: common rtl83xx module

[Luiz Angelo Daros de Luca]

> On Tue, Jan 23, 2024 at 06:55:57PM -0300, Luiz Angelo Daros de Luca wrote:
> > Some code can be shared between both interface modules (MDIO and SMI)
> > and among variants. These interface functions migrated to a common
> > module:
> >
> > - rtl83xx_lock
> > - rtl83xx_unlock
> > - rtl83xx_probe
> > - rtl83xx_register_switch
>
> For API uniformity, I would also introduce rtl83xx_unregister_switch()
> to make it clear that there is a teardown step associated with this.
>
> > - rtl83xx_remove
>
> No rtl83xx_shutdown()?

I was avoiding one-line functions. However, I'll add those functions
for API uniformity. In the end, it will naturally untangle some other
problems you mentioned in other patches.

> You could centralize the comments from the mdio and smi interfaces into
> the rtl83xx common layer.

I'll use a more generic kdoc for each interface and a detailed one in common.

>
> >
> > The reset during probe was moved to the end of the common probe. This way,
> > we avoid a reset if anything else fails.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > ---
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 57bd5d8814c2..26b8371ecc87 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -303,3 +194,5 @@ EXPORT_SYMBOL_NS_GPL(realtek_mdio_shutdown, REALTEK_DSA);
> >  MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
> >  MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
> >  MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS(REALTEK_DSA);
> > +
>
> Stray blank line at the end of the file.

OK. I'll remove all of them.

> > diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> > new file mode 100644
> > index 000000000000..57d185226b03
> > --- /dev/null
> > +++ b/drivers/net/dsa/realtek/rtl83xx.c
> > @@ -0,0 +1,201 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <linux/module.h>
> > +
> > +#include "realtek.h"
> > +#include "rtl83xx.h"
> > +
> > +/**
> > + * rtl83xx_lock() - Locks the mutex used by regmaps
> > + * @ctx: realtek_priv pointer
> > + *
> > + * This function is passed to regmap to be used as the lock function.
> > + * It is also used externally to block regmap before executing multiple
> > + * operations that must happen in sequence (which will use
> > + * realtek_priv.map_nolock instead).
> > + *
> > + * Context: Any context. Holds priv->map_lock lock.
>
> Not "any context", but "sleepable context". You cannot acquire a mutex
> in atomic context. Actually this applies across the board in this patch
> set. The entry points into DSA also take mutex_lock(&dsa2_mutex), so
> this also applies to its callers' context.

Yes. I'll update all kdocs that might use a lock. I guess just the
unlock that is actually "any context".
I believe it will be much easier to write a kdoc when all called
symbols also have a kdoc with a Context: field. It would avoid all
kdoc writers to dig down the stack with every called function. It's a
good move to require kdoc for all exported symbols.

> > + * Return: nothing
> > + */
> > +void rtl83xx_lock(void *ctx)
> > +{
> > +     struct realtek_priv *priv = ctx;
> > +
> > +     mutex_lock(&priv->map_lock);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(rtl83xx_lock, REALTEK_DSA);
> > +
> > +/**
> > + * rtl83xx_unlock() - Unlocks the mutex used by regmaps
> > + * @ctx: realtek_priv pointer
> > + *
> > + * This function unlocks the lock acquired by rtl83xx_lock.
> > + *
> > + * Context: Any context. Releases priv->map_lock lock
> > + * Return: nothing
> > + */
> > +void rtl83xx_unlock(void *ctx)
> > +{
> > +     struct realtek_priv *priv = ctx;
> > +
> > +     mutex_unlock(&priv->map_lock);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
> > +
> > +/**
> > + * rtl83xx_probe() - probe a Realtek switch
> > + * @dev: the device being probed
> > + * @interface_info: reg read/write methods for a specific management interface.
>
> Leave this description more open-ended, otherwise it will have to be
> modified when struct realtek_interface_info gains more fields.

OK, it makes sense. However I'm not sure management interfaces might
differ in anything but the reg read/write.

>
> > + *
> > + * This function initializes realtek_priv and read data from the device-tree
>
> s/read/reads/
> s/device-tree/device tree/

OK

> > + * node. The switch is hard resetted if a method is provided.
> > + *
> > + * Context: Any context.
> > + * Return: Pointer to the realtek_priv or ERR_PTR() in case of failure.
> > + *
> > + * The realtek_priv pointer does not need to be freed as it is controlled by
> > + * devres.
> > + *
> > + */
> > +struct realtek_priv *
> > +rtl83xx_probe(struct device *dev,
> > +           const struct realtek_interface_info *interface_info)
> > +{
> > +     const struct realtek_variant *var;
> > +     struct realtek_priv *priv;
> > +     struct regmap_config rc = {
> > +                     .reg_bits = 10, /* A4..A0 R4..R0 */
> > +                     .val_bits = 16,
> > +                     .reg_stride = 1,
> > +                     .max_register = 0xffff,
> > +                     .reg_format_endian = REGMAP_ENDIAN_BIG,
> > +                     .reg_read = interface_info->reg_read,
> > +                     .reg_write = interface_info->reg_write,
> > +                     .cache_type = REGCACHE_NONE,
> > +                     .lock = rtl83xx_lock,
> > +                     .unlock = rtl83xx_unlock,
>
> Too many levels of indentation.

OK

> > +/**
> > + * rtl83xx_remove() - Cleanup a realtek switch driver
> > + * @ctx: realtek_priv pointer
>
> s/ctx/priv/

I was missing some kernel warnings with too much debug output (V=s1c).

> > --- /dev/null
> > +++ b/drivers/net/dsa/realtek/rtl83xx.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +
> > +#ifndef _RTL83XX_H
> > +#define _RTL83XX_H
> > +
> > +#include <linux/regmap.h>
>
> I don't think anything from this header needs regmap.h?
> For testing what is needed, you can create a dummy.c file which includes
> only rtl83xx.h, and add just the necessary headers so that dummy.c
> builds cleanly.
>

It is a leftover from the time probe was receiving a regmap_config. It
is, indeed, missing in rtl83xx.c.

Regards,

Luiz

Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Luiz Angelo Daros de Luca]

> On Tue, Jan 23, 2024 at 06:56:00PM -0300, Luiz Angelo Daros de Luca wrote:
> > The line assigning dev.of_node in mdio_bus has been removed since the
> > subsequent of_mdiobus_register will always overwrite it.
>
> Please use present tense and imperative mood. "Remove the line assigning
> dev.of_node, because ...".

OK

> >
> > ds->user_mii_bus is not assigned anymore[1].
>
> "As discussed in [1], allow the DSA core to be simplified, by not
> assigning ds->user_mii_bus when the MDIO bus is described in OF, as it
> is unnecessary."

OK

> > It should work as before as long as the switch ports have a valid
> > phy-handle property.
> >
> > Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> > to mdiobus"), we can put the "mdio" node just after the MDIO bus
> > registration.
>
> > The switch unregistration was moved into realtek_common_remove() as
> > both interfaces now use the same code path.
>
> Hopefully you can sort this part out in a separate patch that's
> unrelated to the user_mii_bus cleanup, ideally in "net: dsa: realtek:
> common rtl83xx module".

Yes. With the introduction of rtl83xx_unregister_switch, this part is gone.

> >
> > [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > ---
> >  drivers/net/dsa/realtek/realtek-mdio.c |  5 -----
> >  drivers/net/dsa/realtek/realtek-smi.c  | 15 ++-------------
> >  drivers/net/dsa/realtek/rtl83xx.c      |  2 ++
> >  3 files changed, 4 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 0171185ec665..c75b4550802c 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -158,11 +158,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev)
> >  {
> >       struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
> >
> > -     if (!priv)
> > -             return;
> > -
>
> The way I would structure these guards is I would keep them here, and
> not in rtl83xx_remove() and rtl83xx_shutdown(). Then I would make sure
> that rtl83xx_remove() is the exact opposite of just rtl83xx_probe(), and
> rtl83xx_unregister_switch() is the exact opposite of just rtl83xx_register_switch().

It looks like it is now, although "remove" mostly leaves the job for devm.

I'm still not sure if we have the correct shutdown/remove code. From
what I could understand, driver shutdown is called during system
shutdown while remove is called when the driver is removed. However,
it looks like that both might be called in sequence. Would it be
shutdown,remove? (it's probably that because there is the
dev_set_drvdata(priv->dev, NULL) in shutdown).
However, if shutdown should prepare the system for another OS, I
believe it should be asserting the hw reset as well or remove should
stop doing it. Are the dsa_switch_shutdown and dsa_switch_unregister
enough to prevent leaking traffic after the driver is gone? It does
disable all ports. Or should we have a fallback "isolate all ports"
when a hw reset is missing? I guess the u-boot driver does something
like that.

I don't think it is mandatory for this series but if we got something
wrong, it would be nice to fix it.

> And I would fix the error handling path of realtek_smi_probe() and
> realtek_mdio_probe() to call rtl83xx_remove() when rtl83xx_register_switch()
> fails.

With the rtl83xx_unregister_switch, it was kept in realtek_smi_probe() and
realtek_mdio_probe.

Regards,

Luiz

Re: [PATCH net-next v4 09/11] net: dsa: realtek: migrate user_mii_bus setup to realtek-dsa

[Luiz Angelo Daros de Luca]

> On Tue, Jan 23, 2024 at 06:56:01PM -0300, Luiz Angelo Daros de Luca wrote:
> > In the user MDIO driver, despite numerous references to SMI, including
> > its compatible string, there's nothing inherently specific about the SMI
> > interface in the user MDIO bus. Consequently, the code has been migrated
> > to the rtl83xx module. All references to SMI have been eliminated.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > ---
> > diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> > index 53bacbacc82e..525d8c014136 100644
> > --- a/drivers/net/dsa/realtek/rtl83xx.c
> > +++ b/drivers/net/dsa/realtek/rtl83xx.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0+
> >
> >  #include <linux/module.h>
> > +#include <linux/of_mdio.h>
> >
> >  #include "realtek.h"
> >  #include "rtl83xx.h"
> > @@ -42,6 +43,72 @@ void rtl83xx_unlock(void *ctx)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
> >
> > +static int rtl83xx_user_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 rtl83xx_user_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);
> > +}
>
> Do we actually need to go through this extra indirection, or can the
> priv->ops->phy_read/write() prototypes be made to take just struct
> mii_bus * as their first argument?

We would just postpone the same code in phy_write/read. We only need priv there.

Using mii_bus will also prevent an easy way for the driver to query
those registers (although not used anymore after ds_switch_ops
.phy_read/write are gone)

>
> > +
> > +/**
> > + * rtl83xx_setup_user_mdio() - register the user mii bus driver
> > + * @ds: DSA switch associated with this user_mii_bus
> > + *
> > + * This function first gets and mdio node under the dev OF node, aborting
> > + * if missing. That mdio node describing an mdio bus is used to register a
> > + * new mdio bus.
> > + *
> > + * Context: Any context.
> > + * Return: 0 on success, negative value for failure.
> > + */
> > +int rtl83xx_setup_user_mdio(struct dsa_switch *ds)
> > +{
> > +     struct realtek_priv *priv =  ds->priv;
>
> Please remove the extra space here in " =  ds->priv".

OK

>
> > +     struct device_node *mdio_np;
> > +     int ret = 0;
> > +
> > +     mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
> > +     if (!mdio_np) {
> > +             dev_err(priv->dev, "no MDIO bus node\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     priv->user_mii_bus = devm_mdiobus_alloc(priv->dev);
> > +     if (!priv->user_mii_bus) {
> > +             ret = -ENOMEM;
> > +             goto err_put_node;
> > +     }
> > +
> > +     priv->user_mii_bus->priv = priv;
> > +     priv->user_mii_bus->name = "Realtek user MII";
> > +     priv->user_mii_bus->read = rtl83xx_user_mdio_read;
> > +     priv->user_mii_bus->write = rtl83xx_user_mdio_write;
> > +     snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "Realtek-%d",
> > +              ds->index);
>
> There isn't much consistency here, but maybe something derived from
> dev_name(dev) or %pOF would make it clearer that it describes a switch's
> internal MDIO bus, rather than just any Realtek thing?

Yes, Realtek-0:<port> is not very informative.

Using only dev_name will give me these device names:

mdio-bus:1d <- switch1 in the conduit mdio bus at address 29 (0x1d)
(not the user mdio bus)
mdio-bus:1d:00 <- port 0 in switch1
mdio-bus:1d:01 <- port 1 in switch1
...

It is quite compact and easy to identify which device is under which.
But, in this case, mdio-bus:1d would be both the switch device name in
the conduit bus and the name of the switch internal mdio bus.

devices/platform/10100000.ethernet/mdio_bus/mdio-bus/mdio-bus:1d/mdio_bus/mdio-bus:1d/mdio-bus:1d:00
devices/platform/10100000.ethernet/mdio_bus/mdio-bus/mdio-bus:1d/mdio_bus/mdio-bus:1d/mdio-bus:1d:01

For SMI-connected switches, it changes a little bit but the essence is the same:

devices/platform/switch/mdio_bus/switch/switch:00
devices/platform/switch/mdio_bus/switch/switch:01

I guess the best approach is to append something that identifies the
other mdio bus, for example ":user_mii". The result is something like
this:

mdio-bus:1d
mdio-bus:1d:user_mii:00
mdio-bus:1d:user_mii:01
...

Or, for SMI:

switch:user_mii:00
switch:user_mii:01
...

It is good enough for me as these switches have only one MDIO bus.

We could also bring up some kind of a general suggestion for naming
user_mii buses. In that case, we should be prepared for multiple mdio
buses and the mdio node name+@unit (%pOFP) might be appropriate. We
would get something like this:

mdio-bus:1d:mdio:00
mdio-bus:1d:mdio:01

Or, for SMI:

switch:mdio:00
switch:mdio:01

If there are multiple MDIO buses, it will be mdio@N (not tested).

mdio-bus:1d:mdio@0:00
mdio-bus:1d:mdio@0:01
...
mdio-bus:1d:mdio@1:00
mdio-bus:1d:mdio@1:01
...

The device_name can also be replaced with %pOFP, which will differ
only for MDIO-connected switches:

mdio-bus:1d
switch1@29:mdio:01
switch1@29:mdio:02
...

But it will not have a clear relation with the parent MDIO bus.

I also considered %pOFf but it gives a more verbose device name
without adding too much useful information:

!ethernet@10100000!mdio-bus!switch1@29:00
!ethernet@10100000!mdio-bus!switch1@29:01
!ethernet@10100000!mdio-bus!switch1@29:02

And I'm reluctant to add those "!" as they may not play nice with some
non-ideal scripts reading sysfs. I would, at least, replace them with
":" .

> > +     priv->user_mii_bus->parent = priv->dev;
> > +
> > +     ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
> > +     if (ret) {
> > +             dev_err(priv->dev, "unable to register MDIO bus %s\n",
> > +                     priv->user_mii_bus->id);
> > +             goto err_put_node;
> > +     }
>
> Maybe this function would look a bit less cluttered with a temporary
> struct mii_bus *bus variable, and priv->user_mii_bus gets assigned to
> "bus" at the end (on success), and another struct device *dev = priv->dev.

Yes, it looks much cleaner.

> Otherwise, this is in principle ok and what I wanted to see.

I'm glad to hear that. Thanks!

Regards,

Luiz

Re: [PATCH net-next v4 09/11] net: dsa: realtek: migrate user_mii_bus setup to realtek-dsa

[Vladimir Oltean]

On Sun, Jan 28, 2024 at 11:49:42PM -0300, Luiz Angelo Daros de Luca wrote:
> Using mii_bus will also prevent an easy way for the driver to query
> those registers (although not used anymore after ds_switch_ops
> .phy_read/write are gone)

Exactly, there is no other remaining call to priv->ops->phy_read() and
priv->ops->phy_write(), so their prototypes can be tailored such that
they need no extra adapter.

> I guess the best approach is to append something that identifies the
> other mdio bus, for example ":user_mii". The result is something like
> this:
> 
> mdio-bus:1d
> mdio-bus:1d:user_mii:00
> mdio-bus:1d:user_mii:01
> ...
> 
> Or, for SMI:
> 
> switch:user_mii:00
> switch:user_mii:01
> ...

This looks good.

> 
> It is good enough for me as these switches have only one MDIO bus.
> 
> We could also bring up some kind of a general suggestion for naming
> user_mii buses. In that case, we should be prepared for multiple mdio
> buses and the mdio node name+@unit (%pOFP) might be appropriate. We
> would get something like this:
> 
> mdio-bus:1d:mdio:00
> mdio-bus:1d:mdio:01
> 
> Or, for SMI:
> 
> switch:mdio:00
> switch:mdio:01
> 
> If there are multiple MDIO buses, it will be mdio@N (not tested).
> 
> mdio-bus:1d:mdio@0:00
> mdio-bus:1d:mdio@0:01
> ...
> mdio-bus:1d:mdio@1:00
> mdio-bus:1d:mdio@1:01
> ...

SJA1110 has 2 MDIO buses and they are named:

	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-base-tx",
		 dev_name(priv->ds->dev));
	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-base-t1",
		 dev_name(priv->ds->dev));

which I think is more descriptive, because in its case, the indices in
"mdio@0" and "mdio@1" are arbitrary numbers.

I don't think we'll find a way to unify the naming convention across the
board. Let's use dev_name(dev) + "-some-driver-specific-qualifier" here,
and hopefully also as a convention from now on.

> I also considered %pOFf but it gives a more verbose device name
> without adding too much useful information:
> 
> !ethernet@10100000!mdio-bus!switch1@29:00
> !ethernet@10100000!mdio-bus!switch1@29:01
> !ethernet@10100000!mdio-bus!switch1@29:02
> 
> And I'm reluctant to add those "!" as they may not play nice with some
> non-ideal scripts reading sysfs. I would, at least, replace them with
> ":" .

I'm also not in love with the exclamation marks that the sysfs code has
to use, to replace the forward slashes that can't be represented in the
filesystem.

Re: [PATCH net-next v4 02/11] net: dsa: realtek: introduce REALTEK_DSA namespace

[Florian Fainelli]

On 1/23/2024 1:55 PM, Luiz Angelo Daros de Luca wrote:
> Create a namespace to group the exported symbols.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

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

Re: [PATCH net-next v4 04/11] net: dsa: realtek: keep variant reference in realtek_priv

[Florian Fainelli]

On 1/23/2024 1:55 PM, Luiz Angelo Daros de Luca wrote:
> Instead of copying values from the variant, we can keep a reference in
> realtek_priv.
> 
> This is a preliminary change for sharing code betwen interfaces. It will
> allow to move most of the probe into a common module while still allow
> code specific to each interface to read variant fields.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---

[snip]

> diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> index e9ee778665b2..0c51b5132c61 100644
> --- a/drivers/net/dsa/realtek/realtek.h
> +++ b/drivers/net/dsa/realtek/realtek.h
> @@ -58,9 +58,6 @@ struct realtek_priv {
>   	struct mii_bus		*bus;
>   	int			mdio_addr;
>   
> -	unsigned int		clk_delay;
> -	u8			cmd_read;
> -	u8			cmd_write;
>   	spinlock_t		lock; /* Locks around command writes */
>   	struct dsa_switch	*ds;
>   	struct irq_domain	*irqdomain;
> @@ -79,6 +76,8 @@ struct realtek_priv {
>   	int			vlan_enabled;
>   	int			vlan4k_enabled;
>   
> +	const struct realtek_variant *variant;

This is not probably performance sensitive but should the variant 
pointer be moved where clk_delay was such that we preserve a somewhat 
similar cacheline alignment?

Regardless of that:

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

Re: [PATCH net-next v4 07/11] net: dsa: realtek: get internal MDIO node by name

[Florian Fainelli]

On 1/23/2024 1:55 PM, Luiz Angelo Daros de Luca wrote:
> The binding docs requires for SMI-connected devices that the switch
> must have a child node named "mdio" and with a compatible string of
> "realtek,smi-mdio". Meanwile, for MDIO-connected switches, the binding
> docs only requires a child node named "mdio".
> 
> This patch changes the driver to use the common denominator for both
> interfaces, looking for the MDIO node by name, ignoring the compatible
> string.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

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

Re: [PATCH net-next v4 06/11] net: dsa: realtek: merge rtl83xx and interface modules into realtek-dsa

[Florian Fainelli]

On 1/23/2024 1:55 PM, Luiz Angelo Daros de Luca wrote:
> Since rtl83xx and realtek-{smi,mdio} are always loaded together,
> we can optimize resource usage by consolidating them into a single
> module.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

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

Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Vladimir Oltean]

On Sun, Jan 28, 2024 at 11:12:25PM -0300, Luiz Angelo Daros de Luca wrote:
> It looks like it is now, although "remove" mostly leaves the job for devm.
> 
> I'm still not sure if we have the correct shutdown/remove code. From
> what I could understand, driver shutdown is called during system
> shutdown while remove is called when the driver is removed.

Yeah, poweroff or reboot or kexec.

> However, it looks like that both might be called in sequence. Would it
> be shutdown,remove? (it's probably that because there is the
> dev_set_drvdata(priv->dev, NULL) in shutdown).

Yeah, while shutting down, the (SPI, I2C, MDIO, ...) bus driver might
call spi_unregister_controller() on shutdown(), and this will also call
remove() on its child devices. Even the Raspberry Pi SPI controller does
this, AFAIR. The idea of implementing .shutdown() as .remove() is to
gain more code coverage by sharing code, which should reduce chances of
bugs in less-tested code (remove). Or at least that's how the saying goes...

> However, if shutdown should prepare the system for another OS, I
> believe it should be asserting the hw reset as well or remove should
> stop doing it. Are the dsa_switch_shutdown and dsa_switch_unregister
> enough to prevent leaking traffic after the driver is gone? It does
> disable all ports.  Or should we have a fallback "isolate all ports"
> when a hw reset is missing? I guess the u-boot driver does something
> like that.
> 
> I don't think it is mandatory for this series but if we got something
> wrong, it would be nice to fix it.

I don't really know anything at all about kexec. You might want to get
input from someone who uses it. All that I know is that this should do
something meaningful (not crash, and still work in the second kernel):

root@debian:~# kexec -l /boot/Image.gz --reuse-cmdline && kexec -e
[   46.335430] mscc_felix 0000:00:00.5 swp3: Link is Down
[   46.345747] fsl_enetc 0000:00:00.2 eno2: Link is Down
[   46.419201] kvm: exiting hardware virtualization
[   46.424036] kexec_core: Starting new kernel
[   46.471657] psci: CPU1 killed (polled 0 ms)
[   46.486060] Bye!
[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd083]
[    0.000000] Linux version 5.16.0-rc2-07010-ga9b9500ffaac-dirty (tigrisor@skbuf) (aarch64-none-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 10.2-2020.11 (arm-10.16)) 10.2.1 20201103, GNU ld (GNU Toolchain for the A-profile Architecture 10.2-2020.11 (arm-10.16)) 2.35.1.20201028) #1519 SMP PREEMPT Wed Dec 1 08:59:13 EET 2021
[    0.000000] Machine model: LS1028A RDB Board
[    0.000000] earlycon: uart8250 at MMIO 0x00000000021c0500 (options '')
[    0.000000] printk: bootconsole [uart8250] enabled
[    0.000000] efi: UEFI not found.
[    0.000000] NUMA: No NUMA configuration found
[    0.000000] NUMA: Faking a node at [mem 0x0000000080000000-0x00000020ffffffff]
[    0.000000] NUMA: NODE_DATA [mem 0x20ff6fab80-0x20ff6fcfff]
(...)

which in this case it does.

From other discussions I've had, there seems to be interest in quite the
opposite thing, in fact. Reboot the SoC running Linux, but do not
disturb traffic flowing through the switch, and somehow pick up the
state from where the previous kernel left it.

Now, obviously that doesn't currently work, but it does raise the
question about the usefulness of resetting the switch on shutdown.

Re: [PATCH net-next v4 05/11] net: dsa: realtek: common rtl83xx module

[Vladimir Oltean]

On Sun, Jan 28, 2024 at 09:09:38PM -0300, Luiz Angelo Daros de Luca wrote:
> > Not "any context", but "sleepable context". You cannot acquire a mutex
> > in atomic context. Actually this applies across the board in this patch
> > set. The entry points into DSA also take mutex_lock(&dsa2_mutex), so
> > this also applies to its callers' context.
> 
> Yes. I'll update all kdocs that might use a lock. I guess just the
> unlock that is actually "any context".

Well, the unlock has to have the lock acquired....

> > > + * rtl83xx_remove() - Cleanup a realtek switch driver
> > > + * @ctx: realtek_priv pointer
> >
> > s/ctx/priv/
> 
> I was missing some kernel warnings with too much debug output (V=s1c).

If it builds cleanly patch by patch with W=1 C=1 you should be good.

Re: [PATCH net-next v4 03/11] net: dsa: realtek: convert variants into real drivers

[Vladimir Oltean]

On Sun, Jan 28, 2024 at 08:34:29PM -0300, Luiz Angelo Daros de Luca wrote:
> > > + * This function should be used as the .shutdown in an mdio_driver. It shuts
> > > + * down the DSA switch and cleans the platform driver data.
> >
> > , to prevent realtek_mdio_remove() from running afterwards, which is
> > possible if the parent bus implements its own .shutdown() as .remove().
> 
> I didn't think that both could be called in sequence. I learned
> something today. Thanks.

And neither did I, until it happened to a user... More details in commit
0650bf52b31f ("net: dsa: be compatible with masters which unregister on
shutdown") after "However, complications arise really quickly".

Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Florian Fainelli]

On 1/29/2024 8:15 AM, Vladimir Oltean wrote:
> On Sun, Jan 28, 2024 at 11:12:25PM -0300, Luiz Angelo Daros de Luca wrote:
>> It looks like it is now, although "remove" mostly leaves the job for devm.
>>
>> I'm still not sure if we have the correct shutdown/remove code. From
>> what I could understand, driver shutdown is called during system
>> shutdown while remove is called when the driver is removed.
> 
> Yeah, poweroff or reboot or kexec.
> 
>> However, it looks like that both might be called in sequence. Would it
>> be shutdown,remove? (it's probably that because there is the
>> dev_set_drvdata(priv->dev, NULL) in shutdown).
> 
> Yeah, while shutting down, the (SPI, I2C, MDIO, ...) bus driver might
> call spi_unregister_controller() on shutdown(), and this will also call
> remove() on its child devices. Even the Raspberry Pi SPI controller does
> this, AFAIR. The idea of implementing .shutdown() as .remove() is to
> gain more code coverage by sharing code, which should reduce chances of
> bugs in less-tested code (remove). Or at least that's how the saying goes...
> 
>> However, if shutdown should prepare the system for another OS, I
>> believe it should be asserting the hw reset as well or remove should
>> stop doing it. Are the dsa_switch_shutdown and dsa_switch_unregister
>> enough to prevent leaking traffic after the driver is gone? It does
>> disable all ports.  Or should we have a fallback "isolate all ports"
>> when a hw reset is missing? I guess the u-boot driver does something
>> like that.
>>
>> I don't think it is mandatory for this series but if we got something
>> wrong, it would be nice to fix it.
> 
> I don't really know anything at all about kexec. You might want to get
> input from someone who uses it. All that I know is that this should do
> something meaningful (not crash, and still work in the second kernel):
> 
> root@debian:~# kexec -l /boot/Image.gz --reuse-cmdline && kexec -e
> [   46.335430] mscc_felix 0000:00:00.5 swp3: Link is Down
> [   46.345747] fsl_enetc 0000:00:00.2 eno2: Link is Down
> [   46.419201] kvm: exiting hardware virtualization
> [   46.424036] kexec_core: Starting new kernel
> [   46.471657] psci: CPU1 killed (polled 0 ms)
> [   46.486060] Bye!
> [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd083]
> [    0.000000] Linux version 5.16.0-rc2-07010-ga9b9500ffaac-dirty (tigrisor@skbuf) (aarch64-none-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 10.2-2020.11 (arm-10.16)) 10.2.1 20201103, GNU ld (GNU Toolchain for the A-profile Architecture 10.2-2020.11 (arm-10.16)) 2.35.1.20201028) #1519 SMP PREEMPT Wed Dec 1 08:59:13 EET 2021
> [    0.000000] Machine model: LS1028A RDB Board
> [    0.000000] earlycon: uart8250 at MMIO 0x00000000021c0500 (options '')
> [    0.000000] printk: bootconsole [uart8250] enabled
> [    0.000000] efi: UEFI not found.
> [    0.000000] NUMA: No NUMA configuration found
> [    0.000000] NUMA: Faking a node at [mem 0x0000000080000000-0x00000020ffffffff]
> [    0.000000] NUMA: NODE_DATA [mem 0x20ff6fab80-0x20ff6fcfff]
> (...)
> 
> which in this case it does.
> 
>  From other discussions I've had, there seems to be interest in quite the
> opposite thing, in fact. Reboot the SoC running Linux, but do not
> disturb traffic flowing through the switch, and somehow pick up the
> state from where the previous kernel left it.

Yes this is actually an use case that is very dear to the users of DSA 
in an airplane. The entertainment system in the seat in front of you 
typically has a left, CPU/display and right set of switch ports. Across 
the 300+ units in the plane each entertainment systems runs STP to avoid 
loops being created when one of the display units goes bad. Occasionally 
cabin crew members will have to swap those units out since they tend to 
wear out. When they do, the switch operates in a headless mode and it 
would be unfortunate that plugging in a display unit into the network 
again would be disrupting existing traffic. I have seen out of tree 
patches doing that, but there was not a good way to make them upstream 
quality.

> 
> Now, obviously that doesn't currently work, but it does raise the
> question about the usefulness of resetting the switch on shutdown.

The users would really care would likely introduce sufficient amounts of 
control knobs (module parameters, ethtool private flags, devlink?) to 
control that behavior. It does seem however universally acceptable to 
stop any DMAs and packets from flowing as a default and safe 
implementation to the upstream kernel.
-- 
Florian

Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Vladimir Oltean]

On Mon, Jan 29, 2024 at 08:22:47AM -0800, Florian Fainelli wrote:
> It does seem however universally acceptable to stop any DMAs and
> packets from flowing as a default and safe implementation to the
> upstream kernel.

DMA I can understand, because you wouldn't want the hardware to notify
you of a buffer you have no idea about. But DSA doesn't assume that
switches have DMA, and generally speaking, stopping the offloaded
traffic path seems unnecessary. It will be stopped when the new kernel
sets up the interfaces as standalone, renegotiates the link, etc.

Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Florian Fainelli]

On 1/29/2024 8:43 AM, Vladimir Oltean wrote:
> On Mon, Jan 29, 2024 at 08:22:47AM -0800, Florian Fainelli wrote:
>> It does seem however universally acceptable to stop any DMAs and
>> packets from flowing as a default and safe implementation to the
>> upstream kernel.
> 
> DMA I can understand, because you wouldn't want the hardware to notify
> you of a buffer you have no idea about. But DSA doesn't assume that
> switches have DMA, and generally speaking, stopping the offloaded
> traffic path seems unnecessary. It will be stopped when the new kernel
> sets up the interfaces as standalone, renegotiates the link, etc.

Very true, I suppose most driver writers might want to stop the packet 
flow from the source however and simply disable switch ports. There are 
also power management considerations at least there were for some of the 
products I worked with.
-- 
Florian

Re: [PATCH net-next v4 04/11] net: dsa: realtek: keep variant reference in realtek_priv

[Luiz Angelo Daros de Luca]

> On 1/23/2024 1:55 PM, Luiz Angelo Daros de Luca wrote:
> > Instead of copying values from the variant, we can keep a reference in
> > realtek_priv.
> >
> > This is a preliminary change for sharing code betwen interfaces. It will
> > allow to move most of the probe into a common module while still allow
> > code specific to each interface to read variant fields.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > ---
>
> [snip]
>
> > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> > index e9ee778665b2..0c51b5132c61 100644
> > --- a/drivers/net/dsa/realtek/realtek.h
> > +++ b/drivers/net/dsa/realtek/realtek.h
> > @@ -58,9 +58,6 @@ struct realtek_priv {
> >       struct mii_bus          *bus;
> >       int                     mdio_addr;
> >
> > -     unsigned int            clk_delay;
> > -     u8                      cmd_read;
> > -     u8                      cmd_write;
> >       spinlock_t              lock; /* Locks around command writes */
> >       struct dsa_switch       *ds;
> >       struct irq_domain       *irqdomain;
> > @@ -79,6 +76,8 @@ struct realtek_priv {
> >       int                     vlan_enabled;
> >       int                     vlan4k_enabled;
> >
> > +     const struct realtek_variant *variant;
>
> This is not probably performance sensitive but should the variant
> pointer be moved where clk_delay was such that we preserve a somewhat
> similar cacheline alignment?

I'll move it. I guess alignment wasn't considered before with two u8
and still might be misaligned with the "bool leds_disabled;". But, as
you said, it is not performance sensitive.

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

Regards,

Luiz

Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Arınç ÜNAL]

On 29.01.2024 19:22, Florian Fainelli wrote:
> 
> 
> On 1/29/2024 8:15 AM, Vladimir Oltean wrote:
>>  From other discussions I've had, there seems to be interest in quite the
>> opposite thing, in fact. Reboot the SoC running Linux, but do not
>> disturb traffic flowing through the switch, and somehow pick up the
>> state from where the previous kernel left it.
> 
> Yes this is actually an use case that is very dear to the users of DSA in an airplane. The entertainment system in the seat in front of you typically has a left, CPU/display and right set of switch ports. Across the 300+ units in the plane each entertainment systems runs STP to avoid loops being created when one of the display units goes bad. Occasionally cabin crew members will have to swap those units out since they tend to wear out. When they do, the switch operates in a headless mode and it would be unfortunate that plugging in a display unit into the network again would be disrupting existing traffic. I have seen out of tree patches doing that, but there was not a good way to make them upstream quality.

This piqued my interest. I'm trying to understand how exactly plugging in a
display unit into the network would disrupt the traffic flow. Is this about
all network interfaces attached to the bridge interface being blocked when
a new link is established to relearn the changed topology?

Arınç

Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Andrew Lunn]

On Tue, Jan 30, 2024 at 05:40:26PM +0300, Arınç ÜNAL wrote:
> On 29.01.2024 19:22, Florian Fainelli wrote:
> > 
> > 
> > On 1/29/2024 8:15 AM, Vladimir Oltean wrote:
> > >  From other discussions I've had, there seems to be interest in quite the
> > > opposite thing, in fact. Reboot the SoC running Linux, but do not
> > > disturb traffic flowing through the switch, and somehow pick up the
> > > state from where the previous kernel left it.
> > 
> > Yes this is actually an use case that is very dear to the users of DSA in an airplane. The entertainment system in the seat in front of you typically has a left, CPU/display and right set of switch ports. Across the 300+ units in the plane each entertainment systems runs STP to avoid loops being created when one of the display units goes bad. Occasionally cabin crew members will have to swap those units out since they tend to wear out. When they do, the switch operates in a headless mode and it would be unfortunate that plugging in a display unit into the network again would be disrupting existing traffic. I have seen out of tree patches doing that, but there was not a good way to make them upstream quality.
> 
> This piqued my interest. I'm trying to understand how exactly plugging in a
> display unit into the network would disrupt the traffic flow. Is this about
> all network interfaces attached to the bridge interface being blocked when
> a new link is established to relearn the changed topology?

The hardware is split into two parts, a cradle and the display
unit. The switch itself is in the cradle embedded in the seat
back. The display unit contains the CPU, GPU, storage etc. There is a
single Ethernet interface between the display unit and the cradle,
along with MDIO, power, audio cables for the headphone jack etc.

When you take out the display unit, you disconnect the switches
management plain. The CPU has gone, and its the CPU running STP,
sending and receiving BPDUs, etc. But the switch is still powered, and
switching packets, keeping the network going, at least for a while.

When you plug in a display unit, it boots. As typical for any computer
booting, it assumes the hardware is in an unknown state, and hits the
switch with a reset. That then kills the local networking, and it
takes a little while of the devices around it to swap to a redundant
path. The move from STP to RSTP has been made, which speeds this all
up, but you do get some disruption.

It can take a while for the display unit to boot into user space and
reconfigure the switch. Its only when that is complete can the switch
rejoin the network.

Rather than hit the switch with a reset, it would be better to somehow
suck the current configuration out of the switch and prime the Linux
network stack with that configuration. But that is a totally alien
concept to Linux.

	Andrew

Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

[Luiz Angelo Daros de Luca]

> > > >  From other discussions I've had, there seems to be interest in quite the
> > > > opposite thing, in fact. Reboot the SoC running Linux, but do not
> > > > disturb traffic flowing through the switch, and somehow pick up the
> > > > state from where the previous kernel left it.
> > >
> > > Yes this is actually an use case that is very dear to the users of DSA in an airplane. The entertainment system in the seat in front of you typically has a left, CPU/display and right set of switch ports. Across the 300+ units in the plane each entertainment systems runs STP to avoid loops being created when one of the display units goes bad. Occasionally cabin crew members will have to swap those units out since they tend to wear out. When they do, the switch operates in a headless mode and it would be unfortunate that plugging in a display unit into the network again would be disrupting existing traffic. I have seen out of tree patches doing that, but there was not a good way to make them upstream quality.
> >
> > This piqued my interest. I'm trying to understand how exactly plugging in a
> > display unit into the network would disrupt the traffic flow. Is this about
> > all network interfaces attached to the bridge interface being blocked when
> > a new link is established to relearn the changed topology?
>
> The hardware is split into two parts, a cradle and the display
> unit. The switch itself is in the cradle embedded in the seat
> back. The display unit contains the CPU, GPU, storage etc. There is a
> single Ethernet interface between the display unit and the cradle,
> along with MDIO, power, audio cables for the headphone jack etc.
>
> When you take out the display unit, you disconnect the switches
> management plain. The CPU has gone, and its the CPU running STP,
> sending and receiving BPDUs, etc. But the switch is still powered, and
> switching packets, keeping the network going, at least for a while.
>
> When you plug in a display unit, it boots. As typical for any computer
> booting, it assumes the hardware is in an unknown state, and hits the
> switch with a reset. That then kills the local networking, and it
> takes a little while of the devices around it to swap to a redundant
> path. The move from STP to RSTP has been made, which speeds this all
> up, but you do get some disruption.
>
> It can take a while for the display unit to boot into user space and
> reconfigure the switch. Its only when that is complete can the switch
> rejoin the network.
>
> Rather than hit the switch with a reset, it would be better to somehow
> suck the current configuration out of the switch and prime the Linux
> network stack with that configuration. But that is a totally alien
> concept to Linux.

This is quite a particular case. You'll need to update userland config
from the kernel state and the kernel state from the HW state. It's
upside down from what we normally see.
Anyway, we are far from that point in realtek DSA drivers. The DSA
driver actually resets the switch twice (HW and then SW) during setup.
Even vendor driver/lib states that without the initialization steps,
the switch behavior is undefined. And those steps would probably mess
with any existing switch state. Parsing the reg values into kernel
state is quite a complex task if you need to be usable for many
scenarios. And we still have opaque jam tables in the driver that I
would love to get rid of.

Now, back to the point I raised:

1) should we continue to HW reset the switch when the driver stops
controlling it?
2) If that is a yes, should we do that both for shutdown and poweroff?
3) Should we take additional precautions to lock the switch before
resetting it (as HW reset might be missing or misconfigured)?

I'll probably send the v5 without touching this topic but it is an
interesting point to think about, at least to assert the reset during
shutdown.

Regards,

Luiz