[PATCH 0/4] fix barebox support for MDIO-connected Realtek Ethernet switches

mail archive of the barebox mailing list
 help / color / mirror / Atom feed

* [PATCH 0/4] fix barebox support for MDIO-connected Realtek Ethernet switches
@ 2023-12-22  2:07 Alvin Šipraga
  2023-12-22  2:07 ` [PATCH 1/4] net: fec_imx: reverse registration order of mdiobus and edev Alvin Šipraga
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Alvin Šipraga @ 2023-12-22  2:07 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX
  Cc: Vladimir Oltean, Luiz Angelo Daros de Luca, Alvin Šipraga,
	realtek-smi 30be0000.ethernet

While bringing up a new board with an MDIO-connected RTL8365MB, I had to
make some changes to barebox to get network boot to work. This small
series addresses all of the problems I encountered along the way.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
Alvin Šipraga (4):
      net: fec_imx: reverse registration order of mdiobus and edev
      net: dsa: realtek: fix support for MDIO-connected switches
      net: dsa: realtek: unify ds_ops
      net: mdio_bus: associate OF nodes with their devices

 drivers/net/fec_imx.c                  |  10 ++--
 drivers/net/phy/mdio_bus.c             |   4 ++
 drivers/net/realtek-dsa/realtek-mdio.c | 105 +++++++++++++++++++++++++--------
 drivers/net/realtek-dsa/realtek-smi.c  |   2 +-
 drivers/net/realtek-dsa/realtek.h      |   3 +-
 drivers/net/realtek-dsa/rtl8365mb.c    |  24 +-------
 drivers/net/realtek-dsa/rtl8366rb.c    |  23 +-------
 7 files changed, 97 insertions(+), 74 deletions(-)
---
base-commit: 934cb2146e5b34eb45c81e3f9bc9b63c95701e43
change-id: 20231222-realtek-mdio-fix-2672b2ef4ae6




^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/4] net: fec_imx: reverse registration order of mdiobus and edev
  2023-12-22  2:07 [PATCH 0/4] fix barebox support for MDIO-connected Realtek Ethernet switches Alvin Šipraga
@ 2023-12-22  2:07 ` Alvin Šipraga
  2023-12-24  9:27   ` Ahmad Fatoum
  2023-12-22  2:07 ` [PATCH 2/4] net: dsa: realtek: fix support for MDIO-connected switches Alvin Šipraga
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 7+ messages in thread
From: Alvin Šipraga @ 2023-12-22  2:07 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX
  Cc: Vladimir Oltean, Luiz Angelo Daros de Luca, Alvin Šipraga

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

This is necessary so that on systems with MDIO-connected Etheret
switches, DSA can find the master edev during switch registration.
Otherwise the switch setup will fail.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/net/fec_imx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 203a2a8aa191..75a65962820b 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -895,18 +895,18 @@ static int fec_probe(struct device *dev)
 	fec->miibus.priv = fec;
 	fec->miibus.parent = dev;
 
-	ret = mdiobus_register(&fec->miibus);
+	ret = eth_register(edev);
 	if (ret)
 		goto free_receive_packets;
 
-	ret = eth_register(edev);
+	ret = mdiobus_register(&fec->miibus);
 	if (ret)
-		goto unregister_mdio;
+		goto unregister_eth;
 
 	return 0;
 
-unregister_mdio:
-	mdiobus_unregister(&fec->miibus);
+unregister_eth:
+	eth_unregister(edev);
 free_receive_packets:
 	fec_free_receive_packets(fec, FEC_RBD_NUM, FEC_MAX_PKT_SIZE);
 free_xbd:

-- 
2.43.0




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/4] net: fec_imx: reverse registration order of mdiobus and edev
  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
  0 siblings, 0 replies; 7+ messages in thread
From: Ahmad Fatoum @ 2023-12-24  9:27 UTC (permalink / raw)
  To: Alvin Šipraga, Sascha Hauer, BAREBOX
  Cc: Vladimir Oltean, Luiz Angelo Daros de Luca, Alvin Šipraga

Hello Alvin,

On 22.12.23 03:07, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> This is necessary so that on systems with MDIO-connected Etheret
> switches, DSA can find the master edev during switch registration.
> Otherwise the switch setup will fail.

On the other hand, systems, where the PHY supplies a clock to the MAC
need the MDIO bus controller to be registered first, so the PHYs can
probe. In the special case of the FEC driver:

  - there's no struct eth_device::init function 
  - struct eth_device::get_ethaddr doesn't do hardware access


