[net-next,v15,3/9] 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 | 10 +++
drivers/phy/qualcomm/Makefile | 1 +
drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c | 117 +++++++++++++++++++++++++
3 files changed, 128 insertions(+)
Comments
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;
[ ... ]
On Wed, 23 Sep 2026 17:14:37 +0200 Bartosz Golaszewski wrote:
> 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 | 10 +++
> drivers/phy/qualcomm/Makefile | 1 +
> drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c | 117 +++++++++++++++++++++++++
> 3 files changed, 128 insertions(+)
Hmm, why is this here? is there a hard dependency?
Otherwise it'd be better to send it separately out to Vinod,
this is drivers/phy after all.
On Thu, 1 Oct 2026 01:32:17 +0200, Jakub Kicinski <kuba@kernel.org> said:
> On Wed, 23 Sep 2026 17:14:37 +0200 Bartosz Golaszewski wrote:
>> 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 | 10 +++
>> drivers/phy/qualcomm/Makefile | 1 +
>> drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c | 117 +++++++++++++++++++++++++
>> 3 files changed, 128 insertions(+)
>
> Hmm, why is this here? is there a hard dependency?
> Otherwise it'd be better to send it separately out to Vinod,
> this is drivers/phy after all.
>
Heh, it's always the same question of whether to split larger series into
per-subsystem threads or keep them together in order to provide the full
picture and let maintainers pick up subsets of patches. I've been yelled at
for doing both in the past. :)
Ok, I'll split the next iteration into smaller chunks.
Bart
@@ -242,3 +242,13 @@ 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)
+ depends on PM_GENERIC_DOMAINS
+ select GENERIC_PHY
+ 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.
@@ -26,3 +26,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,117 @@
+// 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/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>
+
+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 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)
+{
+ struct device *dev = &pdev->dev;
+ struct phy_provider *provider;
+ struct phy *phy;
+ int ret;
+
+ ret = devm_add_action_or_reset(dev, qcom_dwmac_sgmii_phy_scmi_reset_perf_state, dev);
+ if (ret)
+ return ret;
+
+ /*
+ * 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.
+ */
+ ret = devm_pm_runtime_enable(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");
+
+ 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");