[v2,4/4] net: mdio: Add RTL9300 MDIO driver

Commit Message

Chris Packham Dec. 16, 2024, 3:13 a.m. UTC

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 but access is done using the switch ports so a single MDIO bus
is presented to the rest of the system.

Signed-off-by: Chris Packham <[e-mail]>
---

Notes:
    Changes in v2:
    - Add clause 22 support
    - Remove commented out code
    - Formatting cleanup
    - Set MAX_PORTS correctly for MDIO interface
    - Fix off-by-one error in pn check

 drivers/net/mdio/Kconfig            |   7 +
 drivers/net/mdio/Makefile           |   1 +
 drivers/net/mdio/mdio-realtek-rtl.c | 341 ++++++++++++++++++++++++++++
 3 files changed, 349 insertions(+)
 create mode 100644 drivers/net/mdio/mdio-realtek-rtl.c

Comments

Simon Horman Dec. 16, 2024, 4:48 p.m. UTC | #1

On Mon, Dec 16, 2024 at 04:13:46PM +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 but access is done using the switch ports so a single MDIO bus
> is presented to the rest of the system.
> 
> Signed-off-by: Chris Packham <[e-mail]>

...

> diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c

...

> +#define MAX_SMI_BUSSES  4
> +#define MAX_SMI_ADDR	0x1f
> +
> +struct realtek_mdio_priv {
> +	struct regmap *regmap;
> +	u8 smi_bus[MAX_PORTS];
> +	u8 smi_addr[MAX_PORTS];
> +	bool smi_bus_isc45[MAX_SMI_BUSSES];
> +	u32 reg_base;
> +};

...

> +static int realtek_mdiobus_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct realtek_mdio_priv *priv;
> +	struct fwnode_handle *child;
> +	struct mii_bus *bus;
> +	int err;
> +
> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "Reaktek Switch MDIO Bus";
> +	bus->read = realtek_mdio_read_c22;
> +	bus->write = realtek_mdio_write_c22;
> +	bus->read_c45 = realtek_mdio_read_c45;
> +	bus->write_c45 =  realtek_mdio_write_c45;
> +	bus->parent = dev;
> +	priv = bus->priv;
> +
> +	priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +
> +	err = device_property_read_u32(dev, "reg", &priv->reg_base);
> +	if (err)
> +		return err;
> +
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> +	device_for_each_child_node(dev, child) {
> +		u32 pn, smi_addr[2];
> +
> +		err = fwnode_property_read_u32(child, "reg", &pn);
> +		if (err)
> +			return err;
> +
> +		if (pn >= MAX_PORTS)
> +			return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
> +
> +		err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> +		if (err) {
> +			smi_addr[0] = 0;
> +			smi_addr[1] = pn;
> +		}
> +
> +		if (smi_addr[0] > MAX_SMI_BUSSES)

Hi Chris,

Should this condition be

		if (smi_addr[0] >= MAX_SMI_BUSSES)


> +			return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
> +					     smi_addr[0]);
> +
> +		if (smi_addr[1] > MAX_SMI_ADDR)
> +			return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
> +
> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> +			priv->smi_bus_isc45[smi_addr[0]] = true;

Otherwise it seems that smi_bus_isc45 may overflow here.

Flagged by Smatch.

> +
> +		priv->smi_bus[pn] = smi_addr[0];
> +		priv->smi_addr[pn] = smi_addr[1];
> +	}
> +
> +	err = realtek_mdiobus_init(priv);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
> +
> +	err = devm_of_mdiobus_register(dev, bus, dev->of_node);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> +
> +	return 0;
> +}

...

Chris Packham Dec. 16, 2024, 9:47 p.m. UTC | #2

