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

[Luiz Angelo Daros de Luca]

Hi Stanislaw,

Thanks for your patch.

> The reset bit clears well before the RTL8365MB/RTL8367S has finished its
> internal bring-up, so configuring it right away makes register writes to
> blocks that are not up yet get lost. The switch is then left half
> configured: the CPU port link comes up and the switch still transmits
> towards the CPU, but nothing the CPU sends is ever forwarded - no MIB TX
> counter moves on any user port, while the MAC reports every frame as
> transmitted without errors.
>
> The driver already documents the 1 s reset time the chip needs and polls
> with a 1 s timeout, but stops waiting as soon as the bit clears. Sleep
> out the remainder of that second instead, measured from the reset write,
> so the total wait stays at 1 s regardless of how long the poll took.
>
> Seen on a TP-Link Archer AX55 v1 (IPQ5018 + RTL8367S, 2.5G HSGMII trunk)
> on roughly three out of four boots.

We discussed a similar issue previously:
https://lore.kernel.org/all/20260721115306.15144-1-kuncy7@xxxxxxxxx/

In that thread, you mentioned it might have been caused by a bad power
supply. Are there any updates or new findings regarding that?

>  Unbinding and rebinding the driver
> always fixed it at runtime. With this patch: 6 out of 6 clean boots.

This part is still puzzling. Why does an unbind/rebind cycle work
every time if a cold boot fails without the extra delay? After the
reset write, both execution paths should behave identically. Perhaps
the interface state during an unbind/rebind cycle slows down the
probe/setup path enough to give the chip the time it needs?

> Fixes: 4af2950c50c8 ("net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC")
> Signed-off-by: Stanislaw Pal <kuncy7@xxxxxxxxx>
> ---
> Changes in v2:
> - Declare "remaining" as long instead of unsigned long and drop the cast
>   from the comparison, so the sign is carried by the type (Andrew Lunn).
> - Name the affected parts in the opening sentence instead of "the chip".
> - Link to v1: https://lore.kernel.org/all/20260907191940.806734-1-kuncy7@xxxxxxxxx/
> ---
> --- 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
> @@ -2980,18 +2983,38 @@
>
>  static int rtl8365mb_reset_chip(struct realtek_priv *priv)
>  {
> +       unsigned long deadline;
> +       long remaining;
>         u32 val;
> +       int ret;
>
>         priv->write_reg_noack(priv, RTL8365MB_CHIP_RESET_REG,
>                               FIELD_PREP(RTL8365MB_CHIP_RESET_HW_MASK, 1));
>
> +       deadline = jiffies + msecs_to_jiffies(RTL8365MB_CHIP_RESET_TIME_MS);
> +
>         /* Realtek documentation says the chip needs 1 second to reset. Sleep
>          * for 100 ms before accessing any registers to prevent ACK timeouts.
>          */
>         msleep(100);
> -       return regmap_read_poll_timeout(priv->map, RTL8365MB_CHIP_RESET_REG, val,
> -                                       !(val & RTL8365MB_CHIP_RESET_HW_MASK),
> -                                       20000, 1e6);
> +       ret = regmap_read_poll_timeout(priv->map, RTL8365MB_CHIP_RESET_REG, val,
> +                                      !(val & RTL8365MB_CHIP_RESET_HW_MASK),
> +                                      20000, 1e6);
> +       if (ret)
> +               return ret;
> +
> +       /* The bit clearing only means the reset was accepted, not that the
> +        * chip is ready: register writes issued before the documented reset
> +        * time has elapsed are silently dropped by blocks that are still
> +        * coming up, which leaves the switch half configured. Wait out
> +        * whatever is left of that second, measured from the reset write, so
> +        * the poll above does not add to the total.
> +        */
> +       remaining = (long)(deadline - jiffies);
> +       if (remaining > 0)
> +               msleep(jiffies_to_msecs(remaining));

Tracking deadline math across regmap_read_poll_timeout() adds
unnecessary complexity. If the chip reliably requires a full 1-second
delay, why not simply msleep(RTL8365MB_CHIP_RESET_TIME_MS) upfront? We
could perform a single regmap_read() check afterward to verify
RTL8365MB_CHIP_RESET_HW_MASK cleared, though even that might be overly
cautious.

FWIW, I noticed that older chips (like the RTL8367R) do require extra
time after the reset bit clears, whereas I haven't seen that on
RTL8367C devices. Checking if RTL8365MB_CHIP_RESET_HW_MASK was cleared
simply might not indicate that the chip has finished booting
internally.

> +
> +       return 0;
>  }
>
>  static int rtl8365mb_setup(struct dsa_switch *ds)
>

Best regards,

Luiz