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@xxxxxxxxxxxxxxxxxxx>
> ---
[...]
> 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
