Re: [PATCH v13 2/6] ufs: host: Add ICE clock scaling during UFS clock changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] ufs: ufs-qcom: inverted ICE clock rounding in
  ufs_qcom_clk_scale_notify()
- [Medium] ufs: ufs-qcom: ICE clock scaling skipped when HBA is inactive
  on reset
- [Medium] ufs: ufs-qcom: broken clock rollback on error in
  ufs_qcom_clk_scale_notify()
--

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

ufs: host: Add ICE clock scaling during UFS clock changes

This commit adds Inline Crypto Engine (ICE) clock scaling to
ufs_qcom_clk_scale_notify() so the ICE clock scales alongside the UFS
controller clock.

> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 6afb8498b8c26..5019803a6aba4 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
[ ... ]
> @@ -1987,6 +2002,12 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
>  			return err;
>  		}
>  
> +		err = ufs_qcom_ice_scale_clk(host, target_freq, !scale_up);
> +		if (err && err != -EOPNOTSUPP) {
> +			ufshcd_uic_hibern8_exit(hba);
> +			return err;
> +		}
> +
>  		ufs_qcom_icc_update_bw(host);
>  		ufshcd_uic_hibern8_exit(hba);
>  	}

[Severity: High]
Is the round_ceil argument (!scale_up) inverted in this
ufs_qcom_ice_scale_clk() call?

qcom_ice_scale_clk() calls dev_pm_opp_find_freq_ceil() when round_ceil
is true and dev_pm_opp_find_freq_floor() when round_ceil is false, while
target_freq is the concrete UFS core_clk frequency in Hz.  On SoCs where
ICE OPP rates are higher than the corresponding UFS core_clk rates for
the same voltage corners (for example on QCS8300 or SM8550, where UFS
core_clk OPPs are 75/150/300 MHz and ICE OPPs are 100/201.5/403 MHz),
passing round_ceil = false during scale_up rounds 300 MHz down to
201.5 MHz, so the ICE clock never reaches its 403 MHz nominal OPP.
Scaling up to an intermediate OPP such as 150 MHz also picks 100 MHz
while scaling down to 150 MHz picks 201.5 MHz, and scaling up to a
frequency below the minimum ICE OPP (such as 75 MHz on SM8550) fails with
-ERANGE and aborts UFS clock scaling.

[Severity: Medium]
Does placing ufs_qcom_ice_scale_clk() after the !ufshcd_is_hba_active()
early return in ufs_qcom_clk_scale_notify() skip ICE clock scaling during
host reset and restore?

ufshcd_host_reset_and_restore() stops the HBA before scaling the clocks
back to maximum:

drivers/ufs/core/ufshcd.c:ufshcd_host_reset_and_restore() {
    ...
	ufshcd_hba_stop(hba);
	hba->silence_err_logs = true;
	ufshcd_complete_requests(hba, true);
	hba->silence_err_logs = false;

	/* scale up clocks to max frequency before full reinitialization */
	if (ufshcd_is_clkscaling_supported(hba))
		ufshcd_scale_clks(hba, ULONG_MAX, true);
    ...
}

Because ufshcd_is_hba_active(hba) is false at that point,
ufs_qcom_clk_scale_notify() returns 0 immediately to avoid sending UIC
Hibern8 commands.  Unlike the UFS timers, UniPro clock control and
interconnect bandwidth, the ICE clock is not reconfigured during the
subsequent ufshcd_hba_enable() and link startup, leaving the ICE clock at
its previous lower OPP while the UFS controller runs at maximum
frequency.

[Severity: Medium]
If ufs_qcom_ice_scale_clk() returns an error here, does the error path
leave the UFS timer and UniPro clock control registers out of sync with
the reverted clock rate?

By the time ufs_qcom_ice_scale_clk() runs in POST_CHANGE,
ufs_qcom_clk_scale_up_pre_change() or
ufs_qcom_clk_scale_down_post_change() has already programmed
REG_UFS_SYS1CLK_1US, DME_VS_CORE_CLK_CTRL and PA_VS_CORE_CLK_40NS_CYCLES
for target_freq.  On error, ufs_qcom_clk_scale_notify() exits Hibern8 and
returns err, causing ufshcd_scale_clks() to roll the UFS clocks back to
previous_freq while the link is active and without restoring those
hardware registers to match previous_freq.

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