[PATCH v3 00/10] scsi: ufs: Fix PMC and low-power mode on MediaTek UFS platforms

[PATCH v3 00/10] scsi: ufs: Fix PMC and low-power mode on MediaTek UFS platforms

[Stanley Chu]

Hi Martin,

This series provides some fixes on MediaTek UFS platforms, please consider this patch series for kernel v5.20.

Sorry to change this patch series so frequently since we have to post and backport these patches to Android kernel recently.

- Provide workaround for power mode change for HS-G5
- Fix and provide regulator features

Changes compared to v2:
- Add patches to support multiple VCC sources

Changes compared to v1:
- Add patches to fix and provide VCCQx low-power support

Alice Chao (1):
  scsi: ufs-mediatek: Support flexible parameters for smc calls

CC Chou (1):
  scsi: ufs-mediatek: Introduce workaround for power mode change

Peter Wang (1):
  scsi: ufs-mediatek: Support low-power mode for VCCQ

Po-Wen Kao (2):
  scsi: ufs-mediatek: Fix the timing of configuring device regulators
  scsi: ufs-mediatek: Prevent device regulators setting as LPM
    incorrectly

Stanley Chu (5):
  scsi: ufs: Export ufshcd_uic_change_pwr_mode()
  scsi: ufs: Fix ADAPT logic for HS-G5
  scsi: ufs-mediatek: Support low-power mode for parents of VCCQx
  scsi: ufs: Export regulator functions
  scsi: ufs-mediatek: Support multiple VCC sources

 drivers/ufs/core/ufshcd.c        |   8 +-
 drivers/ufs/host/ufs-mediatek.c  | 232 ++++++++++++++++++++++++++-----
 drivers/ufs/host/ufs-mediatek.h  |  75 ++++++++++
 drivers/ufs/host/ufshcd-pltfrm.c |   3 +-
 drivers/ufs/host/ufshcd-pltfrm.h |   2 +
 include/ufs/ufshcd.h             |   3 +
 include/ufs/unipro.h             |   1 +
 7 files changed, 289 insertions(+), 35 deletions(-)

-- 
2.18.0

[PATCH v3 01/10] scsi: ufs: Export ufshcd_uic_change_pwr_mode()

[Stanley Chu]

Export ufshcd_uic_change_pwr_mode() to allow vendors to
use it for SoC-specific power mode change design limitation.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 3 ++-
 include/ufs/ufshcd.h      | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 936a6d5467c9..19e17c898319 100755
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4091,7 +4091,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
  *
  * Returns 0 on success, non-zero value on failure
  */
-static int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
+int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
 {
 	struct uic_command uic_cmd = {0};
 	int ret;
@@ -4116,6 +4116,7 @@ static int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
 out:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ufshcd_uic_change_pwr_mode);
 
 int ufshcd_link_recovery(struct ufs_hba *hba)
 {
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a92271421718..aa778418703f 100755
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1087,6 +1087,7 @@ extern int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
 			       u32 *mib_val, u8 peer);
 extern int ufshcd_config_pwr_mode(struct ufs_hba *hba,
 			struct ufs_pa_layer_attr *desired_pwr_mode);
+extern int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode);
 
 /* UIC command interfaces for DME primitives */
 #define DME_LOCAL	0
-- 
2.18.0

[PATCH v3 02/10] scsi: ufs: Fix ADAPT logic for HS-G5

[Stanley Chu]

