Re: [PATCH v6 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Incomplete feature enablement: PHY_INTERFACE_MODE_1000BASEX is
  now announced to phylink and programmed into the MAC (NCFGR.PCSSEL in…
- [Low] Checkpatch/netdev style violations in the new code, plus a
  redundant local-variable idiom.

--

net: macb: add support for 1000BASE-X autonegotiation to PCS

The patch decouples PCS enablement from the SGMII mode selection so the
gigabit PCS can also be used in 1000BASE-X mode, and announces
PHY_INTERFACE_MODE_1000BASEX to phylink when MACB_CAPS_PCS is present.
It also adds an autonegotiation restart implementation and makes
pcs_config() return 1 when the advertisement was changed.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d48ad5b587d09..3c18e43fca069 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -583,7 +583,12 @@ static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>  
>  static void macb_pcs_an_restart(struct phylink_pcs *pcs)
>  {
> -	/* Not supported */
> +	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
> +	u32 old, new;
> +
> +	old = gem_readl(bp, PCSCNTRL);
> +	new = old | BMCR_ANRESTART;
> +	gem_writel(bp, PCSCNTRL, new);
>  }
>  
>  static int macb_pcs_config(struct phylink_pcs *pcs,
> @@ -594,11 +599,16 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
>  {
>  	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
>  	u32 old, new;
> +	int ret = 0;
>  
>  	old = gem_readl(bp, PCSANADV);
>  	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
> -	if (new != -EINVAL && old != new)
> +	if (new != -EINVAL && old != new) {
> +		/* pcs_config() is supposed to return 1 if AN advertisement
> +		 * has changed */

[Severity: Low]
This isn't a bug, but a few style points that netdev's checkpatch run will
likely flag:

The new block comment in macb_pcs_config() closes on the text line, and
checkpatch wants the trailing */ on a line of its own:

	/* pcs_config() is supposed to return 1 if AN advertisement
	 * has changed
	 */

The continuation lines of the two new conditions, in
macb_mac_select_pcs() and in macb_mii_probe(), are indented with tabs
rather than aligned to the open parenthesis (checkpatch
PARENTHESIS_ALIGNMENT), which makes the continuation look like a
statement body:

	else if (interface == PHY_INTERFACE_MODE_1000BASEX ||
			interface == PHY_INTERFACE_MODE_SGMII)

In macb_pcs_an_restart(), the old/new pair mirrors the conditional-write
idiom used by macb_pcs_config() but there is no old != new comparison,
so the split reads as though a comparison was dropped.  Could this just
be written as a single read-modify-write?

> +		ret = 1;
>  		gem_writel(bp, PCSANADV, new);
> +	}
>  
>  	/* Disable AN if it's not to be used, enable otherwise.
>  	 * Must be written after PCSSEL is set in NCFGR which is done in
> @@ -612,7 +622,7 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
>  	if (old != new)
>  		gem_writel(bp, PCSCNTRL, new);
>  
> -	return 0;
> +	return ret;
>  }

[ ... ]

> @@ -957,7 +969,8 @@ static struct phylink_pcs *macb_mac_select_pcs(struct phylink_config *config,
>  
>  	if (interface == PHY_INTERFACE_MODE_10GBASER)
>  		return &bp->phylink_usx_pcs;
> -	else if (interface == PHY_INTERFACE_MODE_SGMII)
> +	else if (interface == PHY_INTERFACE_MODE_1000BASEX ||
> +			interface == PHY_INTERFACE_MODE_SGMII)
>  		return &bp->phylink_sgmii_pcs;
>  	else
>  		return NULL;
> @@ -1032,7 +1045,8 @@ static int macb_mii_probe(struct net_device *netdev)
>  	bp->phylink_config.type = PHYLINK_NETDEV;
>  	bp->phylink_config.mac_managed_pm = true;
>  
> -	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX ||
> +			bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
>  		bp->phylink_config.poll_fixed_state = true;
>  		bp->phylink_config.get_fixed_state = macb_get_pcs_fixed_state;
>  		/* The PCSAUTONEG bit in PCSCNTRL is on out of reset. Setting
> @@ -1061,9 +1075,12 @@ static int macb_mii_probe(struct net_device *netdev)
>  			  bp->phylink_config.supported_interfaces);
>  		phy_interface_set_rgmii(bp->phylink_config.supported_interfaces);
>  
> -		if (bp->caps & MACB_CAPS_PCS)
> +		if (bp->caps & MACB_CAPS_PCS) {
> +			__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +				  bp->phylink_config.supported_interfaces);
>  			__set_bit(PHY_INTERFACE_MODE_SGMII,
>  				  bp->phylink_config.supported_interfaces);
> +		}

[Severity: Medium]
1000BASE-X is now announced purely on the runtime-detected MACB_CAPS_PCS
bit, so it becomes selectable on the zynqmp/versal/mpfs/pic64hpsc
configs, all of which use .init = init_reset_optional.

init_reset_optional() still gates the entire serial transceiver bring-up
on the SGMII mode only:

drivers/net/ethernet/cadence/macb_main.c:init_reset_optional() {
	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
		/* Ensure PHY device used in SGMII mode is ready */
		bp->phy = devm_phy_optional_get(&pdev->dev, NULL);
		...
		ret = phy_init(bp->phy);
		...
		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0);
		...
		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);
	}
}

With phy-mode = "1000base-x", that block is skipped, so bp->phy stays
NULL (netdev priv is zero allocated), phy_init() never runs and the
firmware calls that route the GEM onto the serial/PCS path are never
issued.

macb_open() then does:

drivers/net/ethernet/cadence/macb_main.c:macb_open() {
	err = phy_set_mode_ext(bp->phy, PHY_MODE_ETHERNET, bp->phy_interface);
	if (err)
		goto reset_hw;

	err = phy_power_on(bp->phy);
	if (err)
		goto reset_hw;
}

Both are NULL tolerant and return 0, so probe and open succeed while the
PS-GTR lane is left unconfigured and unpowered (xpsgtr_phy_init() is what
enables the lane reference clock and runs xpsgtr_phy_init_sgmii()).

Does this mean the SFP use case named in the commit message ("This
enables direct attachment of the PCS to e.g. an SFP") silently never
links on those platforms, with no diagnostic?  Should
init_reset_optional() take the same path for
PHY_INTERFACE_MODE_1000BASEX?

Separately, macb_open() passes the device tree bp->phy_interface to
phy_set_mode_ext() rather than the interface phylink resolved.  Now that
the same PCS can run as either SGMII or 1000BASE-X, can the transceiver
submode end up differing from the mode programmed into the MAC PCS?

>  
>  		if (bp->caps & MACB_CAPS_HIGH_SPEED) {
>  			__set_bit(PHY_INTERFACE_MODE_10GBASER,
> @@ -4911,7 +4928,9 @@ static int macb_init_dflt(struct platform_device *pdev)
>  	/* Set MII management clock divider */
>  	val = macb_mdc_clk_div(bp);
>  	val |= macb_dbw(bp);
> -	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX)
> +		val |= GEM_BIT(PCSSEL);
> +	else if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>  		val |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>  	macb_writel(bp, NCFGR, val);
>  

This is the counterpart site: the MAC side PCS is enabled for 1000BASE-X
on exactly the platforms whose transceiver init above was left SGMII only.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922151429.23272-1-nwhitehorn%40pa.msu.edu