Thread (22 messages) 22 messages, 4 authors, 2021-10-18

Re: [PATCH v4 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x

From: Vladimir Oltean <olteanv@gmail.com>
Date: 2021-10-07 20:00:15
Also in: linux-devicetree, lkml

On Thu, Oct 07, 2021 at 08:41:55PM +0530, Prasanna Vengateshan wrote:
+static int lan937x_mdio_register(struct dsa_switch *ds)
+{
+	struct device_node *mdio_np;
+	int ret;
+
+	mdio_np = of_get_child_by_name(ds->dev->of_node, "mdio");
+	if (!mdio_np) {
+		dev_err(ds->dev, "no MDIO bus node\n");
+		return -ENODEV;
+	}
+
+	ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
+	if (!ds->slave_mii_bus)
+		return -ENOMEM;
+
+	ds->slave_mii_bus->priv = ds->priv;
+	ds->slave_mii_bus->read = lan937x_sw_mdio_read;
+	ds->slave_mii_bus->write = lan937x_sw_mdio_write;
+	ds->slave_mii_bus->name = "lan937x slave smi";
+	snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d", ds->index);
+	ds->slave_mii_bus->parent = ds->dev;
+	ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
+
+	ret = of_mdiobus_register(ds->slave_mii_bus, mdio_np);
Please use devm_of_mdiobus_register if you're going to use
devm_mdiobus_alloc, or no devres at all.
https://patchwork.kernel.org/project/netdevbpf/patch/20210920214209.1733768-3-vladimir.oltean@nxp.com/
+	if (ret) {
+		dev_err(ds->dev, "unable to register MDIO bus %s\n",
+			ds->slave_mii_bus->id);
+	}
+
+	of_node_put(mdio_np);
+
+	return ret;
+}
+
+static phy_interface_t lan937x_get_interface(struct ksz_device *dev, int port)
+{
+	phy_interface_t interface;
+	u8 data8;
+	int ret;
+
+	if (lan937x_is_internal_phy_port(dev, port))
+		return PHY_INTERFACE_MODE_NA;
Typically we use PHY_INTERFACE_MODE_INTERNAL.
+
+	/* read interface from REG_PORT_XMII_CTRL_1 register */
+	ret = lan937x_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
+	if (ret < 0)
+		return PHY_INTERFACE_MODE_NA;
+
+	switch (data8 & PORT_MII_SEL_M) {
+	case PORT_RMII_SEL:
+		interface = PHY_INTERFACE_MODE_RMII;
+		break;
+	case PORT_RGMII_SEL:
+		interface = PHY_INTERFACE_MODE_RGMII;
+		if (data8 & PORT_RGMII_ID_EG_ENABLE)
+			interface = PHY_INTERFACE_MODE_RGMII_TXID;
+		if (data8 & PORT_RGMII_ID_IG_ENABLE) {
+			interface = PHY_INTERFACE_MODE_RGMII_RXID;
+			if (data8 & PORT_RGMII_ID_EG_ENABLE)
+				interface = PHY_INTERFACE_MODE_RGMII_ID;
+		}
+		break;
+	case PORT_MII_SEL:
+	default:
+		/* Interface is MII */
+		interface = PHY_INTERFACE_MODE_MII;
+		break;
+	}
+
+	return interface;
+}
+
+static void lan937x_config_cpu_port(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p;
+	int i;
+
+	ds->num_ports = dev->port_cnt;
+
+	for (i = 0; i < dev->port_cnt; i++) {
+		if (dsa_is_cpu_port(ds, i) && (dev->cpu_ports & (1 << i))) {
+			phy_interface_t interface;
+
+			dev->cpu_port = i;
+			dev->host_mask = (1 << dev->cpu_port);
+			dev->port_mask |= dev->host_mask;
+			p = &dev->ports[i];
+
+			/* Check if the device tree have specific interface
+			 * setting otherwise read & assign from XMII register
+			 * for host port interface
+			 */
+			interface = lan937x_get_interface(dev, i);
What does the CPU port have so special that you override it here?
Again some compatibility with out-of-tree DT bindings?
+			if (!p->interface)
+				p->interface = interface;
+
+			dev_info(dev->dev,
+				 "Port%d: using phy mode %s\n",
+				 i,
+				 phy_modes(p->interface));
+
+			/* enable cpu port */
+			lan937x_port_setup(dev, i, true);
+			p->vid_member = dev->port_mask;
+		}
+	}
+
+static u8 lan937x_rgmii_dly_reg_val(int port, u32 val)
+{
+	u8 reg_val;
+
+	/* force minimum delay if delay is less than min delay */
+	if (val && val < 2170)
+		val = 2170;
+
+	/* maximum delay is 4ns */
+	if (val > 4000)
+		val = 4000;
These bindings are new. Given that you also document their min and max
values, why don't you just error out on out-of-range values instead of
silently doing what you think is going to be fine?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help