On 17/12/2024 05:48, Simon Horman wrote:
> On Mon, Dec 16, 2024 at 04:13:46PM +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 but access is done using the switch ports so a single MDIO bus
>> is presented to the rest of the system.
>>
>> Signed-off-by: Chris Packham <[e-mail]>
> ...
>
>> diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c
> ...
>
>> +#define MAX_SMI_BUSSES  4
>> +#define MAX_SMI_ADDR	0x1f
>> +
>> +struct realtek_mdio_priv {
>> +	struct regmap *regmap;
>> +	u8 smi_bus[MAX_PORTS];
>> +	u8 smi_addr[MAX_PORTS];
>> +	bool smi_bus_isc45[MAX_SMI_BUSSES];
>> +	u32 reg_base;
>> +};
> ...
>
>> +static int realtek_mdiobus_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct realtek_mdio_priv *priv;
>> +	struct fwnode_handle *child;
>> +	struct mii_bus *bus;
>> +	int err;
>> +
>> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
>> +	if (!bus)
>> +		return -ENOMEM;
>> +
>> +	bus->name = "Reaktek Switch MDIO Bus";
>> +	bus->read = realtek_mdio_read_c22;
>> +	bus->write = realtek_mdio_write_c22;
>> +	bus->read_c45 = realtek_mdio_read_c45;
>> +	bus->write_c45 =  realtek_mdio_write_c45;
>> +	bus->parent = dev;
>> +	priv = bus->priv;
>> +
>> +	priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
>> +	if (IS_ERR(priv->regmap))
>> +		return PTR_ERR(priv->regmap);
>> +
>> +	err = device_property_read_u32(dev, "reg", &priv->reg_base);
>> +	if (err)
>> +		return err;
>> +
>> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
>> +
>> +	device_for_each_child_node(dev, child) {
>> +		u32 pn, smi_addr[2];
>> +
>> +		err = fwnode_property_read_u32(child, "reg", &pn);
>> +		if (err)
>> +			return err;
>> +
>> +		if (pn >= MAX_PORTS)
>> +			return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
>> +
>> +		err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
>> +		if (err) {
>> +			smi_addr[0] = 0;
>> +			smi_addr[1] = pn;
>> +		}
>> +
>> +		if (smi_addr[0] > MAX_SMI_BUSSES)
> Hi Chris,
>
> Should this condition be
>
> 		if (smi_addr[0] >= MAX_SMI_BUSSES)
Yes. You are correct.
>> +			return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
>> +					     smi_addr[0]);
>> +
>> +		if (smi_addr[1] > MAX_SMI_ADDR)
>> +			return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
>> +
>> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
>> +			priv->smi_bus_isc45[smi_addr[0]] = true;
> Otherwise it seems that smi_bus_isc45 may overflow here.
>
> Flagged by Smatch.

Sounds like something I should start looking at for myself. Have you got 
a link to share?

>> +
>> +		priv->smi_bus[pn] = smi_addr[0];
>> +		priv->smi_addr[pn] = smi_addr[1];
>> +	}
>> +
>> +	err = realtek_mdiobus_init(priv);
>> +	if (err)
>> +		return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
>> +
>> +	err = devm_of_mdiobus_register(dev, bus, dev->of_node);
>> +	if (err)
>> +		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
>> +
>> +	return 0;
>> +}
> ...
>

Simon Horman Dec. 17, 2024, 10:35 a.m. UTC | #3

+ Dan Carpenter

On Tue, Dec 17, 2024 at 10:47:10AM +1300, Chris Packham wrote:
> 
> On 17/12/2024 05:48, Simon Horman wrote:
> > On Mon, Dec 16, 2024 at 04:13:46PM +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 but access is done using the switch ports so a single MDIO bus
> > > is presented to the rest of the system.
> > > 
> > > Signed-off-by: Chris Packham <[e-mail]>

...

> > > +		if (smi_addr[0] > MAX_SMI_BUSSES)
> > Hi Chris,
> > 
> > Should this condition be
> > 
> > 		if (smi_addr[0] >= MAX_SMI_BUSSES)
> Yes. You are correct.
> > > +			return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
> > > +					     smi_addr[0]);
> > > +
> > > +		if (smi_addr[1] > MAX_SMI_ADDR)
> > > +			return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
> > > +
> > > +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> > > +			priv->smi_bus_isc45[smi_addr[0]] = true;
> > Otherwise it seems that smi_bus_isc45 may overflow here.
> > 
> > Flagged by Smatch.
> 
> Sounds like something I should start looking at for myself. Have you got a
> link to share?

Hi Chris,

Smatch is here: https://github.com/error27/smatch
And my usage of it is informed by
https://blogs.oracle.com/linux/post/smatch-static-analysis-tool-overview-by-dan-carpenter

FWIIW, I run it usking kchecker on individual source files.

I've also CCed the author, Dan Carpenter, for good measure.

...

Luiz Angelo Daros de Luca Dec. 19, 2024, 4:46 a.m. UTC | #4

Hello Chris,

> +++ b/drivers/net/mdio/mdio-realtek-rtl.c

I wonder if the name might be dubious in the future with other realtek
products with MDIO. Realtek is quite a large company with many
products. Would a version/model/family/usage in that name help a far
future reader to identify what this file is about?

How would this realtek MDIO driver interact with the
drivers/net/dsa/realtek drivers? I guess it might not be too much as
this is the SoC MDIO bus and not the user MDIO bus (also something
called "realtek MDIO driver"). Also, the code logic there just rhymes
with some register access implemented there.

> @@ -0,0 +1,341 @@
> +// 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.
> + */
> +
> +#include <linux/mdio.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mod_devicetable.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                   0x000
> +#define   GLB_CTRL_INTF_SEL(intf)      BIT(16 + (intf))
> +#define SMI_PORT0_15_POLLING_SEL       0x008
> +#define SMI_ACCESS_PHY_CTRL_0          0x170
> +#define SMI_ACCESS_PHY_CTRL_1          0x174
> +#define   PHY_CTRL_RWOP                        BIT(2)
> +#define   PHY_CTRL_TYPE                        BIT(1)
> +#define   PHY_CTRL_CMD                 BIT(0)
> +#define   PHY_CTRL_FAIL                        BIT(25)
> +#define SMI_ACCESS_PHY_CTRL_2          0x178
> +#define SMI_ACCESS_PHY_CTRL_3          0x17c
> +#define SMI_PORT0_5_ADDR_CTRL          0x180
> +
> +#define MAX_PORTS       28
> +#define MAX_SMI_BUSSES  4
> +#define MAX_SMI_ADDR   0x1f
> +
> +struct realtek_mdio_priv {
> +       struct regmap *regmap;
> +       u8 smi_bus[MAX_PORTS];
> +       u8 smi_addr[MAX_PORTS];
> +       bool smi_bus_isc45[MAX_SMI_BUSSES];
> +       u32 reg_base;
> +};
> +
> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)

All those realtek_mdio_* prefix might collide with realtek_mdio_* from
drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
is about the interface (MDIO or SMI) between (the other vendor) SoC
and the switch. I don't know if the maintainers are OK with it but
listing those symbols in alphabetic order from both sources might be
confusing.

> +{
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +
> +       return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,

All regmap funcs are adding reg_base to the register address. Isn't a
remap job to do that sum? It just looks odd but I never worked with
MFD. It looks like it is missing a subregmap-like variant.

> +                                       val, !(val & PHY_CTRL_CMD), 10, 500);
> +}
> +
> +static int realtek_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
> +{
> +       struct realtek_mdio_priv *priv = bus->priv;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +       int err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                          regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD);
> +       if (err)
> +               return err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
> +       if (err)
> +               return err;
> +
> +       return val & 0xffff;
> +}
> +
> +static int realtek_mdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16 value)
> +{
> +       struct realtek_mdio_priv *priv = bus->priv;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +       int err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                          regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_RWOP | PHY_CTRL_CMD);
> +       if (err)
> +               return err;
> +
> +       err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                                      val, !(val & PHY_CTRL_CMD), 10, 100);
> +       if (err)
> +               return err;
> +
> +       if (val & PHY_CTRL_FAIL)
> +               return -ENXIO;
> +
> +       return 0;
> +}
> +
> +static int realtek_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr, int regnum)
> +{
> +       struct realtek_mdio_priv *priv = bus->priv;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +       int err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
> +                          dev_addr << 16 | (regnum & 0xffff));
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                          PHY_CTRL_TYPE | PHY_CTRL_CMD);
> +       if (err)
> +               return err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
> +       if (err)
> +               return err;
> +
> +       return val & 0xffff;
> +}
> +
> +static int realtek_mdio_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
> +                                 int regnum, u16 value)
> +{
> +       struct realtek_mdio_priv *priv = bus->priv;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +       int err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
> +                          dev_addr << 16 | (regnum & 0xffff));
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                          PHY_CTRL_RWOP | PHY_CTRL_TYPE | PHY_CTRL_CMD);
> +       if (err)
> +               return err;
> +
> +       err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                                      val, !(val & PHY_CTRL_CMD), 10, 100);
> +       if (err)
> +               return err;
> +
> +       if (val & PHY_CTRL_FAIL)
> +               return -ENXIO;
> +
> +       return 0;
> +}
> +
> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> +       u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       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;
> +       }
> +
> +       /* 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);
> +                       glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> +               }
> +       }
> +
> +       err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_5_ADDR_CTRL,
> +                               port_addr, 5);
> +       if (err)
> +               return err;
> +
> +       err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_15_POLLING_SEL,
> +                               poll_sel, 2);
> +       if (err)
> +               return err;
> +
> +       err = regmap_update_bits(regmap, reg_base + SMI_GLB_CTRL,
> +                                glb_ctrl_mask, glb_ctrl_val);
> +       if (err)
> +               return err;
> +
> +       return 0;
> +}
> +
> +static int realtek_mdiobus_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct realtek_mdio_priv *priv;
> +       struct fwnode_handle *child;
> +       struct mii_bus *bus;
> +       int err;
> +
> +       bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> +       if (!bus)
> +               return -ENOMEM;
> +
> +       bus->name = "Reaktek Switch MDIO Bus";
> +       bus->read = realtek_mdio_read_c22;
> +       bus->write = realtek_mdio_write_c22;
> +       bus->read_c45 = realtek_mdio_read_c45;
> +       bus->write_c45 =  realtek_mdio_write_c45;
> +       bus->parent = dev;
> +       priv = bus->priv;
> +
> +       priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
> +       if (IS_ERR(priv->regmap))
> +               return PTR_ERR(priv->regmap);
> +
> +       err = device_property_read_u32(dev, "reg", &priv->reg_base);
> +       if (err)
> +               return err;
> +
> +       snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> +       device_for_each_child_node(dev, child) {
> +               u32 pn, smi_addr[2];
> +
> +               err = fwnode_property_read_u32(child, "reg", &pn);
> +               if (err)
> +                       return err;
> +
> +               if (pn >= MAX_PORTS)
> +                       return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
> +
> +               err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> +               if (err) {
> +                       smi_addr[0] = 0;
> +                       smi_addr[1] = pn;
> +               }
> +
> +               if (smi_addr[0] > MAX_SMI_BUSSES)
> +                       return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
> +                                            smi_addr[0]);
> +
> +               if (smi_addr[1] > MAX_SMI_ADDR)
> +                       return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
> +
> +               if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> +                       priv->smi_bus_isc45[smi_addr[0]] = true;
> +
> +               priv->smi_bus[pn] = smi_addr[0];
> +               priv->smi_addr[pn] = smi_addr[1];
> +       }
> +
> +       err = realtek_mdiobus_init(priv);
> +       if (err)
> +               return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
> +
> +       err = devm_of_mdiobus_register(dev, bus, dev->of_node);
> +       if (err)
> +               return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id realtek_mdio_ids[] = {
> +       { .compatible = "realtek,rtl9301-mdio" },
> +       { .compatible = "realtek,rtl9302b-mdio" },
> +       { .compatible = "realtek,rtl9302c-mdio" },
> +       { .compatible = "realtek,rtl9303-mdio" },

Do these different compatible strings really matter? AFAIK, compatible
are not for listing all supported models/variants but to describe
devices that have a different behavior and indicating that (with
different strings) is needed to decide how the driver will work. If
the driver does not use which compatible was set, it might indicate
that we don't really need 4 compatible but 1.

> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, realtek_mdio_ids);
> +
> +static struct platform_driver rtl9300_mdio_driver = {
> +       .probe = realtek_mdiobus_probe,
> +       .driver = {
> +               .name = "mdio-rtl9300",
> +               .of_match_table = realtek_mdio_ids,
> +       },
> +};
> +
> +module_platform_driver(rtl9300_mdio_driver);
> +
> +MODULE_DESCRIPTION("RTL9300 MDIO driver");
> +MODULE_LICENSE("GPL");
> --
> 2.47.1
>
>

Regards,

Luiz

Andrew Lunn Dec. 19, 2024, 9:40 a.m. UTC | #5

On Thu, Dec 19, 2024 at 01:46:41AM -0300, Luiz Angelo Daros de Luca wrote:
> Hello Chris,
> 
> > +++ b/drivers/net/mdio/mdio-realtek-rtl.c
> 
> I wonder if the name might be dubious in the future with other realtek
> products with MDIO. Realtek is quite a large company with many
> products. Would a version/model/family/usage in that name help a far
> future reader to identify what this file is about?

Isnt rtl the family name? Or would you prefer mdio-realtek-rtl9300.c?

> > +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
> 
> All those realtek_mdio_* prefix might collide with realtek_mdio_* from
> drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
> Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
> is about the interface (MDIO or SMI) between (the other vendor) SoC
> and the switch. I don't know if the maintainers are OK with it but
> listing those symbols in alphabetic order from both sources might be
> confusing.

rtl9300_ as a prefix?

> > +static const struct of_device_id realtek_mdio_ids[] = {
> > +       { .compatible = "realtek,rtl9301-mdio" },
> > +       { .compatible = "realtek,rtl9302b-mdio" },
> > +       { .compatible = "realtek,rtl9302c-mdio" },
> > +       { .compatible = "realtek,rtl9303-mdio" },
> 
> Do these different compatible strings really matter? AFAIK, compatible
> are not for listing all supported models/variants but to describe
> devices that have a different behavior and indicating that (with
> different strings) is needed to decide how the driver will work. If
> the driver does not use which compatible was set, it might indicate
> that we don't really need 4 compatible but 1.

It can be useful when we initially think they are compatible, but
later find out they are not, and we need different behaviour.

FYI: Please trim the text when replying.

	Andrew

Chris Packham Dec. 19, 2024, 8:36 p.m. UTC | #6

On 19/12/2024 22:40, Andrew Lunn wrote:
> On Thu, Dec 19, 2024 at 01:46:41AM -0300, Luiz Angelo Daros de Luca wrote:
>> Hello Chris,
>>
>>> +++ b/drivers/net/mdio/mdio-realtek-rtl.c
>> I wonder if the name might be dubious in the future with other realtek
>> products with MDIO. Realtek is quite a large company with many
>> products. Would a version/model/family/usage in that name help a far
>> future reader to identify what this file is about?
> Isnt rtl the family name? Or would you prefer mdio-realtek-rtl9300.c?

Yes my intention was that "rtl" was the family name. I'm happy to change 
to rtl9300.

I suspect this probably will be compatible with the rtl9310. I've just 
received a RTL9313 based board so will probably start looking at that in 
the new year.

>>> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
>> All those realtek_mdio_* prefix might collide with realtek_mdio_* from
>> drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
>> Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
>> is about the interface (MDIO or SMI) between (the other vendor) SoC
>> and the switch. I don't know if the maintainers are OK with it but
>> listing those symbols in alphabetic order from both sources might be
>> confusing.
> rtl9300_ as a prefix?

I'd happily  change to rtl_ or rtl9300_

>>> +static const struct of_device_id realtek_mdio_ids[] = {
>>> +       { .compatible = "realtek,rtl9301-mdio" },
>>> +       { .compatible = "realtek,rtl9302b-mdio" },
>>> +       { .compatible = "realtek,rtl9302c-mdio" },
>>> +       { .compatible = "realtek,rtl9303-mdio" },
>> Do these different compatible strings really matter? AFAIK, compatible
>> are not for listing all supported models/variants but to describe
>> devices that have a different behavior and indicating that (with
>> different strings) is needed to decide how the driver will work. If
>> the driver does not use which compatible was set, it might indicate
>> that we don't really need 4 compatible but 1.
> It can be useful when we initially think they are compatible, but
> later find out they are not, and we need different behaviour.

The way I've written the dt-binding any board should include 
"realtek,rtl9301-mdio" and may also include one of 
"realtek,rtl9302b-mdio", "realtek,rtl9302c-mdio", 
"realtek,rtl9303-mdio". For the MDIO driver the specific chip could 
possibly tell us the maximum SMI bus number. Unfortunately I've only got 
a block diagram of the RTL9302C, I know that does have 4 SMI interfaces, 
the others may have fewer. Things would probably work fine for now with 
just "realtek,rtl9301-mdio" but is there any harm in including the others?

Chris Packham Dec. 19, 2024, 8:50 p.m. UTC | #7

On 19/12/2024 17:46, Luiz Angelo Daros de Luca wrote:
>> +#define SMI_GLB_CTRL                   0x000
>> +#define   GLB_CTRL_INTF_SEL(intf)      BIT(16 + (intf))
>> +#define SMI_PORT0_15_POLLING_SEL       0x008
>> +#define SMI_ACCESS_PHY_CTRL_0          0x170
>> +#define SMI_ACCESS_PHY_CTRL_1          0x174
>> +#define   PHY_CTRL_RWOP                        BIT(2)
>> +#define   PHY_CTRL_TYPE                        BIT(1)
>> +#define   PHY_CTRL_CMD                 BIT(0)
>> +#define   PHY_CTRL_FAIL                        BIT(25)
>> +#define SMI_ACCESS_PHY_CTRL_2          0x178
>> +#define SMI_ACCESS_PHY_CTRL_3          0x17c
>> +#define SMI_PORT0_5_ADDR_CTRL          0x180
>> +
>> +#define MAX_PORTS       28
>> +#define MAX_SMI_BUSSES  4
>> +#define MAX_SMI_ADDR   0x1f
>> +
>> +struct realtek_mdio_priv {
>> +       struct regmap *regmap;
>> +       u8 smi_bus[MAX_PORTS];
>> +       u8 smi_addr[MAX_PORTS];
>> +       bool smi_bus_isc45[MAX_SMI_BUSSES];
>> +       u32 reg_base;
>> +};
>> +
>> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
>> +{
>> +       struct regmap *regmap = priv->regmap;
>> +       u32 reg_base = priv->reg_base;
>> +       u32 val;
>> +
>> +       return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> All regmap funcs are adding reg_base to the register address. Isn't a
> remap job to do that sum? It just looks odd but I never worked with
> MFD. It looks like it is missing a subregmap-like variant.
I'm thinking about dropping the base and just using the full 16-bit 
address. I've already confused myself between this code and the datasheet.

Luiz Angelo Daros de Luca Dec. 20, 2024, 5:11 a.m. UTC | #8

> On 19/12/2024 22:40, Andrew Lunn wrote:
> > On Thu, Dec 19, 2024 at 01:46:41AM -0300, Luiz Angelo Daros de Luca wrote:
> >> Hello Chris,
> >>
> >>> +++ b/drivers/net/mdio/mdio-realtek-rtl.c
> >> I wonder if the name might be dubious in the future with other realtek
> >> products with MDIO. Realtek is quite a large company with many
> >> products. Would a version/model/family/usage in that name help a far
> >> future reader to identify what this file is about?
> > Isnt rtl the family name? Or would you prefer mdio-realtek-rtl9300.c?
>
> Yes my intention was that "rtl" was the family name.

rtl just means realtek. It is the same prefix for a wide range of
Realtek products (audio, usb, storage, network).

> I'm happy to change to rtl9300.

Even the product number might be confusing. For example, the switch
rtl8365mb is newer than (and incompatible with) the rtl8366rb. Realtek
has a library/driver name that could indicate the family (rtl8367b,
rtl8367c, rtl8367d), but it is not tightly related to model numbers.
In the DSA case, we simply adopted the first device that was supported
as the filename, even after that file was expanded to include other
models. I hope rtl93xx model numbers will be less confusing but it is
a Realtek decision. With the realtek past cases, if you want to be
sure it will not be confused with another model in the future, just
use the model "most significant" to your driver, like
modio-realtek-rtl9301. Do not expect that a future rtl93xx model will
be compatible with rtl9300. If rtl9300 does not really mean rtl93??,
rtl9301 would be, at least, more informative.

mdio-realtek-rtl9300.c or even mdio-rtl9300.c are just fine, but I
prefer the former as the common prefix will group different future
models, even if realtek abandons the rtl prefix.

> I suspect this probably will be compatible with the rtl9310. I've just
> received a RTL9313 based board so will probably start looking at that in
> the new year.

At least for the realtek switch-only products, if you have access to
the Realtek Programming Guide or the vendor library/driver, it would
be easy to spot compatible models as they share the same driver
generation. From what I saw from the difference between vendor switch
drivers rtl8367b, rtl8367c and rtl8367d, Relatek tends to use an
incremental model, but breaking the support to older models on every
new family release. They share a lot of code but they differ on
registers (normally constants on each generation). If you intend to
also upstream a DSA driver, you'll probably need to share some code
with the existing drivers as duplicating that much code is normally
rejected in the upstream kernel.

> >>> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
> >> All those realtek_mdio_* prefix might collide with realtek_mdio_* from
> >> drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
> >> Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
> >> is about the interface (MDIO or SMI) between (the other vendor) SoC
> >> and the switch. I don't know if the maintainers are OK with it but
> >> listing those symbols in alphabetic order from both sources might be
> >> confusing.
> > rtl9300_ as a prefix?
>
> I'd happily  change to rtl_ or rtl9300_

"rtl9300_", please (or "rtl9301_") a "rtl" just means realtek.

> >>> +static const struct of_device_id realtek_mdio_ids[] = {
> >>> +       { .compatible = "realtek,rtl9301-mdio" },
> >>> +       { .compatible = "realtek,rtl9302b-mdio" },
> >>> +       { .compatible = "realtek,rtl9302c-mdio" },
> >>> +       { .compatible = "realtek,rtl9303-mdio" },
> >> Do these different compatible strings really matter? AFAIK, compatible
> >> are not for listing all supported models/variants but to describe
> >> devices that have a different behavior and indicating that (with
> >> different strings) is needed to decide how the driver will work. If
> >> the driver does not use which compatible was set, it might indicate
> >> that we don't really need 4 compatible but 1.
> > It can be useful when we initially think they are compatible, but
> > later find out they are not, and we need different behaviour.
>
> The way I've written the dt-binding any board should include
> "realtek,rtl9301-mdio" and may also include one of
> "realtek,rtl9302b-mdio", "realtek,rtl9302c-mdio",
> "realtek,rtl9303-mdio". For the MDIO driver the specific chip could
> possibly tell us the maximum SMI bus number. Unfortunately I've only got
> a block diagram of the RTL9302C, I know that does have 4 SMI interfaces,
> the others may have fewer. Things would probably work fine for now with
> just "realtek,rtl9301-mdio" but is there any harm in including the others?

If "realtek,rtl9301-mdio" is mandatory, until you need to
differentiate each model, the extra compatible strings are not useful.
It would increase the compatible table a little bit. My concern was
not about the extra data but that a similar approach was rejected in
the past. If maintainers are OK with it, I have nothing else to say.

>>> +{
>>> +       struct regmap *regmap = priv->regmap;
>>> +       u32 reg_base = priv->reg_base;
>>> +       u32 val;
>>> +
>>> +       return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
>> All regmap funcs are adding reg_base to the register address. Isn't a
>> remap job to do that sum? It just looks odd but I never worked with
>> MFD. It looks like it is missing a subregmap-like variant.
>
> I'm thinking about dropping the base and just using the full 16-bit
> address. I've already confused myself between this code and the datasheet.

That's what I thought when I saw the sum. I would definitely miss it
at some point.
If their positions are fixed related to syscon base, I would use the
full 16-bit. You could use the base+relative_reg_addr in the macro
that defines the register address without increasing the complexity.

Regards,

Luiz

Patch

diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 4a7a303be2f7..0c6240c4a7e9 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -185,6 +185,13 @@  config MDIO_IPQ8064
 	  This driver supports the MDIO interface found in the network
 	  interface units of the IPQ8064 SoC
 
+config MDIO_REALTEK_RTL
+	tristate "Realtek RTL9300 MDIO interface support"
+	depends on MACH_REALTEK_RTL || COMPILE_TEST
+	help
+	  This driver supports the MDIO interface found in the Realtek
+	  RTL9300 family of Ethernet switches with integrated SoC.
+
 config MDIO_REGMAP
 	tristate
 	help
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 1015f0db4531..2cd8b491f301 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -19,6 +19,7 @@  obj-$(CONFIG_MDIO_MOXART)		+= mdio-moxart.o
 obj-$(CONFIG_MDIO_MSCC_MIIM)		+= mdio-mscc-miim.o
 obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
 obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
+obj-$(CONFIG_MDIO_REALTEK_RTL)		+= mdio-realtek-rtl.o
 obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
 obj-$(CONFIG_MDIO_SUN4I)		+= mdio-sun4i.o
 obj-$(CONFIG_MDIO_THUNDER)		+= mdio-thunder.o
diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c
new file mode 100644
index 000000000000..a13b84279138
--- /dev/null
+++ b/drivers/net/mdio/mdio-realtek-rtl.c
@@ -0,0 +1,341 @@ 
+// 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.
+ */
+
+#include <linux/mdio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.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			0x000
+#define   GLB_CTRL_INTF_SEL(intf)	BIT(16 + (intf))
+#define SMI_PORT0_15_POLLING_SEL	0x008
+#define SMI_ACCESS_PHY_CTRL_0		0x170
+#define SMI_ACCESS_PHY_CTRL_1		0x174
+#define   PHY_CTRL_RWOP			BIT(2)
+#define   PHY_CTRL_TYPE			BIT(1)
+#define   PHY_CTRL_CMD			BIT(0)
+#define   PHY_CTRL_FAIL			BIT(25)
+#define SMI_ACCESS_PHY_CTRL_2		0x178
+#define SMI_ACCESS_PHY_CTRL_3		0x17c
+#define SMI_PORT0_5_ADDR_CTRL		0x180
+
+#define MAX_PORTS       28
+#define MAX_SMI_BUSSES  4
+#define MAX_SMI_ADDR	0x1f
+
+struct realtek_mdio_priv {
+	struct regmap *regmap;
+	u8 smi_bus[MAX_PORTS];
+	u8 smi_addr[MAX_PORTS];
+	bool smi_bus_isc45[MAX_SMI_BUSSES];
+	u32 reg_base;
+};
+
+static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
+{
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+
+	return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+					val, !(val & PHY_CTRL_CMD), 10, 500);
+}
+
+static int realtek_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
+{
+	struct realtek_mdio_priv *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+			   regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;
+}
+
+static int realtek_mdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16 value)
+{
+	struct realtek_mdio_priv *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+			   regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_RWOP | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+				       val, !(val & PHY_CTRL_CMD), 10, 100);
+	if (err)
+		return err;
+
+	if (val & PHY_CTRL_FAIL)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int realtek_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr, int regnum)
+{
+	struct realtek_mdio_priv *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
+			   dev_addr << 16 | (regnum & 0xffff));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+			   PHY_CTRL_TYPE | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;
+}
+
+static int realtek_mdio_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
+				  int regnum, u16 value)
+{
+	struct realtek_mdio_priv *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
+			   dev_addr << 16 | (regnum & 0xffff));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+			   PHY_CTRL_RWOP | PHY_CTRL_TYPE | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+				       val, !(val & PHY_CTRL_CMD), 10, 100);
+	if (err)
+		return err;
+
+	if (val & PHY_CTRL_FAIL)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
+{
+	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	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;
+	}
+
+	/* 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);
+			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
+		}
+	}
+
+	err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_5_ADDR_CTRL,
+				port_addr, 5);
+	if (err)
+		return err;
+
+	err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_15_POLLING_SEL,
+				poll_sel, 2);
+	if (err)
+		return err;
+
+	err = regmap_update_bits(regmap, reg_base + SMI_GLB_CTRL,
+				 glb_ctrl_mask, glb_ctrl_val);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int realtek_mdiobus_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct realtek_mdio_priv *priv;
+	struct fwnode_handle *child;
+	struct mii_bus *bus;
+	int err;
+
+	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
+	if (!bus)
+		return -ENOMEM;
+
+	bus->name = "Reaktek Switch MDIO Bus";
+	bus->read = realtek_mdio_read_c22;
+	bus->write = realtek_mdio_write_c22;
+	bus->read_c45 = realtek_mdio_read_c45;
+	bus->write_c45 =  realtek_mdio_write_c45;
+	bus->parent = dev;
+	priv = bus->priv;
+
+	priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	err = device_property_read_u32(dev, "reg", &priv->reg_base);
+	if (err)
+		return err;
+
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
+
+	device_for_each_child_node(dev, child) {
+		u32 pn, smi_addr[2];
+
+		err = fwnode_property_read_u32(child, "reg", &pn);
+		if (err)
+			return err;
+
+		if (pn >= MAX_PORTS)
+			return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
+
+		err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
+		if (err) {
+			smi_addr[0] = 0;
+			smi_addr[1] = pn;
+		}
+
+		if (smi_addr[0] > MAX_SMI_BUSSES)
+			return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
+					     smi_addr[0]);
+
+		if (smi_addr[1] > MAX_SMI_ADDR)
+			return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
+
+		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
+			priv->smi_bus_isc45[smi_addr[0]] = true;
+
+		priv->smi_bus[pn] = smi_addr[0];
+		priv->smi_addr[pn] = smi_addr[1];
+	}
+
+	err = realtek_mdiobus_init(priv);
+	if (err)
+		return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
+
+	err = devm_of_mdiobus_register(dev, bus, dev->of_node);
+	if (err)
+		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
+
+	return 0;
+}
+
+static const struct of_device_id realtek_mdio_ids[] = {
+	{ .compatible = "realtek,rtl9301-mdio" },
+	{ .compatible = "realtek,rtl9302b-mdio" },
+	{ .compatible = "realtek,rtl9302c-mdio" },
+	{ .compatible = "realtek,rtl9303-mdio" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, realtek_mdio_ids);
+
+static struct platform_driver rtl9300_mdio_driver = {
+	.probe = realtek_mdiobus_probe,
+	.driver = {
+		.name = "mdio-rtl9300",
+		.of_match_table = realtek_mdio_ids,
+	},
+};
+
+module_platform_driver(rtl9300_mdio_driver);
+
+MODULE_DESCRIPTION("RTL9300 MDIO driver");
+MODULE_LICENSE("GPL");