Message-ID: <63d6cf16-9581-4736-8592-bc5836fa51af@alliedtelesis.co.nz>
Date: Tue, 21 Jan 2025 09:32:52 +1300
From: Chris Packham <chris.packham@...iedtelesis.co.nz>
To: Sander Vanheule <sander@...nheule.net>, 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 Sander,

On 20/01/2025 23:28, Sander Vanheule wrote:
> 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?
Oops, will fix
>
>> + */
>> +
>> +#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.
Ack.
>> +#define   PHY_CTRL_TYPE			BIT(1)
> Similar here:
> #define	PHY_CTRL_TYPE_C22	0
> #define PHY_CTRL_TYPE_C45	BIT(1)

Ack

>> +#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

Ack

>
>> +	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.
Yes that makes sense.
>
>> +
>> +	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.
Ack.
>
>> +	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.
Not sure it buys us much in this case since it's just the lower 16 bits 
but for symmetry and a little extra type checking may as well.
>
>> +}
>> +
> [...]
>
>> +
>> +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.

I believe the POLL_SEL configuration actually affects an internal port 
polling unit. From the datasheets I have it seems pretty configurable, 
you can tell it which phy registers to poll and what values indicate 
link up/down (the defaults are conveniently setup to match the Realtek 
PHYs). So I don't think they are arbitrary and I don't think it would be 
a good idea to change them on the fly. I did consider at one point 
finding an unused port and re-mapping that to the desired bus/addr on 
the fly but I'm not sure what that'd do to the PPU and there's no 
guarantee that there will be a unused port.

>> +	}
>> +
>> +	/* 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)?
I guess it could be. Doing it this way avoids undoing things that may 
have been set by an earlier boot stage but even as I type that I don't 
find it a good argument against 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
I'll consider that. Certainly `u32 smi_bus` and `u32 smi_addr` can be 
renamed to match their usage from the dts. Not so sure about 
priv->smi_bus/priv->smi_addr as I am trying to match the usage in the 
datasheet. I guess technically they should be smi_set and port_addr but 
I find "port" here particularly confusing.
>> +	}
> [...]
>
>> +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().
The check in probe_one() checks that the mdio bus number is valid 
whereas this checks that there are at most 4. So not totally redundant 
but could probably be removed without doing any harm.
>
> Best,
> Sander

Powered by blists - more mailing lists