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

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: macb: add support for 1000BASE-X autonegotiation to PCS

This decouples PCS enablement from the SGMII mode selection, advertises
PHY_INTERFACE_MODE_1000BASEX to phylink when the controller has a PCS, and
adds an autonegotiation restart implementation plus a return of 1 from
pcs_config() when the advertisement changed.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c15a9c7e69d37..9097ccb14c13b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -1061,9 +1075,12 @@ static int macb_mii_probe(struct net_device *dev)
>  			  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]

Advertising PHY_INTERFACE_MODE_1000BASEX here makes a device tree with
phy-mode = "1000base-x" acceptable on every PCS-capable GEM, including the
zynqmp, versal, mpfs and pic64hpsc configs whose .init hook is
init_reset_optional().  Should that hook also learn about 1000BASE-X?

It still gates the whole SerDes and firmware bring-up on SGMII 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 branch is skipped, so bp->phy stays NULL,
phy_init() is never called on the PS-GTR lane, and GEM_SGMII_MODE is never
set in the ZynqMP GEM_CLK_CTRL register, which is what routes the MAC onto
the serial/PCS clock path.

Is the failure also silent?  macb_open() calls
phy_set_mode_ext(bp->phy, PHY_MODE_ETHERNET, bp->phy_interface) and
phy_power_on(bp->phy), and both return 0 immediately for a NULL phy in
drivers/phy/phy-core.c, so probe and open succeed with no diagnostic while
the link can never come up.

> @@ -4932,7 +4949,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);

[Severity: Medium]

macb_init_dflt() is reached from init_reset_optional() a few statements after
the SGMII-only branch above, and macb_mac_config() sets PCSSEL for
1000BASE-X too.  Does that leave NCFGR.PCSSEL enabled while the SerDes lane
and the firmware GEM clock mux were never programmed on those platforms?

Also, the binding in Documentation/devicetree/bindings/net/cdns,macb.yaml
documents phys with maxItems: 1 for the controller without tying it to
phy-mode = "sgmii", so can a 1000BASE-X board legitimately expect the driver
to initialize that SerDes?

One related asymmetry: for a board declaring phy-mode = "sgmii" with an SFP
attached, phylink may select 1000BASE-X for the PCS while macb_open() still
programs the SerDes submode from the device tree value in bp->phy_interface.
Should the submode follow the interface phylink actually selected?

Note this does not regress a previously working setup, since 1000BASE-X was
not in supported_interfaces before this patch.
-- 
This is an AI-generated review.