[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?
> +}
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
phy: qcom: add the SGMII SerDes PHY driver for SCMI systems
This adds a firmware-managed variant of the Qualcomm DWMAC SGMII
SerDes/PHY driver for platforms where the SerDes hardware (clocks and
registers) is owned by the firmware and controlled via an SCMI power
domain performance state.
A few questions on the new driver below.
> 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 000000000000..aa7ce9393b28
> --- /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);
> + 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);
[Medium]
What is the firmware contract that the 5-10 ms sleep is enforcing here?
If firmware synchronously waits for PLL lock / PCS ready before returning
from the SCMI perf-state call, this sleep looks unnecessary and could use
a comment saying so.
If firmware returns before the SerDes has stabilized at the requested rate,
then the fixed 5-10 ms delay may be too short. The sibling HLOS driver
drivers/phy/qualcomm/phy-qcom-sgmii-eth.c polls QSERDES_COM_C_READY_STATUS,
QPHY_PCS_PCS_READY_STATUS (PCS_READY and SGMIIPHY_READY), and
QSERDES_V5_COM_CMN_STATUS (PLL_LOCKED) with a timeout on the order of
750 ms in qcom_dwmac_sgmii_phy_calibrate(), which suggests worst-case
ready time can be considerably larger than 10 ms.
The same concern applies below in set_mode(): when phy->power_count > 0
the driver issues the same perf-state vote to switch between SPEED_1000
and SPEED_2500 with no delay at all. Should the two paths agree on
whether a settling delay is required?
> +
> + 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;
> +}
[Low]
Should power_off check the return value of
dev_pm_genpd_set_performance_state(dev, 0)?
The power_on path unwinds pm_runtime on failure, but here a failing SCMI
transport call is silently discarded and the function still returns 0.
If the vote is not cleared, the domain retains an active performance
state while the PHY core believes the PHY has been powered off, leaving
firmware and driver state out of sync with no dev_warn/dev_err.
> +
> +static int qcom_dwmac_sgmii_phy_scmi_validate(struct phy *phy, enum phy_mode mode,
> + int submode,
> + union phy_configure_opts *opts)
> +{
[ ... ]
> +}
> +
> +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);
> +}
[Low]
Would it be worth documenting the contract between these ethtool link-
speed constants and the SCMI provider's OPP table?
SPEED_1000 (=1000) and SPEED_2500 (=2500) are passed directly to
dev_pm_genpd_set_performance_state() as opaque state tokens looked up in
the domain's OPP table by the genpd core. Nothing in the driver, the
binding, or the commit message states that the firmware must expose
OPPs numbered exactly 1000 and 2500. If a future firmware indexes its
OPPs differently (for example 0/1, or in MBd), mode changes would
silently misconfigure the SerDes here. A short comment or a small helper
that maps submode to a named state would make the assumption explicit.
[ ... ]
> +static const struct of_device_id qcom_dwmac_sgmii_phy_scmi_of_match[] = {
> + { .compatible = "qcom,sa8255p-dwmac-sgmii-phy" },
> + { }
> +};
[Low, Low]
Two questions on the accompanying DT binding for
qcom,sa8255p-dwmac-sgmii-phy for this compatible.
The binding makes reg a required property with a 0xe10-byte range
describing the SerDes, but this driver never calls
platform_get_resource(), devm_platform_ioremap_resource(), or any
register-access API, and the commit message explicitly states that
firmware owns the SerDes and no register or clock access is done here.
Should reg be dropped from required (or removed from the schema), or
should the binding document why the OS must describe an address block
it never touches?
The binding also requires power-domain-names (const "serdes"), but the
driver relies on the single implicit genpd attached to dev->pm_domain by
the driver core and never looks up a domain by name (no
dev_pm_domain_attach_by_name() or similar). Would it be reasonable to
either make power-domain-names optional, or note in the binding what
future consumer the named lookup is intended for?
@@ -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");