Re: [PATCH v10 18/26] scsi: ufs: mediatek: Don't acquire dvfsrc-vcore twice

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Consolidating `dvfsrc-vcore` into a single shared regulator handle prevents the regulator core from aggregating voltage constraints, causing independent features to overwrite each other's voltage requests and leading to hardware undervoltage.
--

--- Patch [18]: [PATCH v10 18/26] scsi: ufs: mediatek: Don't acquire dvfsrc-vcore twice ---
commit f445ade145082cbe60e3dee2e8a70aa32aec9d9b
Author: Nicolas Frattaroli <[email protected]>

scsi: ufs: mediatek: Don't acquire dvfsrc-vcore twice

This patch consolidates the dvfsrc-vcore regulator into a single shared
handle on the host structure, rather than acquiring two separate references.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 0821de5b8a8fc..8214cf0b3a169 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
[ ... ]
> @@ -2005,7 +1998,7 @@ static void _ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
>  		}
>  
>  		if (clk_bind_vcore) {
> -			ret = regulator_set_voltage(reg, 0, INT_MAX);
> +			ret = regulator_set_voltage(host->reg_vcore, 0, INT_MAX);

[Severity: High]
Will consolidating to a single host->reg_vcore handle prevent the regulator
core from properly aggregating voltage constraints?

During a clock scale-down, ufs_mtk_clk_scale_notify() runs _ufs_mtk_clk_scale()
with scale_up = false during the pre-change notification phase. Since
host->reg_vcore is now a single shared consumer handle, calling
regulator_set_voltage() with 0V here will immediately drop the physical
voltage constraint.

However, the crypto boost clock isn't downclocked until the post-change
notification phase calls ufs_mtk_boost_crypt(). Can this violate DVFS
constraints and cause a hardware undervoltage by dropping the voltage while
the crypto clock is still running at high frequency?

It appears the regulator core relies on separate consumer handles to aggregate
maximum voltage requests across independent features like clock scaling and
crypto boost, which might explain why two separate references were originally
used.

>  			if (ret) {
>  				dev_err(hba->dev, "%s: Failed to set vcore to minimum: %pe\n",
>  					__func__, ERR_PTR(ret));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18