Re: [net-next,v15,11/13] net: mdio: realtek-rtl9300: Add support for RTL838x

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] On the newly added RTL838x variant, a device tree that
  declares a Clause 45 PHY (compatible = "ethernet-phy-ieee802.3-c45")…
- [Low] DISCARDED (fixed later in the same series): Kconfig prompt/help
  text and MODULE_DESCRIPTION still advertise only the RTL9300 family…
- [Low] The driver now matches "realtek,rtl8380-mdio", but no in-tree
  binding documents a parent node that satisfies what the driver…
- [Low] The RTL838x variant introduced here declares RTL8380_NUM_BUSES =
  1, but Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml…

--

net: mdio: realtek-rtl9300: Add support for RTL838x

This patch adds the RTL838x (maple) variant to the Realtek MDIO driver by
duplicating the RTL9300-style register defines with an RTL8380 prefix,
adding C22/C45 accessors, a setup_controller() callback that sets the PHY
patch-done bit, and the new "realtek,rtl8380-mdio" match entry.

A few questions below on the new variant description and the device tree
side.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index b90972c1dc6a2..57ab89afb9e5e 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
> @@ -126,6 +126,28 @@
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  
> +#define RTL8380_NUM_BUSES			1
      ^^^^

[Severity: Low]
Should the binding also express the one-bus limit for this compatible?
Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml applies the
same subnode pattern to every compatible and has no per-compatible if/then
block:

  patternProperties:
    '^mdio-bus@[0-3]$':
      $ref: mdio.yaml#

So a device tree for realtek,rtl8380-mdio with mdio-bus@1 through
mdio-bus@3 validates cleanly, but the driver then refuses it in
otto_emdio_probe_one():

  if (mdio_bus >= priv->info->num_buses)
          return dev_err_probe(dev, -EINVAL,
                               "illegal (dangling) smi bus number %d\n", mdio_bus);

and otto_emdio_map_ports() likewise returns -EINVAL for
"illegal smi bus number". Since otto_emdio_probe() propagates that error,
the whole controller fails to probe and even the valid bus 0 is never
registered. The later 2-bus RTL839x compatible in this series has the same
gap.

> +#define RTL8380_NUM_PAGES			4096
> +#define RTL8380_NUM_PORTS			28
> +#define RTL8380_SMI_GLB_CTRL			0xa100
> +#define   RTL8380_SMI_PHY_PATCH_DONE		BIT(15)
> +#define RTL8380_SMI_ACCESS_PHY_CTRL_0		0xa1b8
> +#define RTL8380_SMI_ACCESS_PHY_CTRL_1		0xa1bc
> +#define   RTL8380_PHY_CTRL_REG_ADDR		GENMASK(24, 20)
> +#define   RTL8380_PHY_CTRL_PARK_PAGE		GENMASK(19, 15)
> +#define   RTL8380_PHY_CTRL_MAIN_PAGE		GENMASK(14, 3)
> +#define   RTL8380_PHY_CTRL_WRITE		BIT(2)
> +#define   RTL8380_PHY_CTRL_READ			0
> +#define   RTL8380_PHY_CTRL_TYPE_C45		BIT(1)
> +#define   RTL8380_PHY_CTRL_TYPE_C22		0
> +#define   RTL8380_PHY_CTRL_FAIL			0 /* no fail indicator */
> +#define RTL8380_SMI_ACCESS_PHY_CTRL_2		0xa1c0
> +#define   RTL8380_PHY_CTRL_INDATA		GENMASK(31, 16)
> +#define   RTL8380_PHY_CTRL_DATA			GENMASK(15, 0)
> +#define RTL8380_SMI_ACCESS_PHY_CTRL_3		0xa1c4
> +#define RTL8380_SMI_POLL_CTRL			0xa17c
> +#define RTL8380_SMI_PORT0_5_ADDR_CTRL		0xa1c8
> +
>  #define RTL9300_NUM_BUSES			4
>  #define RTL9300_NUM_PAGES			4096
>  #define RTL9300_NUM_PORTS			28

[ ... ]

> @@ -586,6 +662,15 @@ static int otto_emdio_setup_topology(struct otto_emdio_priv *priv)
>  	return 0;
>  }
>  
> +static int otto_emdio_8380_setup_controller(struct otto_emdio_priv *priv)
> +{
> +	/*
> +	 * PHY_PATCH_DONE enables PHY control via SoC. This is required for PHY access, including
> +	 * patching and must be set before the PHYs are probed.
> +	 */
> +	return regmap_set_bits(priv->regmap, RTL8380_SMI_GLB_CTRL, RTL8380_SMI_PHY_PATCH_DONE);
> +}

