Message-ID: <aXtS-Yan4HkXZlDD@shell.armlinux.org.uk>
Date: Thu, 29 Jan 2026 12:30:49 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vincent Guittot <vincent.guittot@...aro.org>
Cc: vkoul@...nel.org, neil.armstrong@...aro.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, ciprianmarian.costea@....nxp.com, s32@....com,
	p.zabel@...gutronix.de, ghennadi.procopciuc@....com,
	bogdan-gabriel.roman@....com, Ionut.Vicovan@....com,
	alexandru-catalin.ionita@....com, linux-phy@...ts.infradead.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
	Frank.li@....com
Subject: Re: [PATCH 3/4] phy: s32g: Add serdes xpcs subsystem

On Mon, Jan 26, 2026 at 10:21:58AM +0100, Vincent Guittot wrote:
> s32g SoC family includes 2 serdes subsystems which are made of one PCIe
> controller, 2 XPCS and one Phy. The Phy got 2 lanes that can be configure
> to output PCIe lanes and/or SGMII.
> 
> Add XPCS and SGMII support.
> 
> Co-developed-by: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>
> Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@....com>
> Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@....com>
> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
> Co-developed-by: Ionut Vicovan <Ionut.Vicovan@....com>
> Signed-off-by: Ionut Vicovan <Ionut.Vicovan@....com>
> Co-developed-by: Bogdan Roman <bogdan-gabriel.roman@....com>
> Signed-off-by: Bogdan Roman <bogdan-gabriel.roman@....com>
> Signed-off-by: Vincent Guittot <vincent.guittot@...aro.org>

I'm not doing a full review for this patch yet.

