Message-ID: <d4194a1560ff297e5ab3e6eae6d51b7c9d469381.camel@svanheule.net>
Date: Mon, 20 Jan 2025 11:28:44 +0100
From: Sander Vanheule <sander@...nheule.net>
To: Chris Packham <chris.packham@...iedtelesis.co.nz>, lee@...nel.org, 
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 andrew+netdev@...n.ch, 	davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, 	tsbogend@...ha.franken.de,
 hkallweit1@...il.com, linux@...linux.org.uk, 	markus.stockhausen@....de
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH v4 4/4] net: mdio: Add RTL9300 MDIO driver

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@...iedtelesis.co.nz>
> ---

[...]


> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> new file mode 100644
> index 000000000000..a9b894eff407
> --- /dev/null
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MDIO controller for RTL9300 switches with integrated SoC.
> + *
> + * The MDIO communication is abstracted by the switch. At the software level
> + * communication uses the switch port to address the PHY with the actual MDIO
> + * bus and address having been setup via the realtek,smi-address property.

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

Powered by blists - more mailing lists