[PATCH net-next v3 0/4] net: mdio: realtek-rtl9300: add bus frequency handling

[PATCH net-next v3 0/4] net: mdio: realtek-rtl9300: add bus frequency handling

[Markus Stockhausen]

The Realtek Otto switch platform consists of four different series

- RTL838x aka maple   : 28 port 1G Switches
- RTL839x aka cypress : 52 port 1G Switches
- RTL930x aka longan  : 28 port 1G/2.5G/10G Switches
- RTL931x aka mango   : 56 port 1G/2.5G/10G Switches

All of them support changing the bus frequency of their one to four
Ethernet MDIO buses, although the available settings differ between
the series. Extend the driver to support this feature.

Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
v2 -> v3:
  - Add Acked-by for devicetree patch. (Conor)
  - Andrews Reviewed-by not added because of Sashiko findings. (Markus)
  - Add patch "Reject duplicate MDIO bus" to harden the bus detection
    and as preparation for the RTL839x frequency setup. (Markus)
  - Rework RTL839x frequency setup to better handle missing/default
    frequencies. (Sashiko)
  - Use for_each_available_child_of_node_scoped(). (Sashiko)
v2: https://lore.kernel.org/netdev/20260906081946.3499898-1-markus.stockhausen@gmx.de/
v2 Sashiko 1: https://sashiko.dev/#/patchset/20260906081946.3499898-1-markus.stockhausen%40gmx.de
v2 Sashiko 2: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906081946.3499898-1-markus.stockhausen%40gmx.de


v1 -> v2:
  - Describe per-device frequencies in devicetree. (Andrew)
  - Add proper per-device frequency checks. (Andrew)
  - Use the initialized parent device for dev_warn(). (Sashiko)
  - Make clear that otto_emdio_probe_one() currently has an
    indentation issue that will be fixed by this series. (Andrew)
  - Throw error if RTL839x devicetree has different speeds set
    for first and second bus.

v1: https://lore.kernel.org/netdev/20260903122119.2638354-1-markus.stockhausen@gmx.de/
v1 Sashiko: https://sashiko.dev/#/patchset/20260903122119.2638354-1-markus.stockhausen%40gmx.de



Markus Stockhausen (4):
  dt-bindings: net: realtek,rtl9301-mdio: Add clock-frequency
  net: mdio: realtek-rtl9300: Convert "fwnode" left-overs to "of"
  net: mdio: realtek-rtl9300: Reject duplicate MDIO bus IDs
  net: mdio: realtek-rtl9300: Support non-default clock frequency

 .../bindings/net/realtek,rtl9301-mdio.yaml    |  40 +++++++
 drivers/net/mdio/mdio-realtek-rtl9300.c       | 110 +++++++++++++++++-
 2 files changed, 145 insertions(+), 5 deletions(-)

-- 
2.55.0

[PATCH net-next v3 1/4] dt-bindings: net: realtek,rtl9301-mdio: Add clock-frequency

[Markus Stockhausen]

The Realtek Otto Switch ethernet MDIO controller supports different MDC
speeds ranging from 1.25MHz to 10MHz. Document the clock-frequency
attribute for the device tree.

There is neither an official documentation nor any obvious registers that
indicate a dependency to one of the hardware clocks. The usable setup bits
are more like hardcoded values than dividers. Without further details do
not add a clock reference.

Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/net/realtek,rtl9301-mdio.yaml    | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
index 67e0b23a8470..88502d017eed 100644
--- a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
@@ -56,6 +56,9 @@ patternProperties:
       reg:
         maxItems: 1
 
+      clock-frequency:
+        default: 2500000
+
     required:
       - reg
 
@@ -71,6 +74,43 @@ required:
   - compatible
   - reg
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - realtek,rtl8380-mdio
+              - realtek,rtl8381-mdio
+              - realtek,rtl8382-mdio
+    then:
+      patternProperties:
+        '^mdio-bus@[0-3]$':
+          properties:
+            clock-frequency:
+              enum: [2500000, 10000000]
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - realtek,rtl8391-mdio
+              - realtek,rtl8392-mdio
+              - realtek,rtl8393-mdio
+              - realtek,rtl9301-mdio
+              - realtek,rtl9302b-mdio
+              - realtek,rtl9302c-mdio
+              - realtek,rtl9303-mdio
+              - realtek,rtl9311-mdio
+              - realtek,rtl9312-mdio
+              - realtek,rtl9313-mdio
+    then:
+      patternProperties:
+        '^mdio-bus@[0-3]$':
+          properties:
+            clock-frequency:
+              enum: [1250000, 2500000, 5000000]
+
 unevaluatedProperties: false
 
 examples:
-- 
2.55.0

[PATCH net-next v3 2/4] net: mdio: realtek-rtl9300: Convert "fwnode" left-overs to "of"

[Markus Stockhausen]

The latest developments of the MDIO driver consistently used the
"of" API. Convert the remaining "fwnode" left-overs too. With this
align the continued parameter of otto_emdio_probe_one() with the
opening parenthesis to follow the kernel coding style.

Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
 drivers/net/mdio/mdio-realtek-rtl9300.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
index 11d078ef941c..de0204d8be48 100644
--- a/drivers/net/mdio/mdio-realtek-rtl9300.c
+++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
@@ -831,14 +831,14 @@ static void otto_emdio_notify_phy_detach(struct phy_device *phydev)
 }
 
 static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv,
-				 struct fwnode_handle *node)
+				struct device_node *node)
 {
 	struct otto_emdio_chan *chan;
 	struct mii_bus *bus;
 	u32 mdio_bus;
 	int err;
 
-	err = fwnode_property_read_u32(node, "reg", &mdio_bus);
+	err = of_property_read_u32(node, "reg", &mdio_bus);
 	if (err)
 		return dev_err_probe(dev, err, "undefined smi bus number\n");
 
@@ -865,7 +865,7 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
 
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d", dev_name(dev), mdio_bus);
 
-	err = devm_of_mdiobus_register(dev, bus, to_of_node(node));
+	err = devm_of_mdiobus_register(dev, bus, node);
 	if (err)
 		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
 
@@ -1027,7 +1027,7 @@ static int otto_emdio_probe(struct platform_device *pdev)
 			return dev_err_probe(dev, err, "failed to setup MDIO bus controller\n");
 	}
 
