[RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568

[Coia Prant]

Thanks for the review.

> [ ... ]
>
> > @@ -884,7 +889,7 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
> >
> >       if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> >               ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> > -                              mdio_ctrl | BMCR_ANENABLE);
> > +                              mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);
>
> [Severity: Medium]
> The commit message mentions improving SGMII AN state handling, but this
> adds BMCR_ANRESTART to the 1000BASE-X configuration path in
> xpcs_config_aneg_c37_1000basex(). Was this unintentional, or should the
> commit message be updated to mention this behavior change for 1000BASE-X?

You are correct. This was unintentional. The `BMCR_ANRESTART` change in
`xpcs_config_aneg_c37_1000basex()` is a mistake that was introduced
during development and should not be there. I will remove it in v2.

The SGMII path (`xpcs_config_aneg_c37_sgmii()`) is the one that actually
needs this change, and that is what I have tested on the Photonicat board.
The 1000BASE-X path was not tested and the change was not intended.

> [ ... ]
>
> > @@ -1086,7 +1094,24 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
> >                       state->duplex = DUPLEX_FULL;
> >               else
> >                       state->duplex = DUPLEX_HALF;
> > -     } else if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
> > +
> > +             return 0;
> > +     }
> > +
> > +     /* Clear AN complete status or interrupt */
> > +     if (state->an_complete)
> > +             xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
>
> [Severity: High]
> Can clearing the interrupt here lead to a permanently stalled link on
> Wangxun platforms if the subsequent xpcs_read() fails?
>
> The original code cleared DW_VR_MII_AN_INTR_STS at the very end of the
> DW_VR_MII_AN_STS_C37_ANCMPLT_INTR block, after successfully reading
> MII_BMCR and MII_ADVERTISE.
>
> By moving the interrupt clear here:
>
>     if (state->an_complete)
>         xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
>
> If a transient MDIO error occurs during the subsequent MII_BMCR read:
>
>     speed = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
>     if (speed < 0)
>         return speed;
>
> The function will exit with an error and the link will remain down, but the
> AN complete status has already been cleared. Will this prevent the Wangxun
> initialization block from running on the next poll and leave the link
> stuck?

No, this is safe. The `ret` value used to determine whether to enter the
Wangxun-specific path is read from the register before the clear operation.
The clear only affects the hardware register, not the stack variable. So
Wangxun's `if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR)` block will still
be reached correctly.

If the subsequent MDIO read fails, the function will return the error and
the link state will remain down. Since the hardware interrupt status has
already been cleared, the next poll will not see `state->an_complete` set
again, so the Wangxun block will not be entered until a new AN event
occurs. This is actually the intended behavior — the interrupt is only
valid for the current AN event and should not be processed more than once.

Therefore, the change does not introduce a regression on Wangxun platforms.

I will remove the 1000BASE-X change in v2 and keep only the SGMII-related
fixes.