Re: [net-next,12/14] net: stmmac: add support for reading inband SGMII status

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: add support for reading inband SGMII status

This commit adds support for reading link status, speed, and duplex for
SGMII links from the RGSMII status register.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
> index 73fc56ce5e55..12fc5038d913 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c

[ ... ]

> @@ -97,7 +107,7 @@ static void dwmac_integrated_pcs_get_state(struct phylink_pcs *pcs,
>  					   struct phylink_link_state *state)
>  {
>  	struct stmmac_pcs *spcs = phylink_pcs_to_stmmac_pcs(pcs);
> -	u32 status, lpa;
> +	u32 status, lpa, rgsmii;
>
>  	status = readl(spcs->base + GMAC_AN_STATUS);
>
> @@ -111,7 +121,35 @@ static void dwmac_integrated_pcs_get_state(struct phylink_pcs *pcs,
>
>  		phylink_mii_c22_pcs_decode_state(state, neg_mode, status, lpa);
>  	} else {
> -		state->link = false;
> +		rgsmii = field_get(spcs->rgsmii_status_mask,
> +				   readl(spcs->rgsmii));
> +		state->link = !!(status & GMAC_RGSMIII_LNKSTS);
                            ^^^^^^

Should this be checking rgsmii instead of status here? The status
variable contains the GMAC_AN_STATUS register which is equivalent to
MII_BMSR according to the comment at line 7. In BMSR, the link status
bit is BMSR_LSTATUS (bit 2), not bit 3.

GMAC_RGSMIII_LNKSTS is defined as BIT(3), which is the correct bit
position for the RGSMII status register (matching GMAC_RGSMIIIS_LNKSTS
in dwmac1000.h), but it appears to be applied to the wrong register
variable.

The code later uses rgsmii correctly for speed and duplex extraction.
Was the intent to use rgsmii for the link status check as well?

> +
> +		if (state->link && neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {

[ ... ]