[PATCH net-next v3] net: dsa: realtek: rtl8365mb: wait out the full chip reset time

[PATCH net-next v3] net: dsa: realtek: rtl8365mb: wait out the full chip reset time

[Stanislaw Pal]

Realtek documentation gives the chip 1 second to reset, and the driver
says so in a comment, but it only sleeps 100 ms and then polls the reset
bit and continues as soon as that bit clears. The bit reports that the
register block is back, not that the chip has finished its internal
bring-up: Luiz notes that older parts such as the RTL8367R need
noticeable extra time after it clears, so a driver that keys off the bit
alone is relying on that margin being zero.

Sleep out the documented second before touching anything, then read the
bit once and fail with -ETIMEDOUT if the chip has not come out of reset.

Probe therefore takes 1 s on every chip the driver supports, not only on
the part this was found on.

Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
---
Changes in v3:
- Drop the deadline arithmetic around regmap_read_poll_timeout() in
  favour of a plain msleep() and a single read (Luiz, Alvin).
- Retarget at net-next and drop the Fixes tag and the stable Cc; see the
  note below.
- Link to v2: https://lore.kernel.org/all/20260908174403.420507-1-kuncy7@gmail.com/

I have deliberately not carried Linus's Reviewed-by from v2, since this
revision replaces the implementation he reviewed.

On the evidence, and why this is no longer posted as a fix:

v1 and v2 justified this with cold-boot failure counts from my Archer
AX55 v1. I no longer think those counts should carry the patch. It is a
single four-year-old board that has already had one confirmed power
supply fault, Johan cannot reproduce anything like it on an MR80X v2.20
with the same IPQ5018 and RTL8367S, and Luiz's own observation is that
family C parts should not need the extra time.

Alvin put the dilemma precisely: either the supplies are stable, in
which case a cold probe and an unbind/rebind should behave the same -
and on my board they do not - or they are not stable, in which case the
board is out of spec and its behaviour proves nothing about the driver.
I think the second branch is the likely one here. For what it is worth
my device tree describes no regulators for the switch at all, so there
is nothing for Oleksij's series to consume; I will follow that up
separately rather than hold this patch to it. The device tree you asked
about is not upstream yet - it is in the OpenWrt submission at
https://github.com/openwrt/openwrt/pull/24197, file
target/linux/qualcommax/dts/ipq5018-archer-ax55-v1.dts; the switch node
has a reset GPIO and no supplies.

What is left is narrow but, I think, still worth fixing: the driver
documents a 1 s reset time and does not wait for it. That argument does
not depend on my hardware. If you would rather this waited for someone
to reproduce a real failure on a healthy board, I have no objection to
it being dropped.

The companion patch, "let the SerDes PLL settle after the data-path
reset", is withdrawn - its evidence came from the same board and was
never isolated from this one.
---
--- a/drivers/net/dsa/realtek/rtl8365mb_main.c
+++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
@@ -136,6 +136,9 @@
 #define RTL8365MB_CHIP_RESET_SW_MASK	0x0002
 #define RTL8365MB_CHIP_RESET_HW_MASK	0x0001
 
+/* Time the chip needs to complete a reset, per Realtek documentation */
+#define RTL8365MB_CHIP_RESET_TIME_MS	1000
+
 /* Interrupt polarity register */
 #define RTL8365MB_INTR_POLARITY_REG	0x1100
 #define   RTL8365MB_INTR_POLARITY_MASK	0x0001
@@ -2981,17 +2984,27 @@
 static int rtl8365mb_reset_chip(struct realtek_priv *priv)
 {
 	u32 val;
+	int ret;
 
 	priv->write_reg_noack(priv, RTL8365MB_CHIP_RESET_REG,
 			      FIELD_PREP(RTL8365MB_CHIP_RESET_HW_MASK, 1));
 
-	/* Realtek documentation says the chip needs 1 second to reset. Sleep
-	 * for 100 ms before accessing any registers to prevent ACK timeouts.
+	/* Realtek documentation says the chip needs 1 second to reset. The
+	 * reset bit clears before that time is up, and it only reports that
+	 * the register block is back, not that the chip has finished its
+	 * internal bring-up, so wait out the documented time before touching
+	 * anything.
 	 */
-	msleep(100);
-	return regmap_read_poll_timeout(priv->map, RTL8365MB_CHIP_RESET_REG, val,
-					!(val & RTL8365MB_CHIP_RESET_HW_MASK),
-					20000, 1e6);
+	msleep(RTL8365MB_CHIP_RESET_TIME_MS);
+
+	ret = regmap_read(priv->map, RTL8365MB_CHIP_RESET_REG, &val);
+	if (ret)
+		return ret;
+
+	if (val & RTL8365MB_CHIP_RESET_HW_MASK)
+		return -ETIMEDOUT;
+
+	return 0;
 }
 
 static int rtl8365mb_setup(struct dsa_switch *ds)

Re: [PATCH net-next v3] net: dsa: realtek: rtl8365mb: wait out the full chip reset time

[Linus Walleij]

On Wed, Sep 9, 2026 at 2:08 PM Stanislaw Pal <kuncy7@gmail.com> wrote:

> Realtek documentation gives the chip 1 second to reset, and the driver
> says so in a comment, but it only sleeps 100 ms and then polls the reset
> bit and continues as soon as that bit clears. The bit reports that the
> register block is back, not that the chip has finished its internal
> bring-up: Luiz notes that older parts such as the RTL8367R need
> noticeable extra time after it clears, so a driver that keys off the bit
> alone is relying on that margin being zero.
>
> Sleep out the documented second before touching anything, then read the
> bit once and fail with -ETIMEDOUT if the chip has not come out of reset.
>
> Probe therefore takes 1 s on every chip the driver supports, not only on
> the part this was found on.
>
> Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
(...)
> I have deliberately not carried Linus's Reviewed-by from v2, since this
> revision replaces the implementation he reviewed.

OK this works too, I was too quick in endorsing the hairy microoptimization.
Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij

Re: [PATCH net-next v3] net: dsa: realtek: rtl8365mb: wait out the full chip reset time

[Alvin Šipraga]

On Wed, Sep 09, 2026 at 02:08:25PM +0200, Stanislaw Pal wrote:
> Realtek documentation gives the chip 1 second to reset, and the driver
> says so in a comment, but it only sleeps 100 ms and then polls the reset
> bit and continues as soon as that bit clears. The bit reports that the
> register block is back, not that the chip has finished its internal
> bring-up:

This claim is still speculative IMO. There are a couple of other reasons
why it could be failing on your end - see below for some debugging
approaches.

> Luiz notes that older parts such as the RTL8367R need
> noticeable extra time after it clears, so a driver that keys off the bit
> alone is relying on that margin being zero.

I don't really understand this statement, or how this patch would help
such chips? FWIW I thought Luiz was talking about some older Realtek IP
which isn't compatible with this driver.

If you or Luiz could point me to the vendor API sources, it would be
great, because I can't remember much of my thought process when I wrote
the original reset code. Maybe there are some additional hints in there.

And a reference to the 1 second reset time would be good for this patch
regardless. IIRC it's part of the vendor API documentation?

Unfortunately I don't have access to these things on the current PC.

> Sleep out the documented second before touching anything, then read the
> bit once and fail with -ETIMEDOUT if the chip has not come out of reset.
> 
> Probe therefore takes 1 s on every chip the driver supports, not only on
> the part this was found on.

Increasing the probe time so significantly for all users without hard
evidence of its necessity is a hard sell IMO :(

[...]

> Alvin put the dilemma precisely: either the supplies are stable, in
> which case a cold probe and an unbind/rebind should behave the same -
> and on my board they do not - or they are not stable, in which case the
> board is out of spec and its behaviour proves nothing about the driver.
> I think the second branch is the likely one here. For what it is worth
> my device tree describes no regulators for the switch at all, so there
> is nothing for Oleksij's series to consume; I will follow that up
> separately rather than hold this patch to it. The device tree you asked
> about is not upstream yet - it is in the OpenWrt submission at
> https://github.com/openwrt/openwrt/pull/24197, file
> target/linux/qualcommax/dts/ipq5018-archer-ax55-v1.dts; the switch node
> has a reset GPIO and no supplies.

You don't necessarily need to describe the regulators perfectly in order
to test the validity of the claims at the top of this patch. It should
suffice to just introduce some artificial sleeps in the driver which
simulate blocking on regulator enablement.

Not knowing the power topology of the board, I think it would be
interesting to add the sleep in one of two places, and do some cold boot
tests for each:

1. Add an msleep(1000) right at the start of rtl83xx_reset_assert().
2. Add an msleep(1000) right at the end of rtl83xx_reset_deassert().

Don't add both, test each case independently.

If both (1) and (2) work, then I assume that it's a matter of some
always-on regulator(s) which need more time to ramp up.

If only (1) works, then I'm not sure why (2) wouldn't...

If only (2) works, then it could be that the reset signal is also
controlling some regulators that the switch consumes. In that case I
would suggest transforming the reset GPIO into a regulator enable GPIO
and using Oleksij's series to enable.

If neither work, then I wonder what else explains the unbind/bind
success case.


... ahh wait, now I see that you actually put the reset-gpios on the
MDIO bus' devicetree node. Well, in that case you also lose the
post-deassert delay here:

#define REALTEK_HW_START_DELAY		100	/* msecs */

...

	if (priv->reset_ctl || priv->reset) {
		rtl83xx_reset_assert(priv);
		dev_dbg(dev, "asserted RESET\n");
		msleep(REALTEK_HW_STOP_DELAY);
		rtl83xx_reset_deassert(priv);
		msleep(REALTEK_HW_START_DELAY);
		dev_dbg(dev, "deasserted RESET\n");
	}

Tests (1) and (2) are no-ops if you don't give the switch a reset, so
move the reset-gpios property into the switch node before performing the
tests.

Kind regards,
Alvin