[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fails to fix the EMAC soft reset timeout bug for boards with an internal PHY that are configured to use an external PHY.
- [High] Uncoordinated MAC soft reset triggered by MDIO mux switching causes silent network interface hang.
--

--- Patch [1]: [PATCH net] net: stmmac: dwmac-sun8i: reset the EMAC when opening, not when probing ---
commit 5f11ae1c8c7c87b062343e837c4c5de6f8022782
Author: Pedro Santos <hartmnn.p@gmail.com>

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

This patch moves the EMAC soft reset from the probe function to the DMA
reset hook (called during interface open). This ensures the PHY is powered
up and driving the RX clock before the MAC reset is triggered, fixing
reset timeouts on boards with external PHYs after a warm reboot.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index 48c52eb962335..748ebabd933cd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c

[ ... ]

> @@ -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: High]
This isn't a bug introduced by this patch, but does the MDIO mux switch
callback cause a silent network interface hang?

If userspace queries the inactive PHY's MDIO bus (for example, via ethtool
or /dev/mdio), the multiplexer invokes mdio_mux_syscon_switch_fn(), which
unconditionally calls sun8i_dwmac_reset().

This calls sun8i_dwmac_soft_reset(), asserting the hardware soft reset bit
and wiping all MAC registers, filters, and DMA engine state. Since the stmmac
core driver is completely unaware of this reset, will it fail to reinitialize
the hardware, resulting in all traffic being dropped until the interface is
administratively restarted?

>  
> -	/* 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) {
>  		dev_err(priv->device, "EMAC reset timeout\n");
> -		return err;
> -	}
> -	return 0;
> +
> +	return err;
>  }

[ ... ]

> @@ -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;
>  	}

[Severity: High]
This is a pre-existing issue, but does this patch miss the reset timeout
for boards with an internal PHY that are configured to use an external PHY?

When sun8i_dwmac_probe() calls sun8i_dwmac_register_mdio_mux() just above
this block, mdio_mux_init() synchronously scans the child buses. This invokes
mdio_mux_syscon_switch_fn() for the first time, which then unconditionally
calls sun8i_dwmac_reset().

Since this still happens at probe time, wouldn't the external PHY still be
powered down from a warm reboot, causing the soft reset to time out here
just like it did before?

>  
>  	pm_runtime_put(&pdev->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260822045641.19282-1-hartmnn.p@gmail.com?part=1

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.