On Thu, Jan 29, 2026 at 02:24:15PM +0100, Vincent Guittot wrote:

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

quoted

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

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

quoted

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

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

...

quoted

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.