[Severity: Medium]
Can priv->smi_bus_is_c45[] end up set on RTL838x with nothing in the
hardware matching it?

otto_emdio_map_ports() sets the flag for any variant:

  if (of_device_is_compatible(phy_dn, "ethernet-phy-ieee802.3-c45"))
          priv->smi_bus_is_c45[bus] = true;

For RTL9300/RTL9310 that flag is consumed by their setup_controller()
callbacks, which program RTL9300_GLB_CTRL_INTF_SEL and
RTL9310_SMI_GLB_FMT_SEL_C45. The new RTL838x callback above never reads it,
which matches the commit message: "The setup_controller() function needs no
C45 configuration as polling only works in C22 mode".

The flag still has a software consumer in otto_emdio_notify_phy_attach():

  if (!priv->smi_bus_is_c45[chan->mdio_bus] &&
      !phy_id_compare_vendor(phydev->phy_id, RTL_VENDOR_ID)) {
          phydev_err(phydev, "Only Realtek PHYs allowed on C22 bus\n");
          return -EOPNOTSUPP;
  }

  return otto_emdio_set_port_polling(priv, port, true);

With RTL8380_NUM_BUSES == 1, a single PHY node declaring
compatible = "ethernet-phy-ieee802.3-c45" disables that check for every PHY
on the device, and polling is then armed anyway for the port. The PHY keeps
being accessed with the Realtek paged C22 encoding (priv->page[port] into
RTL8380_PHY_CTRL_MAIN_PAGE) by a polling engine that only does C22.

Because RTL8380_PHY_CTRL_FAIL is 0, otto_emdio_run_cmd() can never report a
hardware failure here:

  return cmdstate & info->cmd_fail ? -ENXIO : 0;

Would it make sense for the RTL838x path to reject (or at least warn about)
a bus flagged as C45, rather than letting the driver bookkeeping and the
hardware mode diverge silently?

[ ... ]

> @@ -923,6 +1030,7 @@ static const struct otto_emdio_info otto_emdio_9310_info = {
>  };
>  
>  static const struct of_device_id otto_emdio_ids[] = {
> +	{ .compatible = "realtek,rtl8380-mdio", .data = &otto_emdio_8380_info },

[Severity: Low]
Is there a binding for the parent node this compatible needs?

otto_emdio_probe() takes its regmap from the parent:

  priv->regmap = syscon_node_to_regmap(dev->parent->of_node);

and otto_emdio_map_ports() requires an "ethernet-ports" child of that same
parent, otherwise probe fails with -EINVAL "missing ethernet-ports":

  ports_dn = of_get_child_by_name(parent->of_node, "ethernet-ports");

The only schema describing such a container (syscon + simple-mfd with
ethernet-ports and an mdio-controller child) is
Documentation/devicetree/bindings/net/realtek,rtl9301-switch.yaml, and its
compatible enum lists only realtek,rtl9301/9302b/9302c/9303-switch, also at
the end of this series. A grep of Documentation/devicetree/bindings finds no
rtl838x/rtl8380 switch compatible.

That leaves a DT author either inventing an undocumented parent compatible
(caught by dtbs_check) or claiming realtek,rtl9301-switch on RTL838x
hardware, whose register window this patch treats as different (0xa1xx
versus 0xcaxx). Should a parent binding for the RTL838x switch/syscon node
be added alongside this?

[Severity: Low]
This isn't a bug, but with RTL838x now matched, the Kconfig prompt and help
text in drivers/net/mdio/Kconfig still say:

  tristate "Realtek RTL9300 MDIO interface support"
  ...
    This driver supports the MDIO interface found in the Realtek
    RTL9300 family of Ethernet switches with integrated SoC.

and MODULE_DESCRIPTION() in this file still says "RTL9300 MDIO driver". The
later patch in this series, "net: mdio: realtek-rtl9300: reword Kconfig and
module description", changes both to RTL83xx/RTL93xx, so this is only a
transient state within the series. Would folding the wording change in
earlier keep each commit self-consistent?

>  	{ .compatible = "realtek,rtl9301-mdio", .data = &otto_emdio_9300_info },
>  	{ .compatible = "realtek,rtl9311-mdio", .data = &otto_emdio_9310_info },
>  	{}

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