[PATCH 0/4] Serdes: s32g: Add support for serdes subsystem

[PATCH 0/4] Serdes: s32g: Add support for serdes subsystem

[Vincent Guittot]

s32g SoC family includes 2 serdes subsystems which are made of one PCIe
controller, 2 XPCS and a shared Phy. The Phy got 2 lanes that can be
configured to output PCIe lanes and/or SGMII.
    
Implement PCIe phy and XPCS support.
    
Vincent Guittot (4):
  dt-bindings: serdes: s32g: Add NXP serdes subsystem
  phy: s32g: Add serdes subsystem phy
  phy: s32g: Add serdes xpcs subsystem
  MAINTAINERS: Add MAINTAINER for NXP S32G Serdes driver

 .../bindings/phy/nxp,s32g-serdes.yaml         |  154 +++
 MAINTAINERS                                   |    9 +
 drivers/phy/freescale/Kconfig                 |   10 +
 drivers/phy/freescale/Makefile                |    1 +
 drivers/phy/freescale/phy-nxp-s32g-serdes.c   |  926 ++++++++++++++
 drivers/phy/freescale/phy-nxp-s32g-xpcs.c     | 1082 +++++++++++++++++
 drivers/phy/freescale/phy-nxp-s32g-xpcs.h     |   47 +
 include/linux/pcs/pcs-nxp-xpcs.h              |   13 +
 8 files changed, 2242 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
 create mode 100644 drivers/phy/freescale/phy-nxp-s32g-serdes.c
 create mode 100644 drivers/phy/freescale/phy-nxp-s32g-xpcs.c
 create mode 100644 drivers/phy/freescale/phy-nxp-s32g-xpcs.h
 create mode 100644 include/linux/pcs/pcs-nxp-xpcs.h

-- 
2.43.0

[PATCH 1/4] dt-bindings: serdes: s32g: Add NXP serdes subsystem

[Vincent Guittot]

Describe the serdes subsystem available on the S32G platforms.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 .../bindings/phy/nxp,s32g-serdes.yaml         | 154 ++++++++++++++++++
 1 file changed, 154 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml

diff --git a/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml b/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
new file mode 100644
index 000000000000..fad34bee2a4f
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
@@ -0,0 +1,154 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/nxp,s32g-serdes.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP S32G2xxx/S32G3xxx SerDes PHY subsystem
+
+maintainers:
+  - Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
+
+description: |
+  The SerDes subsystem on S32G SoC Family includes two types of PHYs:
+    - One PCIe PHY: Supports various PCIe operation modes
+    - Two Ethernet Physical Coding Sublayer (XPCS) controllers
+
+  SerDes operation mode selects the enabled PHYs and speeds. Clock frequency
+  must be adapted accordingly. Below table describes all possible operation
+  modes.
+
+  Mode  PCIe	XPCS0		XPCS1		PHY clock	Description
+                SGMII		SGMII		  (MHz)
+  -------------------------------------------------------------------------
+  0	Gen3	N/A		N/A		100		Single PCIe
+  1	Gen2	1.25Gbps	N/A		100		PCIe/SGMII
+  2	Gen2	N/A		1.25Gbps	100		PCIe/SGMII
+  3	N/A	1.25Gbps	1.25Gbps	100,125		SGMII
+  4	N/A	3.125/1.25Gbps	3.125/1.25Gbps 	125		SGMII
+  5	Gen2	N/A	        3.125Gbps     	100		PCIe/SGMII
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - nxp,s32g2-serdes
+      - items:
+          - const: nxp,s32g3-serdes
+          - const: nxp,s32g2-serdes
+
+  reg:
+    maxItems: 4
+
+  reg-names:
+    items:
+      - const: ss_pcie
+      - const: pcie_phy
+      - const: xpcs0
+      - const: xpcs1
+
+  clocks:
+    minItems: 4
+    maxItems: 5
+
+  clock-names:
+    items:
+      - const: axi
+      - const: aux
+      - const: apb
+      - const: ref
+      - const: ext
+    minItems: 4
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: serdes
+      - const: pcie
+
+  nxp,sys-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      SerDes operational mode. See above table for possible values.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  '^serdes[0,1]_lane@[0,1]$':
+    description:
+      Describe a serdes lane.
+    type: object
+
+    properties:
+      compatible:
+        enum:
+          - nxp,s32g2-serdes-pcie-phy
+          - nxp,s32g2-serdes-xpcs
+
+      reg:
+        maxItems: 1
+
+      '#phy-cells':
+        const: 0
+
+    required:
+      - reg
+      - compatible
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - nxp,sys-mode
+  - '#address-cells'
+  - '#size-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        serdes0: serdes@40480000 {
+          compatible = "nxp,s32g3-serdes", "nxp,s32g2-serdes";
+            reg = <0x0 0x40480000 0x0 0x108>,
+                  <0x0 0x40483008 0x0 0x10>,
+                  <0x0 0x40482000 0x0 0x800>,
+                  <0x0 0x40482800 0x0 0x800>;
+            reg-names = "ss_pcie", "pcie_phy", "xpcs0", "xpcs1";
+            clocks = <&clks 1>,
+                     <&clks 2>,
+                     <&clks 3>,
+                     <&clks 4>,
+                     <&serdes_100_ext>;
+            clock-names = "axi", "aux", "apb", "ref", "ext";
+            resets = <&reset 9>,
+                     <&reset 8>;
+            reset-names = "serdes", "pcie";
+            nxp,sys-mode = <1>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            phy_pcie0: serdes0_lane@0 {
+              compatible = "nxp,s32g2-serdes-pcie-phy";
+              #phy-cells = <0>;
+              reg = <0>;
+            };
+            phy_xpcs0_0: serdes0_lane@1 {
+              compatible = "nxp,s32g2-serdes-xpcs";
+              reg = <0>;
+            };
+        };
+    };
-- 
2.43.0

Re: [PATCH 1/4] dt-bindings: serdes: s32g: Add NXP serdes subsystem

[Russell King (Oracle)]

On Mon, Jan 26, 2026 at 10:21:56AM +0100, Vincent Guittot wrote:
> Describe the serdes subsystem available on the S32G platforms.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>  .../bindings/phy/nxp,s32g-serdes.yaml         | 154 ++++++++++++++++++
>  1 file changed, 154 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml b/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
> new file mode 100644
> index 000000000000..fad34bee2a4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
> @@ -0,0 +1,154 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/nxp,s32g-serdes.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32G2xxx/S32G3xxx SerDes PHY subsystem
> +
> +maintainers:
> +  - Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> +
> +description: |
> +  The SerDes subsystem on S32G SoC Family includes two types of PHYs:
> +    - One PCIe PHY: Supports various PCIe operation modes
> +    - Two Ethernet Physical Coding Sublayer (XPCS) controllers
> +
> +  SerDes operation mode selects the enabled PHYs and speeds. Clock frequency
> +  must be adapted accordingly. Below table describes all possible operation
> +  modes.
> +
> +  Mode  PCIe	XPCS0		XPCS1		PHY clock	Description
> +                SGMII		SGMII		  (MHz)
> +  -------------------------------------------------------------------------
> +  0	Gen3	N/A		N/A		100		Single PCIe
> +  1	Gen2	1.25Gbps	N/A		100		PCIe/SGMII
> +  2	Gen2	N/A		1.25Gbps	100		PCIe/SGMII
> +  3	N/A	1.25Gbps	1.25Gbps	100,125		SGMII
> +  4	N/A	3.125/1.25Gbps	3.125/1.25Gbps 	125		SGMII
> +  5	Gen2	N/A	        3.125Gbps     	100		PCIe/SGMII

Shouldn't the mode be configured via phy_set_mode_ext()?

This identifies whether it is operating as PCIe or for networking and
in the case of networking, the PHY interface mode should be passed as
the submode.

Have a look at include/linux/phy/pcie.h to see the submodes that may
be appropriate to pass to phy_set_mode_ext() - but talk to the PHY
subsystem maintainers.

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

Re: [PATCH 1/4] dt-bindings: serdes: s32g: Add NXP serdes subsystem

[Vincent Guittot]

On Thu, 29 Jan 2026 at 13:50, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Mon, Jan 26, 2026 at 10:21:56AM +0100, Vincent Guittot wrote:
> > Describe the serdes subsystem available on the S32G platforms.
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >  .../bindings/phy/nxp,s32g-serdes.yaml         | 154 ++++++++++++++++++
> >  1 file changed, 154 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml b/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
> > new file mode 100644
> > index 000000000000..fad34bee2a4f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
> > @@ -0,0 +1,154 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/nxp,s32g-serdes.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP S32G2xxx/S32G3xxx SerDes PHY subsystem
> > +
> > +maintainers:
> > +  - Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > +
> > +description: |
> > +  The SerDes subsystem on S32G SoC Family includes two types of PHYs:
> > +    - One PCIe PHY: Supports various PCIe operation modes
> > +    - Two Ethernet Physical Coding Sublayer (XPCS) controllers
> > +
> > +  SerDes operation mode selects the enabled PHYs and speeds. Clock frequency
> > +  must be adapted accordingly. Below table describes all possible operation
> > +  modes.
> > +
> > +  Mode  PCIe XPCS0           XPCS1           PHY clock       Description
> > +                SGMII                SGMII             (MHz)
> > +  -------------------------------------------------------------------------
> > +  0  Gen3    N/A             N/A             100             Single PCIe
> > +  1  Gen2    1.25Gbps        N/A             100             PCIe/SGMII
> > +  2  Gen2    N/A             1.25Gbps        100             PCIe/SGMII
> > +  3  N/A     1.25Gbps        1.25Gbps        100,125         SGMII
> > +  4  N/A     3.125/1.25Gbps  3.125/1.25Gbps  125             SGMII
> > +  5  Gen2    N/A             3.125Gbps       100             PCIe/SGMII
>
> Shouldn't the mode be configured via phy_set_mode_ext()?

There is a phy for only pcie only. In mode 3 to 5 there is no generic
phy created

>
> This identifies whether it is operating as PCIe or for networking and
> in the case of networking, the PHY interface mode should be passed as
> the submode.
>
> Have a look at include/linux/phy/pcie.h to see the submodes that may
> be appropriate to pass to phy_set_mode_ext() - but talk to the PHY
> subsystem maintainers.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Vincent Guittot]

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.

Implement PCIe phy 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 <alexandru-catalin.ionita@nxp.com>
Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Co-developed-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
Signed-off-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
Co-developed-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
Signed-off-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/phy/freescale/Kconfig               |   9 +
 drivers/phy/freescale/Makefile              |   1 +
 drivers/phy/freescale/phy-nxp-s32g-serdes.c | 569 ++++++++++++++++++++
 3 files changed, 579 insertions(+)
 create mode 100644 drivers/phy/freescale/phy-nxp-s32g-serdes.c

diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
index 81f53564ee15..45184a3cdd69 100644
--- a/drivers/phy/freescale/Kconfig
+++ b/drivers/phy/freescale/Kconfig
@@ -61,3 +61,12 @@ config PHY_FSL_LYNX_28G
 	  found on NXP's Layerscape platforms such as LX2160A.
 	  Used to change the protocol running on SerDes lanes at runtime.
 	  Only useful for a restricted set of Ethernet protocols.
+
+config PHY_S32G_SERDES
+	tristate "NXP S32G SERDES support"
+	depends on ARCH_S32 || COMPILE_TEST
+	select GENERIC_PHY
+	help
+	  This option enables support for S23G SerDes PHY used for
+	  PCIe & Ethernet
+
diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
index 658eac7d0a62..86d948417252 100644
--- a/drivers/phy/freescale/Makefile
+++ b/drivers/phy/freescale/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)	+= phy-fsl-imx8m-pcie.o
 obj-$(CONFIG_PHY_FSL_IMX8QM_HSIO)	+= phy-fsl-imx8qm-hsio.o
 obj-$(CONFIG_PHY_FSL_LYNX_28G)		+= phy-fsl-lynx-28g.o
 obj-$(CONFIG_PHY_FSL_SAMSUNG_HDMI_PHY)	+= phy-fsl-samsung-hdmi.o
+obj-$(CONFIG_PHY_S32G_SERDES)		+= phy-nxp-s32g-serdes.o
diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
new file mode 100644
index 000000000000..8336c868c8dc
--- /dev/null
+++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
@@ -0,0 +1,569 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * SerDes driver for S32G SoCs
+ *
+ * Copyright 2021-2026 NXP
+ */
+
+#include <dt-bindings/phy/phy.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/processor.h>
+#include <linux/reset.h>
+#include <linux/units.h>
+
+#define S32G_SERDES_MODE_MAX			5
+
+#define EXTERNAL_CLK_NAME			"ext"
+#define INTERNAL_CLK_NAME			"ref"
+
+/* Serdes Sub system registers */
+
+#define S32G_PCIE_PHY_GEN_CTRL			0x0
+#define  REF_USE_PAD				BIT(17)
+#define  RX_SRIS_MODE				BIT(9)
+
+#define S32G_PCIE_PHY_MPLLA_CTRL		0x10
+#define  MPLL_STATE				BIT(30)
+
+#define S32G_SS_RW_REG_0			0xF0
+#define  SUBMODE_MASK				GENMASK(3, 0)
+#define  CLKEN_MASK				BIT(23)
+#define  PHY0_CR_PARA_SEL			BIT(9)
+
+/* PCIe phy subsystem registers */
+
+#define S32G_PHY_REG_ADDR			0x0
+#define  PHY_REG_EN				BIT(31)
+
+#define S32G_PHY_REG_DATA			0x4
+
+#define RAWLANE0_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN	0x3019
+#define RAWLANE1_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN	0x3119
+
+/*
+ * Until now, there is no generic way to describe and set PCIe clock mode.
+ * PCIe controller uses the default CRNS = 0 mode.
+ */
+enum pcie_phy_mode {
+	CRNS = 0, /* Common Reference Clock, No Spread Spectrum */
+	CRSS = 1, /* Common Reference Clock, Spread Spectrum */
+	SRNS = 2, /* Separate Reference Clock, No Spread Spectrum */
+	SRIS = 3  /* Separate Reference Clock, Spread Spectrum */
+};
+
+struct s32g_serdes_ctrl {
+	void __iomem *ss_base;
+	struct reset_control *rst;
+	struct clk_bulk_data *clks;
+	int nclks;
+	u32 ss_mode;
+	unsigned long ref_clk_rate;
+	bool ext_clk;
+};
+
+struct s32g_pcie_ctrl {
+	void __iomem *phy_base;
+	struct reset_control *rst;
+	struct phy *phy;
+	enum pcie_phy_mode phy_mode;
+	bool powered_on;
+};
+
+struct s32g_serdes {
+	struct s32g_serdes_ctrl ctrl;
+	struct s32g_pcie_ctrl pcie;
+	struct device *dev;
+};
+
+/* PCIe phy subsystem */
+
+#define S32G_SERDES_PCIE_FREQ			(100 * HZ_PER_MHZ)
+
+static int s32g_pcie_check_clk(struct s32g_serdes *serdes)
+{
+	struct s32g_serdes_ctrl *sctrl = &serdes->ctrl;
+	unsigned long rate = sctrl->ref_clk_rate;
+
+	if (rate != S32G_SERDES_PCIE_FREQ) {
+		dev_err(serdes->dev, "PCIe PHY cannot operate at %lu HZ\n", rate);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static bool s32g_pcie_phy_is_locked(struct s32g_serdes *serdes)
+{
+	u32 mplla = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_MPLLA_CTRL);
+	const u32 mask = MPLL_STATE;
+
+	return (mplla & mask) == mask;
+}
+
+/* Serdes RFM says between 3.4 and 5.2ms depending of pll */
+#define S32G_SERDES_LOCK_TIMEOUT_MS		6
+
+static int s32g_pcie_phy_power_on_common(struct s32g_serdes *serdes)
+{
+	struct s32g_serdes_ctrl *sctrl = &serdes->ctrl;
+	struct s32g_pcie_ctrl *pcie = &serdes->pcie;
+	u32 reg;
+	int val, ret = 0;
+
+	ret = s32g_pcie_check_clk(serdes);
+	if (ret)
+		return ret;
+
+	reg = readl(sctrl->ss_base + S32G_PCIE_PHY_GEN_CTRL);
+
+	/* if PCIE PHY is in SRIS mode */
+	if (pcie->phy_mode == SRIS)
+		reg |= RX_SRIS_MODE;
+
+	if (sctrl->ext_clk)
+		reg |= REF_USE_PAD;
+	else
+		reg &= ~REF_USE_PAD;
+
+	writel(reg, sctrl->ss_base + S32G_PCIE_PHY_GEN_CTRL);
+
+	/* Monitor Serdes MPLL state */
+	ret = read_poll_timeout(s32g_pcie_phy_is_locked, val,
+				(val),
+				0,
+				S32G_SERDES_LOCK_TIMEOUT_MS, false, serdes);
+	if (ret) {
+		dev_err(serdes->dev, "Failed to lock PCIE phy\n");
+		return -ETIMEDOUT;
+	}
+
+	/* Set PHY register access to CR interface */
+	reg = readl(sctrl->ss_base + S32G_SS_RW_REG_0);
+	reg |=  PHY0_CR_PARA_SEL;
+	writel(reg, sctrl->ss_base + S32G_SS_RW_REG_0);
+
+	return ret;
+}
+
+static void s32g_pcie_phy_write(struct s32g_serdes *serdes, u32 reg, u32 val)
+{
+	writel(PHY_REG_EN, serdes->pcie.phy_base + S32G_PHY_REG_ADDR);
+	writel(reg | PHY_REG_EN, serdes->pcie.phy_base + S32G_PHY_REG_ADDR);
+	usleep_range(100, 110);
+	writel(val, serdes->pcie.phy_base + S32G_PHY_REG_DATA);
+	usleep_range(100, 110);
+	writel(0, serdes->pcie.phy_base + S32G_PHY_REG_ADDR);
+}
+
+static int s32g_pcie_phy_power_on(struct s32g_serdes *serdes)
+{
+	struct s32g_pcie_ctrl *pcie = &serdes->pcie;
+	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
+	u32 iq_ovrd_in;
+	int ret = 0;
+
+	ret = s32g_pcie_phy_power_on_common(serdes);
+	if (ret)
+		return ret;
+
+	/* RX_EQ_DELTA_IQ_OVRD enable and override value for PCIe lanes */
+	iq_ovrd_in = RAWLANE0_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN;
+
+	s32g_pcie_phy_write(serdes, iq_ovrd_in, 0x3);
+	s32g_pcie_phy_write(serdes, iq_ovrd_in, 0x13);
+
+	if (ctrl->ss_mode == 0) {
+		iq_ovrd_in = RAWLANE1_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN;
+
+		s32g_pcie_phy_write(serdes, iq_ovrd_in, 0x3);
+		s32g_pcie_phy_write(serdes, iq_ovrd_in, 0x13);
+	}
+
+	pcie->powered_on = true;
+
+	return 0;
+}
+
+/* PCIe phy ops function */
+
+static int s32g_serdes_phy_power_on(struct phy *p)
+{
+	struct s32g_serdes *serdes = phy_get_drvdata(p);
+
+	return s32g_pcie_phy_power_on(serdes);
+}
+
+static inline bool is_pcie_phy_mode_valid(int mode)
+{
+	switch (mode) {
+	case CRNS:
+	case CRSS:
+	case SRNS:
+	case SRIS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static int s32g_serdes_phy_set_mode_ext(struct phy *p,
+					enum phy_mode mode, int submode)
+{
+	struct s32g_serdes *serdes = phy_get_drvdata(p);
+
+	if (mode == PHY_MODE_PCIE)
+		return -EINVAL;
+
+	if (!is_pcie_phy_mode_valid(submode))
+		return -EINVAL;
+
+	/*
+	 * Do not configure SRIS or CRSS PHY MODE in conjunction
+	 * with any SGMII mode on the same SerDes subsystem
+	 */
+	if ((submode == CRSS || submode == SRIS) &&
+	    serdes->ctrl.ss_mode != 0)
+		return -EINVAL;
+
+	/*
+	 * Internal reference clock cannot be used with either Common clock
+	 * or Spread spectrum, leaving only SRNSS
+	 */
+	if (submode != SRNS &&  !serdes->ctrl.ext_clk)
+		return -EINVAL;
+
+	serdes->pcie.phy_mode = submode;
+
+	return 0;
+}
+
+static const struct phy_ops serdes_pcie_ops = {
+	.power_on	= s32g_serdes_phy_power_on,
+	.set_mode	= s32g_serdes_phy_set_mode_ext,
+};
+
+static struct phy *s32g_serdes_phy_xlate(struct device *dev,
+					 const struct of_phandle_args *args)
+{
+	struct s32g_serdes *serdes;
+	struct phy *phy;
+
+	serdes = dev_get_drvdata(dev);
+	if (!serdes)
+		return ERR_PTR(-EINVAL);
+
+	phy = serdes->pcie.phy;
+
+	return phy;
+}
+
+/* Serdes subsystem */
+
+static int s32g_serdes_assert_reset(struct s32g_serdes *serdes)
+{
+	struct device *dev = serdes->dev;
+	int ret;
+
+	ret = reset_control_assert(serdes->pcie.rst);
+	if (ret) {
+		dev_err(dev, "Failed to assert PCIE reset: %d\n", ret);
+		return ret;
+	}
+
+	ret = reset_control_assert(serdes->ctrl.rst);
+	if (ret) {
+		dev_err(dev, "Failed to assert SerDes reset: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int s32g_serdes_deassert_reset(struct s32g_serdes *serdes)
+{
+	struct device *dev = serdes->dev;
+	int ret;
+
+	ret = reset_control_deassert(serdes->pcie.rst);
+	if (ret) {
+		dev_err(dev, "Failed to assert PCIE reset: %d\n", ret);
+		return ret;
+	}
+
+	ret = reset_control_deassert(serdes->ctrl.rst);
+	if (ret) {
+		dev_err(dev, "Failed to assert SerDes reset: %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int s32g_serdes_init(struct s32g_serdes *serdes)
+{
+	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
+	u32 reg0;
+	int ret;
+
+	ret = clk_bulk_prepare_enable(ctrl->nclks, ctrl->clks);
+	if (ret) {
+		dev_err(serdes->dev, "Failed to enable SerDes clocks\n");
+		return ret;
+	}
+
+	ret = s32g_serdes_assert_reset(serdes);
+	if (ret)
+		goto disable_clks;
+
+	/* Set serdes mode */
+	reg0 = readl(ctrl->ss_base + S32G_SS_RW_REG_0);
+	reg0 &= ~SUBMODE_MASK;
+	if (ctrl->ss_mode == 5)
+		reg0 |= 2;
+	else
+		reg0 |= ctrl->ss_mode;
+	writel(reg0, ctrl->ss_base + S32G_SS_RW_REG_0);
+
+	/* Set Clock source: internal or external */
+	reg0 = readl(ctrl->ss_base + S32G_SS_RW_REG_0);
+	if (ctrl->ext_clk)
+		reg0 &= ~CLKEN_MASK;
+	else
+		reg0 |= CLKEN_MASK;
+
+	writel(reg0, ctrl->ss_base + S32G_SS_RW_REG_0);
+
+	/* Wait for the selection of working mode (as per the manual specs) */
+	usleep_range(100, 110);
+
+	ret = s32g_serdes_deassert_reset(serdes);
+	if (ret)
+		goto disable_clks;
+
+	dev_info(serdes->dev, "Using mode %d for SerDes subsystem\n",
+		 ctrl->ss_mode);
+
+	return 0;
+
+disable_clks:
+	clk_bulk_disable_unprepare(serdes->ctrl.nclks,
+				   serdes->ctrl.clks);
+
+	return ret;
+}
+
+static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
+{
+	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
+	struct device *dev = &pdev->dev;
+	int ret, idx;
+
+	ret = of_property_read_u32(dev->of_node, "nxp,sys-mode",
+				   &ctrl->ss_mode);
+	if (ret) {
+		dev_err(dev, "Failed to get SerDes subsystem mode\n");
+		return -EINVAL;
+	}
+
+	if (ctrl->ss_mode > S32G_SERDES_MODE_MAX) {
+		dev_err(dev, "Invalid SerDes subsystem mode %u\n",
+			ctrl->ss_mode);
+		return -EINVAL;
+	}
+
+	ctrl->ss_base = devm_platform_ioremap_resource_byname(pdev, "ss_pcie");
+	if (IS_ERR(ctrl->ss_base)) {
+		dev_err(dev, "Failed to map 'ss_pcie'\n");
+		return PTR_ERR(ctrl->ss_base);
+	}
+
+	ctrl->rst = devm_reset_control_get(dev, "serdes");
+	if (IS_ERR(ctrl->rst))
+		return dev_err_probe(dev, PTR_ERR(ctrl->rst),
+				     "Failed to get 'serdes' reset control\n");
+
+	ctrl->nclks = devm_clk_bulk_get_all(dev, &ctrl->clks);
+	if (ctrl->nclks < 1)
+		return dev_err_probe(dev, ctrl->nclks,
+				     "Failed to get SerDes clocks\n");
+
+	idx = of_property_match_string(dev->of_node, "clock-names", EXTERNAL_CLK_NAME);
+	if (idx < 0)
+		idx = of_property_match_string(dev->of_node, "clock-names", INTERNAL_CLK_NAME);
+	else
+		ctrl->ext_clk = true;
+
+	if (idx < 0) {
+		dev_err(dev, "Failed to get Phy reference clock source\n");
+		return -EINVAL;
+	}
+
+	ctrl->ref_clk_rate = clk_get_rate(ctrl->clks[idx].clk);
+	if (!ctrl->ref_clk_rate) {
+		dev_err(dev, "Failed to get Phy reference clock rate\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int s32g_serdes_get_pcie_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
+{
+	struct s32g_pcie_ctrl *pcie = &serdes->pcie;
+	struct device *dev = &pdev->dev;
+
+	pcie->phy_base = devm_platform_ioremap_resource_byname(pdev,
+							       "pcie_phy");
+	if (IS_ERR(pcie->phy_base)) {
+		dev_err(dev, "Failed to map 'pcie_phy'\n");
+		return PTR_ERR(pcie->phy_base);
+	}
+
+	pcie->rst = devm_reset_control_get(dev, "pcie");
+	if (IS_ERR(pcie->rst))
+		return dev_err_probe(dev, IS_ERR(pcie->rst),
+				     "Failed to get 'pcie' reset control\n");
+
+	return 0;
+}
+
+static int s32g2_serdes_create_phy(struct s32g_serdes *serdes, struct device_node *child_node)
+{
+	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
+	struct phy_provider *phy_provider;
+	struct device *dev = serdes->dev;
+	int ss_mode = ctrl->ss_mode;
+	struct phy *phy;
+
+	if (of_device_is_compatible(child_node, "nxp,s32g2-serdes-pcie-phy")) {
+		/* no PCIe phy lane */
+		if (ss_mode > 2)
+			return 0;
+
+		phy = devm_phy_create(dev, child_node, &serdes_pcie_ops);
+		if (IS_ERR(phy))
+			return PTR_ERR(phy);
+
+		phy_set_drvdata(phy, serdes);
+
+		phy->attrs.mode = PHY_MODE_PCIE;
+		phy->id = 0;
+		serdes->pcie.phy = phy;
+
+		phy_provider = devm_of_phy_provider_register(&phy->dev, s32g_serdes_phy_xlate);
+		if (IS_ERR(phy_provider))
+			return PTR_ERR(phy_provider);
+
+	} else {
+		dev_warn(dev, "Skipping unknown child node %pOFn\n", child_node);
+	}
+
+	return 0;
+}
+
+static int s32g_serdes_parse_lanes(struct device *dev, struct s32g_serdes *serdes)
+{
+	int ret;
+
+	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
+		ret = s32g2_serdes_create_phy(serdes, of_port);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static int s32g_serdes_probe(struct platform_device *pdev)
+{
+	struct s32g_serdes *serdes;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	serdes = devm_kzalloc(dev, sizeof(*serdes), GFP_KERNEL);
+	if (!serdes)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, serdes);
+	serdes->dev = dev;
+
+	ret = s32g_serdes_get_ctrl_resources(pdev, serdes);
+	if (ret)
+		return ret;
+
+	ret = s32g_serdes_get_pcie_resources(pdev, serdes);
+	if (ret)
+		return ret;
+
+	ret = s32g_serdes_parse_lanes(dev, serdes);
+	if (ret)
+		return ret;
+
+	ret = s32g_serdes_init(serdes);
+
+	return ret;
+}
+
+static int __maybe_unused s32g_serdes_suspend(struct device *device)
+{
+	struct s32g_serdes *serdes = dev_get_drvdata(device);
+
+	clk_bulk_disable_unprepare(serdes->ctrl.nclks, serdes->ctrl.clks);
+
+	return 0;
+}
+
+static int __maybe_unused s32g_serdes_resume(struct device *device)
+{
+	struct s32g_serdes *serdes = dev_get_drvdata(device);
+	struct s32g_pcie_ctrl *pcie = &serdes->pcie;
+	int ret;
+
+	ret = s32g_serdes_init(serdes);
+	if (ret) {
+		dev_err(device, "Failed to initialize\n");
+		return ret;
+	}
+
+	/* Restore PCIe phy power */
+	if (pcie->powered_on) {
+		ret = s32g_pcie_phy_power_on(serdes);
+		if (ret)
+			dev_err(device, "Failed to power-on PCIe phy\n");
+	}
+
+	return ret;
+}
+
+static const struct of_device_id s32g_serdes_match[] = {
+	{
+		.compatible = "nxp,s32g2-serdes",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, s32g_serdes_match);
+
+static const struct dev_pm_ops s32g_serdes_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(s32g_serdes_suspend,
+				  s32g_serdes_resume)
+};
+
+static struct platform_driver s32g_serdes_driver = {
+	.probe		= s32g_serdes_probe,
+	.driver		= {
+		.name	= "phy-s32g-serdes",
+		.of_match_table = s32g_serdes_match,
+		.pm = &s32g_serdes_pm_ops,
+	},
+};
+module_platform_driver(s32g_serdes_driver);
+
+MODULE_AUTHOR("Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>");
+MODULE_DESCRIPTION("S32CC SerDes driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Philipp Zabel]

On Mo, 2026-01-26 at 10:21 +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.
> 
> Implement PCIe phy 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 <alexandru-catalin.ionita@nxp.com>
> Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Co-developed-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> Signed-off-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> Co-developed-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> Signed-off-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>  drivers/phy/freescale/Kconfig               |   9 +
>  drivers/phy/freescale/Makefile              |   1 +
>  drivers/phy/freescale/phy-nxp-s32g-serdes.c | 569 ++++++++++++++++++++
>  3 files changed, 579 insertions(+)
>  create mode 100644 drivers/phy/freescale/phy-nxp-s32g-serdes.c
> 
[...]
> diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> new file mode 100644
> index 000000000000..8336c868c8dc
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> @@ -0,0 +1,569 @@
[...]
> +static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> +{
[...]
> +	ctrl->rst = devm_reset_control_get(dev, "serdes");

Please use devm_reset_control_get_exclusive() directly.

[...]
> +static int s32g_serdes_get_pcie_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> +{
[...]
> +	pcie->rst = devm_reset_control_get(dev, "pcie");

Same here.

regards
Philipp

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Vincent Guittot]

On Mon, 26 Jan 2026 at 14:11, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Mo, 2026-01-26 at 10:21 +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.
> >
> > Implement PCIe phy 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 <alexandru-catalin.ionita@nxp.com>
> > Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > Co-developed-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> > Signed-off-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> > Co-developed-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> > Signed-off-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >  drivers/phy/freescale/Kconfig               |   9 +
> >  drivers/phy/freescale/Makefile              |   1 +
> >  drivers/phy/freescale/phy-nxp-s32g-serdes.c | 569 ++++++++++++++++++++
> >  3 files changed, 579 insertions(+)
> >  create mode 100644 drivers/phy/freescale/phy-nxp-s32g-serdes.c
> >
> [...]
> > diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> > new file mode 100644
> > index 000000000000..8336c868c8dc
> > --- /dev/null
> > +++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> > @@ -0,0 +1,569 @@
> [...]
> > +static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> > +{
> [...]
> > +     ctrl->rst = devm_reset_control_get(dev, "serdes");
>
> Please use devm_reset_control_get_exclusive() directly.

Okay

>
> [...]
> > +static int s32g_serdes_get_pcie_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> > +{
> [...]
> > +     pcie->rst = devm_reset_control_get(dev, "pcie");
>
> Same here.
>
> regards
> Philipp

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Simon Horman]

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

...

> diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> new file mode 100644
> index 000000000000..8336c868c8dc
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> @@ -0,0 +1,569 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * SerDes driver for S32G SoCs
> + *
> + * Copyright 2021-2026 NXP
> + */
> +
> +#include <dt-bindings/phy/phy.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>

Hi Vincent, all,

I think that you also need:

#include <linux/iopoll.h>

So that read_poll_timeout() is declared.
Else this patch causes a transient build failure
(for x86_64 allmodconfig)

> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/processor.h>
> +#include <linux/reset.h>
> +#include <linux/units.h>

...

> +static int s32g_serdes_phy_set_mode_ext(struct phy *p,
> +					enum phy_mode mode, int submode)
> +{
> +	struct s32g_serdes *serdes = phy_get_drvdata(p);
> +
> +	if (mode == PHY_MODE_PCIE)
> +		return -EINVAL;

This is part of an AI Generated review.
I have looked over it and I think it warrants investigation.
For information on how to reproduce locally, as I did, please see [1].

[1] https://netdev-ai.bots.linux.dev/ai-local.html

  This returns error if mode IS PHY_MODE_PCIE, but this is a PCIe PHY!
  This looks like a typo. Should be !=.

> +
> +	if (!is_pcie_phy_mode_valid(submode))
> +		return -EINVAL;
> +
> +	/*
> +	 * Do not configure SRIS or CRSS PHY MODE in conjunction
> +	 * with any SGMII mode on the same SerDes subsystem
> +	 */
> +	if ((submode == CRSS || submode == SRIS) &&
> +	    serdes->ctrl.ss_mode != 0)
> +		return -EINVAL;
> +
> +	/*
> +	 * Internal reference clock cannot be used with either Common clock
> +	 * or Spread spectrum, leaving only SRNSS
> +	 */
> +	if (submode != SRNS &&  !serdes->ctrl.ext_clk)
> +		return -EINVAL;
> +
> +	serdes->pcie.phy_mode = submode;

The AI review also suggested that it may be unsafe
to set the submode after s32g_serdes_phy_power_on()
has been called. And that there is nothing preventing that.

TBH, I am unsure if either of those statements are true.
But it seems worth validating with you.

> +
> +	return 0;
> +}

...

> +static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> +{
> +	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> +	struct device *dev = &pdev->dev;
> +	int ret, idx;
> +
> +	ret = of_property_read_u32(dev->of_node, "nxp,sys-mode",
> +				   &ctrl->ss_mode);
> +	if (ret) {
> +		dev_err(dev, "Failed to get SerDes subsystem mode\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ctrl->ss_mode > S32G_SERDES_MODE_MAX) {
> +		dev_err(dev, "Invalid SerDes subsystem mode %u\n",
> +			ctrl->ss_mode);
> +		return -EINVAL;
> +	}
> +
> +	ctrl->ss_base = devm_platform_ioremap_resource_byname(pdev, "ss_pcie");
> +	if (IS_ERR(ctrl->ss_base)) {
> +		dev_err(dev, "Failed to map 'ss_pcie'\n");
> +		return PTR_ERR(ctrl->ss_base);
> +	}
> +
> +	ctrl->rst = devm_reset_control_get(dev, "serdes");
> +	if (IS_ERR(ctrl->rst))
> +		return dev_err_probe(dev, PTR_ERR(ctrl->rst),
> +				     "Failed to get 'serdes' reset control\n");
> +
> +	ctrl->nclks = devm_clk_bulk_get_all(dev, &ctrl->clks);
> +	if (ctrl->nclks < 1)
> +		return dev_err_probe(dev, ctrl->nclks,
> +				     "Failed to get SerDes clocks\n");

If devm_clk_bulk_get_all returns 0 then this value will
be passed to dev_err_probe(). And 0 will, in turn be returned by
dev_err_probe() and this function. However, that will be treated
as success by the caller, even though this is an error condition.

Perhaps something like this is more appropriate if ctrl->nclks
must be greater than 0. (Completely untested!)

	if (ctrl->nclks < 1) {
		ret = ctrl->nclks ? : -EINVAL;
		return dev_err_probe(dev, ret,
				     "Failed to get SerDes clocks\n");
	}

Flagged by Smatch.

...

> +static int s32g_serdes_parse_lanes(struct device *dev, struct s32g_serdes *serdes)
> +{
> +	int ret;
> +
> +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +		ret = s32g2_serdes_create_phy(serdes, of_port);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;

Perhaps it cannot occur.
But if the loop above iterates zero times,
then ret will be used uninitialised here.

Also flagged by Smatch.

> +}
> +
> +static int s32g_serdes_probe(struct platform_device *pdev)
> +{
> +	struct s32g_serdes *serdes;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	serdes = devm_kzalloc(dev, sizeof(*serdes), GFP_KERNEL);
> +	if (!serdes)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, serdes);
> +	serdes->dev = dev;
> +
> +	ret = s32g_serdes_get_ctrl_resources(pdev, serdes);
> +	if (ret)
> +		return ret;
> +
> +	ret = s32g_serdes_get_pcie_resources(pdev, serdes);
> +	if (ret)
> +		return ret;
> +
> +	ret = s32g_serdes_parse_lanes(dev, serdes);
> +	if (ret)
> +		return ret;

The I review also says:

  The probe function calls s32g_serdes_init() which enables clocks,
  configures hardware, and deasserts reset. However,
  s32g_serdes_parse_lanes() creates PHY providers via
  devm_of_phy_provider_register().

  Problem: PHY consumers can start calling PHY ops (like power_on) as soon
  as the provider is registered, but the hardware isn't initialized until
  s32g_serdes_init() runs afterward. This creates a race window.

  Recommendation: Move s32g_serdes_init() before s32g_serdes_parse_lanes().

> +
> +	ret = s32g_serdes_init(serdes);
> +
> +	return ret;

nit: This could be more succinctly written as:

	return s32g_serdes_init(serdes);

> +}

...

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Vincent Guittot]

On Thu, 29 Jan 2026 at 10:54, Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Jan 26, 2026 at 10:21:57AM +0100, Vincent Guittot wrote:
>
> ...
>
> > diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> > new file mode 100644
> > index 000000000000..8336c868c8dc
> > --- /dev/null
> > +++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> > @@ -0,0 +1,569 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * SerDes driver for S32G SoCs
> > + *
> > + * Copyright 2021-2026 NXP
> > + */
> > +
> > +#include <dt-bindings/phy/phy.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
>
> Hi Vincent, all,
>
> I think that you also need:
>
> #include <linux/iopoll.h>
>
> So that read_poll_timeout() is declared.
> Else this patch causes a transient build failure
> (for x86_64 allmodconfig)

ok, i will add it

>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/processor.h>
> > +#include <linux/reset.h>
> > +#include <linux/units.h>
>
> ...
>
> > +static int s32g_serdes_phy_set_mode_ext(struct phy *p,
> > +                                     enum phy_mode mode, int submode)
> > +{
> > +     struct s32g_serdes *serdes = phy_get_drvdata(p);
> > +
> > +     if (mode == PHY_MODE_PCIE)
> > +             return -EINVAL;
>
> This is part of an AI Generated review.
> I have looked over it and I think it warrants investigation.
> For information on how to reproduce locally, as I did, please see [1].
>
> [1] https://netdev-ai.bots.linux.dev/ai-local.html
>
>   This returns error if mode IS PHY_MODE_PCIE, but this is a PCIe PHY!
>   This looks like a typo. Should be !=.

yes don't know what happened here but it should be !=

>
> > +
> > +     if (!is_pcie_phy_mode_valid(submode))
> > +             return -EINVAL;
> > +
> > +     /*
> > +      * Do not configure SRIS or CRSS PHY MODE in conjunction
> > +      * with any SGMII mode on the same SerDes subsystem
> > +      */
> > +     if ((submode == CRSS || submode == SRIS) &&
> > +         serdes->ctrl.ss_mode != 0)
> > +             return -EINVAL;
> > +
> > +     /*
> > +      * Internal reference clock cannot be used with either Common clock
> > +      * or Spread spectrum, leaving only SRNSS
> > +      */
> > +     if (submode != SRNS &&  !serdes->ctrl.ext_clk)
> > +             return -EINVAL;
> > +
> > +     serdes->pcie.phy_mode = submode;
>
> The AI review also suggested that it may be unsafe
> to set the submode after s32g_serdes_phy_power_on()
> has been called. And that there is nothing preventing that.
>
> TBH, I am unsure if either of those statements are true.
> But it seems worth validating with you.

yes, the usual pattern is :
- phy_set_mode_ext()
- then phy_power_on()
but I can add an additional check

>
> > +
> > +     return 0;
> > +}
>
> ...
>
> > +static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> > +{
> > +     struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> > +     struct device *dev = &pdev->dev;
> > +     int ret, idx;
> > +
> > +     ret = of_property_read_u32(dev->of_node, "nxp,sys-mode",
> > +                                &ctrl->ss_mode);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to get SerDes subsystem mode\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (ctrl->ss_mode > S32G_SERDES_MODE_MAX) {
> > +             dev_err(dev, "Invalid SerDes subsystem mode %u\n",
> > +                     ctrl->ss_mode);
> > +             return -EINVAL;
> > +     }
> > +
> > +     ctrl->ss_base = devm_platform_ioremap_resource_byname(pdev, "ss_pcie");
> > +     if (IS_ERR(ctrl->ss_base)) {
> > +             dev_err(dev, "Failed to map 'ss_pcie'\n");
> > +             return PTR_ERR(ctrl->ss_base);
> > +     }
> > +
> > +     ctrl->rst = devm_reset_control_get(dev, "serdes");
> > +     if (IS_ERR(ctrl->rst))
> > +             return dev_err_probe(dev, PTR_ERR(ctrl->rst),
> > +                                  "Failed to get 'serdes' reset control\n");
> > +
> > +     ctrl->nclks = devm_clk_bulk_get_all(dev, &ctrl->clks);
> > +     if (ctrl->nclks < 1)
> > +             return dev_err_probe(dev, ctrl->nclks,
> > +                                  "Failed to get SerDes clocks\n");
>
> If devm_clk_bulk_get_all returns 0 then this value will
> be passed to dev_err_probe(). And 0 will, in turn be returned by
> dev_err_probe() and this function. However, that will be treated
> as success by the caller, even though this is an error condition.
>
> Perhaps something like this is more appropriate if ctrl->nclks
> must be greater than 0. (Completely untested!)
>
>         if (ctrl->nclks < 1) {
>                 ret = ctrl->nclks ? : -EINVAL;
>                 return dev_err_probe(dev, ret,
>                                      "Failed to get SerDes clocks\n");
>         }
>
> Flagged by Smatch.

okay

>
> ...
>
> > +static int s32g_serdes_parse_lanes(struct device *dev, struct s32g_serdes *serdes)
> > +{
> > +     int ret;
> > +
> > +     for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> > +             ret = s32g2_serdes_create_phy(serdes, of_port);
> > +             if (ret)
> > +                     break;
> > +     }
> > +
> > +     return ret;
>
> Perhaps it cannot occur.
> But if the loop above iterates zero times,
> then ret will be used uninitialised here.

should not but will fix it

>
> Also flagged by Smatch.
>
> > +}
> > +
> > +static int s32g_serdes_probe(struct platform_device *pdev)
> > +{
> > +     struct s32g_serdes *serdes;
> > +     struct device *dev = &pdev->dev;
> > +     int ret;
> > +
> > +     serdes = devm_kzalloc(dev, sizeof(*serdes), GFP_KERNEL);
> > +     if (!serdes)
> > +             return -ENOMEM;
> > +
> > +     platform_set_drvdata(pdev, serdes);
> > +     serdes->dev = dev;
> > +
> > +     ret = s32g_serdes_get_ctrl_resources(pdev, serdes);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = s32g_serdes_get_pcie_resources(pdev, serdes);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = s32g_serdes_parse_lanes(dev, serdes);
> > +     if (ret)
> > +             return ret;
>
> The I review also says:
>
>   The probe function calls s32g_serdes_init() which enables clocks,
>   configures hardware, and deasserts reset. However,
>   s32g_serdes_parse_lanes() creates PHY providers via
>   devm_of_phy_provider_register().
>
>   Problem: PHY consumers can start calling PHY ops (like power_on) as soon
>   as the provider is registered, but the hardware isn't initialized until
>   s32g_serdes_init() runs afterward. This creates a race window.
>
>   Recommendation: Move s32g_serdes_init() before s32g_serdes_parse_lanes().

I will look at this more deeply but part of s32g_serdes_init() needs
lanes to be parsed for configuring clock

>
> > +
> > +     ret = s32g_serdes_init(serdes);
> > +
> > +     return ret;
>
> nit: This could be more succinctly written as:

fair enough

>
>         return s32g_serdes_init(serdes);
>
> > +}
>
> ...

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Russell King (Oracle)]

On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote:
> yes, the usual pattern is :
> - phy_set_mode_ext()
> - then phy_power_on()
> but I can add an additional check

Please read Documentation/driver-api/phy/phy.rst section "Order of API
calls" which suggests phy_set_mode_ext() after phy_power_on().

Having different requirements for different SerDes PHYs quickly becomes
annoying for users of the SerDes PHYs.

E.g. I'm trying to add SerDes PHY support to stmmac, which is used
across different platforms. Having all SerDes PHY drivers behave the
same as far as their PHY API calls are concerned means that the whole
point of having an abstracted interface is maintained. Otherwise, it's
completely pointless.

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

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Vincent Guittot]

On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote:
> > yes, the usual pattern is :
> > - phy_set_mode_ext()
> > - then phy_power_on()
> > but I can add an additional check
>
> Please read Documentation/driver-api/phy/phy.rst section "Order of API
> calls" which suggests phy_set_mode_ext() after phy_power_on().

Fair enough.
That being said, all pcie drivers  that use phy_set_mode_ext(), call
it before phy_power_on()

>
> Having different requirements for different SerDes PHYs quickly becomes
> annoying for users of the SerDes PHYs.
>
> E.g. I'm trying to add SerDes PHY support to stmmac, which is used
> across different platforms. Having all SerDes PHY drivers behave the
> same as far as their PHY API calls are concerned means that the whole
> point of having an abstracted interface is maintained. Otherwise, it's
> completely pointless.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Russell King (Oracle)]

On Thu, Jan 29, 2026 at 02:36:01PM +0100, Vincent Guittot wrote:
> On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote:
> > > yes, the usual pattern is :
> > > - phy_set_mode_ext()
> > > - then phy_power_on()
> > > but I can add an additional check
> >
> > Please read Documentation/driver-api/phy/phy.rst section "Order of API
> > calls" which suggests phy_set_mode_ext() after phy_power_on().
> 
> Fair enough.
> That being said, all pcie drivers  that use phy_set_mode_ext(), call
> it before phy_power_on()

It looks like many ethernet drivers do the same, so I think maybe the
generic PHY documentation is incorrect or misleading, or is expressing
a preference that almost no one follows. Something for the generic PHY
maintainers to look at and/or comment on.

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

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Vinod Koul]

On 29-01-26, 13:51, Russell King (Oracle) wrote:
> On Thu, Jan 29, 2026 at 02:36:01PM +0100, Vincent Guittot wrote:
> > On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote:
> > > > yes, the usual pattern is :
> > > > - phy_set_mode_ext()
> > > > - then phy_power_on()
> > > > but I can add an additional check
> > >
> > > Please read Documentation/driver-api/phy/phy.rst section "Order of API
> > > calls" which suggests phy_set_mode_ext() after phy_power_on().
> > 
> > Fair enough.
> > That being said, all pcie drivers  that use phy_set_mode_ext(), call
> > it before phy_power_on()
> 
> It looks like many ethernet drivers do the same, so I think maybe the
> generic PHY documentation is incorrect or misleading, or is expressing
> a preference that almost no one follows. Something for the generic PHY
> maintainers to look at and/or comment on.

I would feel it makes sense to configure the mode first and then power
the phy up. As commented above yes it looks like apart from one tegra
driver rest seem to do it this way.

Lets update the documentation

-- 
~Vinod

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Russell King (Oracle)]

On Thu, Jan 29, 2026 at 08:00:38PM +0530, Vinod Koul wrote:
> On 29-01-26, 13:51, Russell King (Oracle) wrote:
> > On Thu, Jan 29, 2026 at 02:36:01PM +0100, Vincent Guittot wrote:
> > > On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle)
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote:
> > > > > yes, the usual pattern is :
> > > > > - phy_set_mode_ext()
> > > > > - then phy_power_on()
> > > > > but I can add an additional check
> > > >
> > > > Please read Documentation/driver-api/phy/phy.rst section "Order of API
> > > > calls" which suggests phy_set_mode_ext() after phy_power_on().
> > > 
> > > Fair enough.
> > > That being said, all pcie drivers  that use phy_set_mode_ext(), call
> > > it before phy_power_on()
> > 
> > It looks like many ethernet drivers do the same, so I think maybe the
> > generic PHY documentation is incorrect or misleading, or is expressing
> > a preference that almost no one follows. Something for the generic PHY
> > maintainers to look at and/or comment on.
> 
> I would feel it makes sense to configure the mode first and then power
> the phy up. As commented above yes it looks like apart from one tegra
> driver rest seem to do it this way.
> 
> Lets update the documentation

Please also indicate in the documentation whether changing the submode
of the serdes (particularly for ethernet) is permitted without doing a
phy_power_down()..phy_power_up() dance around the phy_set_mode_ext()
call.

Thanks.

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

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Russell King (Oracle)]

On Thu, Jan 29, 2026 at 02:36:46PM +0000, Russell King (Oracle) wrote:
> On Thu, Jan 29, 2026 at 08:00:38PM +0530, Vinod Koul wrote:
> > On 29-01-26, 13:51, Russell King (Oracle) wrote:
> > > On Thu, Jan 29, 2026 at 02:36:01PM +0100, Vincent Guittot wrote:
> > > > On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle)
> > > > <linux@armlinux.org.uk> wrote:
> > > > >
> > > > > On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote:
> > > > > > yes, the usual pattern is :
> > > > > > - phy_set_mode_ext()
> > > > > > - then phy_power_on()
> > > > > > but I can add an additional check
> > > > >
> > > > > Please read Documentation/driver-api/phy/phy.rst section "Order of API
> > > > > calls" which suggests phy_set_mode_ext() after phy_power_on().
> > > > 
> > > > Fair enough.
> > > > That being said, all pcie drivers  that use phy_set_mode_ext(), call
> > > > it before phy_power_on()
> > > 
> > > It looks like many ethernet drivers do the same, so I think maybe the
> > > generic PHY documentation is incorrect or misleading, or is expressing
> > > a preference that almost no one follows. Something for the generic PHY
> > > maintainers to look at and/or comment on.
> > 
> > I would feel it makes sense to configure the mode first and then power
> > the phy up. As commented above yes it looks like apart from one tegra
> > driver rest seem to do it this way.
> > 
> > Lets update the documentation
> 
> Please also indicate in the documentation whether changing the submode
> of the serdes (particularly for ethernet) is permitted without doing a
> phy_power_down()..phy_power_up() dance around the phy_set_mode_ext()
> call.

Maybe something like this, which simply alters the documentation to
indicate that phy_set_mode*() is permissible prior to phy_power_on(),
and should be used at that point where drivers know the mode which
will be used.

Leaving the existing phy_set_mode*() in the sequence also indicates
that it's permissible to call this while the PHY is still powered
on.

For drivers such as stmmac, it will be important that details such as
whether phy_est_mode*() can be called with the PHY powered on are
riveted down and not left up to the generic PHY driver author - without
that, generic PHYs basically aren't usable from SoC/platform
independent code, and stmmac has bazillions of platform specific glue
already because of (a) bad code structuring and (b) lack of
generalisation through standardised interfaces that abstract platform
differences.

I want to be able for core stmmac code, or even phylink code (which
is even more platform generic) to be able to make use of generic PHY
stuff, but if the calls that can be made into generic PHY are platform
dependent, that is a blocking issue against that, and makes me question
why we have the generic PHY subsystem... it's not very generic if it
exposes the differences of each implementation to users of its
interfaces.

I think generic PHY has had the idea that its interfaces will only be
used from platform specific code that knows about the behaviour of it's
generic PHY driver, but as can be seen above, this will not remain the
case given that we have hardware designs where the core of the driver
is one vendor's IP that gets re-used across many different platforms,
but the SerDes PHY is one of many other vendor's IP.

diff --git a/Documentation/driver-api/phy/phy.rst b/Documentation/driver-api/phy/phy.rst
index 719a2b3fd2ab..cf73e4fb0951 100644
--- a/Documentation/driver-api/phy/phy.rst
+++ b/Documentation/driver-api/phy/phy.rst
@@ -142,6 +142,7 @@ Order of API calls
 
     [devm_][of_]phy_get()
     phy_init()
+    [phy_set_mode[_ext]()]
     phy_power_on()
     [phy_set_mode[_ext]()]
     ...
@@ -154,7 +155,7 @@ but controllers should always call these functions to be compatible with other
 PHYs. Some PHYs may require :c:func:`phy_set_mode <phy_set_mode_ext>`, while
 others may use a default mode (typically configured via devicetree or other
 firmware). For compatibility, you should always call this function if you know
-what mode you will be using. Generally, this function should be called after
+what mode you will be using. Generally, this function should be called before
 :c:func:`phy_power_on`, although some PHY drivers may allow it at any time.
 
 Releasing a reference to the PHY

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

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Russell King (Oracle)]

On Mon, Jan 26, 2026 at 10:21:57AM +0100, Vincent Guittot wrote:
> +/*
> + * Until now, there is no generic way to describe and set PCIe clock mode.
> + * PCIe controller uses the default CRNS = 0 mode.
> + */
> +enum pcie_phy_mode {
> +	CRNS = 0, /* Common Reference Clock, No Spread Spectrum */
> +	CRSS = 1, /* Common Reference Clock, Spread Spectrum */
> +	SRNS = 2, /* Separate Reference Clock, No Spread Spectrum */
> +	SRIS = 3  /* Separate Reference Clock, Spread Spectrum */
> +};

So this is a PCIe thing. If it's part of the driver's API, then it
should be common and not driver-private.

> +static inline bool is_pcie_phy_mode_valid(int mode)
> +{
> +	switch (mode) {
> +	case CRNS:
> +	case CRSS:
> +	case SRNS:
> +	case SRIS:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}

This checks that the submode is one of the PCIe private modes that this
driver wants to see.

> +
> +static int s32g_serdes_phy_set_mode_ext(struct phy *p,
> +					enum phy_mode mode, int submode)
> +{
> +	struct s32g_serdes *serdes = phy_get_drvdata(p);
> +
> +	if (mode == PHY_MODE_PCIE)
> +		return -EINVAL;
> +
> +	if (!is_pcie_phy_mode_valid(submode))
> +		return -EINVAL;

This checks for the PCIe submode, but notice the test immediately
above. PCIE mode is being rejected. So, this driver supports
everything else but PCIe.

That doesn't seem right.

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

Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy

[Vincent Guittot]

On Thu, 29 Jan 2026 at 12:17, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Mon, Jan 26, 2026 at 10:21:57AM +0100, Vincent Guittot wrote:
> > +/*
> > + * Until now, there is no generic way to describe and set PCIe clock mode.
> > + * PCIe controller uses the default CRNS = 0 mode.
> > + */
> > +enum pcie_phy_mode {
> > +     CRNS = 0, /* Common Reference Clock, No Spread Spectrum */
> > +     CRSS = 1, /* Common Reference Clock, Spread Spectrum */
> > +     SRNS = 2, /* Separate Reference Clock, No Spread Spectrum */
> > +     SRIS = 3  /* Separate Reference Clock, Spread Spectrum */
> > +};
>
> So this is a PCIe thing. If it's part of the driver's API, then it
> should be common and not driver-private.
>
> > +static inline bool is_pcie_phy_mode_valid(int mode)
> > +{
> > +     switch (mode) {
> > +     case CRNS:
> > +     case CRSS:
> > +     case SRNS:
> > +     case SRIS:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
>
> This checks that the submode is one of the PCIe private modes that this
> driver wants to see.
>
> > +
> > +static int s32g_serdes_phy_set_mode_ext(struct phy *p,
> > +                                     enum phy_mode mode, int submode)
> > +{
> > +     struct s32g_serdes *serdes = phy_get_drvdata(p);
> > +
> > +     if (mode == PHY_MODE_PCIE)
> > +             return -EINVAL;
> > +
> > +     if (!is_pcie_phy_mode_valid(submode))
> > +             return -EINVAL;
>
> This checks for the PCIe submode, but notice the test immediately
> above. PCIE mode is being rejected. So, this driver supports
> everything else but PCIe.
>
> That doesn't seem right.

It's a mistake

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

[PATCH 3/4] phy: s32g: Add serdes xpcs subsystem

[Vincent Guittot]

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 <alexandru-catalin.ionita@nxp.com>
Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Co-developed-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
Signed-off-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
Co-developed-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
Signed-off-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/phy/freescale/Kconfig               |    1 +
 drivers/phy/freescale/Makefile              |    2 +-
 drivers/phy/freescale/phy-nxp-s32g-serdes.c |  361 ++++++-
 drivers/phy/freescale/phy-nxp-s32g-xpcs.c   | 1082 +++++++++++++++++++
 drivers/phy/freescale/phy-nxp-s32g-xpcs.h   |   47 +
 include/linux/pcs/pcs-nxp-xpcs.h            |   13 +
 6 files changed, 1503 insertions(+), 3 deletions(-)
 create mode 100644 drivers/phy/freescale/phy-nxp-s32g-xpcs.c
 create mode 100644 drivers/phy/freescale/phy-nxp-s32g-xpcs.h
 create mode 100644 include/linux/pcs/pcs-nxp-xpcs.h

diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
index 45184a3cdd69..bb7f59897faf 100644
--- a/drivers/phy/freescale/Kconfig
+++ b/drivers/phy/freescale/Kconfig
@@ -66,6 +66,7 @@ config PHY_S32G_SERDES
 	tristate "NXP S32G SERDES support"
 	depends on ARCH_S32 || COMPILE_TEST
 	select GENERIC_PHY
+	select REGMAP
 	help
 	  This option enables support for S23G SerDes PHY used for
 	  PCIe & Ethernet
diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
index 86d948417252..2a1231cd466b 100644
--- a/drivers/phy/freescale/Makefile
+++ b/drivers/phy/freescale/Makefile
@@ -6,4 +6,4 @@ obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)	+= phy-fsl-imx8m-pcie.o
 obj-$(CONFIG_PHY_FSL_IMX8QM_HSIO)	+= phy-fsl-imx8qm-hsio.o
 obj-$(CONFIG_PHY_FSL_LYNX_28G)		+= phy-fsl-lynx-28g.o
 obj-$(CONFIG_PHY_FSL_SAMSUNG_HDMI_PHY)	+= phy-fsl-samsung-hdmi.o
-obj-$(CONFIG_PHY_S32G_SERDES)		+= phy-nxp-s32g-serdes.o
+obj-$(CONFIG_PHY_S32G_SERDES)		+= phy-nxp-s32g-serdes.o phy-nxp-s32g-xpcs.o
diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
index 8336c868c8dc..e0065c61a994 100644
--- a/drivers/phy/freescale/phy-nxp-s32g-serdes.c
+++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
@@ -11,12 +11,16 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/of_address.h>
+#include <linux/pcs/pcs-nxp-xpcs.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/processor.h>
 #include <linux/reset.h>
 #include <linux/units.h>
 
+#include "phy-nxp-s32g-xpcs.h"
+
+#define S32G_SERDES_XPCS_MAX			2
 #define S32G_SERDES_MODE_MAX			5
 
 #define EXTERNAL_CLK_NAME			"ext"
@@ -31,6 +35,52 @@
 #define S32G_PCIE_PHY_MPLLA_CTRL		0x10
 #define  MPLL_STATE				BIT(30)
 
+#define S32G_PCIE_PHY_MPLLB_CTRL		0x14
+#define  MPLLB_SSC_EN				BIT(1)
+
+#define S32G_PCIE_PHY_EXT_CTRL_SEL		0x18
+#define  EXT_PHY_CTRL_SEL			BIT(0)
+
+#define S32G_PCIE_PHY_EXT_BS_CTRL		0x1C
+#define  EXT_BS_TX_LOWSWING			BIT(6)
+#define  EXT_BS_RX_BIGSWING			BIT(5)
+#define  EXT_BS_RX_LEVEL_MASK			GENMASK(4, 0)
+
+#define S32G_PCIE_PHY_REF_CLK_CTRL		0x20
+#define  EXT_REF_RANGE_MASK			GENMASK(5, 3)
+#define  REF_CLK_DIV2_EN			BIT(2)
+#define  REF_CLK_MPLLB_DIV2_EN			BIT(1)
+
+#define S32G_PCIE_PHY_EXT_MPLLA_CTRL_1		0x30
+#define  EXT_MPLLA_BANDWIDTH_MASK		GENMASK(15, 0)
+
+#define S32G_PCIE_PHY_EXT_MPLLB_CTRL_1		0x40
+#define  EXT_MPLLB_DIV_MULTIPLIER_MASK		GENMASK(31, 24)
+#define  EXT_MPLLB_DIV_CLK_EN			BIT(19)
+#define  EXT_MPLLB_DIV8_CLK_EN			BIT(18)
+#define  EXT_MPLLB_DIV10_CLK_EN			BIT(16)
+#define  EXT_MPLLB_BANDWIDTH_MASK		GENMASK(15, 0)
+
+#define S32G_PCIE_PHY_EXT_MPLLB_CTRL_2		0x44
+#define  EXT_MPLLB_FRACN_CTRL_MASK		GENMASK(22, 12)
+#define  MPLLB_MULTIPLIER_MASK			GENMASK(8, 0)
+
+#define S32G_PCIE_PHY_EXT_MPLLB_CTRL_3		0x48
+#define  EXT_MPLLB_WORD_DIV2_EN			BIT(31)
+#define  EXT_MPLLB_TX_CLK_DIV_MASK		GENMASK(30, 28)
+
+#define S32G_PCIE_PHY_EXT_MISC_CTRL_1		0xA0
+#define  EXT_RX_LOS_THRESHOLD_MASK		GENMASK(7, 1)
+#define  EXT_RX_VREF_CTRL_MASK			GENMASK(28, 24)
+
+#define S32G_PCIE_PHY_EXT_MISC_CTRL_2		0xA4
+#define  EXT_TX_VBOOST_LVL_MASK			GENMASK(18, 16)
+#define  EXT_TX_TERM_CTRL_MASK			GENMASK(26, 24)
+
+#define S32G_PCIE_PHY_XPCS1_RX_OVRD_CTRL	0xD0
+#define  XPCS1_RX_VCO_LD_VAL_MASK		GENMASK(28, 16)
+#define  XPCS1_RX_REF_LD_VAL_MASK		GENMASK(14, 8)
+
 #define S32G_SS_RW_REG_0			0xF0
 #define  SUBMODE_MASK				GENMASK(3, 0)
 #define  CLKEN_MASK				BIT(23)
@@ -43,6 +93,9 @@
 
 #define S32G_PHY_REG_DATA			0x4
 
+#define S32G_PHY_RST_CTRL			0x8
+#define  WARM_RST				BIT(1)
+
 #define RAWLANE0_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN	0x3019
 #define RAWLANE1_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN	0x3119
 
@@ -75,16 +128,33 @@ struct s32g_pcie_ctrl {
 	bool powered_on;
 };
 
+struct s32g_xpcs_ctrl {
+	struct s32g_xpcs *phys[2];
+	void __iomem *base0, *base1;
+};
+
 struct s32g_serdes {
 	struct s32g_serdes_ctrl ctrl;
 	struct s32g_pcie_ctrl pcie;
+	struct s32g_xpcs_ctrl xpcs;
 	struct device *dev;
+	u8 lanes_status;
 };
 
 /* PCIe phy subsystem */
 
 #define S32G_SERDES_PCIE_FREQ			(100 * HZ_PER_MHZ)
 
+static void s32g_pcie_phy_reset(struct s32g_serdes *serdes)
+{
+	u32 val;
+
+	val = readl(serdes->pcie.phy_base + S32G_PHY_RST_CTRL);
+	writel(val | WARM_RST, serdes->pcie.phy_base + S32G_PHY_RST_CTRL);
+	usleep_range(1000, 1100);
+	writel(val, serdes->pcie.phy_base + S32G_PHY_RST_CTRL);
+}
+
 static int s32g_pcie_check_clk(struct s32g_serdes *serdes)
 {
 	struct s32g_serdes_ctrl *sctrl = &serdes->ctrl;
@@ -263,6 +333,187 @@ static struct phy *s32g_serdes_phy_xlate(struct device *dev,
 	return phy;
 }
 
+/* XPCS subsystem */
+
+static int s32g_serdes_xpcs_init(struct s32g_serdes *serdes, int id)
+{
+	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
+	struct s32g_xpcs_ctrl *xpcs = &serdes->xpcs;
+	enum pcie_xpcs_mode shared = NOT_SHARED;
+	unsigned long rate = ctrl->ref_clk_rate;
+	struct device *dev = serdes->dev;
+	void __iomem *base;
+
+	if (!id)
+		base = xpcs->base0;
+	else
+		base = xpcs->base1;
+
+	if (ctrl->ss_mode == 1 || ctrl->ss_mode == 2)
+		shared = PCIE_XPCS_1G;
+	else if (ctrl->ss_mode == 5)
+		shared = PCIE_XPCS_2G5;
+
+	return s32g_xpcs_init(xpcs->phys[id], dev, id, base,
+			       ctrl->ext_clk, rate, shared);
+}
+
+static void s32g_serdes_prepare_pma_mode5(struct s32g_serdes *serdes)
+{
+	u32 val;
+	/* Configure TX_VBOOST_LVL and TX_TERM_CTRL */
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
+	val &= ~(EXT_TX_VBOOST_LVL_MASK | EXT_TX_TERM_CTRL_MASK);
+	val |= FIELD_PREP(EXT_TX_VBOOST_LVL_MASK, 0x3) |
+		FIELD_PREP(EXT_TX_TERM_CTRL_MASK, 0x4);
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
+
+	/* Enable phy external control */
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_CTRL_SEL);
+	val |= EXT_PHY_CTRL_SEL;
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_CTRL_SEL);
+
+	/* Configure ref range, disable PLLB/ref div2 */
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_REF_CLK_CTRL);
+	val &= ~(REF_CLK_DIV2_EN | REF_CLK_MPLLB_DIV2_EN | EXT_REF_RANGE_MASK);
+	val |= FIELD_PREP(EXT_REF_RANGE_MASK, 0x3);
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_REF_CLK_CTRL);
+
+	/* Configure multiplier */
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MPLLB_CTRL_2);
+	val &= ~(MPLLB_MULTIPLIER_MASK | EXT_MPLLB_FRACN_CTRL_MASK | BIT(24) | BIT(28));
+	val |= FIELD_PREP(MPLLB_MULTIPLIER_MASK, 0x27U) |
+		FIELD_PREP(EXT_MPLLB_FRACN_CTRL_MASK, 0x414);
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MPLLB_CTRL_2);
+
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_MPLLB_CTRL);
+	val &= ~MPLLB_SSC_EN;
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_MPLLB_CTRL);
+
+	/* Configure tx lane division, disable word clock div2*/
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MPLLB_CTRL_3);
+	val &= ~(EXT_MPLLB_WORD_DIV2_EN | EXT_MPLLB_TX_CLK_DIV_MASK);
+	val |= FIELD_PREP(EXT_MPLLB_TX_CLK_DIV_MASK, 0x5);
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MPLLB_CTRL_3);
+
+	/* Configure bandwidth for filtering and div10*/
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MPLLB_CTRL_1);
+	val &= ~(EXT_MPLLB_BANDWIDTH_MASK | EXT_MPLLB_DIV_CLK_EN |
+		 EXT_MPLLB_DIV8_CLK_EN | EXT_MPLLB_DIV_MULTIPLIER_MASK);
+	val |= FIELD_PREP(EXT_MPLLB_BANDWIDTH_MASK, 0x5f) | EXT_MPLLB_DIV10_CLK_EN;
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MPLLB_CTRL_1);
+
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MPLLA_CTRL_1);
+	val &= ~(EXT_MPLLA_BANDWIDTH_MASK);
+	val |= FIELD_PREP(EXT_MPLLA_BANDWIDTH_MASK, 0xc5);
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MPLLA_CTRL_1);
+
+	/* Configure VCO */
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_XPCS1_RX_OVRD_CTRL);
+	val &= ~(XPCS1_RX_VCO_LD_VAL_MASK | XPCS1_RX_REF_LD_VAL_MASK);
+	val |= FIELD_PREP(XPCS1_RX_VCO_LD_VAL_MASK, 0x540) |
+		FIELD_PREP(XPCS1_RX_REF_LD_VAL_MASK, 0x2b);
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_XPCS1_RX_OVRD_CTRL);
+
+	/* Boundary scan control */
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_BS_CTRL);
+	val &= ~(EXT_BS_RX_LEVEL_MASK | EXT_BS_TX_LOWSWING);
+	val |= FIELD_PREP(EXT_BS_RX_LEVEL_MASK, 0xB) | EXT_BS_RX_BIGSWING;
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_BS_CTRL);
+
+	/* Rx loss threshold */
+	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_1);
+	val &= ~(EXT_RX_LOS_THRESHOLD_MASK | EXT_RX_VREF_CTRL_MASK);
+	val |= FIELD_PREP(EXT_RX_LOS_THRESHOLD_MASK, 0x3U) |
+		FIELD_PREP(EXT_RX_VREF_CTRL_MASK, 0x11U);
+	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_1);
+}
+
+#define XPCS_DISABLED	-1
+
+static int s32g_serdes_init_clks(struct s32g_serdes *serdes)
+{
+	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
+	struct s32g_xpcs_ctrl *xpcs = &serdes->xpcs;
+	int ret, order[2], xpcs_id;
+	size_t i;
+
+	switch (ctrl->ss_mode) {
+	case 0:
+		return 0;
+	case 1:
+		order[0] = 0;
+		order[1] = XPCS_DISABLED;
+		break;
+	case 2:
+	case 5:
+		order[0] = 1;
+		order[1] = XPCS_DISABLED;
+		break;
+	case 3:
+		order[0] = 1;
+		order[1] = 0;
+		break;
+	case 4:
+		order[0] = 0;
+		order[1] = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(order); i++) {
+		xpcs_id = order[i];
+
+		if (xpcs_id == XPCS_DISABLED)
+			continue;
+
+		ret = s32g_xpcs_init_plls(xpcs->phys[xpcs_id]);
+		if (ret)
+			return ret;
+	}
+
+	if (ctrl->ss_mode == 5) {
+		s32g_serdes_prepare_pma_mode5(serdes);
+
+		ret = s32g_xpcs_pre_pcie_2g5(xpcs->phys[1]);
+		if (ret) {
+			dev_err(serdes->dev,
+				"Failed to prepare SerDes for PCIE & XPCS @ 2G5 mode\n");
+			return ret;
+		}
+
+		s32g_pcie_phy_reset(serdes);
+	} else {
+		for (i = 0; i < ARRAY_SIZE(order); i++) {
+			xpcs_id = order[i];
+
+			if (xpcs_id == XPCS_DISABLED)
+				continue;
+
+			ret = s32g_xpcs_vreset(xpcs->phys[xpcs_id]);
+			if (ret)
+				return ret;
+		}
+	}
+
+	for (i = 0; i < ARRAY_SIZE(order); i++) {
+		xpcs_id = order[i];
+
+		if (xpcs_id == XPCS_DISABLED)
+			continue;
+
+		ret = s32g_xpcs_wait_vreset(xpcs->phys[xpcs_id]);
+		if (ret)
+			return ret;
+
+		s32g_xpcs_reset_rx(xpcs->phys[xpcs_id]);
+		s32g_xpcs_disable_an(xpcs->phys[xpcs_id]);
+	}
+
+	return 0;
+}
+
 /* Serdes subsystem */
 
 static int s32g_serdes_assert_reset(struct s32g_serdes *serdes)
