Hi Chris,

On Mon, 2025-01-20 at 17:02 +1300, Chris Packham wrote:

Add a driver for the MDIO controller on the RTL9300 family of Ethernet
switches with integrated SoC. There are 4 physical SMI interfaces on the
RTL9300 however access is done using the switch ports. The driver takes
the MDIO bus hierarchy from the DTS and uses this to configure the
switch ports so they are associated with the correct PHY. This mapping
is also used when dealing with software requests from phylib.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

[...]

realtek,smi-address is a leftover from a previous spin?

+ */
+
+#include <linux/cleanup.h>
+#include <linux/mdio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#define SMI_GLB_CTRL			0xca00
+#define   GLB_CTRL_INTF_SEL(intf)	BIT(16 + (intf))
+#define SMI_PORT0_15_POLLING_SEL	0xca08
+#define SMI_ACCESS_PHY_CTRL_0		0xcb70
+#define SMI_ACCESS_PHY_CTRL_1		0xcb74
+#define   PHY_CTRL_RWOP			BIT(2)

With

#define PHY_CTRL_WRITE		BIT(2)
#define PHY_CTRL_READ		0

you could use both macros in the write/read functions. Now I have to go and parse the write/read
functions to see what it means when this bit is set.

+#define   PHY_CTRL_TYPE			BIT(1)

Similar here:
#define	PHY_CTRL_TYPE_C22	0
#define PHY_CTRL_TYPE_C45	BIT(1)

+#define   PHY_CTRL_CMD			BIT(0)
+#define   PHY_CTRL_FAIL			BIT(25)
+#define SMI_ACCESS_PHY_CTRL_2		0xcb78
+#define SMI_ACCESS_PHY_CTRL_3		0xcb7c
+#define SMI_PORT0_5_ADDR_CTRL		0xcb80
+
+#define MAX_PORTS       28
+#define MAX_SMI_BUSSES  4
+#define MAX_SMI_ADDR	0x1f
+
+struct rtl9300_mdio_priv;
+
+struct rtl9300_mdio_chan {
+	struct rtl9300_mdio_priv *priv;
+	u8 smi_bus;
+};
+
+struct rtl9300_mdio_priv {
+	struct regmap *regmap;
+	struct mutex lock; /* protect HW access */
+	u8 smi_bus[MAX_PORTS];
+	u8 smi_addr[MAX_PORTS];
+	bool smi_bus_isc45[MAX_SMI_BUSSES];

Nit: add an underscore: smi_bus_is_c45

+	struct mii_bus *bus[MAX_SMI_BUSSES];
+};
+
+static int rtl9300_mdio_phy_to_port(struct mii_bus *bus, int phy_id)
+{
+	struct rtl9300_mdio_chan *chan = bus->priv;
+	struct rtl9300_mdio_priv *priv = chan->priv;
+	int i;
+
+	for (i = 0; i < MAX_PORTS; i++)
+		if (priv->smi_bus[i] == chan->smi_bus &&
+		    priv->smi_addr[i] == phy_id)
+			return i;

This may break if some lower port numbers are not configured by the user, e.g. phy 0-7 on bus 0 are
mapped to ports 8-15 and ports 0-7 are unused.
When looking up the port number of phy 0 on bus 0, you would get a match on an unconfigured port
(port 0) since smi_bus/smi_addr are zero-initialized. This could be fixed by adding a bitmap
indicating which ports are actually configured.

+
+	return -ENOENT;
+}

[...]

+static int rtl9300_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
+{

[...]

+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_1,
+			   regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD);

You could use FIELD_PREP() to pack the bitfields.

+	if (err)
+		return err;
+
+	err = rtl9300_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, SMI_ACCESS_PHY_CTRL_2, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;

... and FIELD_GET() to unpack.

+}
+

[...]

+
+static int rtl9300_mdiobus_init(struct rtl9300_mdio_priv *priv)
+{
+	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
+	struct regmap *regmap = priv->regmap;
+	u32 port_addr[5] = { 0 };
+	u32 poll_sel[2] = { 0 };
+	int i, err;
+
+	/* Associate the port with the SMI interface and PHY */
+	for (i = 0; i < MAX_PORTS; i++) {
+		int pos;
+
+		if (priv->smi_bus[i] > 3)
+			continue;
+
+		pos = (i % 6) * 5;
+		port_addr[i / 6] |= priv->smi_addr[i] << pos;
+
+		pos = (i % 16) * 2;
+		poll_sel[i / 16] |= priv->smi_bus[i] << pos;

I've read the discussion on v1-v3 and had a quick look at the available documentation. Thinking out
loud here, so you can correct me if I'm making any false assumptions.

As I understand, the SoC has four physical MDIO/MDC pin pairs. Using the DT description, phy-s are
matched with to specific MDIO bus. PORT_ADDR tells the switch which phy address a port maps to.
POLL_SEL then tells the MDIO controller which bus this port (phy) is assigned to. I have the
impression this [port] <-> [bus, phy] mapping is completely arbitrary. If configured correctly, it
can probably serve as a convenience to match a front panel port number to a specific phy.

If the port numbers are in fact arbitrary, I think they could be hidden from the user, removing the
need for a custom DT property. You could probably populate the port numbers one by one as phy-s are
enumerated, as you are already storing the assigned port number in the MDIO controller's private
data.

One complication this might have, is that I suspect these port numbers are not used exclusively by
the MDIO controller, but also by the switch itself. So then there may have to be a way to resolve
(auto-assigned) port numbers outside of this driver too.

+	}
+
+	/* Put the interfaces into C45 mode if required */
+	for (i = 0; i < MAX_SMI_BUSSES; i++) {
+		if (priv->smi_bus_isc45[i]) {
+			glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);

Can't glb_ctrl_mask be fixed to GENMASK(19, 16)?

+			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
+		}
+	}

[...]

+static int rtl9300_mdiobus_probe_one(struct device *dev, struct rtl9300_mdio_priv *priv,
+				     struct fwnode_handle *node)
+{
+	struct rtl9300_mdio_chan *chan;
+	struct fwnode_handle *child;
+	struct mii_bus *bus;
+	u32 smi_bus;
+	int err;
+
+	err = fwnode_property_read_u32(node, "reg", &smi_bus);
+	if (err)
+		return err;
+
+	if (smi_bus >= MAX_SMI_BUSSES)
+		return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n", smi_bus);
+
+	fwnode_for_each_child_node(node, child) {
+		u32 smi_addr;
+		u32 pn;
+
+		err = fwnode_property_read_u32(child, "reg", &smi_addr);
+		if (err)
+			return err;

[...]

+
+		priv->smi_bus[pn] = smi_bus;
+		priv->smi_addr[pn] = smi_addr;

Nitpicking a bit here, but perhaps the code might be a tad bit easier to read for the non-Realtek
initiated by renaming:
  - smi_bus -> mdio_bus or just bus_id (matching the mii_bus *bus member)
  - smi_addr -> phy_addr

+	}

[...]

+static int rtl9300_mdiobus_probe(struct platform_device *pdev)
+{

[...]

+
+	if (device_get_child_node_count(dev) >= MAX_SMI_BUSSES)
+		return dev_err_probe(dev, -EINVAL, "Too many SMI busses\n");

This seems redundant with the MAX_SMI_BUSSES comparison in probe_one().

Best,
Sander