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

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

On Mon, Jan 26, 2026 at 10:21:58AM +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

...

quoted

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

...

quoted

+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

quoted

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

...

quoted

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

...

quoted

+             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

quoted

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

...

quoted

+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

quoted

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

...

quoted

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

...

quoted

+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

...