[PATCH net] net: stmmac: dwmac-sun8i: reset the EMAC when opening, not when probing

[PATCH net] net: stmmac: dwmac-sun8i: reset the EMAC when opening, not when probing

[Pedro Santos]

sun8i_dwmac_reset() asserts EMAC_BASIC_CTL1.SOFT_RST and polls for the
hardware to clear it. That bit only clears once the MAC has a running
receive clock, which on external-PHY boards is driven by the PHY.

Bringing the interface down powers the PHY down: phy_detach() calls
phy_suspend(), which for a PHY without wake-on-LAN ends in BMCR_PDOWN.
Boards that wire no reset line to their PHY, and share its supply with
other always-on consumers, have nothing that undoes that. On the Orange
Pi Zero 3 the Motorcomm YT8531 reset is, in the words of the board's
upstream author, "hardwired via a simple RC circuit, so there is no
GPIO", and phy-supply points at a regulator-always-on rail shared with
four GPIO banks and the SD card.

So after a warm reboot the PHY comes back still powered down. Probe
asserts SOFT_RST, no receive clock arrives, and the reset never
completes. Read off an affected board at boot, before anything touched
the PHY: BMCR 0x1800 (PDOWN set) and EMAC_BASIC_CTL1 0x08000001, still
set after polling for ten seconds -- so raising the 100 ms timeout does
not help. Probe fails with -ETIMEDOUT and the interface never appears.

That the interface teardown is what does it can be shown directly. A
reboot via sysrq-b, which skips both the ifdown and device_shutdown(),
comes up with PDOWN clear and resets fine; taking the interface down
first and then using sysrq-b -- so the driver's own shutdown path still
never runs -- reproduces the failure.

The driver already has the right place for the reset.
sun8i_dwmac_dma_reset() is registered as stmmac_dma_ops->reset and is
documented as "reset the EMAC", but only zeroes a few registers. stmmac
calls it from stmmac_init_dma_engine(), under stmmac_hw_setup(), whose
only two callers are __stmmac_open() and stmmac_resume() -- both of which
run after stmmac_init_phy() has attached and resumed the PHY. Doing the
soft reset there means the receive clock is running by construction, for
every PHY, whether or not a PHY driver is bound, without the MAC driver
reaching into phylib.

sun8i is the only stmmac variant that soft-resets at probe rather than in
the reset hook; this brings it into line with the others.

sun8i_dwmac_reset() stays for the mdio-mux switch callback, which needs a
reset after changing the syscon and cannot use the hook.

Tested on an Orange Pi Zero 3 (H618, YT8531, rgmii-rxid). Five warm
reboots: no EMAC reset timeout, interface up at 1Gbps each time. One cold
boot with power physically cycled, to cover the path this moves for
boards that never hit the bug: same result. 20000 and 5000 1472-byte
frames respectively, no loss, every MAC error counter at zero.

Fixes: 9f93ac8d4085 ("net-next: stmmac: Add dwmac-sun8i")
Signed-off-by: Pedro Santos <hartmnn.p@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 49 ++++++++++++-------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 48c52eb..748ebab 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -272,8 +272,34 @@ static const struct emac_variant emac_variant_h6 = {
 /* sun8i_dwmac_dma_reset() - reset the EMAC
  * Called from stmmac via stmmac_dma_ops->reset
  */
+static int sun8i_dwmac_soft_reset(void __iomem *ioaddr)
+{
+	u32 v;
+
+	v = readl(ioaddr + EMAC_BASIC_CTL1);
+	writel(v | 0x01, ioaddr + EMAC_BASIC_CTL1);
+
+	/* The timeout was previously set to 10ms, but some board (OrangePI0)
+	 * need more if no cable plugged. 100ms seems OK
+	 */
+	return readl_poll_timeout(ioaddr + EMAC_BASIC_CTL1, v,
+				  !(v & 0x01), 100, 100000);
+}
+
 static int sun8i_dwmac_dma_reset(void __iomem *ioaddr)
 {
+	int err;
+
+	/* The MAC soft reset only completes once the PHY is driving the RX
+	 * clock. Doing it here rather than at probe means phylib has already
+	 * attached and resumed the PHY, so the clock is running by
+	 * construction -- including after a warm reboot that left the PHY
+	 * powered down.
+	 */
+	err = sun8i_dwmac_soft_reset(ioaddr);
+	if (err)
+		return err;
+
 	writel(0, ioaddr + EMAC_RX_CTL1);
 	writel(0, ioaddr + EMAC_TX_CTL1);
 	writel(0, ioaddr + EMAC_RX_FRM_FLT);
@@ -740,23 +766,12 @@ static void sun8i_dwmac_flow_ctrl(struct mac_device_info *hw,
 
 static int sun8i_dwmac_reset(struct stmmac_priv *priv)
 {
-	u32 v;
-	int err;
-
-	v = readl(priv->ioaddr + EMAC_BASIC_CTL1);
-	writel(v | 0x01, priv->ioaddr + EMAC_BASIC_CTL1);
+	int err = sun8i_dwmac_soft_reset(priv->ioaddr);
 
-	/* The timeout was previously set to 10ms, but some board (OrangePI0)
-	 * need more if no cable plugged. 100ms seems OK
-	 */
-	err = readl_poll_timeout(priv->ioaddr + EMAC_BASIC_CTL1, v,
-				 !(v & 0x01), 100, 100000);
-
-	if (err) {
+	if (err)
 		dev_err(priv->device, "EMAC reset timeout\n");
-		return err;
-	}
-	return 0;
+
+	return err;
 }
 
 /* Search in mdio-mux node for internal PHY node and get its clk/reset */
@@ -1217,10 +1232,6 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "Failed to register mux\n");
 			goto dwmac_mux;
 		}
-	} else {
-		ret = sun8i_dwmac_reset(priv);
-		if (ret)
-			goto dwmac_remove;
 	}
 
 	pm_runtime_put(&pdev->dev);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH net] net: stmmac: dwmac-sun8i: reset the EMAC when opening, not when probing

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: stmmac: dwmac-sun8i: reset the EMAC when opening, not when probing

The EMAC soft reset is moved out of sun8i_dwmac_probe() and into the
stmmac_dma_ops->reset hook, so it runs from stmmac_init_dma_engine()
instead of at probe time.  A new helper sun8i_dwmac_soft_reset() holds the
assert-and-poll sequence, and sun8i_dwmac_reset() is kept as a thin
wrapper for the mdio-mux switch callback.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index 48c52eb96233..748ebabd933c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -272,8 +272,34 @@ static const struct emac_variant emac_variant_h6 = {
>  /* sun8i_dwmac_dma_reset() - reset the EMAC
>   * Called from stmmac via stmmac_dma_ops->reset
>   */
> +static int sun8i_dwmac_soft_reset(void __iomem *ioaddr)

[Severity: Low]
This isn't a bug, but the existing header block now sits above the wrong
function.  Neither statement in it holds for sun8i_dwmac_soft_reset():
the ops table binds .reset = sun8i_dwmac_dma_reset, and the new helper
has a second caller, sun8i_dwmac_reset(), used by the mdio-mux switch
callback.  sun8i_dwmac_dma_reset() is then the only dma ops callback in
this file without a header comment naming it, unlike
sun8i_dwmac_dma_init() just below.  Should the comment move down with the
function it describes?

> +{
> +	u32 v;
> +
> +	v = readl(ioaddr + EMAC_BASIC_CTL1);
> +	writel(v | 0x01, ioaddr + EMAC_BASIC_CTL1);
> +
> +	/* The timeout was previously set to 10ms, but some board (OrangePI0)
> +	 * need more if no cable plugged. 100ms seems OK
> +	 */
> +	return readl_poll_timeout(ioaddr + EMAC_BASIC_CTL1, v,
> +				  !(v & 0x01), 100, 100000);
> +}

[Severity: Medium]
Is 100 ms still enough in the new context?  The comment above the poll
was written for a probe-time reset, where the PHY had been powered and
clocking since boot.

In the new call site the PHY may have left BMCR_PDOWN only microseconds
earlier.  __stmmac_open() and stmmac_resume() both do:

	phylink_prepare_resume(priv->phylink);
	...
	ret = stmmac_hw_setup(dev);

and phylink_prepare_resume() itself says:

	/* IEEE 802.3 22.2.4.1.5 allows PHYs to stop their receive clock
	 * when PDOWN is set. ... Note that 802.3 allows PHYs 500ms before
	 * the clock meets requirements. We do not implement this delay.
	 */
	if (pl->config->mac_requires_rxc && phydev && phydev->suspended)
		phy_resume(phydev);

Only stmmac_reset_queues_param() and descriptor clearing sit between
phy_resume() and stmmac_init_dma_engine() -> sun8i_dwmac_dma_reset(), so
on a PHY that is slow to restart RXC after power-down the poll can expire
and stmmac_hw_setup() returns -ETIMEDOUT on ifup, MTU change and system
resume.

The other stmmac variants that soft-reset in this hook allow more
headroom: 200 ms in dwmac_lib.c, 1 s in dwmac4_lib.c and dwmac-imx.c, 2 s
in dwmac-loongson.c.  Would raising the budget here be reasonable?

>  static int sun8i_dwmac_dma_reset(void __iomem *ioaddr)
>  {
> +	int err;
> +
> +	/* The MAC soft reset only completes once the PHY is driving the RX
> +	 * clock. Doing it here rather than at probe means phylib has already
> +	 * attached and resumed the PHY, so the clock is running by
> +	 * construction -- including after a warm reboot that left the PHY
> +	 * powered down.
> +	 */

[Severity: Medium]
Is the stated precondition accurate for all callers of the hook?  The
commit message says:

  "stmmac calls it from stmmac_init_dma_engine(), under stmmac_hw_setup(),
   whose only two callers are __stmmac_open() and stmmac_resume() -- both
   of which run after stmmac_init_phy() has attached and resumed the PHY."

stmmac_resume() does not call stmmac_init_phy() at all, and __stmmac_open()
is also reached from stmmac_change_mtu() and the XDP re-open path without
going through stmmac_open() -> stmmac_init_phy().

What actually guarantees a running receive clock before stmmac_hw_setup()
on those paths is phylink_prepare_resume(), added by commit 06232cb44bc8e
three weeks before this patch:

	phylink_prepare_resume(priv->phylink);

	stmmac_reset_queues_param(priv);

	ret = stmmac_hw_setup(dev);

In the current tree the behaviour is fine, since stmmac sets
mac_requires_rxc unconditionally, so phylink_prepare_resume() does resume
a suspended PHY first.  But the patch carries Fixes: 9f93ac8d4085 (2017).
In a stable tree without 06232cb44bc8e, would sun8i_dwmac_dma_reset()
assert SOFT_RST with an external PHY still in BMCR_PDOWN on every resume
and every MTU change, turning a probe-time failure into a resume and
MTU-change failure?

Could the commit message name phylink_prepare_resume() as the actual
dependency, and note it explicitly for backporters?

> +	err = sun8i_dwmac_soft_reset(ioaddr);
> +	if (err)
> +		return err;
> +
>  	writel(0, ioaddr + EMAC_RX_CTL1);
>  	writel(0, ioaddr + EMAC_TX_CTL1);
>  	writel(0, ioaddr + EMAC_RX_FRM_FLT);
> @@ -740,23 +766,12 @@ static void sun8i_dwmac_flow_ctrl(struct mac_device_info *hw,
>  
>  static int sun8i_dwmac_reset(struct stmmac_priv *priv)
>  {
> -	u32 v;
> -	int err;
> -
> -	v = readl(priv->ioaddr + EMAC_BASIC_CTL1);
> -	writel(v | 0x01, priv->ioaddr + EMAC_BASIC_CTL1);
> +	int err = sun8i_dwmac_soft_reset(priv->ioaddr);

[Severity: Medium]
This isn't a problem introduced by this patch -- the mdio-mux reset call
site predates it -- but does the failure mode survive there for the
H3/V3s variants, the only ones with soc_has_internal_phy = true?

The commit message mentions the retained caller:

  "sun8i_dwmac_reset() stays for the mdio-mux switch callback, which needs
   a reset after changing the syscon and cannot use the hook."

That callback still runs at probe time.  mdio_mux_read() invokes the
switch callback before forwarding the first MDIO transfer:

drivers/net/mdio/mdio-mux.c:mdio_mux_read() {
	...
	r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
	if (r)
		goto out;
	...
}

so during sun8i_dwmac_probe() -> sun8i_dwmac_register_mdio_mux() ->
mdio_mux_init() -> of_mdiobus_register(), the callback runs with
current_child == -1, selects the external child, unpowers the internal
PHY and calls sun8i_dwmac_reset() -> sun8i_dwmac_soft_reset() before
phylib has attached or resumed the external PHY.

On a board whose external PHY came back from a warm reboot in BMCR_PDOWN
-- the scenario in the commit message -- would the poll return -ETIMEDOUT,
switch_fn fail, mdio_mux_read() abort, and the external PHY never be
discovered?

The new comment in sun8i_dwmac_dma_reset() states that phylib has already
attached and resumed the PHY, which this second caller of the same helper
does not honour.

>  
> -	/* The timeout was previously set to 10ms, but some board (OrangePI0)
> -	 * need more if no cable plugged. 100ms seems OK
> -	 */
> -	err = readl_poll_timeout(priv->ioaddr + EMAC_BASIC_CTL1, v,
> -				 !(v & 0x01), 100, 100000);
> -
> -	if (err) {
> +	if (err)
>  		dev_err(priv->device, "EMAC reset timeout\n");
> -		return err;
> -	}
> -	return 0;
> +
> +	return err;
>  }

[ ... ]
-- 
This is an AI-generated review.