mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: "Alvin Šipraga" <alvin@pqrs.dk>
To: Sascha Hauer <s.hauer@pengutronix.de>,
	 BAREBOX <barebox@lists.infradead.org>
Cc: "Vladimir Oltean" <olteanv@gmail.com>,
	"Luiz Angelo Daros de Luca" <luizluca@gmail.com>,
	"Alvin Šipraga" <alsi@bang-olufsen.dk>
Subject: [PATCH 2/4] net: dsa: realtek: fix support for MDIO-connected switches
Date: Fri, 22 Dec 2023 03:07:26 +0100	[thread overview]
Message-ID: <20231222-realtek-mdio-fix-v1-2-6bbf80d6a756@bang-olufsen.dk> (raw)
In-Reply-To: <20231222-realtek-mdio-fix-v1-0-6bbf80d6a756@bang-olufsen.dk>

From: Alvin Šipraga <alsi@bang-olufsen.dk>

DSA offers drivers the option to have the core register an MDIO bus,
intended for platforms which do not have a specific OF node for the MDIO
bus or corresponding phy-handle properties on the switch port nodes.
This logic works OK, but is incidentally broken in barebox because the
dsa_switch::phys_mii_mask field is not being set. That means all reads
and writes to the internal PHYs are dropped before anything goes out on
the bus.

Notwithstanding that barebox issue, there is an ongoing discussion [1]
on the Linux mailing list as to the virtues (or lack thereof) of
realtek-mdio opting into this core functionality to begin with. The fact
is that, for SMI-connected switches, realtek-smi absolutely expects an
MDIO bus node. And the device tree bindings strongly (albeit not
strictly) assert that this node for both MDIO and SMI connected
switches. In the specific case of barebox, the driver likely has even
fewer users, and realtek-mdio is currently broken, so there is nothing
to break by strictly enforcing the presence of an MDIO node.

The same discussion [1] centers around an effort to actually unify
large, common parts of the SMI and MDIO drivers, and ditching the
asymmetric MDIO bus registration would make this effort even easier.

To that end, add a little bit more code in order to bury this particular
issue once and for all in barebox. Further cleanup can be done to reduce
the overall quantity of code, probably following whatever ends up in
Linux.

A slice dependency is also added to the parent MDIO bus to prevent the
PHY status polling from interleaving MDIO accesses during switch
management.

Link: https://lore.kernel.org/all/20231221174746.hylsmr3f7g5byrsi@skbuf/ [1]
Fixes: f9f31a9a21e4 ("net: dsa: add Realtek (rtl8365mb/rtl8366rb) switch support")
Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/net/realtek-dsa/realtek-mdio.c | 103 ++++++++++++++++++++++++++-------
 drivers/net/realtek-dsa/rtl8365mb.c    |  13 -----
 drivers/net/realtek-dsa/rtl8366rb.c    |  13 -----
 3 files changed, 81 insertions(+), 48 deletions(-)

diff --git a/drivers/net/realtek-dsa/realtek-mdio.c b/drivers/net/realtek-dsa/realtek-mdio.c
index 463757711198..2b8661b95677 100644
--- a/drivers/net/realtek-dsa/realtek-mdio.c
+++ b/drivers/net/realtek-dsa/realtek-mdio.c
@@ -47,22 +47,27 @@ static int realtek_mdio_write(void *ctx, u32 reg, u32 val)
 	struct mii_bus *bus = priv->bus;
 	int ret;
 
-	ret = bus->write(bus, priv->mdio_addr, REALTEK_MDIO_CTRL0_REG, REALTEK_MDIO_ADDR_OP);
+	ret = mdiobus_write(bus, priv->mdio_addr, REALTEK_MDIO_CTRL0_REG,
+			    REALTEK_MDIO_ADDR_OP);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
-	ret = bus->write(bus, priv->mdio_addr, REALTEK_MDIO_ADDRESS_REG, reg);
+	ret = mdiobus_write(bus, priv->mdio_addr, REALTEK_MDIO_ADDRESS_REG,
+			    reg);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
-	ret = bus->write(bus, priv->mdio_addr, REALTEK_MDIO_DATA_WRITE_REG, val);
+	ret = mdiobus_write(bus, priv->mdio_addr, REALTEK_MDIO_DATA_WRITE_REG,
+			    val);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
-	ret = bus->write(bus, priv->mdio_addr, REALTEK_MDIO_CTRL1_REG, REALTEK_MDIO_WRITE_OP);
+	ret = mdiobus_write(bus, priv->mdio_addr, REALTEK_MDIO_CTRL1_REG,
+			    REALTEK_MDIO_WRITE_OP);
+	if (ret)
+		return ret;
 
-out_unlock:
-	return ret;
+	return 0;
 }
 
 static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
@@ -71,26 +76,43 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
 	struct mii_bus *bus = priv->bus;
 	int ret;
 
-	ret = bus->write(bus, priv->mdio_addr, REALTEK_MDIO_CTRL0_REG, REALTEK_MDIO_ADDR_OP);
+	ret = mdiobus_write(bus, priv->mdio_addr, REALTEK_MDIO_CTRL0_REG,
+			    REALTEK_MDIO_ADDR_OP);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
-	ret = bus->write(bus, priv->mdio_addr, REALTEK_MDIO_ADDRESS_REG, reg);
+	ret = mdiobus_write(bus, priv->mdio_addr, REALTEK_MDIO_ADDRESS_REG,
+			    reg);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
-	ret = bus->write(bus, priv->mdio_addr, REALTEK_MDIO_CTRL1_REG, REALTEK_MDIO_READ_OP);
+	ret = mdiobus_write(bus, priv->mdio_addr, REALTEK_MDIO_CTRL1_REG,
+			    REALTEK_MDIO_READ_OP);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
-	ret = bus->read(bus, priv->mdio_addr, REALTEK_MDIO_DATA_READ_REG);
-	if (ret >= 0) {
-		*val = ret;
-		ret = 0;
-	}
+	ret = mdiobus_read(bus, priv->mdio_addr, REALTEK_MDIO_DATA_READ_REG);
+	if (ret < 0)
+		return ret;
 
