Re: [PATCH net-next v4 5/5] net: mdio: realtek-rtl9300: support non-default clock frequencies
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-16 13:35:18
Also in:
linux-devicetree
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 1 - [Low] otto_emdio_8390_set_bus_frequency() (drivers/net/mdio/mdio- realtek-rtl9300.c:769-806) can reject a frequency taken from a… -- net: mdio: realtek-rtl9300: support non-default clock frequencies This adds support for selecting the MDC clock frequency of the Realtek Otto switch MDIO controllers via the clock-frequency device tree property, defaulting to 2.5 MHz. RTL839x is handled specially because both of its MDIO buses share one frequency selector field.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c index 45cbefc52100a..bde0cf5e0b798 100644 --- a/drivers/net/mdio/mdio-realtek-rtl9300.c +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -748,6 +766,60 @@ 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); + u32 mdio_bus, child_freq, val; + bool freq_set = false; + int err; + + /* + * RTL839x uses one frequency shared by both MDIO buses. Use the frequency passed by the + * caller as fallback, but prefer an explicitly requested frequency from any valid, + * available bus. Reject conflicting explicit requests. Only the first call programs the + * shared frequency. + */ + if (priv->bus[0] || priv->bus[1]) + return 0; + + for_each_available_child_of_node_scoped(bus->parent->of_node, child) { + err = of_property_read_u32(child, "reg", &mdio_bus); + if (err || mdio_bus >= priv->info->num_buses) + continue; + + 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;
[Severity: Low] Can the diagnostic printed by the caller name a frequency and a bus that were never the ones rejected here? In otto_emdio_8390_set_bus_frequency() the freq argument is overwritten by any sibling child node that carries clock-frequency: freq = child_freq; freq_set = true; and the enum check afterwards then validates that inherited value, but the bare -EINVAL carries none of that context back to otto_emdio_probe_one(). For a RTL839x mdio-controller node where mdio-bus@0 has no clock-frequency and mdio-bus@1 has clock-frequency = <7000000>, probing bus 0 defaults freq to 2500000, the loop replaces it with 7000000, the enum check fails, and the message reads: cannot set frequency 2500000 for MDIO bus 0 2500000 on bus 0 is a legal combination, so would it be clearer to report the offending child node and its frequency from inside otto_emdio_8390_set_bus_frequency() instead? The same applies to the conflict path: with mdio-bus@0 = 1250000 and mdio-bus@1 = 5000000, the message names bus 0 and 1250000 even though the rejected condition is the conflict with mdio-bus@1.
+ + val = FIELD_PREP(RTL8390_SMI_FREQ_SEL, freq / 2500000); + + return regmap_update_bits(priv->regmap, RTL8390_SMI_GLB_CTRL, RTL8390_SMI_FREQ_SEL, val); +} +
[ ... ]
quoted hunk ↗ jump to hunk
@@ -867,6 +954,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); +
[Severity: Low] This is the message referred to above: it always uses the local node's freq and mdio_bus, which for the RTL839x callback need not be the value or the bus that was actually rejected.
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/20260912130451.3757276-1-markus.stockhausen%40gmx.de