diff mbox series

[v3,04/10] scsi: ufs-mediatek: Fix the timing of configuring device regulators

Message ID 20220614141655.14409-5-stanley.chu@mediatek.com
State New
Headers show
Series scsi: ufs: Fix PMC and low-power mode on MediaTek UFS platforms | expand

Commit Message

Stanley Chu June 14, 2022, 2:16 p.m. UTC 
From: Po-Wen Kao <powen.kao@mediatek.com>

Currently the LPM configurations of device regulators
may not work since VCC is not disabled yet while
ufs_mtk_vreg_set_lpm() is executed.

Fix it by changing the timing of invoking
ufs_mtk_vreg_set_lpm().

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/host/ufs-mediatek.c | 60 ++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 5 deletions(-)

Comments

Stanley Chu June 15, 2022, 3:49 a.m. UTC | #1 
On Tue, 2022-06-14 at 09:33 -0700, Bart Van Assche wrote:
> On 6/14/22 07:16, Stanley Chu wrote:
> > +int ufs_mtk_system_suspend(struct device *dev)
> > +{
> > +	int ret = 0;
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +	ret = ufshcd_system_suspend(dev);
> > +
> > +	if (!ret)
> > +		ufs_mtk_vreg_set_lpm(hba, true);
> > +
> > +	return ret;
> > +}
> 
> 
> Please use the traditional kernel coding style and return early in
> case 
> of an error. For the above code, that means to rewrite it as follows:
> 
> 	struct ufs_hba *hba = dev_get_drvdata(dev);
> 	int ret;
> 
> 	ret = ufshcd_system_suspend(dev);
> 	if (ret)
> 		return ret;
> 
> 	ufs_mtk_vreg_set_lpm(hba, true);
> 
> 	return 0;

OK! Fixed in v4.

> 
> > +int ufs_mtk_system_resume(struct device *dev)
> > +{
> > +	int ret = 0;
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +	ufs_mtk_vreg_set_lpm(hba, false);
> > +
> > +	ret = ufshcd_system_resume(dev);
> > +
> > +	return ret;
> > +}
> 
> Please remove the variable 'ret' from the above function.

OK! Fixed in v4.
> 
> > +int ufs_mtk_runtime_suspend(struct device *dev)
> > +{
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +	int ret = 0;
> > +
> > +	ret = ufshcd_runtime_suspend(dev);
> > +
> > +	if (!ret)
> > +		ufs_mtk_vreg_set_lpm(hba, true);
> > +
> > +	return ret;
> > +}
> 
> Please use the "early return" style.

OK! Fixed in v4.
> 
> > +int ufs_mtk_runtime_resume(struct device *dev)
> > +{
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +	int ret = 0;
> > +
> > +	ufs_mtk_vreg_set_lpm(hba, false);
> > +
> > +	ret = ufshcd_runtime_resume(dev);
> > +
> > +	return ret;
> > +}
> 
> Please remove the variable 'ret' from the above function.

OK! Fixed in v4.
> 
> Thanks,
> 
> Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 21d591925dc4..3b3fe5470b71 100755
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1082,7 +1082,6 @@  static int ufs_mtk_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
 		 * ufshcd_suspend() re-enabling regulators while vreg is still
 		 * in low-power mode.
 		 */
-		ufs_mtk_vreg_set_lpm(hba, true);
 		err = ufs_mtk_mphy_power_on(hba, false);
 		if (err)
 			goto fail;
@@ -1106,12 +1105,13 @@  static int ufs_mtk_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 {
 	int err;
 
+	if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL)
+		ufs_mtk_vreg_set_lpm(hba, false);
+
 	err = ufs_mtk_mphy_power_on(hba, true);
 	if (err)
 		goto fail;
 
-	ufs_mtk_vreg_set_lpm(hba, false);
-
 	if (ufshcd_is_link_hibern8(hba)) {
 		err = ufs_mtk_link_set_hpm(hba);
 		if (err)
@@ -1276,9 +1276,59 @@  static int ufs_mtk_remove(struct platform_device *pdev)
 	return 0;
 }
 
+int ufs_mtk_system_suspend(struct device *dev)
+{
+	int ret = 0;
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	ret = ufshcd_system_suspend(dev);
+
+	if (!ret)
+		ufs_mtk_vreg_set_lpm(hba, true);
+
+	return ret;
+}
+
+int ufs_mtk_system_resume(struct device *dev)
+{
+	int ret = 0;
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	ufs_mtk_vreg_set_lpm(hba, false);
+
+	ret = ufshcd_system_resume(dev);
+
+	return ret;
+}
+
+int ufs_mtk_runtime_suspend(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ret = ufshcd_runtime_suspend(dev);
+
+	if (!ret)
+		ufs_mtk_vreg_set_lpm(hba, true);
+
+	return ret;
+}
+
+int ufs_mtk_runtime_resume(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ufs_mtk_vreg_set_lpm(hba, false);
+
+	ret = ufshcd_runtime_resume(dev);
+
+	return ret;
+}
+
 static const struct dev_pm_ops ufs_mtk_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
-	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(ufs_mtk_system_suspend, ufs_mtk_system_resume)
+	SET_RUNTIME_PM_OPS(ufs_mtk_runtime_suspend, ufs_mtk_runtime_resume, NULL)
 	.prepare	 = ufshcd_suspend_prepare,
 	.complete	 = ufshcd_resume_complete,
 };