and operations in struct eth_device are only invoked when ->open is initially
called, which won't happen at driver probe time.

I thus think the change is ok, even if not generally applicable to all drivers.

Acked-by: Ahmad Fatoum <a.fatoum@pengutronix.de>


Thanks,
Ahmad

> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  drivers/net/fec_imx.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
> index 203a2a8aa191..75a65962820b 100644
> --- a/drivers/net/fec_imx.c
> +++ b/drivers/net/fec_imx.c
> @@ -895,18 +895,18 @@ static int fec_probe(struct device *dev)
>  	fec->miibus.priv = fec;
>  	fec->miibus.parent = dev;
>  
> -	ret = mdiobus_register(&fec->miibus);
> +	ret = eth_register(edev);
>  	if (ret)
>  		goto free_receive_packets;
>  
> -	ret = eth_register(edev);
> +	ret = mdiobus_register(&fec->miibus);
>  	if (ret)
> -		goto unregister_mdio;
> +		goto unregister_eth;
>  
>  	return 0;
>  
> -unregister_mdio:
> -	mdiobus_unregister(&fec->miibus);
> +unregister_eth:
> +	eth_unregister(edev);
>  free_receive_packets:
>  	fec_free_receive_packets(fec, FEC_RBD_NUM, FEC_MAX_PKT_SIZE);
>  free_xbd:
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |




^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/4] net: dsa: realtek: fix support for MDIO-connected switches
  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-22  2:07 ` Alvin Šipraga
  2023-12-22  2:07 ` [PATCH 3/4] net: dsa: realtek: unify ds_ops Alvin Šipraga
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Alvin Šipraga @ 2023-12-22  2:07 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX
  Cc: Vladimir Oltean, Luiz Angelo Daros de Luca, Alvin Šipraga

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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 3/4] net: dsa: realtek: unify ds_ops
  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-22  2:07 ` [PATCH 2/4] net: dsa: realtek: fix support for MDIO-connected switches Alvin Šipraga
@ 2023-12-22  2:07 ` 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
  4 siblings, 0 replies; 7+ messages in thread
From: Alvin Šipraga @ 2023-12-22  2:07 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX
  Cc: Vladimir Oltean, Luiz Angelo Daros de Luca, Alvin Šipraga

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

Now that neither interface driver requires .phy_read or .phy_write, the
ops are the same and some code can be deleted. No functional change.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/net/realtek-dsa/realtek-mdio.c |  2 +-
 drivers/net/realtek-dsa/realtek-smi.c  |  2 +-
 drivers/net/realtek-dsa/realtek.h      |  3 +--
 drivers/net/realtek-dsa/rtl8365mb.c    | 11 ++---------
 drivers/net/realtek-dsa/rtl8366rb.c    | 10 ++--------
 5 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/net/realtek-dsa/realtek-mdio.c b/drivers/net/realtek-dsa/realtek-mdio.c
index 2b8661b95677..4fc2295b1b5f 100644
--- a/drivers/net/realtek-dsa/realtek-mdio.c
+++ b/drivers/net/realtek-dsa/realtek-mdio.c
@@ -236,7 +236,7 @@ static int realtek_mdio_probe(struct phy_device *mdiodev)
 	priv->ds->dev = dev;
 	priv->ds->num_ports = priv->num_ports;
 	priv->ds->priv = priv;
-	priv->ds->ops = var->ds_ops_mdio;
+	priv->ds->ops = var->ds_ops;
 
 	ret = realtek_dsa_init_tagger(priv);
 	if (ret)
diff --git a/drivers/net/realtek-dsa/realtek-smi.c b/drivers/net/realtek-dsa/realtek-smi.c
index f93024ace516..da150dbc5d8b 100644
--- a/drivers/net/realtek-dsa/realtek-smi.c
+++ b/drivers/net/realtek-dsa/realtek-smi.c
@@ -450,7 +450,7 @@ static int realtek_smi_probe(struct device *dev)
 	priv->ds->dev = dev;
 	priv->ds->num_ports = priv->num_ports;
 	priv->ds->priv = priv;
-	priv->ds->ops = var->ds_ops_smi;
+	priv->ds->ops = var->ds_ops;
 
 	ret = realtek_dsa_init_tagger(priv);
 	if (ret)
