Re: [PATCH net-next v2 8/9] net: stmmac: qcom-ethqos: add per-platform NOC clock voting

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] New hard dependency on the PM_OPP subsystem is not expressed
  in Kconfig.

--

net: stmmac: qcom-ethqos: add per-platform NOC clock voting

This adds an ethqos_noc_clk_cfg description so each compatible can declare
its own set of System NOC clocks with per-clock rates. The clocks are
acquired in probe via qcom_ethqos_init_noc_clks() and enabled/disabled
alongside the link clock in ethqos_clks_config(), with an OPP table used to
propagate the VDD_CX performance state.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 2471871562ed9..8871d28594116 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -7,6 +7,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/phy.h>
>  #include <linux/phy/phy.h>
> +#include <linux/pm_opp.h>
>  
>  #include "stmmac.h"
>  #include "stmmac_platform.h"

[ ... ]

> @@ -689,15 +703,51 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  static int ethqos_clks_config(void *priv, bool enabled)
>  {
>  	struct qcom_ethqos *ethqos = priv;
> +	unsigned int i;
>  	int ret = 0;
>  
>  	if (enabled) {
> +		if (ethqos->num_noc_clks) {
> +			ret = dev_pm_opp_set_rate(&ethqos->pdev->dev,
> +						  ethqos->noc_clk_rates[0]);
> +			if (ret) {
> +				dev_err(&ethqos->pdev->dev,
> +					"NOC OPP rate set failed: %d\n", ret);
> +				return ret;
> +			}

[ ... ]

> @@ -734,6 +789,51 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
>  	netdev_dbg(priv->dev, "PTP rate %lu\n", plat_dat->clk_ptp_rate);
>  }
>  
> +static void qcom_ethqos_noc_opp_cleanup(void *dev)
> +{
> +	dev_pm_opp_set_rate(dev, 0);
> +}
> +
> +/* Some SoCs gate NOC access behind dedicated clocks. Acquire them here
> + * so ethqos_clks_config() can enable/disable them at runtime. The OPP
> + * table is used to propagate the required VDD_CX performance state via
> + * dev_pm_opp_set_rate().
> + */
> +static int qcom_ethqos_init_noc_clks(struct qcom_ethqos *ethqos,
> +				     const struct ethqos_emac_driver_data *data)
> +{

[ ... ]

> +	ret = devm_clk_bulk_get(dev, ethqos->num_noc_clks, ethqos->noc_clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get NOC clocks\n");
> +
> +	ret = devm_pm_opp_set_clkname(dev, data->noc_clk_cfg[0].id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set OPP clock name\n");
> +
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add OPP table\n");
> +
> +	ret = dev_pm_opp_set_rate(dev, data->noc_clk_cfg[0].rate);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set initial NOC OPP rate\n");

[Severity: Medium]
Should DWMAC_QCOM_ETHQOS gain a PM_OPP dependency now that these calls are
mandatory for compatibles that declare NOC clocks?

The Kconfig entry currently reads:

drivers/net/ethernet/stmicro/stmmac/Kconfig:
config DWMAC_QCOM_ETHQOS
	tristate "Qualcomm ETHQOS support"
	default ARCH_QCOM
	depends on OF && (ARCH_QCOM || COMPILE_TEST)

There is no depends on PM_OPP and no select PM_OPP, and PM_OPP itself is a
hidden symbol with no prompt:

drivers/opp/Kconfig:
config PM_OPP
	bool

so CONFIG_DWMAC_QCOM_ETHQOS=y with CONFIG_PM_OPP=n is a reachable
configuration. In that case the stubs are used:

include/linux/pm_opp.h:
static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
	return -EOPNOTSUPP;
}
...
static inline int devm_pm_opp_of_add_table(struct device *dev)
{
	return -EOPNOTSUPP;
}

qcom_ethqos_probe() -> qcom_ethqos_init_noc_clks() ->
devm_pm_opp_set_clkname() would then return -EOPNOTSUPP, and the
dev_err_probe() above aborts probe with "Failed to set OPP clock name", so
no netdev is registered for any compatible that sets num_noc_clks
(qcom,shikra-ethqos, added later in this series).

The same applies on the runtime path: ethqos_clks_config(priv, true)
propagates the -EOPNOTSUPP from dev_pm_opp_set_rate() on every enable and
resume.

Would adding select PM_OPP (or depends on PM_OPP) to DWMAC_QCOM_ETHQOS, or
tolerating -EOPNOTSUPP on the OPP path, be preferable here?

> +
> +	return devm_add_action_or_reset(dev, qcom_ethqos_noc_opp_cleanup, dev);
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-shikra_ethernet-v2-0-bbe3389d0652%40oss.qualcomm.com