| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKfTPtDdpfGK2JE57kwLMEzpi2MuLOBe0FaeOaqgjqPPV-9pLA@mail.gmail.com>
Date: Thu, 29 Jan 2026 14:45:59 +0100
From: Vincent Guittot <vincent.guittot@...aro.org>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 Thu, 29 Jan 2026 at 13:30, Russell King (Oracle)
<linux@...linux.org.uk> wrote:
>
> 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().
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
>
> > + 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
>
> > + 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
>
> > +
> > + 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
>
> > + 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.
okay
>
> > +
> > +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
>
> > +}
> > +
> > +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?
will clean this
>
> > +}
> > +
> > +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!
Powered by blists - more mailing lists