diff --git a/drivers/net/realtek-dsa/realtek.h b/drivers/net/realtek-dsa/realtek.h
index ac84b18cdd14..dbca9494627a 100644
--- a/drivers/net/realtek-dsa/realtek.h
+++ b/drivers/net/realtek-dsa/realtek.h
@@ -69,8 +69,7 @@ struct realtek_ops {
 };
 
 struct realtek_variant {
-	const struct dsa_switch_ops *ds_ops_smi;
-	const struct dsa_switch_ops *ds_ops_mdio;
+	const struct dsa_switch_ops *ds_ops;
 	const struct realtek_ops *ops;
 	unsigned int clk_delay;
 	u8 cmd_read;
diff --git a/drivers/net/realtek-dsa/rtl8365mb.c b/drivers/net/realtek-dsa/rtl8365mb.c
index 0f4c471715d1..588998235827 100644
--- a/drivers/net/realtek-dsa/rtl8365mb.c
+++ b/drivers/net/realtek-dsa/rtl8365mb.c
@@ -1225,13 +1225,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 	return 0;
 }
 
-static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
-	.port_pre_enable = rtl8365mb_phylink_mac_config,
-	.port_disable = rtl8365mb_phylink_mac_link_down,
-	.port_enable = rtl8365mb_phylink_mac_link_up,
-};
-
-static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
+static const struct dsa_switch_ops rtl8365mb_switch_ops = {
 	.port_pre_enable = rtl8365mb_phylink_mac_config,
 	.port_disable = rtl8365mb_phylink_mac_link_down,
 	.port_enable = rtl8365mb_phylink_mac_link_up,
@@ -1247,8 +1241,7 @@ static const struct realtek_ops rtl8365mb_ops = {
 };
 
 const struct realtek_variant rtl8365mb_variant = {
-	.ds_ops_smi = &rtl8365mb_switch_ops_smi,
-	.ds_ops_mdio = &rtl8365mb_switch_ops_mdio,
+	.ds_ops = &rtl8365mb_switch_ops,
 	.ops = &rtl8365mb_ops,
 	.clk_delay = 10,
 	.cmd_read = 0xb9,
diff --git a/drivers/net/realtek-dsa/rtl8366rb.c b/drivers/net/realtek-dsa/rtl8366rb.c
index 6fd2b1852159..35028d319ecd 100644
--- a/drivers/net/realtek-dsa/rtl8366rb.c
+++ b/drivers/net/realtek-dsa/rtl8366rb.c
@@ -1079,12 +1079,7 @@ static int rtl8366rb_detect(struct realtek_priv *priv)
 	return rtl8366rb_reset_chip(priv);
 }
 
-static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = {
-	.port_enable = rtl8366rb_port_enable,
-	.port_disable = rtl8366rb_port_disable,
-};
-
-static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = {
+static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.port_enable = rtl8366rb_port_enable,
 	.port_disable = rtl8366rb_port_disable,
 };
@@ -1098,8 +1093,7 @@ static const struct realtek_ops rtl8366rb_ops = {
 };
 
 const struct realtek_variant rtl8366rb_variant = {
-	.ds_ops_smi = &rtl8366rb_switch_ops_smi,
-	.ds_ops_mdio = &rtl8366rb_switch_ops_mdio,
+	.ds_ops = &rtl8366rb_switch_ops,
 	.ops = &rtl8366rb_ops,
 	.clk_delay = 10,
 	.cmd_read = 0xa9,

-- 
2.43.0




^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 4/4] net: mdio_bus: associate OF nodes with their devices
  2023-12-22  2:07 [PATCH 0/4] fix barebox support for MDIO-connected Realtek Ethernet switches Alvin Šipraga
                   ` (2 preceding siblings ...)
  2023-12-22  2:07 ` [PATCH 3/4] net: dsa: realtek: unify ds_ops Alvin Šipraga
@ 2023-12-22  2:07 ` Alvin Šipraga
  2024-01-02  9:01 ` [PATCH 0/4] fix barebox support for MDIO-connected Realtek Ethernet switches Sascha Hauer
  4 siblings, 0 replies; 7+ messages in thread
From: Alvin Šipraga @ 2023-12-22  2:07 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX
  Cc: Vladimir Oltean, Luiz Angelo Daros de Luca, Alvin Šipraga,
	realtek-smi 30be0000.ethernet

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

barebox deep-probe will walk the device tree to ensure dependent devices
have been probed. In so doing, it uses the device_node::dev pointer to
check whether a given node has a device; if not, a device is created on
demand. The behaviour is recursive, so parent nodes without an
associated device will also have devices created on their behalf. The
recursion stops when a parent with a device is found.

Weird behaviour can ensure if, when devices with a device_node are
registered, the device_node::dev field is not populated. This patch
addresses a niche, benign, but also noisy error observed as a result of
this behaviour.

One mostly harmless consequence is that spurious devices can get created
when a suitable one already exists. In my case, I have an MDIO-connected
Ethernet switch with an internal MDIO bus and some internal PHYs. With
deep-probe, but without these changes, the devinfo output looks as
follows:

  `-- 30be0000.ethernet@30be0000.of
     `-- eth0
     `-- miibus0
        `-- mdio0-dev1d
           `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio0-phy1d
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@6.of
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@0.of
              `-- eth1
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@1.of
              `-- eth2
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@2.of
              `-- eth3
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@3.of
              `-- eth4
           `-- miibus1
              `-- mdio1-phy00
                 `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio1-phy00
              `-- mdio1-phy01
                 `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio1-phy01
              `-- mdio1-phy02
                 `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio1-phy02
              `-- mdio1-phy03
                 `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio1-phy03
     `-- 30be0000.ethernet@30be0000:mdio.of
        `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:mdio.of

Notice the last three devices with generic names; they are spurious
creations of the deep-probe algorithm. In fact, the devices have already
been created:

  real dev        spurious dev
  --------        ------------
  miibus0         30be0000.ethernet@30be0000:mdio.of
  mdio0-devid1d   30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of
  miibus1         30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:mdio.of

The only reason there aren't even more spurious devices created is that
deep-probe stopped at 30be0000.ethernet@30be0000.of, a platform device
whose device_node had its dev field populated correctly. But these
so-called real devices are all created by mdio_bus, which only links one
way, i.e. sets device::of_node, but not device_node::dev.

This issue probably goes unnoticed on most boards because, while the
call to of_device_ensure_probed() returns -ENODEV on the bottom listed
node, the code in phy.c doesn't check the return code, and the real
devices have already been probed, so no harm is done.

I observed much more surprising behaviour on my board because the switch
I am using, an RTL8365MB, has multiple possible management interfaces,
for which barebox has two drivers: realtek-smi and realtek-mdio (for
SMI- and MDIO-connected switches, respectively). The compatible strings
are the same, "realtek,rtl8365mb", but the bus types are different: the
former is a platform driver, the latter a phy (read: mdio) driver. In my
bootloader I have both drivers built in, because some of my boards use
SMI while others use MDIO.

When I would run the command 'boot bnet', bringing up my network
interface, DSA would call phy_device_connect() to connect my edev with
its corresponding phy, and a deep-probe would kick off because the phy's
parent's device_node::dev field was not populated. And during the
creation of the spurious device for my (already probed) Ethernet switch,
the platform code would find a compatible string match with realtek-smi
and try to probe the device with that driver:

  Booting entry 'bnet'
  ERROR: gpiolib: _gpio_request: gpio-233 (30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of-reset) status -16
  ERROR: realtek-smi 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of: error Device or resource busy: failed to get RESET GPIO
  ERROR: realtek-smi 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of: probe failed: Device or resource busy

As stated above, my switch is connected over MDIO, not SMI, so I really
should not be seeing any "realtek-smi" in my log. Nevertheless, because
phy.c doesn't check for errors, the errors are benign and my network
boot runs just fine.

Dumping the stack around the above error print in the realtek-smi driver
reveals exactly why things are going wrong:

  ERROR: gpiolib: _gpio_request: gpio-233 (30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of-reset) status -16
  Call trace:
  [<7dd8f400>] (unwind_backtrace+0x0/0x90)
  [<7dd8f4a0>] (dump_stack+0x10/0x18)
  [<7dd2d4d4>] (realtek_smi_probe+0x16c/0x2f4)
  [<7dd1da28>] (platform_probe+0x48/0x4c)
  [<7dd1cf24>] (device_probe+0x80/0x13c)
  [<7dd1d028>] (match+0x48/0x58)
  [<7dd1d3c8>] (register_device+0x178/0x184)
  [<7dd1da80>] (platform_device_register+0x18/0x20)
  [<7dd5501c>] (of_platform_device_create+0x2b8/0x2e0)
  [<7dd55170>] (of_device_create_on_demand+0x84/0xc0)
  [<7dd5513c>] (of_device_create_on_demand+0x50/0xc0)
  [<7dd552e0>] (of_device_ensure_probed+0x40/0x6c)
  [<7dd21398>] (phy_device_connect+0x174/0x230)
  [<7dd1fd74>] (dsa_port_start+0x60/0x1f0)
  [<7dd7a0e4>] (eth_open+0x24/0x44)
  [<7dd7d7e4>] (ifup_edev+0x9c/0x2ec)
  [<7dd7dbe4>] (ifup_all+0x110/0x204)
  [<7dd7dd2c>] (do_ifup+0x54/0xd4)
  [<7dd09e78>] (execute_command+0x44/0x8c)
  ...

The whole mess is rectified if the proper linking is done by mdio_bus,
since it is responsible for creating all of the devices to begin with.
So, make sure that the device_node::dev field is populated in all cases
there.

Note that while I also make this change in of_mdiobus_register_phy(), I
did not actually observe any unwanted behaviour by not doing so - the
problem above is specifically because it's not done in
of_mdiobus_register() or of_mdiobus_register_device(). But this is only
because of the way phys are looked up to begin with, so it seemed
reasonable to do it for them as well.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/net/phy/mdio_bus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 332db6c05b11..94123ef6141c 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -119,6 +119,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 	 * Associate the OF node with the device structure so it
 	 * can be looked up later
 	 */
+	child->dev = &phy->dev;
 	phy->dev.of_node = child;

 	/*
@@ -145,6 +146,7 @@ static int of_mdiobus_register_device(struct mii_bus *mdio,
 	if (IS_ERR(mdiodev))
 		return PTR_ERR(mdiodev);

+	child->dev = &mdiodev->dev;
 	mdiodev->dev.of_node = child;

 	ret = mdio_register_device(mdiodev);
@@ -315,6 +317,8 @@ int mdiobus_register(struct mii_bus *bus)
 	pr_info("%s: probed\n", dev_name(&bus->dev));

 	if (bus->dev.of_node) {
+		bus->dev.of_node->dev = &bus->dev;
+
 		/* Register PHY's as child node to mdio node */
 		of_mdiobus_register(bus, bus->dev.of_node);
 	}

