[net-next,v14,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 | 115 +++++++++++++++++++++++++
3 files changed, 127 insertions(+)
Comments
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver's devres teardown sequence cancels an asynchronous suspend of the platform device, leaving the device permanently active.
- [High] Missing cleanup of the genpd performance state on driver unbind leaves a persistent power vote on the PM domain.
- [Low] The driver forcefully selects the user-visible PM subsystem from a leaf hardware driver, overriding user configuration.
--
commit dae3dafc17dff2635ed1048ccc9e18ab67fdf8a3
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Subject: phy: qcom: add the SGMII SerDes PHY driver for SCMI systems
This commit adds a new firmware-managed variant of the Qualcomm DWMAC SGMII
SerDes/PHY driver. It introduces a separate driver from the HLOS-managed
variant since they share little code.
> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index d910a5d1a1ac3..677662f642e42 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
[ ... ]
> +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
[Severity: Low]
Is it intended to forcefully select the user-visible PM subsystem from a leaf
hardware driver?
Leaf drivers typically use "depends on" for user-visible features like PM
to avoid overriding user configurations.
> + 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.
> 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..ae961975dcd87
> --- /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);
[Severity: High]
If the genpd performance state is dynamically set to a value greater than 0
here, where does it get reset to 0?
Without a teardown path (such as a .remove callback or a dedicated devres
action) to explicitly reset the performance state, the PM domain hardware
might remain forced at a high performance state permanently after driver
unbind, wasting power. The genpd core does not automatically drop dynamically
set performance states on unbind.
> +}
[ ... ]
> +static void qcom_dwmac_sgmii_phy_scmi_runtime_disable(void *data)
> +{
> + struct device *dev = data;
> +
> + pm_runtime_disable(dev);
[Severity: High]
Will this devres teardown sequence cancel the asynchronous suspend of the
platform device?
In the devres unwinding sequence, phy_destroy() unregisters the PHY child,
which causes the PM core to queue an asynchronous pm_request_idle() for the
parent platform device.
If qcom_dwmac_sgmii_phy_scmi_runtime_disable() executes pm_runtime_disable()
immediately after, it aborts the pending idle request. This leaves the
platform device permanently locked in the active state. Could this be avoided
by ensuring the device suspends synchronously before disabling runtime PM?
> +}
[ ... ]
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 new platform driver for the firmware-managed SGMII/SerDes PHY
on Qualcomm sa8255p, kept separate from the HLOS-managed variant. The
driver's only functional operation is a genpd performance-state vote from
its set_mode() callback.
A few questions about the new Kconfig entry and the probe path below.
> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index d910a5d1a1ac3..677662f642e42 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
> @@ -242,3 +242,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
[Severity: Low]
Should this driver be forcing CONFIG_PM on with select?
kernel/power/Kconfig has:
config PM
bool "Device power management core functionality"
Since PM is a user-visible prompt symbol, selecting it from a leaf driver
means that enabling PHY_QCOM_SGMII_ETH_SCMI silently turns device power
management on for the whole kernel and the user can no longer unset it.
PM_GENERIC_DOMAINS is likewise normally owned by genpd providers and SoC
glue rather than by consumers.
This driver only consumes a domain (dev_pm_genpd_set_performance_state()
on its own device), so would "depends on PM_GENERIC_DOMAINS" (which
implies PM) express the requirement instead?
> + 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.
[ ... ]
> 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..ae961975dcd87
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c
> @@ -0,0 +1,115 @@
[ ... ]
> +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);
> +}
[Severity: High]
Who drops the performance-state vote when this driver goes away?
qcom_dwmac_sgmii_phy_scmi_set_mode() raises the vote on the provider
device to SPEED_1000 or SPEED_2500, but nothing in the driver ever
lowers it again. There is no .remove callback, and the only teardown
action registered in probe is
qcom_dwmac_sgmii_phy_scmi_runtime_disable(), which merely calls
pm_runtime_disable(). So after a sysfs unbind or an rmmod of
phy-qcom-sgmii-eth-scmi, the last level requested by set_mode() is
never reset to 0.
For the SCMI perf domain the binding example points at, the level was
pushed all the way down to firmware via scmi_pd_set_perf_state() ->
perf_ops->level_set(), and the domain carries GENPD_FLAG_ALWAYS_ON, so
there is no power-off transition that would incidentally clear it
either. The practical effect is that firmware keeps the SerDes clocked
for 2.5G indefinitely after the driver is gone, and the vote is also
stale across a rebind until the first set_mode() call arrives.
We are not sure the core covers this for you: on detach,
genpd_dev_pm_detach() -> genpd_remove_device() removes the device from
genpd->dev_list and frees its generic_pm_domain_data, which is what
_genpd_reeval_performance_state() aggregates over, so it is not obvious
whether the aggregated level is recomputed downwards at that point or
just left where it was. Could you confirm which core path resets it,
and if there is none, add an explicit dev_pm_genpd_set_performance_state(dev, 0)
in a .remove callback or a devres action registered next to the
runtime-PM one?
[Severity: Low]
Where is the performance-level namespace for this device defined?
qcom_dwmac_sgmii_phy_scmi_set_mode() feeds the ethtool link-speed
constants SPEED_1000 and SPEED_2500 from <linux/ethtool.h> (Mb/s values
1000 and 2500) directly into dev_pm_genpd_set_performance_state(), and
scmi_pd_set_perf_state() in drivers/pmdomain/arm/scmi_perf_domain.c hands
the number on unchanged:
ret = pd->perf_ops->level_set(pd->ph, pd->domain_id, state, false);
The new binding qcom,sa8255p-dwmac-sgmii-phy.yaml declares no OPP table,
no required-opps and no description of valid performance levels, so the
"Mb/s as SCMI level" contract is not written down anywhere. If firmware
uses indices or different units instead, does the wrong SerDes rate get
selected? Could the meaning of these levels be documented in the
binding, or the values derived from an OPP table?
[Severity: Low]
(same hunk) Other in-tree genpd consumers derive the level from
OPP-supplied pstates, INT_MAX, or a driver-defined enum rather than from
an ethtool speed constant. Is reusing SPEED_* here intentional?
[ ... ]
> +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;
> +
> + /*
> + * 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);
[Severity: Low]
Is the claim that "runtime PM votes propagate to firmware through the
genpd device link" true for the provider the binding example names?
The example in qcom,sa8255p-dwmac-sgmii-phy.yaml wires the PHY to
power-domains = <&scmi7_dvfs 0>;
and drivers/pmdomain/arm/scmi_perf_domain.c registers those domains as:
scmi_pd->genpd.flags = GENPD_FLAG_ALWAYS_ON |
GENPD_FLAG_OPP_TABLE_FW |
GENPD_FLAG_DEV_NAME_FW;
scmi_pd->genpd.set_performance_state = scmi_pd_set_perf_state;
There is no power_on/power_off callback and the domain is ALWAYS_ON, so
no runtime PM power vote can reach firmware through it. The other SCMI
provider, drivers/pmdomain/arm/scmi_pm_domain.c, has power_on/power_off
but no set_performance_state, so it cannot serve set_mode() either.
Since the binding allows only one phandle, can any single in-tree SCMI
provider offer both capabilities? Would it be clearer to say the runtime
PM plumbing exists so genpd re-applies the performance state on resume?
[Severity: Low]
Related to the same comment: the binding added earlier in this series,
Documentation/devicetree/bindings/phy/qcom,sa8255p-dwmac-sgmii-phy.yaml,
lists
required:
- compatible
- reg
- "#phy-cells"
- power-domains
- power-domain-names
but this driver consumes neither reg nor power-domain-names. There is no
platform_get_resource()/devm_platform_ioremap_resource() call and no
of_property_match_string()/devm_pm_domain_attach_by_name() call anywhere
in the file, and the comment above states that registers are not touched.
With power-domains capped at maxItems: 1 the automatic attach in
genpd_dev_pm_attach() never looks at power-domain-names either.
Comparable sa8255p bindings that require power-domain-names (for example
qcom,sa8255p-geni-uart.yaml) declare two domains, "power" and "perf", and
their drivers attach by name. Should reg and power-domain-names still be
mandatory here?
> +
> + 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");
> +
> + 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;
> +}
[Severity: Medium]
Should qcom_dwmac_sgmii_phy_scmi_probe() verify that a genpd was really
attached before publishing the PHY provider?
The comment above asserts the precondition, but nothing checks it.
genpd_dev_pm_attach() in drivers/pmdomain/core.c returns success without
attaching anything when the phandle count is not exactly one:
if (of_count_phandle_with_args(dev->of_node, "power-domains",
"#power-domain-cells") != 1)
return 0;
So with zero or two or more power-domains entries in the DT node,
dev->pm_domain stays NULL while pm_runtime_enable(), devm_phy_create()
and devm_of_phy_provider_register() all succeed.
Every later phy_set_mode_ext() from ethqos then hits:
genpd = dev_to_genpd_safe(dev);
if (!genpd)
return -ENODEV;
in dev_pm_genpd_set_performance_state(), and
qcom_dwmac_sgmii_phy_scmi_set_mode() returns that -ENODEV without any
message. Does this leave the SerDes rate unprogrammed for the lifetime
of the device, with the link never coming up and no diagnostic pointing
at the PHY provider? Would a check such as dev_to_genpd_safe(dev) (or
dev->pm_domain) in probe, with dev_err_probe(), make the failure visible?
[ ... ]
@@ -242,3 +242,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.
@@ -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,115 @@
+// 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_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 device *dev = &pdev->dev;
+ struct phy_provider *provider;
+ struct phy *phy;
+ int 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.
+ */
+ 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");
+
+ 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");