Re: [PATCH net-next 08/13] net: dsa: realtek: add new mdio interface for drivers

Hi Luiz,

On 12/16/21 21:13, luizluca@gmail.com wrote:

From: Luiz Angelo Daros de Luca <luizluca@gmail.com>

This driver is a mdio_driver instead of a platform driver (like
realtek-smi).

Tested-by: Arınç ÜNAL <redacted>
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---

Great work giving the switches MDIO support. Left comments down below.

I'm not so pleased with how this driver is split up - nor was I before - 
but something feels a bit off with all the code duplication. I'll think 
about it but maybe somebody else can share some experience they have 
from other drivers.

I didn't check the register access details just yet.

No BUG_ON please, just make sure that this never happens. This will 
crash the system.

+
+        mutex_lock(&bus->mdio_lock);
+        /* Write Start command to register 29 */
+        bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP);

Just noticed you are mixing tabs and spaces too. You can try running 
clang-format or similar on your code before sending v2. Be aware it will 
mess up the #defines.

+
+        /* Write address control code to register 31 */
+        bus->write(bus, phy_id, MDC_MDIO_CTRL0_REG, MDC_MDIO_ADDR_OP);
+
+        /* Write Start command to register 29 */
+        bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP);
+
+        /* Write address to register 23 */
+        bus->write(bus, phy_id, MDC_MDIO_ADDRESS_REG, addr);
+
+        /* Write Start command to register 29 */
+        bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP);
+
+        /* Write read control code to register 21 */
+        bus->write(bus, phy_id, MDC_MDIO_CTRL1_REG, MDC_MDIO_READ_OP);
+
+        /* Write Start command to register 29 */
+        bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP);
+
+        /* Read data from register 25 */
+        *data = bus->read(bus, phy_id, MDC_MDIO_DATA_READ_REG);
+
+        mutex_unlock(&bus->mdio_lock);
+
+        return 0;
+}
+
+static int realtek_mdio_write_reg(struct realtek_priv *priv, u32 addr, u32 data)
+{
+        u32 phy_id = priv->phy_id;
+        struct mii_bus *bus = priv->bus;
+
+        BUG_ON(in_interrupt());
+
+        mutex_lock(&bus->mdio_lock);
+
+        /* Write Start command to register 29 */
+        bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP);
+
+        /* Write address control code to register 31 */
+        bus->write(bus, phy_id, MDC_MDIO_CTRL0_REG, MDC_MDIO_ADDR_OP);
+
+        /* Write Start command to register 29 */
+        bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP);
+
+        /* Write address to register 23 */
+        bus->write(bus, phy_id, MDC_MDIO_ADDRESS_REG, addr);
+
+        /* Write Start command to register 29 */
+        bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP);
+
+        /* Write data to register 24 */
+        bus->write(bus, phy_id, MDC_MDIO_DATA_WRITE_REG, data);
+
+        /* Write Start command to register 29 */
+        bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP);
+
+        /* Write data control code to register 21 */
+        bus->write(bus, phy_id, MDC_MDIO_CTRL1_REG, MDC_MDIO_WRITE_OP);
+
+        mutex_unlock(&bus->mdio_lock);
+        return 0;
+}
+
+
+/* Regmap accessors */
+
+static int realtek_mdio_write(void *ctx, u32 reg, u32 val)
+{
+	struct realtek_priv *priv = ctx;
+
+	return realtek_mdio_write_reg(priv, reg, val);
+}
+
+static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
+{
+	struct realtek_priv *priv = ctx;
+
+	return realtek_mdio_read_reg(priv, reg, val);
+}
+
+static const struct regmap_config realtek_mdio_regmap_config = {
+	.reg_bits = 10, /* A4..A0 R4..R0 */
+	.val_bits = 16,
+	.reg_stride = 1,
+	/* PHY regs are at 0x8000 */
+	.max_register = 0xffff,
+	.reg_format_endian = REGMAP_ENDIAN_BIG,
+	.reg_read = realtek_mdio_read,
+	.reg_write = realtek_mdio_write,
+	.cache_type = REGCACHE_NONE,
+};
+
+static int realtek_mdio_probe(struct mdio_device *mdiodev)
+{
+	struct realtek_priv *priv;
+	struct device *dev = &mdiodev->dev;
+	const struct realtek_variant *var;
+	int ret;
+	struct device_node *np;
+
+	var = of_device_get_match_data(dev);
+	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->phy_id = mdiodev->addr;
+
+	// Start by setting up the register mapping
+	priv->map = devm_regmap_init(dev, NULL, priv, &realtek_mdio_regmap_config);
+
+	priv->bus = mdiodev->bus;
+	priv->dev = &mdiodev->dev;
+	priv->chip_data = (void *)priv + sizeof(*priv);
+
+	priv->clk_delay = var->clk_delay;
+	priv->cmd_read = var->cmd_read;
+	priv->cmd_write = var->cmd_write;
+	priv->ops = var->ops;
+
+	if (IS_ERR(priv->map))
+		dev_warn(dev, "regmap initialization failed");
+
+	priv->write_reg_noack=realtek_mdio_write_reg;
+
+	np = dev->of_node;
+
+	dev_set_drvdata(dev, priv);
+	spin_lock_init(&priv->lock);

It's not even used here.

+
+	/* TODO: if power is software controlled, set up any regulators here */
+
+	/* FIXME: maybe skip if no gpio but reset after the switch was detected */
+	/* Assert then deassert RESET */
+	/*
+	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->reset)) {
+		dev_err(dev, "failed to get RESET GPIO\n");
+		return PTR_ERR(priv->reset);
+	}
+	msleep(REALTEK_SMI_HW_STOP_DELAY);

After all the renaming I'm surprised to see _SMI in the MDIO code now...

+	gpiod_set_value(priv->reset, 0);
+	msleep(REALTEK_SMI_HW_START_DELAY);
+	dev_info(dev, "deasserted RESET\n");
+	*/

Leftover debug? Nobody wants to see block commented code :-)

