Re: [PATCH net-next v4 5/5] net: mdio: realtek-rtl9300: support non-default clock frequencies

[netdev-bot+sashiko@kernel.org]

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.

> 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

[ ... ]

> @@ -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);
> +}
> +

[ ... ]

> @@ -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