On Thu, 29 Jan 2026 at 10:54, Simon Horman [off-list ref] wrote:

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

...

quoted

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

quoted

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

...

quoted

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

quoted

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

quoted

+
+     return 0;
+}

...

quoted

+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

...

quoted

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

quoted

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

quoted

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

nit: This could be more succinctly written as:

fair enough

        return s32g_serdes_init(serdes);

quoted

+}

...