[net] net: stmmac: dwmac-sun8i: reset the EMAC when opening, not when probing
Commit Message
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(-)
Comments
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);
@@ -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);