ADAPT now is added not only for HS Gear4 mode but also higher gears.
Fix the logic for higher gears.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 19e17c898319..0d16739c67bb 100755
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3802,7 +3802,7 @@ int ufshcd_dme_configure_adapt(struct ufs_hba *hba,
 {
 	int ret;
 
-	if (agreed_gear != UFS_HS_G4)
+	if (agreed_gear < UFS_HS_G4)
 		adapt_val = PA_NO_ADAPT;
 
 	ret = ufshcd_dme_set(hba,
-- 
2.18.0

[PATCH v3 03/10] scsi: ufs-mediatek: Introduce workaround for power mode change

[Stanley Chu]

From: CC Chou <cc.chou@mediatek.com>

Some MediaTek SoC chips need special flow for power mode change,
especially for chips supporting HS-G5.

Enable the workaround by setting the host-specific capability.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: CC Chou <cc.chou@mediatek.com>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
Signed-off-by: Dennis Yu <tun-yu.yu@mediatek.com>
Signed-off-by: Peter Wang <peter.want@medaitek.com>
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/host/ufs-mediatek.c | 60 +++++++++++++++++++++++++++++++--
 drivers/ufs/host/ufs-mediatek.h |  1 +
 include/ufs/unipro.h            |  1 +
 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index beabc3ccd30b..21d591925dc4 100755
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -82,6 +82,13 @@ static bool ufs_mtk_is_broken_vcc(struct ufs_hba *hba)
 	return !!(host->caps & UFS_MTK_CAP_BROKEN_VCC);
 }
 
+static bool ufs_mtk_is_pmc_via_fastauto(struct ufs_hba *hba)
+{
+	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
+
+	return (host->caps & UFS_MTK_CAP_PMC_VIA_FASTAUTO);
+}
+
 static void ufs_mtk_cfg_unipro_cg(struct ufs_hba *hba, bool enable)
 {
 	u32 tmp;
@@ -579,6 +586,9 @@ static void ufs_mtk_init_host_caps(struct ufs_hba *hba)
 	if (of_property_read_bool(np, "mediatek,ufs-broken-vcc"))
 		host->caps |= UFS_MTK_CAP_BROKEN_VCC;
 
+	if (of_property_read_bool(np, "mediatek,ufs-pmc-via-fastauto"))
+		host->caps |= UFS_MTK_CAP_PMC_VIA_FASTAUTO;
+
 	dev_info(hba->dev, "caps: 0x%x", host->caps);
 }
 
@@ -754,6 +764,26 @@ static int ufs_mtk_init(struct ufs_hba *hba)
 	return err;
 }
 
+static bool ufs_mtk_pmc_via_fastauto(struct ufs_hba *hba,
+	struct ufs_pa_layer_attr *dev_req_params)
+{
+	if (!ufs_mtk_is_pmc_via_fastauto(hba))
+		return false;
+
+	if (dev_req_params->hs_rate == hba->pwr_info.hs_rate)
+		return false;
+
+	if ((dev_req_params->pwr_tx != FAST_MODE) &&
+		(dev_req_params->gear_tx < UFS_HS_G4))
+		return false;
+
+	if ((dev_req_params->pwr_rx != FAST_MODE) &&
+		(dev_req_params->gear_rx < UFS_HS_G4))
+		return false;
+
+	return true;
+}
+
 static int ufs_mtk_pre_pwr_change(struct ufs_hba *hba,
 				  struct ufs_pa_layer_attr *dev_max_params,
 				  struct ufs_pa_layer_attr *dev_req_params)
@@ -763,8 +793,8 @@ static int ufs_mtk_pre_pwr_change(struct ufs_hba *hba,
 	int ret;
 
 	ufshcd_init_pwr_dev_param(&host_cap);
-	host_cap.hs_rx_gear = UFS_HS_G4;
-	host_cap.hs_tx_gear = UFS_HS_G4;
+	host_cap.hs_rx_gear = UFS_HS_G5;
+	host_cap.hs_tx_gear = UFS_HS_G5;
 
 	ret = ufshcd_get_pwr_dev_param(&host_cap,
 				       dev_max_params,
@@ -774,6 +804,32 @@ static int ufs_mtk_pre_pwr_change(struct ufs_hba *hba,
 			__func__);
 	}
 
+	if (ufs_mtk_pmc_via_fastauto(hba, dev_req_params)) {
+		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_TXTERMINATION), true);
+		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_TXGEAR), UFS_HS_G1);
+
+		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_RXTERMINATION), true);
+		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_RXGEAR), UFS_HS_G1);
+
+		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_ACTIVETXDATALANES),
+						dev_req_params->lane_tx);
+		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_ACTIVERXDATALANES),
+						dev_req_params->lane_rx);
+		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_HSSERIES),
+						dev_req_params->hs_rate);
+
+		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_TXHSADAPTTYPE),
+		     PA_NO_ADAPT);
+
+		ret = ufshcd_uic_change_pwr_mode(hba,
+			FASTAUTO_MODE << 4 | FASTAUTO_MODE);
+
+		if (ret) {
+			dev_err(hba->dev, "%s: HSG1B FASTAUTO failed ret=%d\n",
+				__func__, ret);
+		}
+	}
+
 	if (host->hw_ver.major >= 3) {
 		ret = ufshcd_dme_configure_adapt(hba,
 					   dev_req_params->gear_tx,
diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-mediatek.h
index 414dca86c09f..7e1913769671 100755
--- a/drivers/ufs/host/ufs-mediatek.h
+++ b/drivers/ufs/host/ufs-mediatek.h
@@ -108,6 +108,7 @@ enum ufs_mtk_host_caps {
 	UFS_MTK_CAP_VA09_PWR_CTRL              = 1 << 1,
 	UFS_MTK_CAP_DISABLE_AH8                = 1 << 2,
 	UFS_MTK_CAP_BROKEN_VCC                 = 1 << 3,
+	UFS_MTK_CAP_PMC_VIA_FASTAUTO           = 1 << 6,
 };
 
 struct ufs_mtk_crypt_cfg {
diff --git a/include/ufs/unipro.h b/include/ufs/unipro.h
index 0521f887e3ac..4b13fbf8ee18 100755
--- a/include/ufs/unipro.h
+++ b/include/ufs/unipro.h
@@ -229,6 +229,7 @@ enum ufs_hs_gear_tag {
 	UFS_HS_G2,		/* HS Gear 2 */
 	UFS_HS_G3,		/* HS Gear 3 */
 	UFS_HS_G4,		/* HS Gear 4 */
+	UFS_HS_G5		/* HS Gear 5 */
 };
 
 enum ufs_unipro_ver {
-- 
2.18.0

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

[Stanley Chu]

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(-)

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,
 };
-- 
2.18.0

[PATCH v3 05/10] scsi: ufs-mediatek: Prevent device regulators setting as LPM incorrectly

[Stanley Chu]

From: Po-Wen Kao <powen.kao@mediatek.com>

Device regulatrs are allowed to enter low-power mode
if neither device is not in active mode, nor VCC does not
keep on.

Simply fix this by adding conditions before LPM decision.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Powen Kao <powen.kao@mediatek.com>
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/host/ufs-mediatek.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 3b3fe5470b71..c9afe373437c 100755
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1034,10 +1034,18 @@ static void ufs_mtk_vreg_set_lpm(struct ufs_hba *hba, bool lpm)
 	if (!hba->vreg_info.vccq2 || !hba->vreg_info.vcc)
 		return;
 
-	if (lpm && !hba->vreg_info.vcc->enabled)
+	/* Bypass LPM when device is still active */
+	if (lpm && ufshcd_is_ufs_dev_active(hba))
+		return;
+
+	/* Bypass LPM if VCC is enabled */
+	if (lpm && hba->vreg_info.vcc->enabled)
+		return;
+
+	if (lpm)
 		regulator_set_mode(hba->vreg_info.vccq2->reg,
 				   REGULATOR_MODE_IDLE);
-	else if (!lpm)
+	else
 		regulator_set_mode(hba->vreg_info.vccq2->reg,
 				   REGULATOR_MODE_NORMAL);
 }
-- 
2.18.0

[PATCH v3 06/10] scsi: ufs-mediatek: Support low-power mode for VCCQ

[Stanley Chu]

From: Peter Wang <peter.wang@mediatek.com>

Also allow VCCQ to enter low-power mode, and meanwhile
remove the restriction of VCC because VCCQ/VCCQ2 can
be changed to low-power mode even if VCC keeps on while
the device is not in active power mode.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/host/ufs-mediatek.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index c9afe373437c..178043ab837c 100755
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1031,7 +1031,13 @@ static int ufs_mtk_link_set_lpm(struct ufs_hba *hba)
 
 static void ufs_mtk_vreg_set_lpm(struct ufs_hba *hba, bool lpm)
 {
-	if (!hba->vreg_info.vccq2 || !hba->vreg_info.vcc)
+	struct ufs_vreg *vccqx = NULL;
+
+	if (!hba->vreg_info.vccq && !hba->vreg_info.vccq2)
+		return;
+
+	/* Skip if VCC is assumed always-on */
+	if (!hba->vreg_info.vcc)
 		return;
 
 	/* Bypass LPM when device is still active */
@@ -1042,12 +1048,13 @@ static void ufs_mtk_vreg_set_lpm(struct ufs_hba *hba, bool lpm)
 	if (lpm && hba->vreg_info.vcc->enabled)
 		return;
 
-	if (lpm)
-		regulator_set_mode(hba->vreg_info.vccq2->reg,
-				   REGULATOR_MODE_IDLE);
+	if (hba->vreg_info.vccq)
+		vccqx = hba->vreg_info.vccq;
 	else
-		regulator_set_mode(hba->vreg_info.vccq2->reg,
-				   REGULATOR_MODE_NORMAL);
+		vccqx = hba->vreg_info.vccq2;
+
+	regulator_set_mode(vccqx->reg,
+		lpm ? REGULATOR_MODE_IDLE : REGULATOR_MODE_NORMAL);
 }
 
 static void ufs_mtk_auto_hibern8_disable(struct ufs_hba *hba)
-- 
2.18.0

[PATCH v3 07/10] scsi: ufs-mediatek: Support flexible parameters for smc calls

[Stanley Chu]

From: Alice Chao <alice.chao@mediatek.com>

Provide flexible number of parameters for UFS SMC calls to be
easily used for future SMC usages.

This is a preparation patch for the next patch.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Alice Chao <alice.chao@mediatek.com>
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/host/ufs-mediatek.c | 16 ----------
 drivers/ufs/host/ufs-mediatek.h | 56 +++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 178043ab837c..9337ce27329b 100755
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -30,22 +30,6 @@
 #define CREATE_TRACE_POINTS
 #include "ufs-mediatek-trace.h"
 
-#define ufs_mtk_smc(cmd, val, res) \
-	arm_smccc_smc(MTK_SIP_UFS_CONTROL, \
-		      cmd, val, 0, 0, 0, 0, 0, &(res))
-
-#define ufs_mtk_va09_pwr_ctrl(res, on) \
-	ufs_mtk_smc(UFS_MTK_SIP_VA09_PWR_CTRL, on, res)
-
-#define ufs_mtk_crypto_ctrl(res, enable) \
-	ufs_mtk_smc(UFS_MTK_SIP_CRYPTO_CTRL, enable, res)
-
-#define ufs_mtk_ref_clk_notify(on, res) \
-	ufs_mtk_smc(UFS_MTK_SIP_REF_CLK_NOTIFICATION, on, res)
-
-#define ufs_mtk_device_reset_ctrl(high, res) \
-	ufs_mtk_smc(UFS_MTK_SIP_DEVICE_RESET, high, res)
-
 static const struct ufs_dev_quirk ufs_mtk_dev_fixups[] = {
 	{ .wmanufacturerid = UFS_VENDOR_MICRON,
 	  .model = UFS_ANY_MODEL,
diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-mediatek.h
index 7e1913769671..9117427ca6c4 100755
--- a/drivers/ufs/host/ufs-mediatek.h
+++ b/drivers/ufs/host/ufs-mediatek.h
@@ -143,4 +143,60 @@ struct ufs_mtk_host {
 	u32 ip_ver;
 };
 
+/*
+ * SMC call wapper function
+ */
+#define _ufs_mtk_smc(cmd, res, v1, v2, v3, v4, v5, v6) \
+		arm_smccc_smc(MTK_SIP_UFS_CONTROL, \
+				  cmd, v1, v2, v3, v4, v5, v6, &(res))
+
+#define _ufs_mtk_smc_0(cmd, res) \
+	_ufs_mtk_smc(cmd, res, 0, 0, 0, 0, 0, 0)
+
+#define _ufs_mtk_smc_1(cmd, res, v1) \
+	_ufs_mtk_smc(cmd, res, v1, 0, 0, 0, 0, 0)
+
+#define _ufs_mtk_smc_2(cmd, res, v1, v2) \
+	_ufs_mtk_smc(cmd, res, v1, v2, 0, 0, 0, 0)
+
+#define _ufs_mtk_smc_3(cmd, res, v1, v2, v3) \
+	_ufs_mtk_smc(cmd, res, v1, v2, v3, 0, 0, 0)
+
+#define _ufs_mtk_smc_4(cmd, res, v1, v2, v3, v4) \
+	_ufs_mtk_smc(cmd, res, v1, v2, v3, v4, 0, 0)
+
+#define _ufs_mtk_smc_5(cmd, res, v1, v2, v3, v4, v5) \
+	_ufs_mtk_smc(cmd, res, v1, v2, v3, v4, v5, 0)
+
+#define _ufs_mtk_smc_6(cmd, res, v1, v2, v3, v4, v5, v6) \
+	_ufs_mtk_smc(cmd, res, v1, v2, v3, v4, v5, v6)
+
+#define _ufs_mtk_smc_selector(cmd, res, v1, v2, v3, v4, v5, v6, FUNC, ...) FUNC
+
+#define ufs_mtk_smc(...) \
+	_ufs_mtk_smc_selector(__VA_ARGS__, \
+	_ufs_mtk_smc_6(__VA_ARGS__), \
+	_ufs_mtk_smc_5(__VA_ARGS__), \
+	_ufs_mtk_smc_4(__VA_ARGS__), \
+	_ufs_mtk_smc_3(__VA_ARGS__), \
+	_ufs_mtk_smc_2(__VA_ARGS__), \
+	_ufs_mtk_smc_1(__VA_ARGS__), \
+	_ufs_mtk_smc_0(__VA_ARGS__) \
+	)
+
+/*
+ * Sip kernel interface
+ */
+#define ufs_mtk_va09_pwr_ctrl(res, on) \
+	ufs_mtk_smc(UFS_MTK_SIP_VA09_PWR_CTRL, res, on)
+
+#define ufs_mtk_crypto_ctrl(res, enable) \
+	ufs_mtk_smc(UFS_MTK_SIP_CRYPTO_CTRL, res, enable)
+
+#define ufs_mtk_ref_clk_notify(on, res) \
+	ufs_mtk_smc(UFS_MTK_SIP_REF_CLK_NOTIFICATION, res, on)
+
+#define ufs_mtk_device_reset_ctrl(high, res) \
+	ufs_mtk_smc(UFS_MTK_SIP_DEVICE_RESET, res, high)
+
 #endif /* !_UFS_MEDIATEK_H */
-- 
2.18.0

[PATCH v3 08/10] scsi: ufs-mediatek: Support low-power mode for parents of VCCQx

[Stanley Chu]

Provide the facility to configure parents of VCCQx power rails
as low-power or full-power mode in MediaTek UFS platforms.

Signed-off-by: Alice Chao <alice.chao@mediatek.com>
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/host/ufs-mediatek.c | 45 +++++++++++++++++++++++----------
 drivers/ufs/host/ufs-mediatek.h |  4 +++
 2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 9337ce27329b..2c1e55f8d7c6 100755
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1013,10 +1013,29 @@ static int ufs_mtk_link_set_lpm(struct ufs_hba *hba)
 	return 0;
 }
 
-static void ufs_mtk_vreg_set_lpm(struct ufs_hba *hba, bool lpm)
+static void ufs_mtk_vccqx_set_lpm(struct ufs_hba *hba, bool lpm)
 {
 	struct ufs_vreg *vccqx = NULL;
 
+	if (hba->vreg_info.vccq)
+		vccqx = hba->vreg_info.vccq;
+	else
+		vccqx = hba->vreg_info.vccq2;
+
+	regulator_set_mode(vccqx->reg,
+		lpm ? REGULATOR_MODE_IDLE : REGULATOR_MODE_NORMAL);
+}
+
+static void ufs_mtk_vsx_set_lpm(struct ufs_hba *hba, bool lpm)
+{
+	struct arm_smccc_res res;
+
+	ufs_mtk_device_pwr_ctrl(!lpm,
+		(unsigned long)hba->dev_info.wspecversion, res);
+}
+
+static void ufs_mtk_dev_vreg_set_lpm(struct ufs_hba *hba, bool lpm)
+{
 	if (!hba->vreg_info.vccq && !hba->vreg_info.vccq2)
 		return;
 
@@ -1032,13 +1051,13 @@ static void ufs_mtk_vreg_set_lpm(struct ufs_hba *hba, bool lpm)
 	if (lpm && hba->vreg_info.vcc->enabled)
 		return;
 
-	if (hba->vreg_info.vccq)
-		vccqx = hba->vreg_info.vccq;
-	else
-		vccqx = hba->vreg_info.vccq2;
-
-	regulator_set_mode(vccqx->reg,
-		lpm ? REGULATOR_MODE_IDLE : REGULATOR_MODE_NORMAL);
+	if (lpm) {
+		ufs_mtk_vccqx_set_lpm(hba, lpm);
+		ufs_mtk_vsx_set_lpm(hba, lpm);
+	} else {
+		ufs_mtk_vsx_set_lpm(hba, lpm);
+		ufs_mtk_vccqx_set_lpm(hba, lpm);
+	}
 }
 
 static void ufs_mtk_auto_hibern8_disable(struct ufs_hba *hba)
@@ -1105,7 +1124,7 @@ 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);
+		ufs_mtk_dev_vreg_set_lpm(hba, false);
 
 	err = ufs_mtk_mphy_power_on(hba, true);
 	if (err)
@@ -1283,7 +1302,7 @@ int ufs_mtk_system_suspend(struct device *dev)
 	ret = ufshcd_system_suspend(dev);
 
 	if (!ret)
-		ufs_mtk_vreg_set_lpm(hba, true);
+		ufs_mtk_dev_vreg_set_lpm(hba, true);
 
 	return ret;
 }
@@ -1293,7 +1312,7 @@ 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);
+	ufs_mtk_dev_vreg_set_lpm(hba, false);
 
 	ret = ufshcd_system_resume(dev);
 
