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
From: CC Chou <[email protected]>
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 <[email protected]>
Signed-off-by: CC Chou <[email protected]>
Signed-off-by: Eddie Huang <[email protected]>
Signed-off-by: Dennis Yu <[email protected]>
Signed-off-by: Peter Wang <[email protected]>
Signed-off-by: Stanley Chu <[email protected]>
---
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
From: Po-Wen Kao <[email protected]>
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 <[email protected]>
Signed-off-by: Po-Wen Kao <[email protected]>
Signed-off-by: Stanley Chu <[email protected]>
---
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
From: Po-Wen Kao <[email protected]>
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 <[email protected]>
Signed-off-by: Powen Kao <[email protected]>
Signed-off-by: Stanley Chu <[email protected]>
---
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
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 <[email protected]>
---
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
From: Alice Chao <[email protected]>
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 <[email protected]>
Signed-off-by: Alice Chao <[email protected]>
Signed-off-by: Stanley Chu <[email protected]>
---
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
On 6/14/22 07:16, Stanley Chu wrote:
> From: Alice Chao <[email protected]>
>
> 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.
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.
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.
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 <[email protected]>
> >
> > 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.
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.
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.