@@ -317,6 +568,10 @@ static int s32g_serdes_init(struct s32g_serdes *serdes)
 		return ret;
 	}
 
+	/*
+	 * We have a tight timing for the init sequence and any delay linked to
+	 * printk as an example can fail the init after reset
+	 */
 	ret = s32g_serdes_assert_reset(serdes);
 	if (ret)
 		goto disable_clks;
@@ -349,7 +604,13 @@ static int s32g_serdes_init(struct s32g_serdes *serdes)
 	dev_info(serdes->dev, "Using mode %d for SerDes subsystem\n",
 		 ctrl->ss_mode);
 
-	return 0;
+	ret = s32g_serdes_init_clks(serdes);
+	if (ret) {
+		dev_err(serdes->dev, "XPCS init failed\n");
+		goto disable_clks;
+	}
+
+	return ret;
 
 disable_clks:
 	clk_bulk_disable_unprepare(serdes->ctrl.nclks,
@@ -433,12 +694,32 @@ static int s32g_serdes_get_pcie_resources(struct platform_device *pdev, struct s
 	return 0;
 }
 
+static int s32g_serdes_get_xpcs_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
+{
+	struct s32g_xpcs_ctrl *xpcs = &serdes->xpcs;
+	struct device *dev = &pdev->dev;
+
+	xpcs->base0 = devm_platform_ioremap_resource_byname(pdev, "xpcs0");
+	if (IS_ERR(xpcs->base0)) {
+		dev_err(dev, "Failed to map 'xpcs0'\n");
+		return PTR_ERR(xpcs->base0);
+	}
+
+	xpcs->base1 = devm_platform_ioremap_resource_byname(pdev, "xpcs1");
+	if (IS_ERR(xpcs->base1)) {
+		dev_err(dev, "Failed to map 'xpcs1'\n");
+		return PTR_ERR(xpcs->base1);
+	}
+
+	return 0;
+}
+
 static int s32g2_serdes_create_phy(struct s32g_serdes *serdes, struct device_node *child_node)
 {
 	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
 	struct phy_provider *phy_provider;
 	struct device *dev = serdes->dev;
-	int ss_mode = ctrl->ss_mode;
+	int ret, ss_mode = ctrl->ss_mode;
 	struct phy *phy;
 
 	if (of_device_is_compatible(child_node, "nxp,s32g2-serdes-pcie-phy")) {
@@ -460,6 +741,41 @@ static int s32g2_serdes_create_phy(struct s32g_serdes *serdes, struct device_nod
 		if (IS_ERR(phy_provider))
 			return PTR_ERR(phy_provider);
 
+	} else if (of_device_is_compatible(child_node, "nxp,s32g2-serdes-xpcs")) {
+		struct s32g_xpcs_ctrl *xpcs_ctrl = &serdes->xpcs;
+		struct s32g_xpcs *xpcs;
+		int port;
+
+		/* no Ethernet phy lane */
+		if (ss_mode == 0)
+			return 0;
+
+		/* Get XPCS port number connected to the lane */
+		if (of_property_read_u32(child_node, "reg", &port))
+			return -EINVAL;
+
+		/* XPCS1 is not used */
+		if (ss_mode == 1 && port == 1)
+			return -EINVAL;
+
+		/* XPCS0 is not used */
+		if (ss_mode == 2 && port == 0)
+			return -EINVAL;
+
+		xpcs = devm_kmalloc(dev, sizeof(*xpcs), GFP_KERNEL);
+		if (IS_ERR(xpcs)) {
+			dev_err(dev, "Failed to allocate xpcs\n");
+			return -ENOMEM;
+		}
+
+		xpcs_ctrl->phys[port] = xpcs;
+
+		xpcs->an = of_property_read_bool(dev->of_node, "nxp,xpcs_an");
+
+		ret = s32g_serdes_xpcs_init(serdes, port);
+		if (ret)
+			return ret;
+
 	} else {
 		dev_warn(dev, "Skipping unknown child node %pOFn\n", child_node);
 	}
@@ -501,6 +817,10 @@ static int s32g_serdes_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = s32g_serdes_get_xpcs_resources(pdev, serdes);
+	if (ret)
+		return ret;
+
 	ret = s32g_serdes_parse_lanes(dev, serdes);
 	if (ret)
 		return ret;
@@ -541,6 +861,43 @@ static int __maybe_unused s32g_serdes_resume(struct device *device)
 	return ret;
 }
 
+struct phylink_pcs *s32g_serdes_pcs_create(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct device_node *pcs_np;
+	struct s32g_serdes *serdes;
+	u32 port;
+
+	if (of_property_read_u32(np, "reg", &port))
+		return ERR_PTR(-EINVAL);
+
+	if (port > S32G_SERDES_XPCS_MAX)
+		return ERR_PTR(-EINVAL);
+
+	/* The PCS pdev is attached to the parent node */
+	pcs_np = of_get_parent(np);
+	if (!pcs_np)
+		return ERR_PTR(-ENODEV);
+
+	if (!of_device_is_available(pcs_np)) {
+		of_node_put(pcs_np);
+		return ERR_PTR(-ENODEV);
+	}
+
+	pdev = of_find_device_by_node(pcs_np);
+	of_node_put(pcs_np);
+	if (!pdev || !platform_get_drvdata(pdev)) {
+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	serdes = platform_get_drvdata(pdev);
+
+	return &serdes->xpcs.phys[port]->pcs;
+}
+EXPORT_SYMBOL(s32g_serdes_pcs_create);
+
 static const struct of_device_id s32g_serdes_match[] = {
 	{
 		.compatible = "nxp,s32g2-serdes",
diff --git a/drivers/phy/freescale/phy-nxp-s32g-xpcs.c b/drivers/phy/freescale/phy-nxp-s32g-xpcs.c
new file mode 100644
index 000000000000..c49bdaecc034
--- /dev/null
+++ b/drivers/phy/freescale/phy-nxp-s32g-xpcs.c
@@ -0,0 +1,1082 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Copyright 2021-2026 NXP
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/units.h>
+#include <linux/io.h>
+#include <linux/processor.h>
+#include <linux/regmap.h>
+#include "phy-nxp-s32g-xpcs.h"
+
+#define XPCS_TIMEOUT_MS				300
+
+#define ADDR1_OFS				0x3FC
+
+#define SR_MII_CTRL				0x1F0000
+#define   SR_RST				BIT(15)
+#define   SS13					BIT(13)
+#define   AN_ENABLE				BIT(12)
+#define   RESTART_AN				BIT(9)
+#define   DUPLEX_MODE				BIT(8)
+#define   SS6					BIT(6)
+#define SR_MII_STS				0x1F0001
+#define   LINK_STS				BIT(2)
+#define   AN_ABL				BIT(3)
+#define   AN_CMPL				BIT(5)
+#define SR_MII_DEV_ID1				0x1F0002
+#define SR_MII_DEV_ID2				0x1F0003
+#define SR_MII_EXT_STS				0x1F000F
+#define   CAP_1G_T_FD				BIT(13)
+#define   CAP_1G_T_HD				BIT(12)
+#define VR_MII_DIG_CTRL1			0x1F8000
+#define   BYP_PWRUP				BIT(1)
+#define   EN_2_5G_MODE				BIT(2)
+#define   CL37_TMR_OVRRIDE			BIT(3)
+#define   INIT					BIT(8)
+#define   MAC_AUTO_SW				BIT(9)
+#define   CS_EN					BIT(10)
+#define   PWRSV					BIT(11)
+#define   EN_VSMMD1				BIT(13)
+#define   R2TLBE				BIT(14)
+#define   VR_RST				BIT(15)
+#define VR_MII_AN_CTRL				0x1F8001
+#define   MII_AN_INTR_EN			BIT(0)
+#define   PCS_MODE_MASK				GENMASK(2, 1)
+#define    PCS_MODE_SGMII			2
+#define   MII_CTRL				BIT(8)
+#define VR_MII_AN_INTR_STS			0x1F8002
+#define  CL37_ANCMPLT_INTR			BIT(0)
+#define  CL37_ANSGM_STS_DUPLEX			BIT(1)
+#define  CL37_ANSGM_STS_SPEED_MASK		GENMASK(3, 2)
+#define   CL37_ANSGM_10MBPS			0
+#define   CL37_ANSGM_100MBPS			1
+#define   CL37_ANSGM_1000MBPS			2
+#define  CL37_ANSGM_STS_LINK			BIT(4)
+#define VR_MII_DBG_CTRL				0x1F8005
+#define   SUPPRESS_LOS_DET			BIT(4)
+#define   RX_DT_EN_CTL				BIT(6)
+#define VR_MII_LINK_TIMER_CTRL			0x1F800A
+#define VR_MII_DIG_STS				0x1F8010
+#define   PSEQ_STATE_MASK			GENMASK(4, 2)
+#define     POWER_GOOD_STATE			0x4
+#define VR_MII_GEN5_12G_16G_TX_GENCTRL1		0x1F8031
+#define   VBOOST_EN_0				BIT(4)
+#define   TX_CLK_RDY_0				BIT(12)
+#define	VR_MII_GEN5_12G_16G_TX_GENCTRL2		0x1F8032
+#define	  TX_REQ_0				BIT(0)
+#define VR_MII_GEN5_12G_16G_TX_RATE_CTRL	0x1F8034
+#define   TX0_RATE_MASK				GENMASK(2, 0)
+#define     TX0_BAUD_DIV_1			0
+#define     TX0_BAUD_DIV_4			2
+#define VR_MII_GEN5_12G_16G_TX_EQ_CTRL0		0x1F8036
+#define   TX_EQ_MAIN_MASK			GENMASK(13, 8)
+#define VR_MII_GEN5_12G_16G_TX_EQ_CTRL1		0x1F8037
+#define   TX_EQ_OVR_RIDE			BIT(6)
+#define VR_MII_CONSUMER_10G_TX_TERM_CTRL	0x1F803C
+#define   TX0_TERM_MASK				GENMASK(2, 0)
+#define VR_MII_GEN5_12G_16G_RX_GENCTRL1		0x1F8051
+#define   RX_RST_0				BIT(4)
+#define VR_MII_GEN5_12G_16G_RX_GENCTRL2		0x1F8052
+#define   RX_REQ_0				BIT(0)
+#define VR_MII_GEN5_12G_16G_RX_RATE_CTRL	0x1F8054
+#define   RX0_RATE_MASK				GENMASK(1, 0)
+#define     RX0_BAUD_DIV_2			0x1
+#define     RX0_BAUD_DIV_8			0x3
+#define VR_MII_GEN5_12G_16G_CDR_CTRL		0x1F8056
+#define   CDR_SSC_EN_0				BIT(4)
+#define   VCO_LOW_FREQ_0			BIT(8)
+#define VR_MII_GEN5_12G_16G_MPLL_CMN_CTRL	0x1F8070
+#define   MPLLB_SEL_0				BIT(4)
+#define VR_MII_GEN5_12G_16G_MPLLA_CTRL0		0x1F8071
+#define   MPLLA_CAL_DISABLE			BIT(15)
+#define   MLLA_MULTIPLIER_MASK			GENMASK(7, 0)
+#define VR_MII_GEN5_12G_MPLLA_CTRL1		0x1F8072
+#define   MPLLA_FRACN_CTRL_MASK			GENMASK(15, 5)
+#define VR_MII_GEN5_12G_16G_MPLLA_CTRL2		0x1F8073
+#define   MPLLA_TX_CLK_DIV_MASK			GENMASK(13, 11)
+#define   MPLLA_DIV10_CLK_EN			BIT(9)
+#define VR_MII_GEN5_12G_16G_MPLLB_CTRL0		0x1F8074
+#define   MPLLB_CAL_DISABLE			BIT(15)
+#define   MLLB_MULTIPLIER_OFF			0
+#define   MLLB_MULTIPLIER_MASK			0xFF
+#define VR_MII_GEN5_12G_MPLLB_CTRL1		0x1F8075
+#define   MPLLB_FRACN_CTRL_MASK			GENMASK(15, 5)
+#define VR_MII_GEN5_12G_16G_MPLLB_CTRL2		0x1F8076
+#define   MPLLB_TX_CLK_DIV_MASK			GENMASK(13, 11)
+#define   MPLLB_DIV10_CLK_EN			BIT(9)
+#define VR_MII_RX_LSTS				0x1F8020
+#define   RX_VALID_0				BIT(12)
+#define VR_MII_GEN5_12G_MPLLA_CTRL3		0x1F8077
+#define   MPLLA_BANDWIDTH_MASK			GENMASK(15, 0)
+#define VR_MII_GEN5_12G_MPLLB_CTRL3		0x1F8078
+#define   MPLLB_BANDWIDTH_MASK			GENMASK(15, 0)
+#define VR_MII_GEN5_12G_16G_MISC_CTRL0		0x1F8090
+#define   PLL_CTRL				BIT(15)
+#define VR_MII_GEN5_12G_16G_REF_CLK_CTRL	0x1F8091
+#define   REF_CLK_EN				BIT(0)
+#define   REF_USE_PAD				BIT(1)
+#define   REF_CLK_DIV2				BIT(2)
+#define   REF_RANGE_MASK			GENMASK(5, 3)
+#define     RANGE_26_53_MHZ			0x1
+#define     RANGE_52_78_MHZ			0x2
+#define     RANGE_78_104_MHZ			0x3
+#define   REF_MPLLA_DIV2			BIT(6)
+#define   REF_MPLLB_DIV2			BIT(7)
+#define   REF_RPT_CLK_EN			BIT(8)
+#define VR_MII_GEN5_12G_16G_VCO_CAL_LD0		0x1F8092
+#define   VCO_LD_VAL_0_MASK			GENMASK(12, 0)
+#define VR_MII_GEN5_12G_VCO_CAL_REF0		0x1F8096
+#define   VCO_REF_LD_0_MASK			GENMASK(5, 0)
+
+#define phylink_pcs_to_s32g_xpcs(pl_pcs) \
+	container_of((pl_pcs), struct s32g_xpcs, pcs)
+
+typedef bool (*xpcs_poll_func_t)(struct s32g_xpcs *);
+
+/*
+ * XPCS registers can't be access directly and an indirect address method
+ * must be used instead.
+ */
+
+static const struct regmap_range s32g_xpcs_wr_ranges[] = {
+	regmap_reg_range(0x1F0000, 0x1F0000),
+	regmap_reg_range(0x1F0004, 0x1F0004),
+	regmap_reg_range(0x1F8000, 0x1F8003),
+	regmap_reg_range(0x1F8005, 0x1F8005),
+	regmap_reg_range(0x1F800A, 0x1F800A),
+	regmap_reg_range(0x1F8012, 0x1F8012),
+	regmap_reg_range(0x1F8015, 0x1F8015),
+	regmap_reg_range(0x1F8030, 0x1F8037),
+	regmap_reg_range(0x1F803C, 0x1F803E),
+	regmap_reg_range(0x1F8050, 0x1F8058),
+	regmap_reg_range(0x1F805C, 0x1F805E),
+	regmap_reg_range(0x1F8064, 0x1F8064),
+	regmap_reg_range(0x1F806B, 0x1F806B),
+	regmap_reg_range(0x1F8070, 0x1F8078),
+	regmap_reg_range(0x1F8090, 0x1F8092),
+	regmap_reg_range(0x1F8096, 0x1F8096),
+	regmap_reg_range(0x1F8099, 0x1F8099),
+	regmap_reg_range(0x1F80A0, 0x1F80A2),
+	regmap_reg_range(0x1F80E1, 0x1F80E1),
+};
+
+static const struct regmap_access_table s32g_xpcs_wr_table = {
+	.yes_ranges = s32g_xpcs_wr_ranges,
+	.n_yes_ranges = ARRAY_SIZE(s32g_xpcs_wr_ranges),
+};
+
+static const struct regmap_range s32g_xpcs_rd_ranges[] = {
+	regmap_reg_range(0x1F0000, 0x1F0006),
+	regmap_reg_range(0x1F000F, 0x1F000F),
+	regmap_reg_range(0x1F0708, 0x1F0710),
+	regmap_reg_range(0x1F8000, 0x1F8003),
+	regmap_reg_range(0x1F8005, 0x1F8005),
+	regmap_reg_range(0x1F800A, 0x1F800A),
+	regmap_reg_range(0x1F8010, 0x1F8012),
+	regmap_reg_range(0x1F8015, 0x1F8015),
+	regmap_reg_range(0x1F8018, 0x1F8018),
+	regmap_reg_range(0x1F8020, 0x1F8020),
+	regmap_reg_range(0x1F8030, 0x1F8037),
+	regmap_reg_range(0x1F803C, 0x1F803C),
+	regmap_reg_range(0x1F8040, 0x1F8040),
+	regmap_reg_range(0x1F8050, 0x1F8058),
+	regmap_reg_range(0x1F805C, 0x1F805E),
+	regmap_reg_range(0x1F8060, 0x1F8060),
+	regmap_reg_range(0x1F8064, 0x1F8064),
+	regmap_reg_range(0x1F806B, 0x1F806B),
+	regmap_reg_range(0x1F8070, 0x1F8078),
+	regmap_reg_range(0x1F8090, 0x1F8092),
+	regmap_reg_range(0x1F8096, 0x1F8096),
+	regmap_reg_range(0x1F8098, 0x1F8099),
+	regmap_reg_range(0x1F80A0, 0x1F80A2),
+	regmap_reg_range(0x1F80E1, 0x1F80E1),
+};
+
+static const struct regmap_access_table s32g_xpcs_rd_table = {
+	.yes_ranges = s32g_xpcs_rd_ranges,
+	.n_yes_ranges = ARRAY_SIZE(s32g_xpcs_rd_ranges),
+};
+
+static int s32g_xpcs_regmap_reg_read(void *context, unsigned int reg,
+				     unsigned int *result)
+{
+	struct s32g_xpcs *xpcs = context;
+	u16 ofsleft = (reg >> 8) & 0xffffU;
+	u16 ofsright = (reg & 0xffU);
+
+	writew(ofsleft, xpcs->base + ADDR1_OFS);
+	*result = readw(xpcs->base + (ofsright * 4));
+
+	return 0;
+}
+
+static int s32g_xpcs_regmap_reg_write(void *context, unsigned int reg,
+				      unsigned int val)
+{
+	struct s32g_xpcs *xpcs = context;
+	u16 ofsleft = (reg >> 8) & 0xffffU;
+	u16 ofsright = (reg & 0xffU);
+
+	writew(ofsleft, xpcs->base + ADDR1_OFS);
+	writew(val, xpcs->base + (ofsright * 4));
+
+	return 0;
+}
+
+static const struct regmap_config s32g_xpcs_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 16,
+	.reg_read = s32g_xpcs_regmap_reg_read,
+	.reg_write = s32g_xpcs_regmap_reg_write,
+	.wr_table = &s32g_xpcs_wr_table,
+	.rd_table = &s32g_xpcs_rd_table,
+	.max_register = 0x1F80E1,
+};
+
+static void s32g_xpcs_write_bits(struct s32g_xpcs *xpcs, unsigned int reg,
+				 unsigned int mask, unsigned int value)
+{
+	int ret = regmap_write_bits(xpcs->regmap, reg, mask, value);
+
+	if (ret)
+		dev_err(xpcs->dev, "Failed to write bits of XPCS reg: 0x%x\n", reg);
+}
+
+static void s32g_xpcs_write(struct s32g_xpcs *xpcs, unsigned int reg,
+			    unsigned int value)
+{
+	int ret = regmap_write(xpcs->regmap, reg, value);
+
+	if (ret)
+		dev_err(xpcs->dev, "Failed to write XPCS reg: 0x%x\n", reg);
+}
+
+static unsigned int s32g_xpcs_read(struct s32g_xpcs *xpcs, unsigned int reg)
+{
+	unsigned int val = 0;
+	int ret;
+
+	ret = regmap_read(xpcs->regmap, reg, &val);
+	if (ret)
+		dev_err(xpcs->dev, "Failed to read XPCS reg: 0x%x\n", reg);
+
+	return val;
+}
+
+/*
+ * Internal XPCS function
+ */
+
+static unsigned int s32g_xpcs_get_an(struct s32g_xpcs *xpcs)
+{
+	unsigned int val = s32g_xpcs_read(xpcs, VR_MII_AN_INTR_STS);
+
+	return !!(val & CL37_ANCMPLT_INTR);
+};
+
+static int s32g_xpcs_wait_an_done(struct s32g_xpcs *xpcs)
+{
+	unsigned int val;
+
+	return read_poll_timeout(s32g_xpcs_get_an, val,
+				 !!(val & CL37_ANCMPLT_INTR),
+				 0,
+				 XPCS_TIMEOUT_MS, false, xpcs);
+};
+
+static bool s32g_xpcs_poll_timeout(struct s32g_xpcs *xpcs, xpcs_poll_func_t func,
+				   ktime_t timeout)
+{
+	ktime_t cur = ktime_get();
+
+	return func(xpcs) || ktime_after(cur, timeout);
+}
+
+static int s32g_xpcs_wait(struct s32g_xpcs *xpcs, xpcs_poll_func_t func)
+{
+	ktime_t timeout = ktime_add_ms(ktime_get(), XPCS_TIMEOUT_MS);
+
+	spin_until_cond(s32g_xpcs_poll_timeout(xpcs, func, timeout));
+	if (!func(xpcs))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int s32g_xpcs_wait_bits(struct s32g_xpcs *xpcs, unsigned int reg,
+			       unsigned int mask, unsigned int bits)
+{
+	ktime_t cur;
+	ktime_t timeout = ktime_add_ms(ktime_get(), XPCS_TIMEOUT_MS);
+
+	spin_until_cond((cur = ktime_get(),
+			 (s32g_xpcs_read(xpcs, reg) & mask) == bits ||
+			 ktime_after(cur, timeout)));
+	if ((s32g_xpcs_read(xpcs, reg) & mask) != bits)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static unsigned int s32g_xpcs_digital_status(struct s32g_xpcs *xpcs)
+{
+	return s32g_xpcs_read(xpcs, VR_MII_DIG_STS);
+}
+
+static int s32g_xpcs_wait_power_good_state(struct s32g_xpcs *xpcs)
+{
+	unsigned int val;
+
+	return read_poll_timeout(s32g_xpcs_digital_status, val,
+				 FIELD_GET(PSEQ_STATE_MASK, val) == POWER_GOOD_STATE,
+				 0,
+				 XPCS_TIMEOUT_MS, false, xpcs);
+}
+
+int s32g_xpcs_vreset(struct s32g_xpcs *xpcs)
+{
+	if (!xpcs)
+		return -EINVAL;
+
+	/* Step 19 */
+	s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1, VR_RST, VR_RST);
+
+	return 0;
+}
+
+static bool s32g_xpcs_is_not_in_reset(struct s32g_xpcs *xpcs)
+{
+	unsigned int val;
+
+	val = s32g_xpcs_read(xpcs, VR_MII_DIG_CTRL1);
+
+	return !(val & VR_RST);
+}
+
+int s32g_xpcs_wait_vreset(struct s32g_xpcs *xpcs)
+{
+	int ret;
+
+	/* Step 20 */
+	ret = s32g_xpcs_wait(xpcs, s32g_xpcs_is_not_in_reset);
+	if (ret)
+		dev_err(xpcs->dev, "XPCS%d is in reset\n", xpcs->id);
+
+	return ret;
+}
+
+int s32g_xpcs_reset_rx(struct s32g_xpcs *xpcs)
+{
+	int ret = 0;
+
+	ret = s32g_xpcs_wait_power_good_state(xpcs);
+	if (ret) {
+		dev_err(xpcs->dev, "Failed to enter in PGOOD state after vendor reset\n");
+		return ret;
+	}
+
+	/* Step 21 */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_RX_GENCTRL1,
+			     RX_RST_0, RX_RST_0);
+
+	/* Step 22 */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_RX_GENCTRL1,
+			     RX_RST_0, 0);
+
+	/* Step 23 */
+	/* Wait until SR_MII_STS[LINK_STS] = 1 */
+
+	return ret;
+}
+
+static int s32g_xpcs_ref_clk_sel(struct s32g_xpcs *xpcs,
+				 enum s32g_xpcs_pll ref_pll)
+{
+	switch (ref_pll) {
+	case XPCS_PLLA:
+		s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_MPLL_CMN_CTRL,
+				     MPLLB_SEL_0, 0);
+		xpcs->ref = XPCS_PLLA;
+		break;
+	case XPCS_PLLB:
+		s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_MPLL_CMN_CTRL,
+				     MPLLB_SEL_0, MPLLB_SEL_0);
+		xpcs->ref = XPCS_PLLB;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void s32g_xpcs_electrical_configure(struct s32g_xpcs *xpcs)
+{
+	/* Step 2 */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_TX_EQ_CTRL0,
+			     TX_EQ_MAIN_MASK, FIELD_PREP(TX_EQ_MAIN_MASK, 0xC));
+
+	/* Step 3 */
+	s32g_xpcs_write_bits(xpcs, VR_MII_CONSUMER_10G_TX_TERM_CTRL,
+			     TX0_TERM_MASK, FIELD_PREP(TX0_TERM_MASK, 0x4));
+}
+
+static int s32g_xpcs_vco_cfg(struct s32g_xpcs *xpcs, enum s32g_xpcs_pll vco_pll)
+{
+	unsigned int vco_ld = 0;
+	unsigned int vco_ref = 0;
+	unsigned int rx_baud = 0;
+	unsigned int tx_baud = 0;
+
+	switch (vco_pll) {
+	case XPCS_PLLA:
+		if (xpcs->mhz125) {
+			vco_ld = FIELD_PREP(VCO_LD_VAL_0_MASK, 1360);
+			vco_ref = FIELD_PREP(VCO_REF_LD_0_MASK, 17);
+		} else {
+			vco_ld = FIELD_PREP(VCO_LD_VAL_0_MASK, 1350);
+			vco_ref = FIELD_PREP(VCO_REF_LD_0_MASK, 27);
+		}
+
+		rx_baud = FIELD_PREP(RX0_RATE_MASK, RX0_BAUD_DIV_8);
+		tx_baud = FIELD_PREP(TX0_RATE_MASK, TX0_BAUD_DIV_4);
+		break;
+	case XPCS_PLLB:
+		if (xpcs->mhz125) {
+			vco_ld = FIELD_PREP(VCO_LD_VAL_0_MASK, 1350);
+			vco_ref = FIELD_PREP(VCO_REF_LD_0_MASK, 27);
+		} else {
+			vco_ld = FIELD_PREP(VCO_LD_VAL_0_MASK, 1344);
+			vco_ref = FIELD_PREP(VCO_REF_LD_0_MASK, 43);
+		}
+
+		rx_baud = FIELD_PREP(RX0_RATE_MASK, RX0_BAUD_DIV_2);
+		tx_baud = FIELD_PREP(TX0_RATE_MASK, TX0_BAUD_DIV_1);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_VCO_CAL_LD0,
+			     VCO_LD_VAL_0_MASK, vco_ld);
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_VCO_CAL_REF0,
+			     VCO_REF_LD_0_MASK, vco_ref);
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_TX_RATE_CTRL,
+			     TX0_RATE_MASK, tx_baud);
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_RX_RATE_CTRL,
+			     RX0_RATE_MASK, rx_baud);
+
+	if (vco_pll == XPCS_PLLB) {
+		s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_CDR_CTRL,
+				     VCO_LOW_FREQ_0, VCO_LOW_FREQ_0);
+	} else {
+		s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_CDR_CTRL,
+				     VCO_LOW_FREQ_0, 0);
+	}
+
+	return 0;
+}
+
+static int s32g_xpcs_init_mplla(struct s32g_xpcs *xpcs)
+{
+	unsigned int val;
+
+	if (!xpcs)
+		return -EINVAL;
+
+	/* Step 7 */
+	val = 0;
+	if (xpcs->ext_clk)
+		val |= REF_USE_PAD;
+
+	if (xpcs->mhz125) {
+		val |= REF_MPLLA_DIV2;
+		val |= REF_CLK_DIV2;
+		val |= FIELD_PREP(REF_RANGE_MASK, RANGE_52_78_MHZ);
+	} else {
+		val |= FIELD_PREP(REF_RANGE_MASK, RANGE_26_53_MHZ);
+	}
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_REF_CLK_CTRL,
+			     REF_MPLLA_DIV2 | REF_USE_PAD | REF_RANGE_MASK |
+			     REF_CLK_DIV2, val);
+
+	/* Step 8 */
+	if (xpcs->mhz125)
+		val = FIELD_PREP(MLLA_MULTIPLIER_MASK, 80);
+	else
+		val = FIELD_PREP(MLLA_MULTIPLIER_MASK, 25);
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_MPLLA_CTRL0,
+			     MPLLA_CAL_DISABLE | MLLA_MULTIPLIER_MASK,
+			     val);
+
+	/* Step 9 */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_MPLLA_CTRL1,
+			     MPLLA_FRACN_CTRL_MASK, 0);
+
+	/* Step 10 */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_MPLLA_CTRL2,
+			     MPLLA_TX_CLK_DIV_MASK | MPLLA_DIV10_CLK_EN,
+			     FIELD_PREP(MPLLA_TX_CLK_DIV_MASK, 1) | MPLLA_DIV10_CLK_EN);
+
+	/* Step 11 */
+	if (xpcs->mhz125)
+		val = FIELD_PREP(MPLLA_BANDWIDTH_MASK, 43);
+	else
+		val = FIELD_PREP(MPLLA_BANDWIDTH_MASK, 357);
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_MPLLA_CTRL3,
+			     MPLLA_BANDWIDTH_MASK, val);
+
+	return 0;
+}
+
+static int s32g_xpcs_init_mpllb(struct s32g_xpcs *xpcs)
+{
+	unsigned int val;
+
+	if (!xpcs)
+		return -EINVAL;
+
+	/* Step 7 */
+	val = 0;
+	if (xpcs->ext_clk)
+		val |= REF_USE_PAD;
+
+	if (xpcs->mhz125) {
+		val |= REF_MPLLB_DIV2;
+		val |= REF_CLK_DIV2;
+		val |= FIELD_PREP(REF_RANGE_MASK, RANGE_52_78_MHZ);
+	} else {
+		val |= FIELD_PREP(REF_RANGE_MASK, RANGE_26_53_MHZ);
+	}
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_REF_CLK_CTRL,
+			     REF_MPLLB_DIV2 | REF_USE_PAD | REF_RANGE_MASK |
+			     REF_CLK_DIV2, val);
+
+	/* Step 8 */
+	if (xpcs->mhz125)
+		val = 125 << MLLB_MULTIPLIER_OFF;
+	else
+		val = 39 << MLLB_MULTIPLIER_OFF;
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_MPLLB_CTRL0,
+			     MPLLB_CAL_DISABLE | MLLB_MULTIPLIER_MASK,
+			     val);
+
+	/* Step 9 */
+	if (xpcs->mhz125)
+		val = FIELD_PREP(MPLLB_FRACN_CTRL_MASK, 0);
+	else
+		val = FIELD_PREP(MPLLB_FRACN_CTRL_MASK, 1044);
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_MPLLB_CTRL1,
+			     MPLLB_FRACN_CTRL_MASK, val);
+
+	/* Step 10 */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_MPLLB_CTRL2,
+			     MPLLB_TX_CLK_DIV_MASK | MPLLB_DIV10_CLK_EN,
+			     FIELD_PREP(MPLLB_TX_CLK_DIV_MASK, 5) | MPLLB_DIV10_CLK_EN);
+
+	/* Step 11 */
+	if (xpcs->mhz125)
+		val = FIELD_PREP(MPLLB_BANDWIDTH_MASK, 68);
+	else
+		val = FIELD_PREP(MPLLB_BANDWIDTH_MASK, 102);
+
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_MPLLB_CTRL3,
+			     MPLLB_BANDWIDTH_MASK, val);
+
+	return 0;
+}
+
+static void s32g_serdes_pma_high_freq_recovery(struct s32g_xpcs *xpcs)
+{
+	/* PCS signal protection, PLL railout recovery */
+	s32g_xpcs_write_bits(xpcs, VR_MII_DBG_CTRL, SUPPRESS_LOS_DET | RX_DT_EN_CTL,
+			     SUPPRESS_LOS_DET | RX_DT_EN_CTL);
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_MISC_CTRL0,
+			     PLL_CTRL, PLL_CTRL);
+}
+
+static void s32g_serdes_pma_configure_tx_eq_post(struct s32g_xpcs *xpcs)
+{
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_TX_EQ_CTRL1,
+			     TX_EQ_OVR_RIDE, TX_EQ_OVR_RIDE);
+}
+
+static int s32g_serdes_bifurcation_pll_transit(struct s32g_xpcs *xpcs,
+					       enum s32g_xpcs_pll target_pll)
+{
+	int ret = 0;
+	struct device *dev = xpcs->dev;
+
+	/* Configure XPCS speed and VCO */
+	if (target_pll == XPCS_PLLA) {
+		s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1, EN_2_5G_MODE, 0);
+		s32g_xpcs_vco_cfg(xpcs, XPCS_PLLA);
+	} else {
+		s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1,
+				     EN_2_5G_MODE, EN_2_5G_MODE);
+		s32g_xpcs_vco_cfg(xpcs, XPCS_PLLB);
+	}
+
+	/* Signal that clock are not available */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_TX_GENCTRL1,
+			     TX_CLK_RDY_0, 0);
+
+	/* Select PLL reference */
+	if (target_pll == XPCS_PLLA)
+		s32g_xpcs_ref_clk_sel(xpcs, XPCS_PLLA);
+	else
+		s32g_xpcs_ref_clk_sel(xpcs, XPCS_PLLB);
+
+	/* Initiate transmitter TX reconfiguration request */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_TX_GENCTRL2,
+			     TX_REQ_0, TX_REQ_0);
+
+	/* Wait for transmitter to reconfigure */
+	ret = s32g_xpcs_wait_bits(xpcs, VR_MII_GEN5_12G_16G_TX_GENCTRL2,
+				  TX_REQ_0, 0);
+	if (ret) {
+		dev_err(dev, "Switch to TX_REQ_0 failed\n");
+		return ret;
+	}
+
+	/* Initiate transmitter RX reconfiguration request */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_RX_GENCTRL2,
+			     RX_REQ_0, RX_REQ_0);
+
+	/* Wait for receiver to reconfigure */
+	ret = s32g_xpcs_wait_bits(xpcs, VR_MII_GEN5_12G_16G_RX_GENCTRL2,
+				  RX_REQ_0, 0);
+	if (ret) {
+		dev_err(dev, "Switch to RX_REQ_0 failed\n");
+		return ret;
+	}
+
+	/* Signal that clock are available */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_TX_GENCTRL1,
+			     TX_CLK_RDY_0, TX_CLK_RDY_0);
+
+	/* Flush internal logic */
+	s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1, INIT, INIT);
+
+	/* Wait for init */
+	ret = s32g_xpcs_wait_bits(xpcs, VR_MII_DIG_CTRL1, INIT, 0);
+	if (ret) {
+		dev_err(dev, "XPCS INIT failed\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+/*
+ * 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;
+	}
+
+	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;
+		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 */
+		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;
+
+	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);
+			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);
+
+		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
+ */
+
+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);
+}
+
+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);
+}
+
+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;
+
+	s32g_xpcs_config(xpcs, state);
+}
+
+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,
+};
+
+/*
+ * Serdes functions for initializing/configuring/releasing the xpcs
+ */
+
+int s32g_xpcs_pre_pcie_2g5(struct s32g_xpcs *xpcs)
+{
+	int ret;
+
+	/* Enable voltage boost */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_TX_GENCTRL1, VBOOST_EN_0,
+			     VBOOST_EN_0);
+
+	/* TX rate baud  */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_TX_RATE_CTRL, 0x7, 0x0U);
+
+	/* Rx rate baud/2 */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_RX_RATE_CTRL, 0x3U, 0x1U);
+
+	/* Set low-frequency operating band */
+	s32g_xpcs_write_bits(xpcs, VR_MII_GEN5_12G_16G_CDR_CTRL, CDR_SSC_EN_0,
+			     VCO_LOW_FREQ_0);
+
+	ret = s32g_serdes_bifurcation_pll_transit(xpcs, XPCS_PLLB);
+	if (ret)
+		dev_err(xpcs->dev, "Switch to PLLB failed\n");
+
+	return ret;
+}
+
+int s32g_xpcs_init_plls(struct s32g_xpcs *xpcs)
+{
+	int ret;
+	struct device *dev = xpcs->dev;
+
+	if (!xpcs->ext_clk) {
+		/* Step 1 */
+		s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1, BYP_PWRUP, BYP_PWRUP);
+	} else if (xpcs->pcie_shared == NOT_SHARED) {
+		ret = s32g_xpcs_wait_power_good_state(xpcs);
+		if (ret)
+			return ret;
+	} else if (xpcs->pcie_shared == PCIE_XPCS_2G5) {
+		ret = s32g_xpcs_wait_power_good_state(xpcs);
+		if (ret)
+			return ret;
+		/* Configure equalization */
+		s32g_serdes_pma_configure_tx_eq_post(xpcs);
+		s32g_xpcs_electrical_configure(xpcs);
+
+		/* Enable receiver recover */
+		s32g_serdes_pma_high_freq_recovery(xpcs);
+		return 0;
+	}
+
+	s32g_xpcs_electrical_configure(xpcs);
+
+	s32g_xpcs_ref_clk_sel(xpcs, XPCS_PLLA);
+	ret = s32g_xpcs_init_mplla(xpcs);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PLLA\n");
+		return ret;
+	}
+	ret = s32g_xpcs_init_mpllb(xpcs);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PLLB\n");
+		return ret;
+	}
+	s32g_xpcs_vco_cfg(xpcs, XPCS_PLLA);
+
+	/* Step 18 */
+	if (!xpcs->ext_clk)
+		s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1, BYP_PWRUP, 0);
+
+	/* Will be cleared by Step 19 Vreset ??? */
+	s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, AN_ENABLE, 0);
+	s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, DUPLEX_MODE, DUPLEX_MODE);
+
+	return ret;
+}
+
+int s32g_xpcs_disable_an(struct s32g_xpcs *xpcs)
+{
+	int ret;
+
+	ret = (s32g_xpcs_read(xpcs, SR_MII_CTRL) & AN_ENABLE);
+
+	s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, DUPLEX_MODE, DUPLEX_MODE);
+	s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, AN_ENABLE, 0);
+
+	return ret;
+}
+
+int s32g_xpcs_init(struct s32g_xpcs *xpcs, struct device *dev,
+		   unsigned char id, void __iomem *base, bool ext_clk,
+		   unsigned long rate, enum pcie_xpcs_mode pcie_shared)
+{
+	struct regmap_config conf;
+
+	if (rate != (125 * HZ_PER_MHZ) && rate != (100 * HZ_PER_MHZ)) {
+		dev_err(dev, "XPCS cannot operate @%lu HZ\n", rate);
+		return -EINVAL;
+	}
+
+	xpcs->base = base;
+	xpcs->ext_clk = ext_clk;
+	xpcs->id = id;
+	xpcs->dev = dev;
+	xpcs->pcie_shared = pcie_shared;
+
+	if (rate == (125 * HZ_PER_MHZ))
+		xpcs->mhz125 = true;
+	else
+		xpcs->mhz125 = false;
+
+	conf = s32g_xpcs_regmap_config;
+
+	if (!id)
+		conf.name = "xpcs0";
+	else
+		conf.name = "xpcs1";
+
+	xpcs->regmap = devm_regmap_init(dev, NULL, xpcs, &conf);
+	if (IS_ERR(xpcs->regmap))
+		return dev_err_probe(dev, PTR_ERR(xpcs->regmap),
+				     "Failed to init register amp\n");
+
+	/* Phylink PCS */
+	xpcs->pcs.ops = &s32cc_phylink_pcs_ops;
+	xpcs->pcs.poll = true;
+	__set_bit(PHY_INTERFACE_MODE_SGMII, xpcs->pcs.supported_interfaces);
+
+	return 0;
+}
diff --git a/drivers/phy/freescale/phy-nxp-s32g-xpcs.h b/drivers/phy/freescale/phy-nxp-s32g-xpcs.h
new file mode 100644
index 000000000000..07cdd292fd5e
--- /dev/null
+++ b/drivers/phy/freescale/phy-nxp-s32g-xpcs.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/**
+ * Copyright 2021-2026 NXP
+ */
+#ifndef NXP_S32G_XPCS_H
+#define NXP_S32G_XPCS_H
+
+#include <linux/phy.h>
+#include <linux/phy/phy.h>
+#include <linux/types.h>
+#include <linux/phylink.h>
+
+enum pcie_xpcs_mode {
+	NOT_SHARED,
+	PCIE_XPCS_1G,
+	PCIE_XPCS_2G5,
+};
+
+enum s32g_xpcs_pll {
+	XPCS_PLLA,	/* Slow PLL */
+	XPCS_PLLB,	/* Fast PLL */
+};
+
+struct s32g_xpcs {
+	void __iomem *base;
+	struct device *dev;
+	unsigned char id;
+	struct regmap *regmap;
+	enum s32g_xpcs_pll ref;
+	bool ext_clk;
+	bool mhz125;
+	bool an;
+	enum pcie_xpcs_mode pcie_shared;
+	struct phylink_pcs pcs;
+};
+
+int s32g_xpcs_init(struct s32g_xpcs *xpcs, struct device *dev,
+		   unsigned char id, void __iomem *base, bool ext_clk,
+		   unsigned long rate, enum pcie_xpcs_mode pcie_shared);
+int s32g_xpcs_init_plls(struct s32g_xpcs *xpcs);
+int s32g_xpcs_pre_pcie_2g5(struct s32g_xpcs *xpcs);
+int s32g_xpcs_vreset(struct s32g_xpcs *xpcs);
+int s32g_xpcs_wait_vreset(struct s32g_xpcs *xpcs);
+int s32g_xpcs_reset_rx(struct s32g_xpcs *xpcs);
+int s32g_xpcs_disable_an(struct s32g_xpcs *xpcs);
+#endif
+
diff --git a/include/linux/pcs/pcs-nxp-xpcs.h b/include/linux/pcs/pcs-nxp-xpcs.h
new file mode 100644
index 000000000000..41f424e8ff5b
--- /dev/null
+++ b/include/linux/pcs/pcs-nxp-xpcs.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/**
+ * Copyright 2026 NXP
+ */
+
+#ifndef __LINUX_PCS_NXP_XPCS_H
+#define __LINUX_PCS_NXP_XPCS_H
+
+#include <linux/phylink.h>
+
+struct phylink_pcs *s32g_serdes_pcs_create(struct device *dev, struct device_node *np);
+
+#endif /* __LINUX_PCS_XPCS_H */
-- 
2.43.0

Re: [PATCH 3/4] phy: s32g: Add serdes xpcs subsystem

[Simon Horman]

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

...

> diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c

...

> +static void s32g_serdes_prepare_pma_mode5(struct s32g_serdes *serdes)
> +{
> +	u32 val;
> +	/* Configure TX_VBOOST_LVL and TX_TERM_CTRL */
> +	val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
> +	val &= ~(EXT_TX_VBOOST_LVL_MASK | EXT_TX_TERM_CTRL_MASK);
> +	val |= FIELD_PREP(EXT_TX_VBOOST_LVL_MASK, 0x3) |
> +		FIELD_PREP(EXT_TX_TERM_CTRL_MASK, 0x4);
> +	writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);

This is part of an AI Generated review.
I have looked over it and I think it warrants investigation.
For information on how to reproduce locally, as I did, please see [1].

	The entire s32g_serdes_prepare_pma_mode5() function is ~70 lines of
	magic numbers with zero explanation. These appear to be
	hardware-specific PLL/PHY tuning parameters for 2.5G mode.

Please consider using #defines, to give values names.

...

> +static int s32g_serdes_init_clks(struct s32g_serdes *serdes)
> +{
> +	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> +	struct s32g_xpcs_ctrl *xpcs = &serdes->xpcs;
> +	int ret, order[2], xpcs_id;
> +	size_t i;
> +
> +	switch (ctrl->ss_mode) {
> +	case 0:
> +		return 0;
> +	case 1:
> +		order[0] = 0;
> +		order[1] = XPCS_DISABLED;
> +		break;
> +	case 2:
> +	case 5:
> +		order[0] = 1;
> +		order[1] = XPCS_DISABLED;
> +		break;
> +	case 3:
> +		order[0] = 1;
> +		order[1] = 0;
> +		break;
> +	case 4:
> +		order[0] = 0;
> +		order[1] = 1;
> +		break;
> +	default:
> +		return -EINVAL;

AI review also flags that s32g_serdes_get_ctrl_resources() ensures that
ss_mode is <= 5.  So this check is unnecessary.

> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(order); i++) {
> +		xpcs_id = order[i];
> +
> +		if (xpcs_id == XPCS_DISABLED)
> +			continue;
> +
> +		ret = s32g_xpcs_init_plls(xpcs->phys[xpcs_id]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (ctrl->ss_mode == 5) {
> +		s32g_serdes_prepare_pma_mode5(serdes);
> +
> +		ret = s32g_xpcs_pre_pcie_2g5(xpcs->phys[1]);

Also from the AI review:

	In mode 5, code directly accesses xpcs->phys[1] without checking if
	it was created. If XPCS1 wasn't created for some reason (DT
	misconfiguration, probe failure), this NULL dereferences.

...

> @@ -460,6 +741,41 @@ static int s32g2_serdes_create_phy(struct s32g_serdes *serdes, struct device_nod

...

> +		xpcs = devm_kmalloc(dev, sizeof(*xpcs), GFP_KERNEL);
> +		if (IS_ERR(xpcs)) {
> +			dev_err(dev, "Failed to allocate xpcs\n");
> +			return -ENOMEM;
> +		}

AI review also flags:

	devm_kmalloc() returns NULL on failure, not an error pointer.
	This check will never trigger.

[1] https://netdev-ai.bots.linux.dev/ai-local.html

> +
> +		xpcs_ctrl->phys[port] = xpcs;
> +
> +		xpcs->an = of_property_read_bool(dev->of_node, "nxp,xpcs_an");

AI review flags that nxp,xpcs_an is not part of the binding.

...

> +struct phylink_pcs *s32g_serdes_pcs_create(struct device *dev, struct device_node *np)
> +{
> +	struct platform_device *pdev;
> +	struct device_node *pcs_np;
> +	struct s32g_serdes *serdes;
> +	u32 port;
> +
> +	if (of_property_read_u32(np, "reg", &port))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (port > S32G_SERDES_XPCS_MAX)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* The PCS pdev is attached to the parent node */
> +	pcs_np = of_get_parent(np);
> +	if (!pcs_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	if (!of_device_is_available(pcs_np)) {
> +		of_node_put(pcs_np);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	pdev = of_find_device_by_node(pcs_np);
> +	of_node_put(pcs_np);
> +	if (!pdev || !platform_get_drvdata(pdev)) {
> +		if (pdev)
> +			put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	serdes = platform_get_drvdata(pdev);

Also from the AI review:

	On success, the function gets a reference to pdev but never
	releases it. This leaks a device reference every time a MAC driver
	calls s32g_serdes_pcs_create().

> +
> +	return &serdes->xpcs.phys[port]->pcs;

Also from the AI review:

	The check port > S32G_SERDES_XPCS_MAX allows port=2,
	but array only has indices 0 and 1.

Also I'm not seeing bounds checking on port in s32g2_serdes_create_phy
which uses port as an index for the same array both directly and indirectly
via s32g_serdes_xpcs_init().

...

> diff --git a/drivers/phy/freescale/phy-nxp-s32g-xpcs.c b/drivers/phy/freescale/phy-nxp-s32g-xpcs.c

...

> +static int s32g_xpcs_regmap_reg_read(void *context, unsigned int reg,
> +				     unsigned int *result)
> +{
> +	struct s32g_xpcs *xpcs = context;
> +	u16 ofsleft = (reg >> 8) & 0xffffU;
> +	u16 ofsright = (reg & 0xffU);
> +
> +	writew(ofsleft, xpcs->base + ADDR1_OFS);
> +	*result = readw(xpcs->base + (ofsright * 4));
> +
> +	return 0;
> +}
> +
> +static int s32g_xpcs_regmap_reg_write(void *context, unsigned int reg,
> +				      unsigned int val)
> +{
> +	struct s32g_xpcs *xpcs = context;
> +	u16 ofsleft = (reg >> 8) & 0xffffU;
> +	u16 ofsright = (reg & 0xffU);
> +
> +	writew(ofsleft, xpcs->base + ADDR1_OFS);
> +	writew(val, xpcs->base + (ofsright * 4));
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config s32g_xpcs_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_read = s32g_xpcs_regmap_reg_read,
> +	.reg_write = s32g_xpcs_regmap_reg_write,
> +	.wr_table = &s32g_xpcs_wr_table,
> +	.rd_table = &s32g_xpcs_rd_table,
> +	.max_register = 0x1F80E1,
> +};

AI review also flags that s32g_xpcs_regmap_reg_read and
s32g_xpcs_regmap_reg_write do not protect against concurrent access.

...

Re: [PATCH 3/4] phy: s32g: Add serdes xpcs subsystem

[Vincent Guittot]

On Thu, 29 Jan 2026 at 12:59, Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Jan 26, 2026 at 10:21:58AM +0100, Vincent Guittot wrote:
>
> ...
>
> > diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
>
> ...
>
> > +static void s32g_serdes_prepare_pma_mode5(struct s32g_serdes *serdes)
> > +{
> > +     u32 val;
> > +     /* Configure TX_VBOOST_LVL and TX_TERM_CTRL */
> > +     val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
> > +     val &= ~(EXT_TX_VBOOST_LVL_MASK | EXT_TX_TERM_CTRL_MASK);
> > +     val |= FIELD_PREP(EXT_TX_VBOOST_LVL_MASK, 0x3) |
> > +             FIELD_PREP(EXT_TX_TERM_CTRL_MASK, 0x4);
> > +     writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
>
> This is part of an AI Generated review.
> I have looked over it and I think it warrants investigation.
> For information on how to reproduce locally, as I did, please see [1].
>
>         The entire s32g_serdes_prepare_pma_mode5() function is ~70 lines of
>         magic numbers with zero explanation. These appear to be
>         hardware-specific PLL/PHY tuning parameters for 2.5G mode.

Unfortunately there is no additional information in the reference
manual other than
*step 4:
- Write 3h to EXT_TX_VBOOST_LVL.
- Write 4h to EXT_TX_TERM_CTRL

>
> Please consider using #defines, to give values names.
>
> ...
>
> > +static int s32g_serdes_init_clks(struct s32g_serdes *serdes)
> > +{
> > +     struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> > +     struct s32g_xpcs_ctrl *xpcs = &serdes->xpcs;
> > +     int ret, order[2], xpcs_id;
> > +     size_t i;
> > +
> > +     switch (ctrl->ss_mode) {
> > +     case 0:
> > +             return 0;
> > +     case 1:
> > +             order[0] = 0;
> > +             order[1] = XPCS_DISABLED;
> > +             break;
> > +     case 2:
> > +     case 5:
> > +             order[0] = 1;
> > +             order[1] = XPCS_DISABLED;
> > +             break;
> > +     case 3:
> > +             order[0] = 1;
> > +             order[1] = 0;
> > +             break;
> > +     case 4:
> > +             order[0] = 0;
> > +             order[1] = 1;
> > +             break;
> > +     default:
> > +             return -EINVAL;
>
> AI review also flags that s32g_serdes_get_ctrl_resources() ensures that
> ss_mode is <= 5.  So this check is unnecessary.

okay but providing a default seems a good practice

>
> > +     }
> > +
> > +     for (i = 0; i < ARRAY_SIZE(order); i++) {
> > +             xpcs_id = order[i];
> > +
> > +             if (xpcs_id == XPCS_DISABLED)
> > +                     continue;
> > +
> > +             ret = s32g_xpcs_init_plls(xpcs->phys[xpcs_id]);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (ctrl->ss_mode == 5) {
> > +             s32g_serdes_prepare_pma_mode5(serdes);
> > +
> > +             ret = s32g_xpcs_pre_pcie_2g5(xpcs->phys[1]);
>
> Also from the AI review:
>
>         In mode 5, code directly accesses xpcs->phys[1] without checking if
>         it was created. If XPCS1 wasn't created for some reason (DT
>         misconfiguration, probe failure), this NULL dereferences.

I will add a check

>
> ...
>
> > @@ -460,6 +741,41 @@ static int s32g2_serdes_create_phy(struct s32g_serdes *serdes, struct device_nod
>
> ...
>
> > +             xpcs = devm_kmalloc(dev, sizeof(*xpcs), GFP_KERNEL);
> > +             if (IS_ERR(xpcs)) {
> > +                     dev_err(dev, "Failed to allocate xpcs\n");
> > +                     return -ENOMEM;
> > +             }
>
> AI review also flags:
>
>         devm_kmalloc() returns NULL on failure, not an error pointer.
>         This check will never trigger.

okay

>
> [1] https://netdev-ai.bots.linux.dev/ai-local.html
>
> > +
> > +             xpcs_ctrl->phys[port] = xpcs;
> > +
> > +             xpcs->an = of_property_read_bool(dev->of_node, "nxp,xpcs_an");
>
> AI review flags that nxp,xpcs_an is not part of the binding.

That's a leftover that should be removed

>
> ...
>
> > +struct phylink_pcs *s32g_serdes_pcs_create(struct device *dev, struct device_node *np)
> > +{
> > +     struct platform_device *pdev;
> > +     struct device_node *pcs_np;
> > +     struct s32g_serdes *serdes;
> > +     u32 port;
> > +
> > +     if (of_property_read_u32(np, "reg", &port))
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     if (port > S32G_SERDES_XPCS_MAX)
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     /* The PCS pdev is attached to the parent node */
> > +     pcs_np = of_get_parent(np);
> > +     if (!pcs_np)
> > +             return ERR_PTR(-ENODEV);
> > +
> > +     if (!of_device_is_available(pcs_np)) {
> > +             of_node_put(pcs_np);
> > +             return ERR_PTR(-ENODEV);
> > +     }
> > +
> > +     pdev = of_find_device_by_node(pcs_np);
> > +     of_node_put(pcs_np);
> > +     if (!pdev || !platform_get_drvdata(pdev)) {
> > +             if (pdev)
> > +                     put_device(&pdev->dev);
> > +             return ERR_PTR(-EPROBE_DEFER);
> > +     }
> > +
> > +     serdes = platform_get_drvdata(pdev);
>
> Also from the AI review:
>
>         On success, the function gets a reference to pdev but never
>         releases it. This leaks a device reference every time a MAC driver
>         calls s32g_serdes_pcs_create().

will fix it

>
> > +
> > +     return &serdes->xpcs.phys[port]->pcs;
>
> Also from the AI review:
>
>         The check port > S32G_SERDES_XPCS_MAX allows port=2,
>         but array only has indices 0 and 1.
>
> Also I'm not seeing bounds checking on port in s32g2_serdes_create_phy
> which uses port as an index for the same array both directly and indirectly
> via s32g_serdes_xpcs_init().

will add it

>
> ...
>
> > diff --git a/drivers/phy/freescale/phy-nxp-s32g-xpcs.c b/drivers/phy/freescale/phy-nxp-s32g-xpcs.c
>
> ...
>
> > +static int s32g_xpcs_regmap_reg_read(void *context, unsigned int reg,
> > +                                  unsigned int *result)
> > +{
> > +     struct s32g_xpcs *xpcs = context;
> > +     u16 ofsleft = (reg >> 8) & 0xffffU;
> > +     u16 ofsright = (reg & 0xffU);
> > +
> > +     writew(ofsleft, xpcs->base + ADDR1_OFS);
> > +     *result = readw(xpcs->base + (ofsright * 4));
> > +
> > +     return 0;
> > +}
> > +
> > +static int s32g_xpcs_regmap_reg_write(void *context, unsigned int reg,
> > +                                   unsigned int val)
> > +{
> > +     struct s32g_xpcs *xpcs = context;
> > +     u16 ofsleft = (reg >> 8) & 0xffffU;
> > +     u16 ofsright = (reg & 0xffU);
> > +
> > +     writew(ofsleft, xpcs->base + ADDR1_OFS);
> > +     writew(val, xpcs->base + (ofsright * 4));
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct regmap_config s32g_xpcs_regmap_config = {
> > +     .reg_bits = 16,
> > +     .val_bits = 16,
> > +     .reg_read = s32g_xpcs_regmap_reg_read,
> > +     .reg_write = s32g_xpcs_regmap_reg_write,
> > +     .wr_table = &s32g_xpcs_wr_table,
> > +     .rd_table = &s32g_xpcs_rd_table,
> > +     .max_register = 0x1F80E1,
> > +};
>
> AI review also flags that s32g_xpcs_regmap_reg_read and
> s32g_xpcs_regmap_reg_write do not protect against concurrent access.

but regmap framework should

>
> ...

Re: [PATCH 3/4] phy: s32g: Add serdes xpcs subsystem

[Simon Horman]

On Thu, Jan 29, 2026 at 02:24:15PM +0100, Vincent Guittot wrote:
> On Thu, 29 Jan 2026 at 12:59, Simon Horman <horms@kernel.org> wrote:
> >
> > On Mon, Jan 26, 2026 at 10:21:58AM +0100, Vincent Guittot wrote:
> >
> > ...
> >
> > > diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> >
> > ...
> >
> > > +static void s32g_serdes_prepare_pma_mode5(struct s32g_serdes *serdes)
> > > +{
> > > +     u32 val;
> > > +     /* Configure TX_VBOOST_LVL and TX_TERM_CTRL */
> > > +     val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
> > > +     val &= ~(EXT_TX_VBOOST_LVL_MASK | EXT_TX_TERM_CTRL_MASK);
> > > +     val |= FIELD_PREP(EXT_TX_VBOOST_LVL_MASK, 0x3) |
> > > +             FIELD_PREP(EXT_TX_TERM_CTRL_MASK, 0x4);
> > > +     writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
> >
> > This is part of an AI Generated review.
> > I have looked over it and I think it warrants investigation.
> > For information on how to reproduce locally, as I did, please see [1].
> >
> >         The entire s32g_serdes_prepare_pma_mode5() function is ~70 lines of
> >         magic numbers with zero explanation. These appear to be
> >         hardware-specific PLL/PHY tuning parameters for 2.5G mode.
> 
> Unfortunately there is no additional information in the reference
> manual other than
> *step 4:
> - Write 3h to EXT_TX_VBOOST_LVL.
> - Write 4h to EXT_TX_TERM_CTRL

Understood. Sometimes you have the play the hand you're dealt.

> 
> >
> > Please consider using #defines, to give values names.
> >
> > ...
> >
> > > +static int s32g_serdes_init_clks(struct s32g_serdes *serdes)
> > > +{
> > > +     struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> > > +     struct s32g_xpcs_ctrl *xpcs = &serdes->xpcs;
> > > +     int ret, order[2], xpcs_id;
> > > +     size_t i;
> > > +
> > > +     switch (ctrl->ss_mode) {
> > > +     case 0:
> > > +             return 0;
> > > +     case 1:
> > > +             order[0] = 0;
> > > +             order[1] = XPCS_DISABLED;
> > > +             break;
> > > +     case 2:
> > > +     case 5:
> > > +             order[0] = 1;
> > > +             order[1] = XPCS_DISABLED;
> > > +             break;
> > > +     case 3:
> > > +             order[0] = 1;
> > > +             order[1] = 0;
> > > +             break;
> > > +     case 4:
> > > +             order[0] = 0;
> > > +             order[1] = 1;
> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> >
> > AI review also flags that s32g_serdes_get_ctrl_resources() ensures that
> > ss_mode is <= 5.  So this check is unnecessary.
> 
> okay but providing a default seems a good practice

Yeah, I was of two minds about forwarding on this part of the report.

...

> > AI review also flags that s32g_xpcs_regmap_reg_read and
> > s32g_xpcs_regmap_reg_write do not protect against concurrent access.
> 
> but regmap framework should

If it does, then I agree there is no problem here.

Re: [PATCH 3/4] phy: s32g: Add serdes xpcs subsystem

[Russell King (Oracle)]

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@oss.nxp.com>
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Co-developed-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> Signed-off-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> Co-developed-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> Signed-off-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.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!

Re: [PATCH 3/4] phy: s32g: Add serdes xpcs subsystem

[Vincent Guittot]

On Thu, 29 Jan 2026 at 13:30, Russell King (Oracle)
<linux@armlinux.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@oss.nxp.com>
> > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > Co-developed-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> > Signed-off-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> > Co-developed-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> > Signed-off-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.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!

[PATCH 4/4] MAINTAINERS: Add MAINTAINER for NXP S32G Serdes driver

[Vincent Guittot]

Add a new entry for S32G Serdes driver.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 765ad2daa218..888674a308a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3202,6 +3202,15 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
 F:	drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
 
+ARM/NXP S32G SERDES DRIVER
+M:	Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
+R:	NXP S32 Linux Team <s32@nxp.com>
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/nxp,s32g-serdes.yaml
+F:	drivers/phy/freescale/phy-nxp-s32g-*
+F:	include/linux/pcs/pcs-nxp-xpcs.h
+
 ARM/Orion SoC/Technologic Systems TS-78xx platform support
 M:	Alexander Clouter <alex@digriz.org.uk>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
-- 
2.43.0

Re: [PATCH 4/4] MAINTAINERS: Add MAINTAINER for NXP S32G Serdes driver

[Simon Horman]

On Mon, Jan 26, 2026 at 10:21:59AM +0100, Vincent Guittot wrote:
> Add a new entry for S32G Serdes driver.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>  MAINTAINERS | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 765ad2daa218..888674a308a5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3202,6 +3202,15 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
>  F:	drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
>  
> +ARM/NXP S32G SERDES DRIVER
> +M:	Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> +R:	NXP S32 Linux Team <s32@nxp.com>
> +L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/pci/nxp,s32g-serdes.yaml

This patchset adds the following file, not the one above:

Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
                                  ^^^

> +F:	drivers/phy/freescale/phy-nxp-s32g-*
> +F:	include/linux/pcs/pcs-nxp-xpcs.h
> +
>  ARM/Orion SoC/Technologic Systems TS-78xx platform support
>  M:	Alexander Clouter <alex@digriz.org.uk>
>  L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> -- 
> 2.43.0
> 

Re: [PATCH 4/4] MAINTAINERS: Add MAINTAINER for NXP S32G Serdes driver

[Vincent Guittot]

On Thu, 29 Jan 2026 at 13:08, Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Jan 26, 2026 at 10:21:59AM +0100, Vincent Guittot wrote:
> > Add a new entry for S32G Serdes driver.
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >  MAINTAINERS | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 765ad2daa218..888674a308a5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3202,6 +3202,15 @@ S:     Maintained
> >  F:   Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> >  F:   drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> >
> > +ARM/NXP S32G SERDES DRIVER
> > +M:   Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > +R:   NXP S32 Linux Team <s32@nxp.com>
> > +L:   linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/pci/nxp,s32g-serdes.yaml
>
> This patchset adds the following file, not the one above:
>
> Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml

yes, that's a typo.

>                                   ^^^
>
> > +F:   drivers/phy/freescale/phy-nxp-s32g-*
> > +F:   include/linux/pcs/pcs-nxp-xpcs.h
> > +
> >  ARM/Orion SoC/Technologic Systems TS-78xx platform support
> >  M:   Alexander Clouter <alex@digriz.org.uk>
> >  L:   linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> > --
> > 2.43.0
> >

Re: [PATCH 0/4] Serdes: s32g: Add support for serdes subsystem

[Russell King (Oracle)]

Please drop these addresses from future patch series:

  alexandru-catalin.ionita@nxp.com
    host nxp-com.mail.protection.outlook.com [2a01:111:f403:ca09::7]
    SMTP error from remote mail server after RCPT TO:<alexandru-catalin.ionita@n
xp.com>:
    550 5.4.1 Recipient address rejected: Access denied. For more information se
e https://aka.ms/EXOSmtpErrors [AM4PEPF00027A62.eurprd04.prod.outlook.com 2026-0
1-29T12:31:01.197Z 08DE5971FB66165F]
  bogdan-gabriel.roman@nxp.com
    host nxp-com.mail.protection.outlook.com [2a01:111:f403:ca09::6]
    SMTP error from remote mail server after RCPT TO:<bogdan-gabriel.roman@nxp.c
om>:
    550 5.4.1 Recipient address rejected: Access denied. For more information se
e https://aka.ms/EXOSmtpErrors [AM3PEPF0000A798.eurprd04.prod.outlook.com 2026-0
1-29T12:31:00.781Z 08DE596525364766]

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

Re: [PATCH 0/4] Serdes: s32g: Add support for serdes subsystem

[Vincent Guittot]

On Thu, 29 Jan 2026 at 13:36, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> Please drop these addresses from future patch series:

Yes, will dorp for next version

>
>   alexandru-catalin.ionita@nxp.com
>     host nxp-com.mail.protection.outlook.com [2a01:111:f403:ca09::7]
>     SMTP error from remote mail server after RCPT TO:<alexandru-catalin.ionita@n
> xp.com>:
>     550 5.4.1 Recipient address rejected: Access denied. For more information se
> e https://aka.ms/EXOSmtpErrors [AM4PEPF00027A62.eurprd04.prod.outlook.com 2026-0
> 1-29T12:31:01.197Z 08DE5971FB66165F]
>   bogdan-gabriel.roman@nxp.com
>     host nxp-com.mail.protection.outlook.com [2a01:111:f403:ca09::6]
>     SMTP error from remote mail server after RCPT TO:<bogdan-gabriel.roman@nxp.c
> om>:
>     550 5.4.1 Recipient address rejected: Access denied. For more information se
> e https://aka.ms/EXOSmtpErrors [AM3PEPF0000A798.eurprd04.prod.outlook.com 2026-0
> 1-29T12:31:00.781Z 08DE596525364766]
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!