+
+	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
+
+	ret = priv->ops->detect(priv);
+	if (ret) {
+		dev_err(dev, "unable to detect switch\n");
+		return ret;
+	}
+
+	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
+	if (!priv->ds)
+		return -ENOMEM;
+
+	priv->ds->dev = dev;
+	priv->ds->num_ports = priv->num_ports;
+	priv->ds->priv = priv;
+	priv->ds->ops = var->ds_ops;
+
+	ret = dsa_register_switch(priv->ds);
+	if (ret) {
+		dev_err(priv->dev, "unable to register switch ret = %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void realtek_mdio_remove(struct mdio_device *mdiodev)
+{
+	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
+
+	if (!priv)
+		return;
+
+	dsa_unregister_switch(priv->ds);
+	//gpiod_set_value(smi->reset, 1);

More here.

+	dev_set_drvdata(&mdiodev->dev, NULL);
+}
+
+static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
+{
+	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
+
+	if (!priv)
+		return;
+
+        dsa_switch_shutdown(priv->ds);
+
+        dev_set_drvdata(&mdiodev->dev, NULL);
+}
+
+static const struct of_device_id realtek_mdio_of_match[] = {
+#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB)
+	{ .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, },
+#endif
+	/* FIXME: add support for RTL8366S and more */
+	{ .compatible = "realtek,rtl8366s", .data = NULL, },
+#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8367C)
+	{ .compatible = "realtek,rtl8365mb", .data = &rtl8367c_variant, },

Did you test on an RTL8365MB-VC with MDIO? I know the 66RB is not tested.

What's this for?

  	int 		 	(*setup_interface)(struct dsa_switch *ds);
  	int 			(*write_reg_noack)(struct realtek_priv *priv, u32 addr, u32 data);