@@ -1308,7 +1327,7 @@ int ufs_mtk_runtime_suspend(struct device *dev)
 	ret = ufshcd_runtime_suspend(dev);
 
 	if (!ret)
-		ufs_mtk_vreg_set_lpm(hba, true);
+		ufs_mtk_dev_vreg_set_lpm(hba, true);
 
 	return ret;
 }
@@ -1318,7 +1337,7 @@ 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);
+	ufs_mtk_dev_vreg_set_lpm(hba, false);
 
 	ret = ufshcd_runtime_resume(dev);
 
diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-mediatek.h
index 9117427ca6c4..eb26306a719f 100755
--- a/drivers/ufs/host/ufs-mediatek.h
+++ b/drivers/ufs/host/ufs-mediatek.h
@@ -83,6 +83,7 @@ enum {
 #define UFS_MTK_SIP_DEVICE_RESET          BIT(1)
 #define UFS_MTK_SIP_CRYPTO_CTRL           BIT(2)
 #define UFS_MTK_SIP_REF_CLK_NOTIFICATION  BIT(3)
+#define UFS_MTK_SIP_DEVICE_PWR_CTRL       BIT(7)
 
 /*
  * VS_DEBUGCLOCKENABLE
@@ -199,4 +200,7 @@ struct ufs_mtk_host {
 #define ufs_mtk_device_reset_ctrl(high, res) \
 	ufs_mtk_smc(UFS_MTK_SIP_DEVICE_RESET, res, high)
 
+#define ufs_mtk_device_pwr_ctrl(on, ufs_ver, res) \
+	ufs_mtk_smc(UFS_MTK_SIP_DEVICE_PWR_CTRL, res, on, ufs_ver)
+
 #endif /* !_UFS_MEDIATEK_H */
-- 
2.18.0

[PATCH v3 09/10] scsi: ufs: Export regulator functions

[Stanley Chu]

Export below regulator functions to allow vendors to
customize regulator configuration in their own platforms.

int ufshcd_populate_vreg(struct device *dev, const char *name,
                         struct ufs_vreg **out_vreg);
int ufshcd_get_vreg(struct device *dev, struct ufs_vreg *vreg);

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/core/ufshcd.c        | 3 ++-
 drivers/ufs/host/ufshcd-pltfrm.c | 3 ++-
 drivers/ufs/host/ufshcd-pltfrm.h | 2 ++
 include/ufs/ufshcd.h             | 2 ++
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0d16739c67bb..8131a75e41e5 100755
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8408,7 +8408,7 @@ static int ufshcd_setup_hba_vreg(struct ufs_hba *hba, bool on)
 	return ufshcd_toggle_vreg(hba->dev, info->vdd_hba, on);
 }
 