-- 
2.43.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/4] fix barebox support for MDIO-connected Realtek Ethernet switches
  2023-12-22  2:07 [PATCH 0/4] fix barebox support for MDIO-connected Realtek Ethernet switches Alvin Šipraga
                   ` (3 preceding siblings ...)
  2023-12-22  2:07 ` [PATCH 4/4] net: mdio_bus: associate OF nodes with their devices Alvin Šipraga
@ 2024-01-02  9:01 ` Sascha Hauer
  4 siblings, 0 replies; 7+ messages in thread
From: Sascha Hauer @ 2024-01-02  9:01 UTC (permalink / raw)
  To: Alvin Šipraga
  Cc: BAREBOX, Vladimir Oltean, Luiz Angelo Daros de Luca,
	Alvin Šipraga, realtek-smi30be0000.ethernet

On Fri, Dec 22, 2023 at 03:07:24AM +0100, Alvin Šipraga wrote:
> While bringing up a new board with an MDIO-connected RTL8365MB, I had to
> make some changes to barebox to get network boot to work. This small
> series addresses all of the problems I encountered along the way.
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
> Alvin Šipraga (4):
>       net: fec_imx: reverse registration order of mdiobus and edev
>       net: dsa: realtek: fix support for MDIO-connected switches
>       net: dsa: realtek: unify ds_ops
>       net: mdio_bus: associate OF nodes with their devices

Applied, thanks

Sascha

> 
>  drivers/net/fec_imx.c                  |  10 ++--
>  drivers/net/phy/mdio_bus.c             |   4 ++
>  drivers/net/realtek-dsa/realtek-mdio.c | 105 +++++++++++++++++++++++++--------
>  drivers/net/realtek-dsa/realtek-smi.c  |   2 +-
>  drivers/net/realtek-dsa/realtek.h      |   3 +-
>  drivers/net/realtek-dsa/rtl8365mb.c    |  24 +-------
>  drivers/net/realtek-dsa/rtl8366rb.c    |  23 +-------
>  7 files changed, 97 insertions(+), 74 deletions(-)
> ---
> base-commit: 934cb2146e5b34eb45c81e3f9bc9b63c95701e43
> change-id: 20231222-realtek-mdio-fix-2672b2ef4ae6
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-01-02  9:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` [PATCH 2/4] net: dsa: realtek: fix support for MDIO-connected switches Alvin Šipraga
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox