diff mbox series

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

Message ID 20241211235342.1573926-5-chris.packham@alliedtelesis.co.nz (mailing list archive)
State Superseded
Headers show
Series RTL9300 MDIO driver | expand

Commit Message

Chris Packham Dec. 11, 2024, 11:53 p.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 <chris.packham@alliedtelesis.co.nz>
---
 drivers/net/mdio/Kconfig            |   7 +
 drivers/net/mdio/Makefile           |   1 +
 drivers/net/mdio/mdio-realtek-rtl.c | 264 ++++++++++++++++++++++++++++
 3 files changed, 272 insertions(+)
 create mode 100644 drivers/net/mdio/mdio-realtek-rtl.c

Comments

Andrew Lunn Dec. 12, 2024, 7:41 a.m. UTC | #1 
> +	/* get_phy_c45_ids() will stop the mdio bus scan if we return an error
> +	 * here. So even though the SMI controller indicates an error for an
> +	 * absent device don't proagate it here.
> +	 */
> +	//if (val & BIT(25)) {
> +	//	err = -ENODEV;
> +	//	return err;
> +	//}

Please don't leave committed out code. It should either be needed, and
uncommented, or deleted.

> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> +	u32 port_addr[5] = { };
> +	u32 poll_sel[2] = { 0, 0 };
> +	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> +	int i, err;
> +
> +	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;
> +	}
> +
> +	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);
> +		}
> +	}

Please could you add some comments because i don't understand this at
all.

> +
> +	err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
> +				port_addr, 5);
> +	if (err)
> +		return err;
> +
> +	err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
> +				poll_sel, 2);
> +	if (err)
> +		return err;
> +
> +	err = regmap_update_bits(priv->regmap, priv->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_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;
> +		}

Would returning -EINVAL be better here, since it indicates a bug in
the device tree?

> +
> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> +			priv->smi_bus_isc45[smi_addr[0]] = true;

If it is not C45 then what? I don't see any support for C22.

	Andrew
Christophe JAILLET Dec. 12, 2024, 8:18 a.m. UTC | #2 
Le 12/12/2024 à 00:53, Chris Packham a écrit :
> 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 <chris.packham@alliedtelesis.co.nz>

...

> +	err = regmap_write(priv->regmap, priv->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(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
> +				       val, !(val & PHY_CTRL_CMD), 10, 100);
> +	if (err)
> +		return err;
> +
> +	if (val & PHY_CTRL_FAIL) {
> +		err = -ENXIO;
> +		return err;

Nitpick: return -ENXIO; and remove the { }

> +	}
> +
> +	return err;

Nitpick: return 0;

> +}
> +
> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> +	u32 port_addr[5] = { };
> +	u32 poll_sel[2] = { 0, 0 };

Nitpick: Why {} in on case and {0,0} in the other one?

> +	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> +	int i, err;
> +
> +	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;
> +	}

...

CJ
Russell King (Oracle) Dec. 12, 2024, 9:59 a.m. UTC | #3 
On Thu, Dec 12, 2024 at 12:53:42PM +1300, Chris Packham 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)

Presumably, reading the code, this bit is set when writing?

> +#define   PHY_CTRL_TYPE			BIT(1)

Presumably, reading the code, this bit indicates we want to use clause
45?

> +#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       32
> +#define MAX_SMI_BUSSES  4
> +
> +struct realtek_mdio_priv {
> +	struct regmap *regmap;
> +	u8 smi_bus[MAX_PORTS];
> +	u8 smi_addr[MAX_PORTS];
> +	bool smi_bus_isc45[MAX_SMI_BUSSES];

Not sure about the support for !C45 - you appear to set this if you
find a PHY as a child of this device which has the PHY C45 compatible,
but as you don't populate the C22 MDIO bus operations, I'm not sure
how a C22 PHY can work.

> +	u32 reg_base;
> +};
> +
> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
> +{
> +	u32 val;
> +
> +	return regmap_read_poll_timeout(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
> +					val, !(val & PHY_CTRL_CMD), 10, 500);
> +}
> +
> +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;
> +	u32 val;
> +	int err;
> +
> +	err = realtek_mdio_wait_ready(priv);
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_3,
> +			   dev_addr << 16 | (regnum & 0xffff));
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
> +			   PHY_CTRL_TYPE | PHY_CTRL_CMD);
> +	if (err)
> +		return err;

Maybe consider using a local variable for "regmap" and "reg_base" to
reduce the line length/wrapping?

> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> +	u32 port_addr[5] = { };
> +	u32 poll_sel[2] = { 0, 0 };
> +	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;

Please use reverse Christmas tree order.

> +	int i, err;
> +
> +	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;

s/  / /

> +
> +		pos = (i % 16) * 2;
> +		poll_sel[i / 16] |= priv->smi_bus[i] << pos;
> +	}
> +
> +	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(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
> +				port_addr, 5);
> +	if (err)
> +		return err;
> +
> +	err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
> +				poll_sel, 2);
> +	if (err)
> +		return err;
> +
> +	err = regmap_update_bits(priv->regmap, priv->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_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);

You validate the port number.

> +
> +		err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> +		if (err) {
> +			smi_addr[0] = 0;
> +			smi_addr[1] = pn;
> +		}

You don't validate the "smi_addr", so:

	realtek,smi-address = <4, ...>;

would silently overflow priv->smi_bus_isc45. However, I haven't checked
whether the binding would warn about this.

Thanks.
Chris Packham Dec. 13, 2024, 12:51 a.m. UTC | #4 
Hi Russell,

On 12/12/2024 22:59, Russell King (Oracle) wrote:
> On Thu, Dec 12, 2024 at 12:53:42PM +1300, Chris Packham 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)
> Presumably, reading the code, this bit is set when writing?
Correct. I've tried to use the bit field names from the datasheet. RWOP 
0=read, 1=write.
>> +#define   PHY_CTRL_TYPE			BIT(1)
> Presumably, reading the code, this bit indicates we want to use clause
> 45?

Yes. Technically the datasheet says 0=normal register, 1=MMD register.

>> +#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       32
>> +#define MAX_SMI_BUSSES  4
>> +
>> +struct realtek_mdio_priv {
>> +	struct regmap *regmap;
>> +	u8 smi_bus[MAX_PORTS];
>> +	u8 smi_addr[MAX_PORTS];
>> +	bool smi_bus_isc45[MAX_SMI_BUSSES];
> Not sure about the support for !C45 - you appear to set this if you
> find a PHY as a child of this device which has the PHY C45 compatible,
> but as you don't populate the C22 MDIO bus operations, I'm not sure
> how a C22 PHY can work.

Oops, yes I forgot to come back to C22. Most of the hardware I have 
access to uses C45 so that's been my main test setup. I'll include C22 
support in v2.

>> +	u32 reg_base;
>> +};
>> +
>> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
>> +{
>> +	u32 val;
>> +
>> +	return regmap_read_poll_timeout(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
>> +					val, !(val & PHY_CTRL_CMD), 10, 500);
>> +}
>> +
>> +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;
>> +	u32 val;
>> +	int err;
>> +
>> +	err = realtek_mdio_wait_ready(priv);
>> +	if (err)
>> +		return err;
>> +
>> +	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
>> +	if (err)
>> +		return err;
>> +
>> +	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_3,
>> +			   dev_addr << 16 | (regnum & 0xffff));
>> +	if (err)
>> +		return err;
>> +
>> +	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
>> +			   PHY_CTRL_TYPE | PHY_CTRL_CMD);
>> +	if (err)
>> +		return err;
> Maybe consider using a local variable for "regmap" and "reg_base" to
> reduce the line length/wrapping?
Ok
>> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
>> +{
>> +	u32 port_addr[5] = { };
>> +	u32 poll_sel[2] = { 0, 0 };
>> +	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> Please use reverse Christmas tree order.
Ok.
>> +	int i, err;
>> +
>> +	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;
> s/  / /
Ok.
>> +
>> +		pos = (i % 16) * 2;
>> +		poll_sel[i / 16] |= priv->smi_bus[i] << pos;
>> +	}
>> +
>> +	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(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
>> +				port_addr, 5);
>> +	if (err)
>> +		return err;
>> +
>> +	err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
>> +				poll_sel, 2);
>> +	if (err)
>> +		return err;
>> +
>> +	err = regmap_update_bits(priv->regmap, priv->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_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);
> You validate the port number.
>
>> +
>> +		err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
>> +		if (err) {
>> +			smi_addr[0] = 0;
>> +			smi_addr[1] = pn;
>> +		}
> You don't validate the "smi_addr", so:
>
> 	realtek,smi-address = <4, ...>;
>
> would silently overflow priv->smi_bus_isc45. However, I haven't checked
> whether the binding would warn about this.

I'll make sure the smi bus and phy address are within an appropriate range.
Simon Horman Dec. 13, 2024, 7:59 p.m. UTC | #5 
On Thu, Dec 12, 2024 at 12:53:42PM +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 <chris.packham@alliedtelesis.co.nz>

...

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

...

> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> +	u32 port_addr[5] = { };
> +	u32 poll_sel[2] = { 0, 0 };
> +	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> +	int i, err;
> +
> +	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;

Hi Chris,

The maximum index of port_addr accessed above is
(MAX_PORTS - 1) / 6 = (32 - 1) / 6 = 5.
But port_addr only has five elements (maximum index of 4).
So this will overflow.

Flagged by Smatch.

> +
> +		pos = (i % 16) * 2;
> +		poll_sel[i / 16] |= priv->smi_bus[i] << pos;
> +	}
> +
> +	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(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
> +				port_addr, 5);
> +	if (err)
> +		return err;
> +
> +	err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
> +				poll_sel, 2);
> +	if (err)
> +		return err;
> +
> +	err = regmap_update_bits(priv->regmap, priv->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_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 (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];

The condition about 15 lines above ensures that the maximum value of pn
is MAX_PORTS. But if this is the case then the above will overflow
both smi_bus and smi_addr as they each have MAX_PORTS elements
(maximum index of MAX_PORTS - 1).

I suspect the condition above should be updated to:

	if (pn >= MAX_PORTS)
		return ...

Also flagged by Smatch.

> +	}
> +
> +	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. 15, 2024, 8:27 p.m. UTC | #6 
On 14/12/2024 08:59, Simon Horman wrote:
> On Thu, Dec 12, 2024 at 12:53:42PM +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 <chris.packham@alliedtelesis.co.nz>
> ...
>
>> diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c
> ...
>
>> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
>> +{
>> +	u32 port_addr[5] = { };
>> +	u32 poll_sel[2] = { 0, 0 };
>> +	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
>> +	int i, err;
>> +
>> +	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;
> Hi Chris,
>
> The maximum index of port_addr accessed above is
> (MAX_PORTS - 1) / 6 = (32 - 1) / 6 = 5.
> But port_addr only has five elements (maximum index of 4).
> So this will overflow.
>
> Flagged by Smatch.

Drat. It's more complicated than that. The maximum number of _physical_ 
ports on the RTL9300 is 28 (i.e. 0-27). In some other places port 28 is 
used to mean the CPU port (i.e. the DMA interface) and in others it just 
uses 32 possible ports because that makes the tables entries align 
nicely. Since this is just the MDIO interface, setting MAX_PORTS to 28 
here seems like the sensible thing to do.

>> +
>> +		pos = (i % 16) * 2;
>> +		poll_sel[i / 16] |= priv->smi_bus[i] << pos;
>> +	}
>> +
>> +	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(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
>> +				port_addr, 5);
>> +	if (err)
>> +		return err;
>> +
>> +	err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
>> +				poll_sel, 2);
>> +	if (err)
>> +		return err;
>> +
>> +	err = regmap_update_bits(priv->regmap, priv->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_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 (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];
> The condition about 15 lines above ensures that the maximum value of pn
> is MAX_PORTS. But if this is the case then the above will overflow
> both smi_bus and smi_addr as they each have MAX_PORTS elements
> (maximum index of MAX_PORTS - 1).
>
> I suspect the condition above should be updated to:
>
> 	if (pn >= MAX_PORTS)
> 		return ...
>
> Also flagged by Smatch.
Good catch thanks. Will fix in v2.
>> +	}
>> +
>> +	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;
>> +}
> ...
diff mbox series

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..27d1bc02e1e0
--- /dev/null
+++ b/drivers/net/mdio/mdio-realtek-rtl.c
@@ -0,0 +1,264 @@ 
+// 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       32
+#define MAX_SMI_BUSSES  4
+
+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)
+{
+	u32 val;
+
+	return regmap_read_poll_timeout(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
+					val, !(val & PHY_CTRL_CMD), 10, 500);
+}
+
+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;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_3,
+			   dev_addr << 16 | (regnum & 0xffff));
+	if (err)
+		return err;
+
+	err = regmap_write(priv->regmap, priv->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;
+
+	/* get_phy_c45_ids() will stop the mdio bus scan if we return an error
+	 * here. So even though the SMI controller indicates an error for an
+	 * absent device don't proagate it here.
+	 */
+	//if (val & BIT(25)) {
+	//	err = -ENODEV;
+	//	return err;
+	//}
+
+	err = regmap_read(priv->regmap, priv->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;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
+	if (err)
+		return err;
+
+	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_3,
+			   dev_addr << 16 | (regnum & 0xffff));
+	if (err)
+		return err;
+
+	err = regmap_write(priv->regmap, priv->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(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
+				       val, !(val & PHY_CTRL_CMD), 10, 100);
+	if (err)
+		return err;
+
+	if (val & PHY_CTRL_FAIL) {
+		err = -ENXIO;
+		return err;
+	}
+
+	return err;
+}
+
+static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
+{
+	u32 port_addr[5] = { };
+	u32 poll_sel[2] = { 0, 0 };
+	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
+	int i, err;
+
+	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;
+	}
+
+	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(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
+				port_addr, 5);
+	if (err)
+		return err;
+
+	err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
+				poll_sel, 2);
+	if (err)
+		return err;
+
+	err = regmap_update_bits(priv->regmap, priv->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_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 (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");