Re: [PATCH RFC v4 1/1] scsi: ufs: Fix ufs power down/on specs violation
From: Can Guo
Date: Mon Dec 28 2020 - 20:19:09 EST
On 2020-12-29 01:55, Bjorn Andersson wrote:
On Tue 22 Dec 07:49 CST 2020, Ziqi Chen wrote:
As per specs, e.g, JESD220E chapter 7.2, while powering
off/on the ufs device, RST_N signal and REF_CLK signal
should be between VSS(Ground) and VCCQ/VCCQ2.
To flexibly control device reset line, refactor the function
ufschd_vops_device_reset(sturct ufs_hba *hba) to ufshcd_
vops_device_reset(sturct ufs_hba *hba, bool asserted). The
new parameter "bool asserted" is used to separate device reset
line pulling down from pulling up.
Cc: Kiwoong Kim <kwmad.kim@xxxxxxxxxxx>
Cc: Stanley Chu <stanley.chu@xxxxxxxxxxxx>
Signed-off-by: Ziqi Chen <ziqichen@xxxxxxxxxxxxxx>
---
drivers/scsi/ufs/ufs-mediatek.c | 32 ++++++++++++++++----------------
drivers/scsi/ufs/ufs-qcom.c | 24 +++++++++++++++---------
drivers/scsi/ufs/ufshcd.c | 36
+++++++++++++++++++++++++-----------
drivers/scsi/ufs/ufshcd.h | 8 ++++----
4 files changed, 60 insertions(+), 40 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-mediatek.c
b/drivers/scsi/ufs/ufs-mediatek.c
index 80618af..072f4db 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -841,27 +841,27 @@ static int ufs_mtk_link_startup_notify(struct
ufs_hba *hba,
return ret;
}
-static int ufs_mtk_device_reset(struct ufs_hba *hba)
+static int ufs_mtk_device_reset(struct ufs_hba *hba, bool asserted)
{
struct arm_smccc_res res;
- ufs_mtk_device_reset_ctrl(0, res);
+ if (asserted) {
+ ufs_mtk_device_reset_ctrl(0, res);
- /*
- * The reset signal is active low. UFS devices shall detect
- * more than or equal to 1us of positive or negative RST_n
- * pulse width.
- *
- * To be on safe side, keep the reset low for at least 10us.
- */
- usleep_range(10, 15);
-
- ufs_mtk_device_reset_ctrl(1, res);
-
- /* Some devices may need time to respond to rst_n */
- usleep_range(10000, 15000);
+ /*
+ * The reset signal is active low. UFS devices shall detect
+ * more than or equal to 1us of positive or negative RST_n
+ * pulse width.
+ *
+ * To be on safe side, keep the reset low for at least 10us.
+ */
+ usleep_range(10, 15);
I see no point in allowing vendors to "tweak" the 1us->10us adjustment.
The specification says 1us and we all agree that 10us gives us good
enough slack. I.e. this is common code.
Hi Bjron,
We tried, but Samsung fellows wanted 5us. We couldn't get a agreement
on this delay in short term, so we chose to leave it in vops.
+ } else {
+ ufs_mtk_device_reset_ctrl(1, res);
- dev_info(hba->dev, "device reset done\n");
+ /* Some devices may need time to respond to rst_n */
+ usleep_range(10000, 15000);
The comment in both the Qualcomm and Mediatek drivers claim that this
is
sleep relates to the UFS device (not host), so why should it be
different?
What happens if I take the device that Mediatek see a need for a 10ms
delay and hook that up to a Qualcomm host? This really should go in the
common code.
Agree, but Qualcomm host didn't have any problems with 10us yet, so if
we put
the 10ms delay to common code, Qualcomm host would suffer longer delay
when
device reset happens - both bootup and resume(xpm_lvl = 5/6) latency
would
be increased.
Regards,
Can Guo.
As such I really would prefer to see these delays in the common code!
You really shouldn't write code based on a speculation that one day
there might come someone who need other values - when that day come we
can just change the code, and if it never comes we're better off with
the cleaner implementation.
Regards,
Bjorn
+ }
return 0;
}
diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 2206b1e..fed10e5 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1406,10 +1406,11 @@ static void ufs_qcom_dump_dbg_regs(struct
ufs_hba *hba)
/**
* ufs_qcom_device_reset() - toggle the (optional) device reset line
* @hba: per-adapter instance
+ * @asserted: assert or deassert device reset line
*
* Toggles the (optional) reset line to reset the attached device.
*/
-static int ufs_qcom_device_reset(struct ufs_hba *hba)
+static int ufs_qcom_device_reset(struct ufs_hba *hba, bool asserted)
{
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
@@ -1417,15 +1418,20 @@ static int ufs_qcom_device_reset(struct
ufs_hba *hba)
if (!host->device_reset)
return -EOPNOTSUPP;
- /*
- * The UFS device shall detect reset pulses of 1us, sleep for 10us
to
- * be on the safe side.
- */
- gpiod_set_value_cansleep(host->device_reset, 1);
- usleep_range(10, 15);
+ if (asserted) {
+ gpiod_set_value_cansleep(host->device_reset, 1);
- gpiod_set_value_cansleep(host->device_reset, 0);
- usleep_range(10, 15);
+ /*
+ * The UFS device shall detect reset pulses of 1us, sleep for 10us
to
+ * be on the safe side.
+ */
+ usleep_range(10, 15);
+ } else {
+ gpiod_set_value_cansleep(host->device_reset, 0);
+
+ /* Some devices may need time to respond to rst_n */
+ usleep_range(10, 15);
+ }
return 0;
}
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e221add..f2daac2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -585,7 +585,13 @@ static void ufshcd_device_reset(struct ufs_hba
*hba)
{
int err;
- err = ufshcd_vops_device_reset(hba);
+ err = ufshcd_vops_device_reset(hba, true);
+ if (err) {
+ dev_err(hba->dev, "asserting device reset failed: %d\n", err);
+ return;
+ }
+
+ err = ufshcd_vops_device_reset(hba, false);
if (!err) {
ufshcd_set_ufs_dev_active(hba);
@@ -593,7 +599,11 @@ static void ufshcd_device_reset(struct ufs_hba
*hba)
hba->wb_enabled = false;
hba->wb_buf_flush_enabled = false;
}
+ dev_dbg(hba->dev, "device reset done\n");
+ } else {
+ dev_err(hba->dev, "deasserting device reset failed: %d\n", err);
}
+
if (err != -EOPNOTSUPP)
ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
}
@@ -8686,8 +8696,6 @@ static int ufshcd_suspend(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
if (ret)
goto set_dev_active;
- ufshcd_vreg_set_lpm(hba);
-
disable_clks:
/*
* Call vendor specific suspend callback. As these callbacks may
access
@@ -8703,6 +8711,9 @@ static int ufshcd_suspend(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
*/
ufshcd_disable_irq(hba);
+ if (ufshcd_is_link_off(hba))
+ ufshcd_vops_device_reset(hba, true);
+
ufshcd_setup_clocks(hba, false);
if (ufshcd_is_clkgating_allowed(hba)) {
@@ -8711,6 +8722,8 @@ static int ufshcd_suspend(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
hba->clk_gating.state);
}
+ ufshcd_vreg_set_lpm(hba);
+
/* Put the host controller in low power mode if possible */
ufshcd_hba_vreg_set_lpm(hba);
goto out;
@@ -8778,18 +8791,19 @@ static int ufshcd_resume(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
old_link_state = hba->uic_link_state;
ufshcd_hba_vreg_set_hpm(hba);
+
+ ret = ufshcd_vreg_set_hpm(hba);
+ if (ret)
+ goto out;
+
/* Make sure clocks are enabled before accessing controller */
ret = ufshcd_setup_clocks(hba, true);
if (ret)
- goto out;
+ goto disable_vreg;
/* enable the host irq as host controller would be active soon */
ufshcd_enable_irq(hba);
- ret = ufshcd_vreg_set_hpm(hba);
- if (ret)
- goto disable_irq_and_vops_clks;
-
/*
* Call vendor specific resume callback. As these callbacks may
access
* vendor specific host controller register space call them when the
@@ -8797,7 +8811,7 @@ static int ufshcd_resume(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
*/
ret = ufshcd_vops_resume(hba, pm_op);
if (ret)
- goto disable_vreg;
+ goto disable_irq_and_vops_clks;
/* For DeepSleep, the only supported option is to have the link off
*/
WARN_ON(ufshcd_is_ufs_dev_deepsleep(hba) &&
!ufshcd_is_link_off(hba));
@@ -8864,8 +8878,6 @@ static int ufshcd_resume(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
ufshcd_link_state_transition(hba, old_link_state, 0);
vendor_suspend:
ufshcd_vops_suspend(hba, pm_op);
-disable_vreg:
- ufshcd_vreg_set_lpm(hba);
disable_irq_and_vops_clks:
ufshcd_disable_irq(hba);
if (hba->clk_scaling.is_allowed)
@@ -8876,6 +8888,8 @@ static int ufshcd_resume(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
trace_ufshcd_clk_gating(dev_name(hba->dev),
hba->clk_gating.state);
}
+disable_vreg:
+ ufshcd_vreg_set_lpm(hba);
out:
hba->pm_op_in_progress = 0;
if (ret)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 9bb5f0e..d5fbaba 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -319,7 +319,7 @@ struct ufs_pwr_mode_info {
* @resume: called during host controller PM callback
* @dbg_register_dump: used to dump controller debug information
* @phy_initialization: used to initialize phys
- * @device_reset: called to issue a reset pulse on the UFS device
+ * @device_reset: called to assert or deassert device reset line
* @program_key: program or evict an inline encryption key
* @event_notify: called to notify important events
*/
@@ -350,7 +350,7 @@ struct ufs_hba_variant_ops {
int (*resume)(struct ufs_hba *, enum ufs_pm_op);
void (*dbg_register_dump)(struct ufs_hba *hba);
int (*phy_initialization)(struct ufs_hba *);
- int (*device_reset)(struct ufs_hba *hba);
+ int (*device_reset)(struct ufs_hba *hba, bool asserted);
void (*config_scaling_param)(struct ufs_hba *hba,
struct devfreq_dev_profile *profile,
void *data);
@@ -1216,10 +1216,10 @@ static inline void
ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
hba->vops->dbg_register_dump(hba);
}
-static inline int ufshcd_vops_device_reset(struct ufs_hba *hba)
+static inline int ufshcd_vops_device_reset(struct ufs_hba *hba, bool
asserted)
{
if (hba->vops && hba->vops->device_reset)
- return hba->vops->device_reset(hba);
+ return hba->vops->device_reset(hba, asserted);
return -EOPNOTSUPP;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project