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

[netdev-bot+sashiko@kernel.org]

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