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

[Simon Horman <horms@kernel.org>]

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);

> +}

...