-static int ufshcd_get_vreg(struct device *dev, struct ufs_vreg *vreg)
+int ufshcd_get_vreg(struct device *dev, struct ufs_vreg *vreg)
 {
 	int ret = 0;
 
@@ -8424,6 +8424,7 @@ static int ufshcd_get_vreg(struct device *dev, struct ufs_vreg *vreg)
 out:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ufshcd_get_vreg);
 
 static int ufshcd_init_vreg(struct ufs_hba *hba)
 {
diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
index e7332cc65b1f..332f66828d80 100755
--- a/drivers/ufs/host/ufshcd-pltfrm.c
+++ b/drivers/ufs/host/ufshcd-pltfrm.c
@@ -109,7 +109,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
 }
 
 #define MAX_PROP_SIZE 32
-static int ufshcd_populate_vreg(struct device *dev, const char *name,
+int ufshcd_populate_vreg(struct device *dev, const char *name,
 		struct ufs_vreg **out_vreg)
 {
 	char prop_name[MAX_PROP_SIZE];
@@ -145,6 +145,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name,
 	*out_vreg = vreg;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ufshcd_populate_vreg);
 
 /**
  * ufshcd_parse_regulator_info - get regulator info from device tree
diff --git a/drivers/ufs/host/ufshcd-pltfrm.h b/drivers/ufs/host/ufshcd-pltfrm.h
index 43c2e412bd99..5130c9471dc2 100755
--- a/drivers/ufs/host/ufshcd-pltfrm.h
+++ b/drivers/ufs/host/ufshcd-pltfrm.h
@@ -32,5 +32,7 @@ void ufshcd_init_pwr_dev_param(struct ufs_dev_params *dev_param);
 int ufshcd_pltfrm_init(struct platform_device *pdev,
 		       const struct ufs_hba_variant_ops *vops);
 void ufshcd_pltfrm_shutdown(struct platform_device *pdev);
+int ufshcd_populate_vreg(struct device *dev, const char *name,
+			 struct ufs_vreg **out_vreg);
 
 #endif /* UFSHCD_PLTFRM_H_ */
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index aa778418703f..18eb253cfd91 100755
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1187,6 +1187,8 @@ void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
 
 u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba);
 
