Re: [PATCH net-next v12 2/7] phy: qcom: add the SGMII SerDes PHY driver for SCMI systems

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `set_mode` callback fails to dynamically apply the new performance state to the PM domain when the PHY is already active.
- [High] Incorrect ordering of `dev_pm_genpd_set_performance_state()` around runtime PM functions causes hardware under-voltage.
--

commit d60c07674fdf361d41a55e9d09fbdf040bae5797
Author: Bartosz Golaszewski <[email protected]>

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

This commit implements support for the firmware-managed SGMII/SerDes PHY
present on Qualcomm platforms. It introduces a separate driver for the SCMI
system variant as it shares almost no code with the HLOS-managed version.

> 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..8ee62189556fe
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c
[ ... ]
> +static int qcom_dwmac_sgmii_phy_scmi_power_on(struct phy *phy)
> +{
> +	struct qcom_dwmac_sgmii_phy_scmi *priv = phy_get_drvdata(phy);
> +	struct device *dev = phy->dev.parent;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = dev_pm_genpd_set_performance_state(dev, priv->perf_state);

[Severity: High]
Should dev_pm_genpd_set_performance_state() be called before
pm_runtime_resume_and_get()?

If pm_runtime_resume_and_get() runs first, it appears the genpd runtime
resume handler will power on the domain at its previous performance state (0)
before the driver raises it. Could this starve the hardware during power-on?

> +	if (ret) {
> +		pm_runtime_put(dev);
> +		return ret;
> +	}
> +
> +	usleep_range(5000, 10000);
> +
> +	return 0;
> +}
> +
> +static int qcom_dwmac_sgmii_phy_scmi_power_off(struct phy *phy)
> +{
> +	struct device *dev = phy->dev.parent;
> +
> +	dev_pm_genpd_set_performance_state(dev, 0);
> +	pm_runtime_put(dev);

[Severity: High]
Does dropping the performance state before putting the device cause a voltage
drop while the hardware is still active?

Since pm_runtime_put() is asynchronous, the hardware might still be actively
powered when dev_pm_genpd_set_performance_state(dev, 0) drops the state. Could
this ordering violate hardware voltage requirements?

> +
> +	return 0;
> +}
[ ... ]
> +static int qcom_dwmac_sgmii_phy_scmi_set_mode(struct phy *phy, enum phy_mode mode,
> +					      int submode)
> +{
> +	struct qcom_dwmac_sgmii_phy_scmi *priv = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = qcom_dwmac_sgmii_phy_scmi_validate(phy, mode, submode, NULL);
> +	if (ret)
> +		return ret;
> +
> +	priv->perf_state = (submode == PHY_INTERFACE_MODE_2500BASEX) ?
> +			   SPEED_2500 : SPEED_1000;
> +
> +	return 0;
> +}

[Severity: High]
Does this code need to apply the new performance state to the active PM domain
if the PHY is already powered on?

When the MAC driver dynamically changes the link speed while the interface is
up, updating only priv->perf_state without calling
dev_pm_genpd_set_performance_state() might leave the hardware operating at an
incorrect voltage state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic