Re: [PATCH v2 3/4] spi: qcom-geni: Use resources helper APIs in runtime PM functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Performance state (OPP) vote is dropped during suspend but never restored on resume, leading to hardware operating at zero voltage.
--

commit 60a92c010d0b0fc6d9cc73f3af65a2635a04cf3c
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: qcom-geni: Use resources helper APIs in runtime PM functions

To manage GENI serial engine resources during runtime power management,
drivers currently need to call functions for ICC, clock, and
SE resource operations in both suspend and resume paths, resulting in
code duplication across drivers.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 333216df922e8..7696412c6e879 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -1119,33 +1119,16 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
>  {
>  	struct spi_controller *spi = dev_get_drvdata(dev);
>  	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
> -	int ret;
> -
> -	/* Drop the performance state vote */
> -	dev_pm_opp_set_rate(dev, 0);
> -
> -	ret = geni_se_resources_off(&mas->se);
> -	if (ret)
> -		return ret;
>  
> -	return geni_icc_disable(&mas->se);
> +	return geni_se_resources_deactivate(&mas->se);
>  }
>  
>  static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
>  {
>  	struct spi_controller *spi = dev_get_drvdata(dev);
>  	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
> -	int ret;
> -
> -	ret = geni_icc_enable(&mas->se);
> -	if (ret)
> -		return ret;
> -
> -	ret = geni_se_resources_on(&mas->se);
> -	if (ret)
> -		return ret;
>  
> -	return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
> +	return geni_se_resources_activate(&mas->se);
>  }

[Severity: High]
How does the performance state (OPP) rate get restored on resume?

Before this patch, spi_geni_runtime_resume() explicitly restored the OPP
rate using dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz).

While the new geni_se_resources_deactivate() helper continues to drop the
OPP rate to 0 on suspend, geni_se_resources_activate() does not restore it.
This seems to leave the hardware operating at zero voltage upon resume.

Furthermore, subsequent transfers won't reapply the rate because
geni_spi_set_clock_and_bw() has a fast-path optimization that skips setting
the clock if the requested frequency hasn't changed:

    if (clk_hz == mas->cur_speed_hz)
            return 0;

Will this lead to bus lockups or transfer timeouts due to operating without
adequate voltage votes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530-enable-spi-on-sa8255p-v2-0-17574601bd63@oss.qualcomm.com?part=3