+int ufshcd_get_vreg(struct device *dev, struct ufs_vreg *vreg);
+
 int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd);
 
 int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
-- 
2.18.0

[PATCH v3 10/10] scsi: ufs-mediatek: Support multiple VCC sources

[Stanley Chu]

Support multiple VCC source in MediaTek UFS platforms.

Two options are provided and distinguished by specific
device tree attributes as below examples,

[Option 1: By numbering]
mediatek,ufs-vcc-by-num;
vcc-opt1-supply = <&mt6373_vbuck4_ufs>;
vcc-opt2-supply = <&mt6363_vemc>;

[Option 2: By UFS version]
mediatek,ufs-vcc-by-ver;
vcc-ufs3-supply = <&mt6373_vbuck4_ufs>;

Signed-off-by: Alice Chao <alice.chao@mediatek.com>
Signed-off-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/ufs/host/ufs-mediatek.c | 46 +++++++++++++++++++++++++++++++++
 drivers/ufs/host/ufs-mediatek.h | 14 ++++++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 2c1e55f8d7c6..a029ec2fc971 100755
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -669,6 +669,50 @@ static u32 ufs_mtk_get_ufs_hci_version(struct ufs_hba *hba)
 	return hba->ufs_version;
 }
 
+#define MAX_VCC_NAME 30
+static int ufs_mtk_vreg_fix_vcc(struct ufs_hba *hba)
+{
+	struct ufs_vreg_info *info = &hba->vreg_info;
+	struct device_node *np = hba->dev->of_node;
+	struct device *dev = hba->dev;
+	char vcc_name[MAX_VCC_NAME];
+	struct arm_smccc_res res;
+	int err = 0;
+	int ver;
+
+	if (hba->vreg_info.vcc)
+		return 0;
+
+	if (of_property_read_bool(np, "mediatek,ufs-vcc-by-num")) {
+		ufs_mtk_get_vcc_num(res);
+		if (res.a1 > UFS_VCC_NONE && res.a1 < UFS_VCC_MAX)
+			snprintf(vcc_name, MAX_VCC_NAME, "vcc-opt%u", res.a1);
+		else
+			return -ENODEV;
+	} else if (of_property_read_bool(np, "mediatek,ufs-vcc-by-ver")) {
+		ver = (hba->dev_info.wspecversion & 0xF00) >> 8;
+		snprintf(vcc_name, MAX_VCC_NAME, "vcc-ufs%u", ver);
+	} else {
+		return 0;
+	}
+
+	err = ufshcd_populate_vreg(dev, vcc_name, &info->vcc);
+	if (err)
+		return err;
+
+	err = ufshcd_get_vreg(dev, info->vcc);
+	if (err)
+		return err;
+
+	err = regulator_enable(info->vcc->reg);
+	if (!err) {
+		info->vcc->enabled = true;
+		dev_info(dev, "%s: %s enabled\n", __func__, vcc_name);
+	}
+
+	return err;
+}
+
 /**
  * ufs_mtk_init - find other essential mmio bases
  * @hba: host controller instance
@@ -1179,6 +1223,8 @@ static int ufs_mtk_apply_dev_quirks(struct ufs_hba *hba)
 		ufs_mtk_setup_ref_clk_wait_us(hba,
 					      REFCLK_DEFAULT_WAIT_US);
 
+	ufs_mtk_vreg_fix_vcc(hba);
+
 	return 0;
 }
 
diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-mediatek.h
index eb26306a719f..b72007ecba05 100755
--- a/drivers/ufs/host/ufs-mediatek.h
+++ b/drivers/ufs/host/ufs-mediatek.h
@@ -83,6 +83,7 @@ enum {
 #define UFS_MTK_SIP_DEVICE_RESET          BIT(1)
 #define UFS_MTK_SIP_CRYPTO_CTRL           BIT(2)
 #define UFS_MTK_SIP_REF_CLK_NOTIFICATION  BIT(3)
+#define UFS_MTK_SIP_GET_VCC_NUM           BIT(6)
 #define UFS_MTK_SIP_DEVICE_PWR_CTRL       BIT(7)
 
 /*
@@ -144,6 +145,16 @@ struct ufs_mtk_host {
 	u32 ip_ver;
 };
 
+/*
+ * Multi-VCC by Numbering
+ */
+enum ufs_mtk_vcc_num {
+	UFS_VCC_NONE = 0,
+	UFS_VCC_1,
+	UFS_VCC_2,
+	UFS_VCC_MAX
+};
+
 /*
  * SMC call wapper function
  */
