On Thu, 29 Jan 2026 at 13:30, Russell King (Oracle)
[off-list ref] wrote:

On Mon, Jan 26, 2026 at 10:21:58AM +0100, Vincent Guittot wrote:

quoted

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@oss.nxp.com>
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
Co-developed-by: Alexandru-Catalin Ionita <redacted>
Signed-off-by: Alexandru-Catalin Ionita <redacted>
Co-developed-by: Ghennadi Procopciuc <redacted>
Signed-off-by: Ghennadi Procopciuc <redacted>
Co-developed-by: Ionut Vicovan <redacted>
Signed-off-by: Ionut Vicovan <redacted>
Co-developed-by: Bogdan Roman <redacted>
Signed-off-by: Bogdan Roman <redacted>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

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

quoted

+/*
+ * 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.

quoted

+
+     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().

okay, I'm going to have a look

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.

Okay

quoted

+             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.

Okay

quoted

+             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.

Okay

quoted

+
+     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.

Okay

quoted

+                     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.

quoted

+
+             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

quoted

+ */
+
+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.

okay

quoted

+
+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?

fair enough

quoted

+}
+
+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.

quoted

+
+     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?

will clean this

quoted

+}
+
+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.

okay

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.

That was an one open point for me because the content of this file is
only called by
drivers/phy/freescale/phy-nxp-s32g-xpcs.c
So locating both in the same place looked reasonable but I can but
files in different dir

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.

Thanks

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