Re: [PATCH v13 3/6] mmc: sdhci-msm: Set ICE clk to TURBO at sdhci ICE init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mmc: sdhci-msm: unreleased ICE OPP vote in
  sdhci_msm_ice_init()
- [Low] mmc: sdhci-msm: unused !CONFIG_MMC_CRYPTO stub for
  sdhci_msm_ice_scale_clk()
--

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

mmc: sdhci-msm: Set ICE clk to TURBO at sdhci ICE init

This commit scales the Inline Crypto Engine (ICE) clock to its maximum
frequency via qcom_ice_scale_clk() during sdhci_msm_ice_init() so that it
matches the MMC controller clock rate.

> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 32b1bcba47d6b..652c5cfc87bcc 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1902,6 +1902,16 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  static const struct blk_crypto_ll_ops sdhci_msm_crypto_ops; /* forward decl */
>  
> +static int sdhci_msm_ice_scale_clk(struct sdhci_msm_host *msm_host,
> +				   unsigned long target_freq,
> +				   bool round_ceil)
> +{
> +	if (msm_host->mmc->caps2 & MMC_CAP2_CRYPTO)
> +		return qcom_ice_scale_clk(msm_host->ice, target_freq, round_ceil);
> +
> +	return 0;
> +}
> +
>  static int sdhci_msm_ice_init(struct sdhci_msm_host *msm_host,
>  			      struct cqhci_host *cq_host)
>  {
> @@ -1960,6 +1970,11 @@ static int sdhci_msm_ice_init(struct sdhci_msm_host *msm_host,
>  
>  	mmc->caps2 |= MMC_CAP2_CRYPTO;
>  	mmc->caps2 |= MMC_CAP2_CRYPTO_NO_REPROG;
> +
> +	err = sdhci_msm_ice_scale_clk(msm_host, ULONG_MAX, false);
> +	if (err && err != -EOPNOTSUPP)
> +		dev_warn(dev, "Unable to boost ICE clock to TURBO\n");
> +
>  	return 0;
>  }

[Severity: Medium]
As reported by Ulf Hansson on v12 [1], does this leave the OPP and
power-domain performance vote on msm_host->ice->dev permanently asserted?

On platforms with a standalone sdhc_ice device and an OPP table with
required-opps (such as sc7280 and qcs8300), qcom_ice_scale_clk() calls
dev_pm_opp_set_rate(ice->dev, ...), which votes on the shared CX power
domain.  Because qcom_ice_probe() does not enable runtime PM on ice->dev,
genpd never drops this performance state automatically.

In sdhci_msm_runtime_suspend(), the driver drops the controller's own vote
with dev_pm_opp_set_rate(dev, 0), but sdhci_msm_ice_suspend() only calls
qcom_ice_suspend() to gate the ICE clocks.  The vote on ice->dev is also
left active if cqhci_init() or __sdhci_add_host() fails after
sdhci_msm_ice_init(), or when sdhci_msm_remove() unbinds the driver,
keeping the shared CX rail elevated during idle and system suspend.

> @@ -2160,6 +2175,13 @@ sdhci_msm_ice_suspend(struct sdhci_msm_host *msm_host)
>  static inline void sdhci_msm_ice_restore(struct sdhci_host *host)
>  {
>  }
> +
> +static inline int
> +sdhci_msm_ice_scale_clk(struct sdhci_msm_host *msm_host, unsigned long target_freq,
> +			bool round_ceil)
> +{
> +	return 0;
> +}
>  #endif /* !CONFIG_MMC_CRYPTO */

[Severity: Low]
This isn't a bug, but sdhci_msm_ice_scale_clk() has no callers outside the
CONFIG_MMC_CRYPTO block.  Its only caller is sdhci_msm_ice_init() right
after setting mmc->caps2 |= MMC_CAP2_CRYPTO, so this stub is unused and
sdhci_msm_ice_init() could call qcom_ice_scale_clk() directly.

[1] https://lore.kernel.org/r/CAPx+jO_Ltc6Q+VdUgOs81azTiM5Vx12GH1z+CXt6NW=M601THQ@mail.gmail.com

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