> +/*
> + * Note: This function should be compatible with phylink.
> + * That means it should only modify link, duplex, speed
> + * an_complete, pause.
> + */
> +static int s32g_xpcs_get_state(struct s32g_xpcs *xpcs,
> +			       struct phylink_link_state *state)
> +{
> +	struct device *dev = xpcs->dev;
> +	unsigned int mii_ctrl, val, ss;
> +	bool ss6, ss13, an_enabled, intr_en;
> +
> +	mii_ctrl = s32g_xpcs_read(xpcs, SR_MII_CTRL);
> +	an_enabled = !!(mii_ctrl & AN_ENABLE);
> +	intr_en = !!(s32g_xpcs_read(xpcs, VR_MII_AN_CTRL) & MII_AN_INTR_EN);
> +
> +	/* Check this important condition */
> +	if (an_enabled && !intr_en) {
> +		dev_err(dev, "Invalid SGMII AN config interrupt is disabled\n");
> +		return -EINVAL;
> +	}

This isn't an interrupt handler. Phylink calls it from the state
resolver, which _may_ be triggered by an interrupt handler, but will
also be called at other times.

> +
> +	if (an_enabled) {
> +		/* MLO_AN_INBAND */
> +		state->speed = SPEED_UNKNOWN;
> +		state->link = 0;
> +		state->duplex =  DUPLEX_UNKNOWN;
> +		state->an_complete = 0;
> +		state->pause = MLO_PAUSE_NONE;

Have you looked at the initial state that phylink sets up before
calling the .pcs_get_state() method? See phylink_mac_pcs_get_state().

speed/duplex/pause/an_complete are already setup like that for you if
autoneg is enabled. link is the only member you'd need to touch.

> +		val = s32g_xpcs_read(xpcs, VR_MII_AN_INTR_STS);
> +
> +		/* Interrupt is raised with each SGMII AN that is in cases
> +		 * Link down - Every SGMII link timer expire
> +		 * Link up - Once before link goes up
> +		 * So either linkup or raised interrupt mean AN was completed
> +		 */
> +		if ((val & CL37_ANCMPLT_INTR) || (val & CL37_ANSGM_STS_LINK)) {
> +			state->an_complete = 1;
> +			if (val & CL37_ANSGM_STS_LINK)
> +				state->link = 1;
> +			else
> +				return 0;
> +			if (val & CL37_ANSGM_STS_DUPLEX)
> +				state->duplex = DUPLEX_FULL;
> +			else
> +				state->duplex = DUPLEX_HALF;
> +			ss = FIELD_GET(CL37_ANSGM_STS_SPEED_MASK, val);
> +		} else {
> +			return 0;
> +		}
> +
> +	} else {
> +		/* MLO_AN_FIXED, MLO_AN_PHY */

This function won't be called in those modes, so this is a misleading
comment. It can be called in MLO_AN_INBAND but when autoneg is disabled.

> +		val = s32g_xpcs_read(xpcs, SR_MII_STS);
> +		state->link = !!(val & LINK_STS);
> +		state->an_complete = 0;
> +		state->pause = MLO_PAUSE_NONE;
> +
> +		if (mii_ctrl & DUPLEX_MODE)
> +			state->duplex = DUPLEX_FULL;
> +		else
> +			state->duplex = DUPLEX_HALF;
> +
> +		/*
> +		 * Build similar value as CL37_ANSGM_STS_SPEED with
> +		 * SS6 and SS13 of SR_MII_CTRL:
> +		 *   - 0 for 10 Mbps
> +		 *   - 1 for 100 Mbps
> +		 *   - 2 for 1000 Mbps
> +		 */
> +		ss6 = !!(mii_ctrl & SS6);
> +		ss13 = !!(mii_ctrl & SS13);
> +		ss = ss6 << 1 | ss13;
> +	}
> +
> +	switch (ss) {
> +	case CL37_ANSGM_10MBPS:
> +		state->speed = SPEED_10;
> +		break;
> +	case CL37_ANSGM_100MBPS:
> +		state->speed = SPEED_100;
> +		break;
> +	case CL37_ANSGM_1000MBPS:
> +		state->speed = SPEED_1000;
> +		break;
> +	default:
> +		dev_err(dev, "Failed to interpret the value of SR_MII_CTRL\n");
> +		break;
> +	}
> +
> +	val = s32g_xpcs_read(xpcs, VR_MII_DIG_CTRL1);
> +	if ((val & EN_2_5G_MODE) && state->speed == SPEED_1000)
> +		state->speed = SPEED_2500;
> +
> +	/* Cover SGMII AN inability to distigunish between 1G and 2.5G */
> +	if ((val & EN_2_5G_MODE) &&
> +	    state->speed != SPEED_2500 && an_enabled) {
> +		dev_err(dev, "Speed not supported in SGMII AN mode\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int s32g_xpcs_config_an(struct s32g_xpcs *xpcs,
> +			       const struct phylink_link_state state)
> +{
> +	bool an_enabled = false;
> +
> +	an_enabled = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +				       state.advertising);
> +	if (!an_enabled)
> +		return 0;

Don't check the autoneg bit. This is the media-side autoneg, not
the PCS autoneg.

> +
> +	s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1,
> +			     CL37_TMR_OVRRIDE, CL37_TMR_OVRRIDE);
> +
> +	s32g_xpcs_write_bits(xpcs, VR_MII_AN_CTRL,
> +			     PCS_MODE_MASK | MII_AN_INTR_EN,
> +			     FIELD_PREP(PCS_MODE_MASK, PCS_MODE_SGMII) | MII_AN_INTR_EN);
> +	/* Enable SGMII AN */
> +	s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, AN_ENABLE, AN_ENABLE);
> +	/* Enable SGMII AUTO SW */
> +	s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1,
> +			     MAC_AUTO_SW, MAC_AUTO_SW);
> +
> +	return 0;
> +}
> +
> +static int s32g_xpcs_config(struct s32g_xpcs *xpcs,
> +			    const struct phylink_link_state state)
> +{
> +	struct device *dev = xpcs->dev;
> +	unsigned int val = 0, duplex = 0;
> +	int ret = 0;
> +	int speed = state.speed;
> +	bool an_enabled;
> +
> +	/* Configure adaptive MII width */
> +	s32g_xpcs_write_bits(xpcs, VR_MII_AN_CTRL, MII_CTRL, 0);
> +
> +	an_enabled = !!(s32g_xpcs_read(xpcs, SR_MII_CTRL) & AN_ENABLE);
> +
> +	dev_dbg(dev, "xpcs_%d: speed=%u duplex=%d an=%d\n", xpcs->id,
> +		speed, state.duplex, an_enabled);
> +
> +	if (an_enabled) {
> +		switch (speed) {
> +		case SPEED_10:
> +		case SPEED_100:
> +		case SPEED_1000:
> +			s32g_xpcs_write(xpcs, VR_MII_LINK_TIMER_CTRL, 0x2faf);
> +			break;
> +		case SPEED_2500:
> +			s32g_xpcs_write(xpcs, VR_MII_LINK_TIMER_CTRL, 0x7a1);
> +			s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1, MAC_AUTO_SW, 0);

Configuring the link timer _after_ the link has already come up looks
completely wrong to me... this should be done when .pcs_config() detects
that the PHY interface mode has changed.

> +			break;
> +		default:
> +			dev_err(dev, "Speed not recognized. Can't setup xpcs\n");
> +			return -EINVAL;
> +		}
> +
> +		s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, RESTART_AN, RESTART_AN);

As this is called from the .pcs_link_up() method, expect the link to
go bouncey bouncy bouncy if you restart AN _after_ the link has
come up.

> +
> +		ret = s32g_xpcs_wait_an_done(xpcs);
> +		if (ret)
> +			dev_warn(dev, "AN did not finish for XPCS%d", xpcs->id);
> +
> +		/* Clear the AN CMPL intr */
> +		s32g_xpcs_write_bits(xpcs, VR_MII_AN_INTR_STS, CL37_ANCMPLT_INTR, 0);
> +	} else {
> +		s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, AN_ENABLE, 0);
> +		s32g_xpcs_write_bits(xpcs, VR_MII_AN_CTRL, MII_AN_INTR_EN, 0);
> +
> +		switch (speed) {
> +		case SPEED_10:
> +			break;
> +		case SPEED_100:
> +			val = SS13;
> +			break;
> +		case SPEED_1000:
> +			val = SS6;
> +			break;
> +		case SPEED_2500:
> +			val = SS6;
> +			break;
> +		default:
> +			dev_err(dev, "Speed not supported\n");
> +			break;
> +		}
> +
> +		if (state.duplex == DUPLEX_FULL)
> +			duplex = DUPLEX_MODE;
> +
> +		s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, DUPLEX_MODE, duplex);
> +
> +		if (speed == SPEED_2500) {
> +			ret = s32g_serdes_bifurcation_pll_transit(xpcs, XPCS_PLLB);
> +			if (ret)
> +				dev_err(dev, "Switch to PLLB failed\n");
> +		} else {
> +			ret = s32g_serdes_bifurcation_pll_transit(xpcs, XPCS_PLLA);
> +			if (ret)
> +				dev_err(dev, "Switch to PLLA failed\n");
> +		}
> +
> +		s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, SS6 | SS13, val);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * phylink_pcs_ops fops

They are not "fops" which commonly refers to struct file_operations

> + */
> +
> +static void s32cc_phylink_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
> +					struct phylink_link_state *state)
> +{
> +	struct s32g_xpcs *xpcs = phylink_pcs_to_s32g_xpcs(pcs);
> +
> +	s32g_xpcs_get_state(xpcs, state);
> +}

Seems to me a pointless wrapper.

> +
> +static int s32cc_phylink_pcs_config(struct phylink_pcs *pcs,
> +				    unsigned int neg_mode,
> +				    phy_interface_t interface,
> +				    const unsigned long *advertising,
> +				    bool permit_pause_to_mac)
> +{
> +	struct s32g_xpcs *xpcs = phylink_pcs_to_s32g_xpcs(pcs);
> +	struct phylink_link_state state  = { 0 };
> +
> +	if (!(neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED))
> +		return 0;
> +
> +	linkmode_copy(state.advertising, advertising);
> +
> +	return s32g_xpcs_config_an(xpcs, state);

Given this is the only callsite for this function, and the only thing
you pass is the advertising mask, why pass a struct phylink_link_state
rather than the advertising mask?

> +}
> +
> +static void s32cc_phylink_pcs_restart_an(struct phylink_pcs *pcs)
> +{
> +	/* Not yet */
> +}
> +
> +static void s32cc_phylink_pcs_link_up(struct phylink_pcs *pcs,
> +				      unsigned int neg_mode,
> +				      phy_interface_t interface, int speed,
> +				      int duplex)
> +{
> +	struct s32g_xpcs *xpcs = phylink_pcs_to_s32g_xpcs(pcs);
> +	struct phylink_link_state state = { 0 };
> +
> +	state.speed = speed;
> +	state.duplex = duplex;
> +	state.an_complete = false;

an_complete is never an "input" to drivers. It's a state from PCS
drivers back to phylink. Also, s32g_xpcs_config never looks at this.

> +
> +	s32g_xpcs_config(xpcs, state);

Again, the only things that this function uses are the speed and
duplex, so why wrap them up into a struct?

> +}
> +
> +static const struct phylink_pcs_ops s32cc_phylink_pcs_ops = {
> +	.pcs_get_state = s32cc_phylink_pcs_get_state,
> +	.pcs_config = s32cc_phylink_pcs_config,
> +	.pcs_an_restart = s32cc_phylink_pcs_restart_an,
> +	.pcs_link_up = s32cc_phylink_pcs_link_up,
> +};

Please implement .pcs_inband_caps. As you don't support disabling
inband for SGMII, that means you can't support MLO_AN_PHY mode
reliably.

Also note that there are PHYs out there that do _not_ provide SGMII
inband, which means if you have it enabled, and you're relying on it
to complete, you won't be able to interface with those PHYs. There's
such a PHY on a SFP module.

If this driver is purely for a network PCS, then please locate it in
drivers/net/pcs.

I'm pretty sure there's other stuff I've missed as far as the phylink
API goes, so please expect further review once you've addressed the
comments above.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists