[net-next,v13,03/10] phy: qcom: add the SGMII SerDes PHY driver for SCMI systems
Commit Message
Implement support for the firmware-managed SGMII/SerDes PHY present on
Qualcomm platforms. Do this as a separate driver from the HLOS-managed
variant as they don't share almost any code.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/phy/qualcomm/Kconfig | 11 ++
drivers/phy/qualcomm/Makefile | 1 +
drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c | 165 +++++++++++++++++++++++++
3 files changed, 177 insertions(+)
Comments
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Improper GenPD performance state management causes hardware to resume with insufficient voltage.
- [Medium] Explicit PM runtime management in PHY power callbacks is redundant and double-increments usage counters.
- [Low] The local `perf_state` cache is updated before the hardware call, causing state mismatch on failure.
--
--- Patch [3]: [PATCH net-next v13 03/10] phy: qcom: add the SGMII SerDes PHY driver for SCMI systems ---
commit 15d41616bc0d5eacc339e6ee3b574f4d5ac6e400
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
phy: qcom: add the SGMII SerDes PHY driver for SCMI systems
Implement support for the firmware-managed SGMII/SerDes PHY present on
Qualcomm platforms. Do this as a separate driver from the HLOS-managed
variant as they don't share almost any code.
> 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..aa7ce9393b285
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c
> @@ -0,0 +1,165 @@
[ ... ]
> +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);
[Severity: Medium]
Is this explicit pm_runtime_resume_and_get() necessary?
The PHY core automatically handles runtime PM for the PHY device, and because
devm_phy_create() is used, the PHY core invokes phy_pm_runtime_get_sync()
which fully resumes the parent device before qcom_dwmac_sgmii_phy_scmi_power_on()
executes. Does this explicit call just double-increment the usage counter?
> + if (ret)
> + return ret;
> +
> + ret = dev_pm_genpd_set_performance_state(dev, priv->perf_state);
[Severity: High]
Will this sequence cause the hardware to resume with insufficient voltage?
Because the PHY core already resumed the device prior to this callback, the
domain is already powered on by GenPD. Since qcom_dwmac_sgmii_phy_scmi_power_off()
manually zeroes the performance state, GenPD will cache a 0 voltage request
and power on the domain with 0 state.
This dev_pm_genpd_set_performance_state() then raises the voltage only after
the hardware has already been resumed. GenPD natively supports dropping the
performance state of runtime-suspended devices. Could we unconditionally set
the performance state in probe() and set_mode() instead of manually toggling
it in the power callbacks?
> + 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);
[Severity: High]
Does manually zeroing the performance state here interfere with GenPD's
built-in state caching, forcing the next resume to happen at 0 voltage?
> + pm_runtime_put(dev);
[Severity: Medium]
Is this pm_runtime_put() redundant since the PHY core will drop the runtime PM
reference after this callback completes?
> +
> + 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);
> + struct device *dev = phy->dev.parent;
> + 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;
> +
> + if (phy->power_count == 0)
> + return 0;
[Severity: High]
Is it necessary to explicitly skip the GenPD call when the device is suspended?
GenPD automatically handles caching performance states for suspended devices.
Bypassing the framework here prevents GenPD from knowing the new state before
the next resume.
> +
> + return dev_pm_genpd_set_performance_state(dev, priv->perf_state);
[Severity: Low]
If dev_pm_genpd_set_performance_state() fails and returns an error,
priv->perf_state has already been updated.
Will this cause a state mismatch where the software caches the new mode but
the hardware remains in the old mode, causing a subsequent power_on() to
apply the wrong state?
> +}
@@ -232,3 +232,14 @@ config PHY_QCOM_SGMII_ETH
help
Enable this to support the internal SerDes/SGMII PHY on various
Qualcomm chipsets.
+
+config PHY_QCOM_SGMII_ETH_SCMI
+ tristate "Qualcomm DWMAC SGMII SerDes/PHY driver (firmware managed)"
+ depends on OF && (ARCH_QCOM || COMPILE_TEST)
+ select GENERIC_PHY
+ select PM
+ select PM_GENERIC_DOMAINS
+ help
+ Enable this to support the internal SerDes/SGMII PHY on Qualcomm
+ chipsets where the SerDes hardware (clocks and registers) is owned
+ by the firmware.
@@ -25,3 +25,4 @@ obj-$(CONFIG_PHY_QCOM_USB_SS) += phy-qcom-usb-ss.o
obj-$(CONFIG_PHY_QCOM_USB_SNPS_FEMTO_V2)+= phy-qcom-snps-femto-v2.o
obj-$(CONFIG_PHY_QCOM_IPQ806X_USB) += phy-qcom-ipq806x-usb.o
obj-$(CONFIG_PHY_QCOM_SGMII_ETH) += phy-qcom-sgmii-eth.o
+obj-$(CONFIG_PHY_QCOM_SGMII_ETH_SCMI) += phy-qcom-sgmii-eth-scmi.o
new file mode 100644
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ *
+ * Firmware-managed variant of the Qualcomm DWMAC SGMII SerDes/PHY driver.
+ */
+
+#include <linux/delay.h>
+#include <linux/device-id/of.h>
+#include <linux/ethtool.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+
+struct qcom_dwmac_sgmii_phy_scmi {
+ unsigned int perf_state;
+};
+
+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);
+ 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);
+
+ return 0;
+}
+
+static int qcom_dwmac_sgmii_phy_scmi_validate(struct phy *phy, enum phy_mode mode,
+ int submode,
+ union phy_configure_opts *opts)
+{
+ if (mode != PHY_MODE_ETHERNET)
+ return -EINVAL;
+
+ switch (submode) {
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_1000BASEX:
+ case PHY_INTERFACE_MODE_2500BASEX:
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+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);
+ struct device *dev = phy->dev.parent;
+ 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;
+
+ if (phy->power_count == 0)
+ return 0;
+
+ return dev_pm_genpd_set_performance_state(dev, priv->perf_state);
+}
+
+static const struct phy_ops qcom_dwmac_sgmii_phy_scmi_ops = {
+ .power_on = qcom_dwmac_sgmii_phy_scmi_power_on,
+ .power_off = qcom_dwmac_sgmii_phy_scmi_power_off,
+ .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_runtime_disable(void *data)
+{
+ struct device *dev = data;
+
+ pm_runtime_disable(dev);
+}
+
+static int qcom_dwmac_sgmii_phy_scmi_probe(struct platform_device *pdev)
+{
+ struct qcom_dwmac_sgmii_phy_scmi *priv;
+ struct device *dev = &pdev->dev;
+ struct phy_provider *provider;
+ struct phy *phy;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->perf_state = SPEED_1000;
+
+ /*
+ * 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.
+ */
+ pm_runtime_enable(dev);
+
+ ret = devm_add_action_or_reset(dev, qcom_dwmac_sgmii_phy_scmi_runtime_disable, dev);
+ if (ret)
+ return ret;
+
+ phy = devm_phy_create(dev, NULL, &qcom_dwmac_sgmii_phy_scmi_ops);
+ if (IS_ERR(phy))
+ return dev_err_probe(dev, PTR_ERR(phy), "failed to create the phy\n");
+
+ phy_set_drvdata(phy, priv);
+
+ provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+ if (IS_ERR(provider))
+ return dev_err_probe(dev, PTR_ERR(provider),
+ "failed to register the PHY provider\n");
+
+ return 0;
+}
+
+static const struct of_device_id qcom_dwmac_sgmii_phy_scmi_of_match[] = {
+ { .compatible = "qcom,sa8255p-dwmac-sgmii-phy" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, qcom_dwmac_sgmii_phy_scmi_of_match);
+
+static struct platform_driver qcom_dwmac_sgmii_phy_scmi_driver = {
+ .probe = qcom_dwmac_sgmii_phy_scmi_probe,
+ .driver = {
+ .name = "qcom-dwmac-sgmii-phy-scmi",
+ .of_match_table = qcom_dwmac_sgmii_phy_scmi_of_match,
+ },
+};
+module_platform_driver(qcom_dwmac_sgmii_phy_scmi_driver);
+
+MODULE_DESCRIPTION("Qualcomm DWMAC SGMII PHY driver (firmware managed)");
+MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>");
+MODULE_LICENSE("GPL");