Re: [PATCH net 4/6 RESEND] net: dsa: b53: configure PCS negotiation and forced link parameters

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] b53_serdes_config() ignores permit_pause_to_mac.

--

net: dsa: b53: configure PCS negotiation and forced link parameters

This commit makes b53_serdes_config() program the advertisement, the BMCR
autonegotiation enable, the protocol selection and SGMII MAC mode based on
phylink's negotiation mode. It adds pcs_inband_caps(). It also adds a
pcs_link_up() handler that forces speed and duplex when in-band negotiation
is disabled.

> diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
> index d521050d7c8f7..fc20e211e67e1 100644
> --- a/drivers/net/dsa/b53/b53_serdes.c
> +++ b/drivers/net/dsa/b53/b53_serdes.c

[ ... ]

>  static int b53_serdes_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  			     phy_interface_t interface,
>  			     const unsigned long *advertising,
>  			     bool permit_pause_to_mac)
>  {

[ ... ]

> +	bmcr = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED ? BMCR_ANENABLE : 0;
> +	/* phylink requests Clause 37 restarts separately, but not SGMII. */
> +	if (bmcr && changed && interface == PHY_INTERFACE_MODE_SGMII)
> +		bmcr |= BMCR_ANRESTART;
> +	ret = b53_serdes_modify(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
> +				SERDES_MII_BLK,
> +				BMCR_ANENABLE | BMCR_ISOLATE | BMCR_ANRESTART, bmcr);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* The MAC applies phylink's resolved pause policy in mac_link_up(). */

[Severity: Low]
Is this comment accurate at this point in the series?

b53_serdes_config() ignores permit_pause_to_mac and relies on this comment
to justify that. However, b53_phylink_mac_link_up() in b53_common.c has no
MLO_AN_INBAND branch. Its MLO_AN_PHY branch only programs pause on 63xx
RGMII ports:

drivers/net/dsa/b53/b53_common.c:b53_phylink_mac_link_up() {
    ...
	if (mode == MLO_AN_PHY) {
		/* Re-negotiate EEE if it was enabled already */
		p->eee_enabled = b53_eee_init(ds, port, phydev);

		if (is63xx(dev) && in_range(port, B53_63XX_RGMII0, 4)) {
			b53_force_port_config(dev, port, speed, duplex,
					      tx_pause, rx_pause);
			b53_force_link(dev, port, true);
		}

		return;
	}
    ...
}

As a result, SGMII and 1000BASE-X SerDes ports in in-band or PHY mode never
get phylink's resolved tx_pause/rx_pause applied to the MAC. This is not a
change in behavior, since pause was not applied before this patch either.

A later patch in this series, "net: dsa: b53: apply resolved link settings
to serial port MACs", adds b53_phylink_mac_needs_override(). That helper
covers SGMII and 1000BASE-X in every mode and calls b53_force_port_config()
with tx_pause and rx_pause, which makes the comment correct.

Could the comment be added in that later patch, or could this commit
message note that the MAC side is handled later in the series?

> +	return changed | ret;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022225656.1387306-1-florian.fainelli%40broadcom.com