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

[netdev-bot+sashiko@kernel.org]

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

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

- [High] RTL839x: a device tree that is fully valid against
  realtek,rtl9301-mdio.yaml can make the entire MDIO controller fail to…

--

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

This adds a per-family set_bus_frequency() callback so the MDC clock of
the Realtek Otto MDIO controllers can be selected from the device tree
via clock-frequency, defaulting to 2.5 MHz. RTL839x only has a single
global frequency field, so the driver caches the first programmed value
and rejects a second bus asking for something else.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index 429321e9114fd..8edb0ba0bcdd1 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -749,6 +768,40 @@ 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 val;
> +	int err;
> +
> +	if (freq != 1250000 && freq != 2500000 && freq != 5000000)
> +		return -EINVAL;
> +	if (priv->global_freq)
> +		return freq == priv->global_freq ? 0 : -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);
> +	if (!err)
> +		priv->global_freq = freq;
> +
> +	return err;
> +}

[Severity: High]
Can a device tree that is valid against realtek,rtl9301-mdio.yaml make the
whole RTL839x controller fail to probe here?

The binding describes clock-frequency as an independent per-bus property of
each mdio-bus@N child, with an implicit default:

Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
      clock-frequency:
        default: 2500000

and the realtek,rtl8391-mdio / rtl8392-mdio / rtl8393-mdio conditional only
restricts the allowed values per child bus:

            clock-frequency:
              enum: [1250000, 2500000, 5000000]

There is nothing in the schema requiring the two RTL839x child buses to name
the same frequency, so the natural way to express "set the shared field to
5 MHz" is:

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

	mdio-bus@1 {
		reg = <1>;
		...
	};

Because otto_emdio_probe_one() substitutes the default before calling the
callback:

	if (of_property_read_u32(node, "clock-frequency", &freq))
		freq = 2500000;

an absent property is indistinguishable from an explicit 2.5 MHz request.
Doesn't that mean the second bus reaches
otto_emdio_8390_set_bus_frequency() with freq = 2500000 while
priv->global_freq is already 5000000, and

	if (priv->global_freq)
		return freq == priv->global_freq ? 0 : -EINVAL;

returns -EINVAL for a device tree that never asked for two different
frequencies?  The reverse node ordering fails the same way.

otto_emdio_probe() aborts on the first failing child:

	for_each_child_of_node_scoped(dev->of_node, child) {
		err = otto_emdio_probe_one(dev, priv, child);
		if (err)
			return err;
	}

so no MDIO bus is registered at all and none of the PHYs behind the switch
are reachable.  The message "cannot set frequency 2500000 for MDIO bus 1"
also does not point at anything the device tree actually contains.

Would it make sense to either express the RTL839x restriction in the binding
(a controller level property or a dedicated conditional/schema), or to treat
an absent per-bus clock-frequency as "no constraint" instead of an explicit
2.5 MHz request, so that a schema legal device tree keeps probing?

> +
>  static int otto_emdio_9300_set_bus_frequency(struct mii_bus *bus, u32 freq)

[ ... ]

> @@ -865,6 +933,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");

[ ... ]

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