Re: [PATCH v13 1/6] soc: qcom: ice: Add OPP-based clock scaling support for ICE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] soc: qcom: ice: qcom_ice_probe() fails with -EOPNOTSUPP when
  !CONFIG_PM_OPP
- [Medium] soc: qcom: ice: unreleased OPP power-domain vote in
  qcom_ice_suspend()
--

commit 9800ed91573a2f35826acc3c3e7577fb8636aaef
Author: Abhinaba Rakshit <abhinaba.rakshit@oss.qualcomm.com>

soc: qcom: ice: Add OPP-based clock scaling support for ICE

This commit registers an optional OPP table for the ICE device during
probe and introduces qcom_ice_scale_clk() to scale the ICE core clock via
the PM OPP framework.

> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index f9ac45505908f..2259dbecb8b51 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c

[ ... ]

> @@ -560,6 +562,50 @@ int qcom_ice_import_key(struct qcom_ice *ice,
>  }
>  EXPORT_SYMBOL_GPL(qcom_ice_import_key);
>  
> +/**
> + * qcom_ice_scale_clk() - Scale ICE clock for DVFS-aware operations
> + * @ice: ICE driver data
> + * @target_freq: requested frequency in Hz
> + * @round_ceil: when true, selects nearest freq >= @target_freq;
> + *              otherwise, selects nearest freq <= @target_freq
> + *
> + * Selects an OPP frequency based on @target_freq and the rounding direction
> + * specified by @round_ceil, then programs it using dev_pm_opp_set_rate(),
> + * including any voltage or power-domain transitions handled by the OPP
> + * framework.
> + *
> + * Return: 0 on success; -EOPNOTSUPP if no OPP table; or errno from
> + *         dev_pm_opp_set_rate()/OPP lookup.
> + */
> +int qcom_ice_scale_clk(struct qcom_ice *ice, unsigned long target_freq,
> +		       bool round_ceil)
> +{
> +	unsigned long ice_freq = target_freq;
> +	struct dev_pm_opp *opp;
> +	int ret;
> +
> +	if (!ice->has_opp)
> +		return -EOPNOTSUPP;
> +
> +	if (round_ceil)
> +		opp = dev_pm_opp_find_freq_ceil(ice->dev, &ice_freq);
> +	else
> +		opp = dev_pm_opp_find_freq_floor(ice->dev, &ice_freq);
> +
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
> +	dev_pm_opp_put(opp);
> +
> +	ret = dev_pm_opp_set_rate(ice->dev, ice_freq);
> +	if (ret) {
> +		dev_err(ice->dev, "Unable to scale ICE clock rate\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_scale_clk);

[Severity: Medium]
Does the performance state vote set here by dev_pm_opp_set_rate() stay
asserted when ICE suspends?

When dev_pm_opp_set_rate() scales up the clock, it sets the required-opps
performance state on ice->dev's power domain (and its parent RPMHPD_CX).
genpd only drops a device's performance state in genpd_runtime_suspend(),
but runtime PM is never enabled on ice->dev, and qcom_ice_suspend() only
disables iface_clk and core_clk without calling
dev_pm_opp_set_rate(ice->dev, 0):

drivers/soc/qcom/ice.c:qcom_ice_suspend() {
	clk_disable_unprepare(ice->iface_clk);
	clk_disable_unprepare(ice->core_clk);
	ice->hwkm_init_complete = false;

	return 0;
}

Once a storage consumer scales the ICE clock, the CX rail vote remains
elevated across both runtime suspend and system suspend.

>  static struct qcom_ice *qcom_ice_create(struct device *dev,
>  					void __iomem *base)
>  {
> @@ -738,6 +784,7 @@ static int qcom_ice_probe(struct platform_device *pdev)
>  	unsigned long phandle = pdev->dev.of_node->phandle;
>  	struct qcom_ice *engine;
>  	void __iomem *base;
> +	int err;
>  
>  	guard(mutex)(&ice_mutex);
>  
> @@ -756,6 +803,34 @@ static int qcom_ice_probe(struct platform_device *pdev)
>  		return PTR_ERR(engine);
>  	}
>  
> +	err = devm_pm_opp_set_clkname(&pdev->dev, "core");
> +	if (err && err != -ENOENT) {
> +		dev_err(&pdev->dev, "Unable to set core clkname to OPP-table\n");
> +		/* Store the error pointer for devm_of_qcom_ice_get() */
> +		xa_store(&ice_handles, phandle, ERR_PTR(err), GFP_KERNEL);
> +		return err;
> +	}
> +
> +	/* OPP table is optional */
> +	err = devm_pm_opp_of_add_table(&pdev->dev);
> +	if (err && err != -ENODEV) {
> +		dev_err(&pdev->dev, "Invalid OPP table in Device tree\n");
> +		/* Store the error pointer for devm_of_qcom_ice_get() */
> +		xa_store(&ice_handles, phandle, ERR_PTR(err), GFP_KERNEL);
> +		return err;
> +	}
> +
> +	/*
> +	 * The OPP table is optional. devm_pm_opp_of_add_table() returns
> +	 * -ENODEV when no OPP table is present in DT, which is not treated
> +	 * as an error. Therefore, track successful OPP registration only
> +	 * when err is not -ENODEV.
> +	 */
> +	if (err == -ENODEV)
> +		dev_dbg(&pdev->dev, "ICE OPP table is not registered, please update your DT\n");
> +	else
> +		engine->has_opp = true;

[Severity: Medium]
Does qcom_ice_probe() fail with -EOPNOTSUPP when CONFIG_PM_OPP is
disabled?

QCOM_INLINE_CRYPTO_ENGINE does not select or depend on PM_OPP.  When
CONFIG_PM_OPP is disabled, the stubs for devm_pm_opp_set_clkname() and
devm_pm_opp_of_add_table() in include/linux/pm_opp.h both return
-EOPNOTSUPP.  Because the error checks only allow -ENOENT and -ENODEV,
qcom_ice_probe() stores ERR_PTR(-EOPNOTSUPP) in ice_handles and fails,
preventing ufs-qcom and sdhci-msm from using inline crypto.  Also, if
-EOPNOTSUPP is ignored above, the final check needs to test !err before
setting engine->has_opp = true so it is not set when CONFIG_PM_OPP is
disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-enable-ice-clock-scaling-v13-0-258dc817bd63@oss.qualcomm.com?part=1