@@ -200,6 +211,9 @@ struct ufs_mtk_host {
 #define ufs_mtk_device_reset_ctrl(high, res) \
 	ufs_mtk_smc(UFS_MTK_SIP_DEVICE_RESET, res, high)
 
+#define ufs_mtk_get_vcc_num(res) \
+	ufs_mtk_smc(UFS_MTK_SIP_GET_VCC_NUM, res)
+
 #define ufs_mtk_device_pwr_ctrl(on, ufs_ver, res) \
 	ufs_mtk_smc(UFS_MTK_SIP_DEVICE_PWR_CTRL, res, on, ufs_ver)
 
-- 
2.18.0

Re: [PATCH v3 07/10] scsi: ufs-mediatek: Support flexible parameters for smc calls

[Bart Van Assche]

On 6/14/22 07:16, Stanley Chu wrote:
> From: Alice Chao <alice.chao@mediatek.com>
> 
> Provide flexible number of parameters for UFS SMC calls to be
> easily used for future SMC usages.

How far in the future? Please only introduce what is needed for this 
patch series.

> +/*
> + * SMC call wapper function
                ^^^^^^
typo

> + */
> +#define _ufs_mtk_smc(cmd, res, v1, v2, v3, v4, v5, v6) \
> +		arm_smccc_smc(MTK_SIP_UFS_CONTROL, \
> +				  cmd, v1, v2, v3, v4, v5, v6, &(res))
> +
> +#define _ufs_mtk_smc_0(cmd, res) \
> +	_ufs_mtk_smc(cmd, res, 0, 0, 0, 0, 0, 0)
> +
> +#define _ufs_mtk_smc_1(cmd, res, v1) \
> +	_ufs_mtk_smc(cmd, res, v1, 0, 0, 0, 0, 0)
> +
> +#define _ufs_mtk_smc_2(cmd, res, v1, v2) \
> +	_ufs_mtk_smc(cmd, res, v1, v2, 0, 0, 0, 0)
> +
> +#define _ufs_mtk_smc_3(cmd, res, v1, v2, v3) \
> +	_ufs_mtk_smc(cmd, res, v1, v2, v3, 0, 0, 0)
> +
> +#define _ufs_mtk_smc_4(cmd, res, v1, v2, v3, v4) \
> +	_ufs_mtk_smc(cmd, res, v1, v2, v3, v4, 0, 0)
> +
> +#define _ufs_mtk_smc_5(cmd, res, v1, v2, v3, v4, v5) \
> +	_ufs_mtk_smc(cmd, res, v1, v2, v3, v4, v5, 0)
> +
> +#define _ufs_mtk_smc_6(cmd, res, v1, v2, v3, v4, v5, v6) \
> +	_ufs_mtk_smc(cmd, res, v1, v2, v3, v4, v5, v6)
> +
> +#define _ufs_mtk_smc_selector(cmd, res, v1, v2, v3, v4, v5, v6, FUNC, ...) FUNC
> +
> +#define ufs_mtk_smc(...) \
> +	_ufs_mtk_smc_selector(__VA_ARGS__, \
> +	_ufs_mtk_smc_6(__VA_ARGS__), \
> +	_ufs_mtk_smc_5(__VA_ARGS__), \
> +	_ufs_mtk_smc_4(__VA_ARGS__), \
> +	_ufs_mtk_smc_3(__VA_ARGS__), \
> +	_ufs_mtk_smc_2(__VA_ARGS__), \
> +	_ufs_mtk_smc_1(__VA_ARGS__), \
> +	_ufs_mtk_smc_0(__VA_ARGS__) \
> +	)

If _ufs_mtk_smc() would be modified to accept an struct _ufs_mtk_args as 
its only argument, would that allow to simplify the above into the 
following?

#define ufs_mtk_smc(...) \
   _ufs_mtk_smc((struct _ufs_mtk_args){__VA_ARGS__})

> +/*
> + * Sip kernel interface
> + */

What is "Sip"? Should it perhaps be spelled as "SIP"?

Thanks,

Bart.

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

[Bart Van Assche]

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;

> +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.

> +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.

> +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.

Thanks,

Bart.

Re: [PATCH v3 03/10] scsi: ufs-mediatek: Introduce workaround for power mode change

[Bart Van Assche]

On 6/14/22 07:16, Stanley Chu wrote:
> +	if ((dev_req_params->pwr_tx != FAST_MODE) &&
> +		(dev_req_params->gear_tx < UFS_HS_G4))
> +		return false;
> +
> +	if ((dev_req_params->pwr_rx != FAST_MODE) &&
> +		(dev_req_params->gear_rx < UFS_HS_G4))
> +		return false;

Please do not use more parentheses than needed. I think 8 parentheses 
can be left out from the above code.

Thanks,

Bart.

Re: [PATCH v3 07/10] scsi: ufs-mediatek: Support flexible parameters for smc calls

[Stanley Chu]

Hi Bart,

On Tue, 2022-06-14 at 09:28 -0700, Bart Van Assche wrote:
> On 6/14/22 07:16, Stanley Chu wrote:
> > From: Alice Chao <alice.chao@mediatek.com>
> > 
> > Provide flexible number of parameters for UFS SMC calls to be
> > easily used for future SMC usages.
> 
> How far in the future? Please only introduce what is needed for this 
> patch series.

Sure, I just rewrote and simplified SMC call macros according to your
good suggestions in v4.

> 
> > +/*
> > + * SMC call wapper function
> 
>                 ^^^^^^
> typo

Fixed in v4.
> 
> > + */
> > +#define _ufs_mtk_smc(cmd, res, v1, v2, v3, v4, v5, v6) \
> > +		arm_smccc_smc(MTK_SIP_UFS_CONTROL, \
> > +				  cmd, v1, v2, v3, v4, v5, v6, &(res))
> > +
> > +#define _ufs_mtk_smc_0(cmd, res) \
> > +	_ufs_mtk_smc(cmd, res, 0, 0, 0, 0, 0, 0)
> > +
> > +#define _ufs_mtk_smc_1(cmd, res, v1) \
> > +	_ufs_mtk_smc(cmd, res, v1, 0, 0, 0, 0, 0)
> > +
> > +#define _ufs_mtk_smc_2(cmd, res, v1, v2) \
> > +	_ufs_mtk_smc(cmd, res, v1, v2, 0, 0, 0, 0)
> > +
> > +#define _ufs_mtk_smc_3(cmd, res, v1, v2, v3) \
> > +	_ufs_mtk_smc(cmd, res, v1, v2, v3, 0, 0, 0)
> > +
> > +#define _ufs_mtk_smc_4(cmd, res, v1, v2, v3, v4) \
> > +	_ufs_mtk_smc(cmd, res, v1, v2, v3, v4, 0, 0)
> > +
> > +#define _ufs_mtk_smc_5(cmd, res, v1, v2, v3, v4, v5) \
> > +	_ufs_mtk_smc(cmd, res, v1, v2, v3, v4, v5, 0)
> > +
> > +#define _ufs_mtk_smc_6(cmd, res, v1, v2, v3, v4, v5, v6) \
> > +	_ufs_mtk_smc(cmd, res, v1, v2, v3, v4, v5, v6)
> > +
> > +#define _ufs_mtk_smc_selector(cmd, res, v1, v2, v3, v4, v5, v6,
> > FUNC, ...) FUNC
> > +
> > +#define ufs_mtk_smc(...) \
> > +	_ufs_mtk_smc_selector(__VA_ARGS__, \
> > +	_ufs_mtk_smc_6(__VA_ARGS__), \
> > +	_ufs_mtk_smc_5(__VA_ARGS__), \
> > +	_ufs_mtk_smc_4(__VA_ARGS__), \
> > +	_ufs_mtk_smc_3(__VA_ARGS__), \
> > +	_ufs_mtk_smc_2(__VA_ARGS__), \
> > +	_ufs_mtk_smc_1(__VA_ARGS__), \
> > +	_ufs_mtk_smc_0(__VA_ARGS__) \
> > +	)
> 
> If _ufs_mtk_smc() would be modified to accept an struct _ufs_mtk_args
> as 
> its only argument, would that allow to simplify the above into the 
> following?
> 
> #define ufs_mtk_smc(...) \
>    _ufs_mtk_smc((struct _ufs_mtk_args){__VA_ARGS__})
> 
> > +/*
> > + * Sip kernel interface
> > + */
> 
> What is "Sip"? Should it perhaps be spelled as "SIP"?
> 
> Thanks,
> 
> Bart.

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

[Stanley Chu]

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.

Re: [PATCH v3 03/10] scsi: ufs-mediatek: Introduce workaround for power mode change

[Stanley Chu]

On Tue, 2022-06-14 at 09:46 -0700, Bart Van Assche wrote:
> On 6/14/22 07:16, Stanley Chu wrote:
> > +	if ((dev_req_params->pwr_tx != FAST_MODE) &&
> > +		(dev_req_params->gear_tx < UFS_HS_G4))
> > +		return false;
> > +
> > +	if ((dev_req_params->pwr_rx != FAST_MODE) &&
> > +		(dev_req_params->gear_rx < UFS_HS_G4))
> > +		return false;
> 
> Please do not use more parentheses than needed. I think 8
> parentheses 
> can be left out from the above code.

Thanks! fixed in v4.

> 
> Thanks,
> 
> Bart.