-out_unlock:
-	return ret;
+	*val = ret;
+
+	return 0;
+}
+
+static int realtek_mdio_mdio_write(struct mii_bus *bus, int addr, int regnum,
+				   u16 val)
+{
+	struct realtek_priv *priv = bus->priv;
+
+	return priv->ops->phy_write(priv, addr, regnum, val);
+}
+
+static int realtek_mdio_mdio_read(struct mii_bus *bus, int addr, int regnum)
+{
+	struct realtek_priv *priv = bus->priv;
+
+	return priv->ops->phy_read(priv, addr, regnum);
 }
 
 static const struct regmap_config realtek_mdio_regmap_config = {
@@ -107,6 +129,42 @@ static const struct regmap_bus realtek_mdio_regmap_bus = {
 	.reg_read = realtek_mdio_read,
 };
 
+static int realtek_mdio_setup_mdio(struct dsa_switch *ds)
+{
+	struct realtek_priv *priv =  ds->priv;
+	struct device_node *np;
+	int ret;
+
+	np = of_get_child_by_name(priv->dev->of_node, "mdio");
+	if (!np) {
+		dev_err(priv->dev, "missing 'mdio' child node\n");
+		return -ENODEV;
+	}
+
+	priv->slave_mii_bus->priv = priv;
+	priv->slave_mii_bus->read = realtek_mdio_mdio_read;
+	priv->slave_mii_bus->write = realtek_mdio_mdio_write;
+	priv->slave_mii_bus->dev.of_node = np;
+	priv->slave_mii_bus->parent = priv->dev;
+
+	ret = mdiobus_register(priv->slave_mii_bus);
+	if (ret) {
+		dev_err(priv->dev, "unable to register MDIO bus %pOF\n", np);
+		goto err_put_node;
+	}
+
+	/* Avoid interleaved MDIO access during PHY status polling */
+	slice_depends_on(mdiobus_slice(priv->slave_mii_bus),
+			 mdiobus_slice(priv->bus));
+
+	return 0;
+
+err_put_node:
+	of_node_put(np);
+
+	return ret;
+}
+
 static int realtek_mdio_probe(struct phy_device *mdiodev)
 {
 	struct realtek_priv *priv;
@@ -142,6 +200,7 @@ static int realtek_mdio_probe(struct phy_device *mdiodev)
 	priv->cmd_write = var->cmd_write;
 	priv->ops = var->ops;
 
+	priv->setup_interface = realtek_mdio_setup_mdio;
 	priv->write_reg_noack = realtek_mdio_write;
 
 	np = dev->of_node;
diff --git a/drivers/net/realtek-dsa/rtl8365mb.c b/drivers/net/realtek-dsa/rtl8365mb.c
index 700773ffa657..0f4c471715d1 100644
--- a/drivers/net/realtek-dsa/rtl8365mb.c
+++ b/drivers/net/realtek-dsa/rtl8365mb.c
@@ -649,17 +649,6 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 	return 0;
 }
 
-static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum)
-{
-	return rtl8365mb_phy_read(ds->priv, phy, regnum);
-}
-
-static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum,
-				   u16 val)
-{
-	return rtl8365mb_phy_write(ds->priv, phy, regnum, val);
-}
-
 static const struct rtl8365mb_extint *
 rtl8365mb_get_port_extint(struct realtek_priv *priv, int port)
 {
@@ -1246,8 +1235,6 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
 	.port_pre_enable = rtl8365mb_phylink_mac_config,
 	.port_disable = rtl8365mb_phylink_mac_link_down,
 	.port_enable = rtl8365mb_phylink_mac_link_up,
-	.phy_read = rtl8365mb_dsa_phy_read,
-	.phy_write = rtl8365mb_dsa_phy_write,
 };
 
 static const struct realtek_ops rtl8365mb_ops = {
diff --git a/drivers/net/realtek-dsa/rtl8366rb.c b/drivers/net/realtek-dsa/rtl8366rb.c
index 26f036dfe570..6fd2b1852159 100644
--- a/drivers/net/realtek-dsa/rtl8366rb.c
+++ b/drivers/net/realtek-dsa/rtl8366rb.c
@@ -1021,17 +1021,6 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 	return ret;
 }
 
-static int rtl8366rb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum)
-{
-	return rtl8366rb_phy_read(ds->priv, phy, regnum);
-}
-
-static int rtl8366rb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum,
-				   u16 val)
-{
-	return rtl8366rb_phy_write(ds->priv, phy, regnum, val);
-}
-
 static int rtl8366rb_reset_chip(struct realtek_priv *priv)
 {
 	int timeout = 10;
@@ -1096,8 +1085,6 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = {
 };
 
 static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = {
-	.phy_read = rtl8366rb_dsa_phy_read,
-	.phy_write = rtl8366rb_dsa_phy_write,
 	.port_enable = rtl8366rb_port_enable,
 	.port_disable = rtl8366rb_port_disable,
 };

-- 
2.43.0




  parent reply	other threads:[~2023-12-22  2:09 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-22  2:07 [PATCH 0/4] fix barebox support for MDIO-connected Realtek Ethernet switches Alvin Šipraga
2023-12-22  2:07 ` [PATCH 1/4] net: fec_imx: reverse registration order of mdiobus and edev Alvin Šipraga
2023-12-24  9:27   ` Ahmad Fatoum
2023-12-22  2:07 ` Alvin Šipraga [this message]
2023-12-22  2:07 ` [PATCH 3/4] net: dsa: realtek: unify ds_ops Alvin Šipraga
2023-12-22  2:07 ` [PATCH 4/4] net: mdio_bus: associate OF nodes with their devices Alvin Šipraga
2024-01-02  9:01 ` [PATCH 0/4] fix barebox support for MDIO-connected Realtek Ethernet switches Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231222-realtek-mdio-fix-v1-2-6bbf80d6a756@bang-olufsen.dk \
    --to=alvin@pqrs.dk \
    --cc=alsi@bang-olufsen.dk \
    --cc=barebox@lists.infradead.org \
    --cc=luizluca@gmail.com \
    --cc=olteanv@gmail.com \
    --cc=s.hauer@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox