[PATCH net-next v15 3/9] phy: qcom: add the SGMII SerDes PHY driver for SCMI systems

[netdev-bot+sashiko at kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Nothing in the new SCMI SerDes PHY driver ever tells firmware
  to power the SerDes down or release its rate vote.

--

phy: qcom: add the SGMII SerDes PHY driver for SCMI systems

This adds a separate PHY driver for the firmware-managed SGMII/SerDes PHY
on Qualcomm sa8255p platforms. The driver never touches registers or
clocks. It turns the requested interface mode into a genpd performance
state (SPEED_1000 or SPEED_2500) and uses runtime PM on the provider
device for power votes.

> diff --git a/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c b/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c
> new file mode 100644
> index 0000000000000..c726a414eb6a4
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c

[ ... ]

> +static int qcom_dwmac_sgmii_phy_scmi_set_mode(struct phy *phy, enum phy_mode mode,
> +					      int submode)
> +{
> +	struct device *dev = phy->dev.parent;
> +	unsigned int perf_state;
> +	int ret;
> +
> +	ret = qcom_dwmac_sgmii_phy_scmi_validate(phy, mode, submode, NULL);
> +	if (ret)
> +		return ret;
> +
> +	perf_state = (submode == PHY_INTERFACE_MODE_2500BASEX) ?
> +		     SPEED_2500 : SPEED_1000;
> +
> +	return dev_pm_genpd_set_performance_state(dev, perf_state);
> +}
> +
> +static const struct phy_ops qcom_dwmac_sgmii_phy_scmi_ops = {
> +	.set_mode	= qcom_dwmac_sgmii_phy_scmi_set_mode,
> +	.validate	= qcom_dwmac_sgmii_phy_scmi_validate,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static void qcom_dwmac_sgmii_phy_scmi_reset_perf_state(void *data)
> +{
> +	struct device *dev = data;
> +
> +	dev_pm_genpd_set_performance_state(dev, 0);
> +}
> +
> +static int qcom_dwmac_sgmii_phy_scmi_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	/*
> +	 * Enable runtime PM on the provider before creating the PHY so that the
> +	 * PHY core enables runtime PM on the PHY device too. The single SCMI
> +	 * power domain has already been attached to this device by the driver
> +	 * core, so runtime PM votes propagate to firmware through the genpd
> +	 * device link. No register or clock access is done here - firmware owns
> +	 * the SerDes.
> +	 */

[Severity: Medium]
Is this comment accurate for the domain type the binding describes?

The binding allows only one power-domains entry, and its example uses
<&scmi7_dvfs 0>. That is an SCMI perf domain, handled by
drivers/pmdomain/arm/scmi_perf_domain.c. When there is only one domain,
the driver core attaches it directly as dev->pm_domain, so no device
link is created.

scmi_perf_domain_probe() registers that genpd with no power_on or
power_off callbacks, and sets these flags:

	scmi_pd->genpd.flags = GENPD_FLAG_ALWAYS_ON |
			       GENPD_FLAG_OPP_TABLE_FW |
			       GENPD_FLAG_DEV_NAME_FW;

In addition, scmi_pd_set_perf_state() returns before it reaches
perf_ops->level_set() when the level is 0:

	if (!state)
		return 0;

The MAC calls phy_power_off()/phy_exit() in
qcom_ethqos_serdes_powerdown(). When this provider then runtime
suspends, the path is:

genpd_runtime_suspend()
    genpd_power_off()                    <- no-op, ALWAYS_ON
    genpd_drop_performance_state()
        scmi_pd_set_perf_state(genpd, 0) <- returns 0, no SCMI message

Does anything ever ask firmware to power down the SerDes or release its
rate vote?

qcom_dwmac_sgmii_phy_scmi_reset_perf_state() has the same problem on
unbind or probe failure. Its dev_pm_genpd_set_performance_state(dev, 0)
call ends at the same early return.

Say qcom_dwmac_sgmii_phy_scmi_set_mode() has requested SPEED_1000 or
SPEED_2500. Firmware then seems to keep that level through interface
down, phy_power_off() and driver unbind. Meanwhile genpd records
performance_state = 0. Can the kernel's view and the firmware's view get
out of sync here?

Whether the SerDes stays physically powered may depend on firmware and on
the MAC's own SCMI power domains. Even so, this driver never sends a
power-off or release request.

The opposite problem shows up if the DT points at an SCMI power-protocol
domain (scmi_pm_domain.c) instead. On/off would then work, but that genpd
has no set_performance_state callback. _genpd_set_performance_state()
would record the state and return 0. set_mode() would then report
success for 2500BASE-X even though nothing reaches firmware.

Other sa8255p firmware-managed devices use separate domains for these
two jobs. For example, qcom,sa8255p-geni-i2c.yaml has 'power' and 'perf'
as <&scmi0_pd 0>, <&scmi0_dvfs 0>. Would this driver need the same power
plus perf split, with the probe comment updated to match?

> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-qcom-sa8255p-emac-v15-0-e82f33720737%40oss.qualcomm.com