-	device_for_each_child_node_scoped(dev, child) {
+	for_each_available_child_of_node_scoped(dev->of_node, child) {
 		err = otto_emdio_probe_one(dev, priv, child);
 		if (err)
 			return err;
-- 
2.55.0

[PATCH net-next v3 3/4] net: mdio: realtek-rtl9300: Reject duplicate MDIO bus IDs

[Markus Stockhausen]

The reg property identifies the hardware MDIO bus and is used as an
index into the per-bus state. However, duplicate bus IDs are currently
not rejected explicitly and only fail later during MDIO bus
registration due to the duplicate bus ID.

Keep track of successfully registered buses and reject a duplicate reg
value before allocating another bus. For this use the already existing
but currently unused priv->bus[] property.

Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
 drivers/net/mdio/mdio-realtek-rtl9300.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
index de0204d8be48..14fc98949fa3 100644
--- a/drivers/net/mdio/mdio-realtek-rtl9300.c
+++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
@@ -846,6 +846,9 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
 		return dev_err_probe(dev, -EINVAL,
 				     "illegal (dangling) smi bus number %d\n", mdio_bus);
 
+	if (priv->bus[mdio_bus])
+		return dev_err_probe(dev, -EINVAL, "duplicated smi bus number %d\n", mdio_bus);
+
 	bus = devm_mdiobus_alloc_size(dev, sizeof(*chan));
 	if (!bus)
 		return -ENOMEM;
@@ -869,6 +872,8 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
 	if (err)
 		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
 
+	priv->bus[mdio_bus] = bus;
+
 	return 0;
 }
 
-- 
2.55.0

[PATCH net-next v3 4/4] net: mdio: realtek-rtl9300: Support non-default clock frequency

[Markus Stockhausen]

The ethernet MDIO controller of the Realtek Otto switches has a default
MDC clock frequency of 2.5MHz. Via configuration bits one can adapt this.
Depending on the series the following can be set

- RTL838x: 2.5 Mhz, 10 MHz
- RTL839x, RTL930x, RTL931x: 1.25 MHz, 2.5 MHz, 5 Mhz

Enhance the driver so it allows to set the clock frequency via device
tree. This helps to

- simplify protocol analysis with a logic analyzer (lower speeds)
- reduce MDIO access times during normal operation (higher speeds)

Note: While the devices normally support a per-bus frequency selection,
the RTL839x is an exception from that. It can only select the speed for
both buses globally. To avoid device tree special treatment for this,
use the per-bus speeds for this series too. Return an error if the
RTL839x Devicetree specifies different frequencies for the first and
second bus.

Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
 drivers/net/mdio/mdio-realtek-rtl9300.c | 97 ++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
index 14fc98949fa3..d40fdd0ed2f0 100644
--- a/drivers/net/mdio/mdio-realtek-rtl9300.c
+++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
@@ -130,6 +130,7 @@
 #define RTL8380_NUM_PAGES			4096
 #define RTL8380_NUM_PORTS			28
 #define RTL8380_SMI_GLB_CTRL			0xa100
+#define   RTL8380_SMI_FREQ_SEL			BIT(1)
 #define   RTL8380_SMI_PHY_PATCH_DONE		BIT(15)
 #define RTL8380_SMI_ACCESS_PHY_CTRL_0		0xa1b8
 #define RTL8380_SMI_ACCESS_PHY_CTRL_1		0xa1bc
@@ -169,12 +170,15 @@
 #define RTL8390_PHYREG_PORT_CTRL_LOW		0x03e4
 #define RTL8390_PHYREG_PORT_CTRL_HIGH		0x03e8
 #define RTL8390_SMI_PORT_POLLING_CTRL		0x03fc
+#define RTL8390_SMI_GLB_CTRL			0x03f8
+#define   RTL8390_SMI_FREQ_SEL			GENMASK(6, 5)
 
 #define RTL9300_NUM_BUSES			4
 #define RTL9300_NUM_PAGES			4096
 #define RTL9300_NUM_PORTS			28
 #define RTL9300_SMI_GLB_CTRL			0xca00
 #define   RTL9300_GLB_CTRL_INTF_SEL(intf)	BIT(16 + (intf))
+#define   RTL9300_SMI_FREQ_SEL(intf)		GENMASK((intf) * 2 + 9, (intf) * 2 + 8)
 #define RTL9300_SMI_PORT0_15_POLLING_SEL	0xca08
 #define RTL9300_SMI_ACCESS_PHY_CTRL_0		0xcb70
 #define RTL9300_SMI_ACCESS_PHY_CTRL_1		0xcb74
@@ -196,6 +200,8 @@
 #define RTL9310_NUM_BUSES			4
 #define RTL9310_NUM_PAGES			8192
 #define RTL9310_NUM_PORTS			56
+#define RTL9310_SMI_GLB_CTRL0			0x0cc0
+#define   RTL9310_SMI_FREQ_SEL(intf)		GENMASK((intf) * 2 + 5, (intf) * 2 + 4)
 #define RTL9310_SMI_GLB_CTRL1			0x0cbc
 #define   RTL9310_SMI_GLB_FMT_SEL_C45(intf)	BIT((intf) * 2 + 1)
 #define RTL9310_SMI_INDRT_ACCESS_CTRL_0		0x0c00
@@ -269,6 +275,7 @@ struct otto_emdio_info {
 	u8 num_ports;
 	u16 num_pages;
 	u32 poll_ctrl;
+	int (*set_bus_frequency)(struct mii_bus *bus, u32 freq);
 	int (*setup_controller)(struct otto_emdio_priv *priv);
 	int (*read_c22)(struct mii_bus *bus, int port, int regnum, u32 *value);
 	int (*read_c45)(struct mii_bus *bus, int port, int dev_addr, int regnum, u32 *value);
@@ -740,6 +747,17 @@ static int otto_emdio_setup_topology(struct otto_emdio_priv *priv)
 	return 0;
 }
 
+static int otto_emdio_8380_set_bus_frequency(struct mii_bus *bus, u32 freq)
+{
+	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
+
+	if (freq != 2500000 && freq != 10000000)
+		return -EINVAL;
+
+	return regmap_assign_bits(priv->regmap, RTL8380_SMI_GLB_CTRL,
+				  RTL8380_SMI_FREQ_SEL, freq == 10000000);
+}
+
 static int otto_emdio_8380_setup_controller(struct otto_emdio_priv *priv)
 {
 	/*
@@ -749,6 +767,57 @@ static int otto_emdio_8380_setup_controller(struct otto_emdio_priv *priv)
 	return regmap_set_bits(priv->regmap, RTL8380_SMI_GLB_CTRL, RTL8380_SMI_PHY_PATCH_DONE);
 }
 
+static int otto_emdio_8390_set_bus_frequency(struct mii_bus *bus, u32 freq)
+{
+	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
+	bool freq_set = false;
+	u32 child_freq, val;
+	int err;
+
+	/*
+	 * RTL839x uses a single frequency for the two MDIO buses. During registration of first
+	 * bus pick an explicitly requested frequency from any enabled bus. Use the default only
+	 * if no frequency is set at all.
+	 */
+	if (priv->bus[0] || priv->bus[1])
+		return 0;
+
+	freq = 2500000;
+	for_each_available_child_of_node_scoped(bus->parent->of_node, child) {
+		if (of_property_read_u32(child, "clock-frequency", &child_freq))
+			continue;
+
+		if (freq_set && freq != child_freq)
+			return -EINVAL;
+
+		freq = child_freq;
+		freq_set = true;
+	}
+
+	if (freq != 1250000 && freq != 2500000 && freq != 5000000)
+		return -EINVAL;
+
+	val = FIELD_PREP(RTL8390_SMI_FREQ_SEL, freq / 2500000);
+	err = regmap_update_bits(priv->regmap, RTL8390_SMI_GLB_CTRL, RTL8390_SMI_FREQ_SEL, val);
+
+	return err;
+}
+
+static int otto_emdio_9300_set_bus_frequency(struct mii_bus *bus, u32 freq)
+{
+	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
+	struct otto_emdio_chan *chan = bus->priv;
+	u32 mask, val;
+
+	if (freq != 1250000 && freq != 2500000 && freq != 5000000)
+		return -EINVAL;
+
+	mask = RTL9300_SMI_FREQ_SEL(chan->mdio_bus);
+	val = field_prep(mask, freq / 2500000);
+
+	return regmap_update_bits(priv->regmap, RTL9300_SMI_GLB_CTRL, mask, val);
+}
+
 static int otto_emdio_9300_setup_controller(struct otto_emdio_priv *priv)
 {
 	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
@@ -769,6 +838,21 @@ static int otto_emdio_9300_setup_controller(struct otto_emdio_priv *priv)
 	return 0;
 }
 
+static int otto_emdio_9310_set_bus_frequency(struct mii_bus *bus, u32 freq)
+{
+	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
+	struct otto_emdio_chan *chan = bus->priv;
+	u32 mask, val;
+
+	if (freq != 1250000 && freq != 2500000 && freq != 5000000)
+		return -EINVAL;
+
+	mask = RTL9310_SMI_FREQ_SEL(chan->mdio_bus);
+	val = field_prep(mask, freq / 2500000);
+
+	return regmap_update_bits(priv->regmap, RTL9310_SMI_GLB_CTRL0, mask, val);
+}
+
 static int otto_emdio_9310_setup_controller(struct otto_emdio_priv *priv)
 {
 	int i, err;
@@ -835,7 +919,7 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
 {
 	struct otto_emdio_chan *chan;
 	struct mii_bus *bus;
-	u32 mdio_bus;
+	u32 mdio_bus, freq;
 	int err;
 
 	err = of_property_read_u32(node, "reg", &mdio_bus);
@@ -868,6 +952,13 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
 
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d", dev_name(dev), mdio_bus);
 
+	if (of_property_read_u32(node, "clock-frequency", &freq))
+		freq = 2500000;
+	err = priv->info->set_bus_frequency(bus, freq);
+	if (err)
+		return dev_err_probe(dev, err, "cannot set frequency %u for MDIO bus %d\n",
+				     freq, mdio_bus);
+
 	err = devm_of_mdiobus_register(dev, bus, node);
 	if (err)
 		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
@@ -1056,6 +1147,7 @@ static const struct otto_emdio_info otto_emdio_8380_info = {
 	.num_pages = RTL8380_NUM_PAGES,
 	.num_ports = RTL8380_NUM_PORTS,
 	.poll_ctrl = RTL8380_SMI_POLL_CTRL,
+	.set_bus_frequency = otto_emdio_8380_set_bus_frequency,
 	.setup_controller = otto_emdio_8380_setup_controller,
 	.read_c22 = otto_emdio_8380_read_c22,
 	.read_c45 = otto_emdio_8380_read_c45,
@@ -1080,6 +1172,7 @@ static const struct otto_emdio_info otto_emdio_8390_info = {
 	.num_pages = RTL8390_NUM_PAGES,
 	.num_ports = RTL8390_NUM_PORTS,
 	.poll_ctrl = RTL8390_SMI_PORT_POLLING_CTRL,
+	.set_bus_frequency = otto_emdio_8390_set_bus_frequency,
 	.read_c22 = otto_emdio_8390_read_c22,
 	.read_c45 = otto_emdio_8390_read_c45,
 	.write_c22 = otto_emdio_8390_write_c22,
@@ -1102,6 +1195,7 @@ static const struct otto_emdio_info otto_emdio_9300_info = {
 	.num_ports = RTL9300_NUM_PORTS,
 	.num_pages = RTL9300_NUM_PAGES,
 	.poll_ctrl = RTL9300_SMI_POLL_CTRL,
+	.set_bus_frequency = otto_emdio_9300_set_bus_frequency,
 	.setup_controller = otto_emdio_9300_setup_controller,
 	.read_c22 = otto_emdio_9300_read_c22,
 	.read_c45 = otto_emdio_9300_read_c45,
@@ -1128,6 +1222,7 @@ static const struct otto_emdio_info otto_emdio_9310_info = {
 	.num_pages = RTL9310_NUM_PAGES,
 	.num_ports = RTL9310_NUM_PORTS,
 	.poll_ctrl = RTL9310_SMI_PORT_POLLING_CTRL,
+	.set_bus_frequency = otto_emdio_9310_set_bus_frequency,
 	.setup_controller = otto_emdio_9310_setup_controller,
 	.read_c22 = otto_emdio_9310_read_c22,
 	.read_c45 = otto_emdio_9310_read_c45,
-- 
2.55.0

Re: [PATCH net-next v3 1/4] dt-bindings: net: realtek,rtl9301-mdio: Add clock-frequency

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] The binding documents `clock-frequency` (with `default: 2500000`
  and per-family enums) for the `mdio-bus@N` children of the Realtek…

--

dt-bindings: net: realtek,rtl9301-mdio: Add clock-frequency

This documents a clock-frequency property on the mdio-bus@N child nodes of
the Realtek Otto switch MDIO controller, with a default of 2500000 and
per-family allowed value sets expressed through new allOf if-then blocks.
No clock reference is added, since the setup bits are not documented as
dividers.

No Link: tags are present in the commit header.

> diff --git a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
> index 67e0b23a8470..88502d017eed 100644
> --- a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
> @@ -56,6 +56,9 @@ patternProperties:
>        reg:
>          maxItems: 1
>  
> +      clock-frequency:
> +        default: 2500000
> +
>      required:
>        - reg
>  

[ ... ]

No regressions found in this patch.

One item was checked and dismissed: at this commit there is no consumer of
clock-frequency, since otto_emdio_probe_one() in
drivers/net/mdio/mdio-realtek-rtl9300.c reads only reg. The driver side
arrives later in the same series with "net: mdio: realtek-rtl9300: Support
non-default clock frequency", which does:

	if (of_property_read_u32(node, "clock-frequency", &freq))
		freq = 2500000;
	err = priv->info->set_bus_frequency(bus, freq);

so the default of 2500000 and the per-family value sets documented here are
honoured by the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909150107.3602067-1-markus.stockhausen%40gmx.de

Re: [PATCH net-next v3 2/4] net: mdio: realtek-rtl9300: Convert "fwnode" left-overs to "of"

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Incomplete conversion vs. commit-message claim: the commit states
  it converts "the remaining 'fwnode' left-overs" of…

--

net: mdio: realtek-rtl9300: Convert "fwnode" left-overs to "of"

This converts otto_emdio_probe_one() to take a struct device_node instead
of a struct fwnode_handle, replaces fwnode_property_read_u32() with
of_property_read_u32(), drops the to_of_node() conversion for
devm_of_mdiobus_register(), and switches the child loop in
otto_emdio_probe() to for_each_available_child_of_node_scoped().

> The latest developments of the MDIO driver consistently used the
> "of" API. Convert the remaining "fwnode" left-overs too.

[Severity: Low]
Is the conversion of "the remaining fwnode left-overs" complete after this
patch?  otto_emdio_map_ports() still prints the parent node through the
fwnode API:

drivers/net/mdio/mdio-realtek-rtl9300.c:otto_emdio_map_ports() {
	ports_dn = of_get_child_by_name(parent->of_node, "ethernet-ports");
	if (!ports_dn)
		return dev_err_probe(dev, -EINVAL, "%pfwP missing ethernet-ports\n",
				     dev_fwnode(parent));
	...
}

The statement just above already uses parent->of_node, so would "%pOF" with
parent->of_node be the matching "of" form here?

A grep for fwnode in the file at this commit shows this dev_err_probe() call
as the only remaining user, and it is still unchanged at the end of the
series (5624848bce74bb4ff1882ee7fa047bde6f4e04aa), so should the commit
message be softened, or the print converted as well?

The message text itself is correct either way, and %pfwP on an OF-backed
fwnode still prints the node path, so this is only about the completeness
claim.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909150107.3602067-1-markus.stockhausen%40gmx.de

Re: [PATCH net-next v3 4/4] net: mdio: realtek-rtl9300: Support non-default clock frequency

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 1

- [Medium] RTL839x: the single global MDC divider is derived from per-bus
  DT properties in a way that both rejects schema-valid device trees and…
- [Medium] `otto_emdio_probe_one()` now programs the MDC frequency
  selector on every probe even when the device tree says nothing: `if…
- [Low] The new diagnostic in `otto_emdio_probe_one()` prints values that
  are not the ones that were rejected on RTL839x: `dev_err_probe(dev,…

--

net: mdio: realtek-rtl9300: Support non-default clock frequency

This adds a per-family set_bus_frequency callback to the Realtek Otto
eMDIO driver and wires it into otto_emdio_probe_one(), so the MDC clock
rate can be selected from the mdio-bus child node via clock-frequency.
RTL839x is handled specially because both of its buses share a single
divider.

A few questions about the new code below.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index 14fc98949fa3..d40fdd0ed2f0 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -740,6 +747,17 @@ static int otto_emdio_setup_topology(struct otto_emdio_priv *priv)
>  	return 0;
>  }
>  
> +static int otto_emdio_8380_set_bus_frequency(struct mii_bus *bus, u32 freq)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
> +
> +	if (freq != 2500000 && freq != 10000000)
> +		return -EINVAL;
> +
> +	return regmap_assign_bits(priv->regmap, RTL8380_SMI_GLB_CTRL,
> +				  RTL8380_SMI_FREQ_SEL, freq == 10000000);
> +}
> +
>  static int otto_emdio_8380_setup_controller(struct otto_emdio_priv *priv)
>  {
>  	/*
> @@ -749,6 +767,57 @@ static int otto_emdio_8380_setup_controller(struct otto_emdio_priv *priv)
>  	return regmap_set_bits(priv->regmap, RTL8380_SMI_GLB_CTRL, RTL8380_SMI_PHY_PATCH_DONE);
>  }
>  
> +static int otto_emdio_8390_set_bus_frequency(struct mii_bus *bus, u32 freq)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
> +	bool freq_set = false;
> +	u32 child_freq, val;
> +	int err;
> +
> +	/*
> +	 * RTL839x uses a single frequency for the two MDIO buses. During registration of first
> +	 * bus pick an explicitly requested frequency from any enabled bus. Use the default only
> +	 * if no frequency is set at all.
> +	 */
> +	if (priv->bus[0] || priv->bus[1])
> +		return 0;
> +
> +	freq = 2500000;
> +	for_each_available_child_of_node_scoped(bus->parent->of_node, child) {
> +		if (of_property_read_u32(child, "clock-frequency", &child_freq))
> +			continue;
> +
> +		if (freq_set && freq != child_freq)
> +			return -EINVAL;
> +
> +		freq = child_freq;
> +		freq_set = true;
> +	}

[Severity: Medium]
Two questions about the arbitration in otto_emdio_8390_set_bus_frequency().

freq_set is only set for children that actually carry clock-frequency, so a
child without the property never takes part in the comparison. With this
device tree:

	mdio-bus@0 { };
	mdio-bus@1 { clock-frequency = <5000000>; };

does bus 0 end up physically clocked at 5 MHz, although its node requests
nothing and the binding documents 2500000 as the per-bus default? There is
no error and no warning on that path.

The other direction is the -EINVAL return when two children carry different
explicit values, which fails the probe of the whole controller. The binding
patch in this series applies the enum [1250000, 2500000, 5000000] per
mdio-bus child for the rtl8391/8392/8393 compatibles in the same way as for
930x/931x, and does not document the RTL839x "one divider for both buses"
restriction anywhere.

Should the binding express that restriction, or should the driver treat a
missing clock-frequency on an available bus as the documented 2500000 when
arbitrating?

One more thing about the loop: it walks every available child of the parent
rather than only the buses that will be registered, so a dangling child such
as mdio-bus@2 on a two-bus RTL839x, which otto_emdio_probe_one() later
rejects with

	if (mdio_bus >= priv->info->num_buses)

can still select or conflict with the global frequency. Is that intended?

Note also that this is the only one of the four implementers that discards
its freq argument and returns 0 for every bus after the first, so the
callback signature int (*set_bus_frequency)(struct mii_bus *bus, u32 freq)
is not really honoured for RTL839x.

> +
> +	if (freq != 1250000 && freq != 2500000 && freq != 5000000)
> +		return -EINVAL;
> +
> +	val = FIELD_PREP(RTL8390_SMI_FREQ_SEL, freq / 2500000);
> +	err = regmap_update_bits(priv->regmap, RTL8390_SMI_GLB_CTRL, RTL8390_SMI_FREQ_SEL, val);
> +
> +	return err;
> +}
> +
> +static int otto_emdio_9300_set_bus_frequency(struct mii_bus *bus, u32 freq)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
> +	struct otto_emdio_chan *chan = bus->priv;
> +	u32 mask, val;
> +
> +	if (freq != 1250000 && freq != 2500000 && freq != 5000000)
> +		return -EINVAL;
> +
> +	mask = RTL9300_SMI_FREQ_SEL(chan->mdio_bus);
> +	val = field_prep(mask, freq / 2500000);
> +
> +	return regmap_update_bits(priv->regmap, RTL9300_SMI_GLB_CTRL, mask, val);
> +}
> +
>  static int otto_emdio_9300_setup_controller(struct otto_emdio_priv *priv)
>  {
>  	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> @@ -769,6 +838,21 @@ static int otto_emdio_9300_setup_controller(struct otto_emdio_priv *priv)
>  	return 0;
>  }
>  
> +static int otto_emdio_9310_set_bus_frequency(struct mii_bus *bus, u32 freq)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
> +	struct otto_emdio_chan *chan = bus->priv;
> +	u32 mask, val;
> +
> +	if (freq != 1250000 && freq != 2500000 && freq != 5000000)
> +		return -EINVAL;
> +
> +	mask = RTL9310_SMI_FREQ_SEL(chan->mdio_bus);
> +	val = field_prep(mask, freq / 2500000);
> +
> +	return regmap_update_bits(priv->regmap, RTL9310_SMI_GLB_CTRL0, mask, val);
> +}
> +

[ ... ]

> @@ -868,6 +952,13 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
>  
>  	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d", dev_name(dev), mdio_bus);
>  
> +	if (of_property_read_u32(node, "clock-frequency", &freq))
> +		freq = 2500000;
> +	err = priv->info->set_bus_frequency(bus, freq);

[Severity: Medium]
There is no "property absent, leave the hardware alone" path here. When
clock-frequency is missing, 2500000 is synthesized and set_bus_frequency()
is still called, so the selector field is written on every probe. Before
this patch nothing in the driver touched RTL8380_SMI_GLB_CTRL bit 1,
RTL8390_SMI_GLB_CTRL bits 6:5, or the new selector fields in
RTL9300_SMI_GLB_CTRL and RTL9310_SMI_GLB_CTRL0.

For RTL838x the default path ends up in:

	regmap_assign_bits(priv->regmap, RTL8380_SMI_GLB_CTRL,
			   RTL8380_SMI_FREQ_SEL, freq == 10000000);

which clears the bit. On a board where the bootloader selected 10 MHz, does
an unmodified device tree now silently drop the MDC rate back to 2.5 MHz?

The 839x/930x/931x implementers derive the register value arithmetically as
freq / 2500000, that is 0 for 1.25 MHz, 1 for 2.5 MHz and 2 for 5 MHz. Is
that mapping taken from the datasheet? The encodings are also asymmetric:
2.5 MHz means a cleared bit on RTL838x but field value 1 on the other three.
If the two-bit field is not ordered 0/1/2 that way, every existing device
tree without clock-frequency now gets an unintended MDC rate while the
driver believes it kept the default.

The commit message says only that the change "allows to set the clock
frequency via device tree". Could it also state that the frequency selector
is now reprogrammed on every probe, including for device trees that do not
set the property?

> +	if (err)
> +		return dev_err_probe(dev, err, "cannot set frequency %u for MDIO bus %d\n",
> +				     freq, mdio_bus);

[Severity: Low]
On RTL839x, can this message name values that were not the ones rejected?
otto_emdio_8390_set_bus_frequency() overwrites freq locally with 2500000 and
decides the failure from a sibling node's clock-frequency, while the caller
prints the freq and mdio_bus of the node currently being probed. For:

	mdio-bus@0 { };
	mdio-bus@1 { clock-frequency = <10000000>; };

the log would read "cannot set frequency 2500000 for MDIO bus 0", even
though 2500000 is a legal value and bus 0 is not the offending node. A
mismatch between the two buses is likewise always reported against whichever
child is probed first.

> +
>  	err = devm_of_mdiobus_register(dev, bus, node);
>  	if (err)
>  		return dev_err_probe(dev, err, "cannot register MDIO bus\n");

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909150107.3602067-1-markus.stockhausen%40gmx.de