linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend
       [not found] <1579764349-15578-1-git-send-email-cang@codeaurora.org>
@ 2020-01-23  7:25 ` Can Guo
  2020-01-23 23:09   ` [EXT] " Bean Huo (beanhuo)
  2020-01-26  3:29   ` Bart Van Assche
  2020-01-23  7:25 ` [PATCH v4 2/8] scsi: ufs: set load before setting voltage in regulators Can Guo
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 24+ messages in thread
From: Can Guo @ 2020-01-23  7:25 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Sayali Lokhande, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen, Stanley Chu, Bean Huo,
	Venkat Gopalakrishnan, Tomas Winkler, open list

From: Sayali Lokhande <sayalil@codeaurora.org>

Exception event can be raised by the device when system
suspend is in progress. This will result in unclocked
register access in exception event handler as clocks will
be turned off during suspend. This change makes sure to flush
exception event handler work in suspend before disabling
clocks to avoid unclocked register access issue.

Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1201578..c2de29f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4760,8 +4760,15 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
 			 * UFS device needs urgent BKOPs.
 			 */
 			if (!hba->pm_op_in_progress &&
-			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
-				schedule_work(&hba->eeh_work);
+			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr)) {
+				/*
+				 * Prevent suspend once eeh_work is scheduled
+				 * to avoid deadlock between ufshcd_suspend
+				 * and exception event handler.
+				 */
+				if (schedule_work(&hba->eeh_work))
+					pm_runtime_get_noresume(hba->dev);
+			}
 			break;
 		case UPIU_TRANSACTION_REJECT_UPIU:
 			/* TODO: handle Reject UPIU Response */
@@ -5215,7 +5222,14 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 
 out:
 	scsi_unblock_requests(hba->host);
-	pm_runtime_put_sync(hba->dev);
+	/*
+	 * pm_runtime_get_noresume is called while scheduling
+	 * eeh_work to avoid suspend racing with exception work.
+	 * Hence decrement usage counter using pm_runtime_put_noidle
+	 * to allow suspend on completion of exception event handler.
+	 */
+	pm_runtime_put_noidle(hba->dev);
+	pm_runtime_put(hba->dev);
 	return;
 }
 
@@ -7901,6 +7915,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 			goto enable_gating;
 	}
 
+	flush_work(&hba->eeh_work);
 	ret = ufshcd_link_state_transition(hba, req_link_state, 1);
 	if (ret)
 		goto set_dev_active;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v4 2/8] scsi: ufs: set load before setting voltage in regulators
       [not found] <1579764349-15578-1-git-send-email-cang@codeaurora.org>
  2020-01-23  7:25 ` [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend Can Guo
@ 2020-01-23  7:25 ` Can Guo
  2020-01-23  7:50   ` hongwus
  2020-01-23  7:25 ` [PATCH v4 3/8] scsi: ufs: Remove the check before call setup clock notify vops Can Guo
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 24+ messages in thread
From: Can Guo @ 2020-01-23  7:25 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Venkat Gopalakrishnan,
	Tomas Winkler, open list

From: Asutosh Das <asutoshd@codeaurora.org>

This sequence change is required to avoid dips in voltage
during boot-up.

Apparently, this dip is caused because in the original
sequence, the regulators are initialized in lpm mode.
And then when the load is set to high, and more current
is drawn, than is allowed in lpm, the dip is seen.

Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c2de29f..c386c2d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7225,6 +7225,11 @@ static int ufshcd_config_vreg(struct device *dev,
 	name = vreg->name;
 
 	if (regulator_count_voltages(reg) > 0) {
+		uA_load = on ? vreg->max_uA : 0;
+		ret = ufshcd_config_vreg_load(dev, vreg, uA_load);
+		if (ret)
+			goto out;
+
 		if (vreg->min_uV && vreg->max_uV) {
 			min_uV = on ? vreg->min_uV : 0;
 			ret = regulator_set_voltage(reg, min_uV, vreg->max_uV);
@@ -7235,11 +7240,6 @@ static int ufshcd_config_vreg(struct device *dev,
 				goto out;
 			}
 		}
-
-		uA_load = on ? vreg->max_uA : 0;
-		ret = ufshcd_config_vreg_load(dev, vreg, uA_load);
-		if (ret)
-			goto out;
 	}
 out:
 	return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v4 3/8] scsi: ufs: Remove the check before call setup clock notify vops
       [not found] <1579764349-15578-1-git-send-email-cang@codeaurora.org>
  2020-01-23  7:25 ` [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend Can Guo
  2020-01-23  7:25 ` [PATCH v4 2/8] scsi: ufs: set load before setting voltage in regulators Can Guo
@ 2020-01-23  7:25 ` Can Guo
  2020-01-23  7:25 ` [PATCH v4 4/8] scsi: ufs-qcom: Adjust bus bandwidth voting and unvoting Can Guo
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Can Guo @ 2020-01-23  7:25 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Bean Huo, Venkat Gopalakrishnan,
	Tomas Winkler, open list

The functionality of vendor specific ops should be handled properly in
platform specific driver, but should not count on the UFS driver.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c386c2d..4dfd705 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7379,16 +7379,9 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
 	if (list_empty(head))
 		goto out;
 
-	/*
-	 * vendor specific setup_clocks ops may depend on clocks managed by
-	 * this standard driver hence call the vendor specific setup_clocks
-	 * before disabling the clocks managed here.
-	 */
-	if (!on) {
-		ret = ufshcd_vops_setup_clocks(hba, on, PRE_CHANGE);
-		if (ret)
-			return ret;
-	}
+	ret = ufshcd_vops_setup_clocks(hba, on, PRE_CHANGE);
+	if (ret)
+		return ret;
 
 	list_for_each_entry(clki, head, list) {
 		if (!IS_ERR_OR_NULL(clki->clk)) {
@@ -7412,16 +7405,9 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
 		}
 	}
 
-	/*
-	 * vendor specific setup_clocks ops may depend on clocks managed by
-	 * this standard driver hence call the vendor specific setup_clocks
-	 * after enabling the clocks managed here.
-	 */
-	if (on) {
-		ret = ufshcd_vops_setup_clocks(hba, on, POST_CHANGE);
-		if (ret)
-			return ret;
-	}
+	ret = ufshcd_vops_setup_clocks(hba, on, POST_CHANGE);
+	if (ret)
+		return ret;
 
 out:
 	if (ret) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v4 4/8] scsi: ufs-qcom: Adjust bus bandwidth voting and unvoting
       [not found] <1579764349-15578-1-git-send-email-cang@codeaurora.org>
                   ` (2 preceding siblings ...)
  2020-01-23  7:25 ` [PATCH v4 3/8] scsi: ufs: Remove the check before call setup clock notify vops Can Guo
@ 2020-01-23  7:25 ` Can Guo
  2020-01-23  7:25 ` [PATCH v4 5/8] scsi: ufs: Fix ufshcd_hold() caused scheduling while atomic Can Guo
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Can Guo @ 2020-01-23  7:25 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Andy Gross, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen,
	open list:ARM/QUALCOMM SUPPORT, open list

The bus bandwidth voting is required to be done before the bus clocks
are enabled, and the unvoting is required to be done only after the bus
clocks are disabled.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 57 +++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index c69c29a1c..85d7c17 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -38,7 +38,6 @@ enum {
 
 static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
 
-static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote);
 static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
 static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
 						       u32 clk_cycles);
@@ -674,7 +673,7 @@ static void ufs_qcom_get_speed_mode(struct ufs_pa_layer_attr *p, char *result)
 	}
 }
 
-static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote)
+static int __ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote)
 {
 	int err = 0;
 
@@ -705,7 +704,7 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)
 
 	vote = ufs_qcom_get_bus_vote(host, mode);
 	if (vote >= 0)
-		err = ufs_qcom_set_bus_vote(host, vote);
+		err = __ufs_qcom_set_bus_vote(host, vote);
 	else
 		err = vote;
 
@@ -716,6 +715,35 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)
 	return err;
 }
 
+static int ufs_qcom_set_bus_vote(struct ufs_hba *hba, bool on)
+{
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	int vote, err;
+
+	/*
+	 * In case ufs_qcom_init() is not yet done, simply ignore.
+	 * This ufs_qcom_set_bus_vote() shall be called from
+	 * ufs_qcom_init() after init is done.
+	 */
+	if (!host)
+		return 0;
+
+	if (on) {
+		vote = host->bus_vote.saved_vote;
+		if (vote == host->bus_vote.min_bw_vote)
+			ufs_qcom_update_bus_bw_vote(host);
+	} else {
+		vote = host->bus_vote.min_bw_vote;
+	}
+
+	err = __ufs_qcom_set_bus_vote(host, vote);
+	if (err)
+		dev_err(hba->dev, "%s: set bus vote failed %d\n",
+				 __func__, err);
+
+	return err;
+}
+
 static ssize_t
 show_ufs_to_mem_max_bus_bw(struct device *dev, struct device_attribute *attr,
 			char *buf)
@@ -792,7 +820,7 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)
 	return 0;
 }
 
-static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote)
+static int ufs_qcom_set_bus_vote(struct ufs_hba *host, bool on)
 {
 	return 0;
 }
@@ -1030,8 +1058,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 				 enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	int err;
-	int vote = 0;
+	int err = 0;
 
 	/*
 	 * In case ufs_qcom_init() is not yet done, simply ignore.
@@ -1041,28 +1068,21 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 	if (!host)
 		return 0;
 
-	if (on && (status == POST_CHANGE)) {
+	if (on && (status == PRE_CHANGE)) {
+		err = ufs_qcom_set_bus_vote(hba, true);
+	} else if (on && (status == POST_CHANGE)) {
 		/* enable the device ref clock for HS mode*/
 		if (ufshcd_is_hs_mode(&hba->pwr_info))
 			ufs_qcom_dev_ref_clk_ctrl(host, true);
-		vote = host->bus_vote.saved_vote;
-		if (vote == host->bus_vote.min_bw_vote)
-			ufs_qcom_update_bus_bw_vote(host);
-
 	} else if (!on && (status == PRE_CHANGE)) {
 		if (!ufs_qcom_is_link_active(hba)) {
 			/* disable device ref_clk */
 			ufs_qcom_dev_ref_clk_ctrl(host, false);
 		}
-
-		vote = host->bus_vote.min_bw_vote;
+	} else if (!on && (status == POST_CHANGE)) {
+		err = ufs_qcom_set_bus_vote(hba, false);
 	}
 
-	err = ufs_qcom_set_bus_vote(host, vote);
-	if (err)
-		dev_err(hba->dev, "%s: set bus vote failed %d\n",
-				__func__, err);
-
 	return err;
 }
 
@@ -1238,6 +1258,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	ufs_qcom_set_caps(hba);
 	ufs_qcom_advertise_quirks(hba);
 
+	ufs_qcom_set_bus_vote(hba, true);
 	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
 
 	if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v4 5/8] scsi: ufs: Fix ufshcd_hold() caused scheduling while atomic
       [not found] <1579764349-15578-1-git-send-email-cang@codeaurora.org>
                   ` (3 preceding siblings ...)
  2020-01-23  7:25 ` [PATCH v4 4/8] scsi: ufs-qcom: Adjust bus bandwidth voting and unvoting Can Guo
@ 2020-01-23  7:25 ` Can Guo
  2020-01-24 18:07   ` Asutosh Das (asd)
  2020-01-23  7:25 ` [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support Can Guo
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 24+ messages in thread
From: Can Guo @ 2020-01-23  7:25 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Venkat Gopalakrishnan,
	Tomas Winkler, open list

The async version of ufshcd_hold(async == true), which is only called
in queuecommand path as for now, is expected to work in atomic context,
thus it should not sleep or schedule out. When it runs into the condition
that clocks are ON but link is still in hibern8 state, it should bail out
without flushing the clock ungate work.

Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Hongwu Su <hongwus@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 4dfd705..c316a07 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1542,6 +1542,11 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		 */
 		if (ufshcd_can_hibern8_during_gating(hba) &&
 		    ufshcd_is_link_hibern8(hba)) {
+			if (async) {
+				rc = -EAGAIN;
+				hba->clk_gating.active_reqs--;
+				break;
+			}
 			spin_unlock_irqrestore(hba->host->host_lock, flags);
 			flush_work(&hba->clk_gating.ungate_work);
 			spin_lock_irqsave(hba->host->host_lock, flags);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support
       [not found] <1579764349-15578-1-git-send-email-cang@codeaurora.org>
                   ` (4 preceding siblings ...)
  2020-01-23  7:25 ` [PATCH v4 5/8] scsi: ufs: Fix ufshcd_hold() caused scheduling while atomic Can Guo
@ 2020-01-23  7:25 ` Can Guo
  2020-01-23 23:11   ` [EXT] " Bean Huo (beanhuo)
                     ` (2 more replies)
  2020-01-23  7:25 ` [PATCH v4 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk Can Guo
  2020-01-23  7:25 ` [PATCH v4 8/8] scsi: ufs: Select INITIAL adapt for HS Gear4 Can Guo
  7 siblings, 3 replies; 24+ messages in thread
From: Can Guo @ 2020-01-23  7:25 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Tomas Winkler, Stanley Chu,
	Venkat Gopalakrishnan, open list

In UFS version 3.0, a newly added attribute bRefClkGatingWaitTime defines
the minimum time for which the reference clock is required by device during
transition to LS-MODE or HIBERN8 state. Make this change to reflect the new
requirement by adding delays before turning off the clock.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs.h    |  3 +++
 drivers/scsi/ufs/ufshcd.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 3327981..385bac8 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -168,6 +168,7 @@ enum attr_idn {
 	QUERY_ATTR_IDN_FFU_STATUS		= 0x14,
 	QUERY_ATTR_IDN_PSA_STATE		= 0x15,
 	QUERY_ATTR_IDN_PSA_DATA_SIZE		= 0x16,
+	QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME	= 0x17,
 };
 
 /* Descriptor idn for Query requests */
@@ -530,6 +531,8 @@ struct ufs_dev_info {
 	bool f_power_on_wp_en;
 	/* Keeps information if any of the LU is power on write protected */
 	bool is_lu_power_on_wp;
+	u16 spec_version;
+	u32 clk_gating_wait_us;
 };
 
 #define MAX_MODEL_LEN 16
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c316a07..1ee2187 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -91,6 +91,9 @@
 /* default delay of autosuspend: 2000 ms */
 #define RPM_AUTOSUSPEND_DELAY_MS 2000
 
+/* Default value of wait time before gating device ref clock */
+#define UFSHCD_REF_CLK_GATING_WAIT_US 0xFF /* microsecs */
+
 #define ufshcd_toggle_vreg(_dev, _vreg, _on)				\
 	({                                                              \
 		int _ret;                                               \
@@ -3357,6 +3360,37 @@ static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba,
 				      param_offset, param_read_buf, param_size);
 }
 
+static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba)
+{
+	int err = 0;
+	u32 gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
+
+	if (hba->dev_info.spec_version >= 0x300) {
+		err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+				QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME, 0, 0,
+				&gating_wait);
+		if (err)
+			dev_err(hba->dev, "Failed reading bRefClkGatingWait. err = %d, use default %uus\n",
+					 err, gating_wait);
+
+		if (gating_wait == 0) {
+			gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
+			dev_err(hba->dev, "Undefined ref clk gating wait time, use default %uus\n",
+					 gating_wait);
+		}
+
+		/*
+		 * bRefClkGatingWaitTime defines the minimum time for which the
+		 * reference clock is required by device during transition from
+		 * HS-MODE to LS-MODE or HIBERN8 state. Give it more time to be
+		 * on the safe side.
+		 */
+		hba->dev_info.clk_gating_wait_us = gating_wait + 50;
+	}
+
+	return err;
+}
+
 /**
  * ufshcd_memory_alloc - allocate memory for host memory space data structures
  * @hba: per adapter instance
@@ -6628,6 +6662,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba,
 	dev_desc->wmanufacturerid = desc_buf[DEVICE_DESC_PARAM_MANF_ID] << 8 |
 				     desc_buf[DEVICE_DESC_PARAM_MANF_ID + 1];
 
+	/* getting Specification Version in big endian format */
+	hba->dev_info.spec_version = desc_buf[DEVICE_DESC_PARAM_SPEC_VER] << 8 |
+				      desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
+
 	model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
 	err = ufshcd_read_string_desc(hba, model_index,
 				      &dev_desc->model, SD_ASCII_STD);
@@ -7046,6 +7084,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 
 		/* clear any previous UFS device information */
 		memset(&hba->dev_info, 0, sizeof(hba->dev_info));
+
+		ufshcd_get_ref_clk_gating_wait(hba);
+
 		if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
 				QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
 			hba->dev_info.f_power_on_wp_en = flag;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v4 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk
       [not found] <1579764349-15578-1-git-send-email-cang@codeaurora.org>
                   ` (5 preceding siblings ...)
  2020-01-23  7:25 ` [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support Can Guo
@ 2020-01-23  7:25 ` Can Guo
  2020-01-24 18:04   ` Asutosh Das (asd)
  2020-01-23  7:25 ` [PATCH v4 8/8] scsi: ufs: Select INITIAL adapt for HS Gear4 Can Guo
  7 siblings, 1 reply; 24+ messages in thread
From: Can Guo @ 2020-01-23  7:25 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Andy Gross, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen,
	open list:ARM/QUALCOMM SUPPORT, open list

After enter hibern8, as UFS JEDEC ver 3.0 requires, a specific gating wait
time is required before disable the device reference clock. If it is not
specified, use the old delay.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 85d7c17..3b5b2d9 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -833,6 +833,8 @@ static int ufs_qcom_bus_register(struct ufs_qcom_host *host)
 
 static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
 {
+	unsigned long gating_wait;
+
 	if (host->dev_ref_clk_ctrl_mmio &&
 	    (enable ^ host->is_dev_ref_clk_enabled)) {
 		u32 temp = readl_relaxed(host->dev_ref_clk_ctrl_mmio);
@@ -845,11 +847,16 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
 		/*
 		 * If we are here to disable this clock it might be immediately
 		 * after entering into hibern8 in which case we need to make
-		 * sure that device ref_clk is active at least 1us after the
+		 * sure that device ref_clk is active for specific time after
 		 * hibern8 enter.
 		 */
-		if (!enable)
-			udelay(1);
+		if (!enable) {
+			gating_wait = host->hba->dev_info.clk_gating_wait_us;
+			if (!gating_wait)
+				udelay(1);
+			else
+				usleep_range(gating_wait, gating_wait + 10);
+		}
 
 		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v4 8/8] scsi: ufs: Select INITIAL adapt for HS Gear4
       [not found] <1579764349-15578-1-git-send-email-cang@codeaurora.org>
                   ` (6 preceding siblings ...)
  2020-01-23  7:25 ` [PATCH v4 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk Can Guo
@ 2020-01-23  7:25 ` Can Guo
  2020-01-24 18:03   ` Asutosh Das (asd)
  2020-01-26  3:38   ` Bart Van Assche
  7 siblings, 2 replies; 24+ messages in thread
From: Can Guo @ 2020-01-23  7:25 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Venkat Gopalakrishnan,
	Tomas Winkler, Bean Huo, Greg Kroah-Hartman, Thomas Gleixner,
	Allison Randal, open list

ADAPT is added specifically for HS Gear4 mode only, select INITIAL adapt
before do power mode change to G4 and select no adapt before switch to
non-G4 modes.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 14 +++++++++++++-
 drivers/scsi/ufs/ufshci.h |  1 +
 drivers/scsi/ufs/unipro.h |  7 +++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1ee2187..f6d4828 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4134,6 +4134,17 @@ static int ufshcd_change_power_mode(struct ufs_hba *hba,
 		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_HSSERIES),
 						pwr_mode->hs_rate);
 
+	if (hba->ufs_version >= UFSHCI_VERSION_30) {
+		if (pwr_mode->gear_tx == UFS_HS_G4)
+			/* INITIAL ADAPT */
+			ufshcd_dme_set(hba, UIC_ARG_MIB(PA_TXHSADAPTTYPE),
+					PA_INITIAL_ADAPT);
+		else
+			/* NO ADAPT */
+			ufshcd_dme_set(hba, UIC_ARG_MIB(PA_TXHSADAPTTYPE),
+					PA_NO_ADAPT);
+	}
+
 	ret = ufshcd_uic_change_pwr_mode(hba, pwr_mode->pwr_rx << 4
 			| pwr_mode->pwr_tx);
 
@@ -8422,7 +8433,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	if ((hba->ufs_version != UFSHCI_VERSION_10) &&
 	    (hba->ufs_version != UFSHCI_VERSION_11) &&
 	    (hba->ufs_version != UFSHCI_VERSION_20) &&
-	    (hba->ufs_version != UFSHCI_VERSION_21))
+	    (hba->ufs_version != UFSHCI_VERSION_21) &&
+	    (hba->ufs_version != UFSHCI_VERSION_30))
 		dev_err(hba->dev, "invalid UFS version 0x%x\n",
 			hba->ufs_version);
 
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index dbb75cd..95b4b03 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -104,6 +104,7 @@ enum {
 	UFSHCI_VERSION_11 = 0x00010100, /* 1.1 */
 	UFSHCI_VERSION_20 = 0x00000200, /* 2.0 */
 	UFSHCI_VERSION_21 = 0x00000210, /* 2.1 */
+	UFSHCI_VERSION_30 = 0x00000300, /* 3.0 */
 };
 
 /*
diff --git a/drivers/scsi/ufs/unipro.h b/drivers/scsi/ufs/unipro.h
index f539f87..960d175 100644
--- a/drivers/scsi/ufs/unipro.h
+++ b/drivers/scsi/ufs/unipro.h
@@ -146,6 +146,12 @@
 #define PA_SLEEPNOCONFIGTIME	0x15A2
 #define PA_STALLNOCONFIGTIME	0x15A3
 #define PA_SAVECONFIGTIME	0x15A4
+#define PA_TXHSADAPTTYPE	0x15D4
+
+/* Adpat type for PA_TXHSADAPTTYPE attribute */
+#define PA_REFRESH_ADAPT	0x00
+#define PA_INITIAL_ADAPT	0x01
+#define PA_NO_ADAPT		0x03
 
 #define PA_TACTIVATE_TIME_UNIT_US	10
 #define PA_HIBERN8_TIME_UNIT_US		100
@@ -192,6 +198,7 @@ enum ufs_hs_gear_tag {
 	UFS_HS_G1,		/* HS Gear 1 (default for reset) */
 	UFS_HS_G2,		/* HS Gear 2 */
 	UFS_HS_G3,		/* HS Gear 3 */
+	UFS_HS_G4,		/* HS Gear 4 */
 };
 
 enum ufs_unipro_ver {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 2/8] scsi: ufs: set load before setting voltage in regulators
  2020-01-23  7:25 ` [PATCH v4 2/8] scsi: ufs: set load before setting voltage in regulators Can Guo
@ 2020-01-23  7:50   ` hongwus
  0 siblings, 0 replies; 24+ messages in thread
From: hongwus @ 2020-01-23  7:50 UTC (permalink / raw)
  To: Can Guo
  Cc: asutoshd, nguyenb, rnayak, linux-scsi, kernel-team, saravanak,
	salyzyn, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen, Stanley Chu,
	Venkat Gopalakrishnan, Tomas Winkler, linux-kernel

Hi Can,
   It makes sense that set the load before enable regulator or set 
voltage. Just avoid voltage drop.

On 2020-01-23 15:25, Can Guo wrote:
> From: Asutosh Das <asutoshd@codeaurora.org>
> 
> This sequence change is required to avoid dips in voltage
> during boot-up.
> 
> Apparently, this dip is caused because in the original
> sequence, the regulators are initialized in lpm mode.
> And then when the load is set to high, and more current
> is drawn, than is allowed in lpm, the dip is seen.
> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c2de29f..c386c2d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7225,6 +7225,11 @@ static int ufshcd_config_vreg(struct device 
> *dev,
>  	name = vreg->name;
> 
>  	if (regulator_count_voltages(reg) > 0) {
> +		uA_load = on ? vreg->max_uA : 0;
> +		ret = ufshcd_config_vreg_load(dev, vreg, uA_load);
> +		if (ret)
> +			goto out;
> +
>  		if (vreg->min_uV && vreg->max_uV) {
>  			min_uV = on ? vreg->min_uV : 0;
>  			ret = regulator_set_voltage(reg, min_uV, vreg->max_uV);
> @@ -7235,11 +7240,6 @@ static int ufshcd_config_vreg(struct device 
> *dev,
>  				goto out;
>  			}
>  		}
> -
> -		uA_load = on ? vreg->max_uA : 0;
> -		ret = ufshcd_config_vreg_load(dev, vreg, uA_load);
> -		if (ret)
> -			goto out;
>  	}
>  out:
>  	return ret;


Reviewed-by: Hongwu Su <hongwus@codeaurora.org>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [EXT] [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend
  2020-01-23  7:25 ` [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend Can Guo
@ 2020-01-23 23:09   ` Bean Huo (beanhuo)
  2020-01-26  3:29   ` Bart Van Assche
  1 sibling, 0 replies; 24+ messages in thread
From: Bean Huo (beanhuo) @ 2020-01-23 23:09 UTC (permalink / raw)
  To: Can Guo, asutoshd, nguyenb, hongwus, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn
  Cc: Sayali Lokhande, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen, Stanley Chu,
	Venkat Gopalakrishnan, Tomas Winkler, open list


Hi, Can

>  			/* TODO: handle Reject UPIU Response */ @@ -5215,7
> +5222,14 @@ static void ufshcd_exception_event_handler(struct work_struct
> *work)
> 
>  out:
>  	scsi_unblock_requests(hba->host);
> -	pm_runtime_put_sync(hba->dev);
> +	/*
> +	 * pm_runtime_get_noresume is called while scheduling
> +	 * eeh_work to avoid suspend racing with exception work.
> +	 * Hence decrement usage counter using pm_runtime_put_noidle
> +	 * to allow suspend on completion of exception event handler.
> +	 */
> +	pm_runtime_put_noidle(hba->dev);
> +	pm_runtime_put(hba->dev);
>  	return;
>  }
> 
Rebased this patch.
Thanks, 
//Bean

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [EXT] [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support
  2020-01-23  7:25 ` [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support Can Guo
@ 2020-01-23 23:11   ` Bean Huo (beanhuo)
  2020-02-03  7:17     ` Can Guo
  2020-01-24 18:06   ` Asutosh Das (asd)
  2020-01-26  3:34   ` Bart Van Assche
  2 siblings, 1 reply; 24+ messages in thread
From: Bean Huo (beanhuo) @ 2020-01-23 23:11 UTC (permalink / raw)
  To: Can Guo, asutoshd, nguyenb, hongwus, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Tomas Winkler, Stanley Chu,
	Venkat Gopalakrishnan, open list

Hi,  Can

> 
> In UFS version 3.0, a newly added attribute bRefClkGatingWaitTime defines the
> minimum time for which the reference clock is required by device during
> transition to LS-MODE or HIBERN8 state. Make this change to reflect the new
> requirement by adding delays before turning off the clock.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  3 +++
>  drivers/scsi/ufs/ufshcd.c | 41
> +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index
> 3327981..385bac8 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -168,6 +168,7 @@ enum attr_idn {
>  	QUERY_ATTR_IDN_FFU_STATUS		= 0x14,
>  	QUERY_ATTR_IDN_PSA_STATE		= 0x15,
>  	QUERY_ATTR_IDN_PSA_DATA_SIZE		= 0x16,
> +	QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME	= 0x17,
>  };
> 
>  /* Descriptor idn for Query requests */ @@ -530,6 +531,8 @@ struct
> ufs_dev_info {
>  	bool f_power_on_wp_en;
>  	/* Keeps information if any of the LU is power on write protected */
>  	bool is_lu_power_on_wp;
> +	u16 spec_version;
> +	u32 clk_gating_wait_us;
>  };
> 
This one also need rebase

Thanks,

//Bean

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 8/8] scsi: ufs: Select INITIAL adapt for HS Gear4
  2020-01-23  7:25 ` [PATCH v4 8/8] scsi: ufs: Select INITIAL adapt for HS Gear4 Can Guo
@ 2020-01-24 18:03   ` Asutosh Das (asd)
  2020-01-26  3:38   ` Bart Van Assche
  1 sibling, 0 replies; 24+ messages in thread
From: Asutosh Das (asd) @ 2020-01-24 18:03 UTC (permalink / raw)
  To: Can Guo, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Venkat Gopalakrishnan,
	Tomas Winkler, Bean Huo, Greg Kroah-Hartman, Thomas Gleixner,
	Allison Randal, open list

On 1/22/2020 11:25 PM, Can Guo wrote:
> ADAPT is added specifically for HS Gear4 mode only, select INITIAL adapt
> before do power mode change to G4 and select no adapt before switch to
> non-G4 modes.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---

LGTM.

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

>   drivers/scsi/ufs/ufshcd.c | 14 +++++++++++++-
>   drivers/scsi/ufs/ufshci.h |  1 +
>   drivers/scsi/ufs/unipro.h |  7 +++++++
>   3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 1ee2187..f6d4828 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4134,6 +4134,17 @@ static int ufshcd_change_power_mode(struct ufs_hba *hba,
>   		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_HSSERIES),
>   						pwr_mode->hs_rate);
>   
> +	if (hba->ufs_version >= UFSHCI_VERSION_30) {
> +		if (pwr_mode->gear_tx == UFS_HS_G4)
> +			/* INITIAL ADAPT */
> +			ufshcd_dme_set(hba, UIC_ARG_MIB(PA_TXHSADAPTTYPE),
> +					PA_INITIAL_ADAPT);
> +		else
> +			/* NO ADAPT */
> +			ufshcd_dme_set(hba, UIC_ARG_MIB(PA_TXHSADAPTTYPE),
> +					PA_NO_ADAPT);
> +	}
> +
>   	ret = ufshcd_uic_change_pwr_mode(hba, pwr_mode->pwr_rx << 4
>   			| pwr_mode->pwr_tx);
>   
> @@ -8422,7 +8433,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>   	if ((hba->ufs_version != UFSHCI_VERSION_10) &&
>   	    (hba->ufs_version != UFSHCI_VERSION_11) &&
>   	    (hba->ufs_version != UFSHCI_VERSION_20) &&
> -	    (hba->ufs_version != UFSHCI_VERSION_21))
> +	    (hba->ufs_version != UFSHCI_VERSION_21) &&
> +	    (hba->ufs_version != UFSHCI_VERSION_30))
>   		dev_err(hba->dev, "invalid UFS version 0x%x\n",
>   			hba->ufs_version);
>   
> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
> index dbb75cd..95b4b03 100644
> --- a/drivers/scsi/ufs/ufshci.h
> +++ b/drivers/scsi/ufs/ufshci.h
> @@ -104,6 +104,7 @@ enum {
>   	UFSHCI_VERSION_11 = 0x00010100, /* 1.1 */
>   	UFSHCI_VERSION_20 = 0x00000200, /* 2.0 */
>   	UFSHCI_VERSION_21 = 0x00000210, /* 2.1 */
> +	UFSHCI_VERSION_30 = 0x00000300, /* 3.0 */
>   };
>   
>   /*
> diff --git a/drivers/scsi/ufs/unipro.h b/drivers/scsi/ufs/unipro.h
> index f539f87..960d175 100644
> --- a/drivers/scsi/ufs/unipro.h
> +++ b/drivers/scsi/ufs/unipro.h
> @@ -146,6 +146,12 @@
>   #define PA_SLEEPNOCONFIGTIME	0x15A2
>   #define PA_STALLNOCONFIGTIME	0x15A3
>   #define PA_SAVECONFIGTIME	0x15A4
> +#define PA_TXHSADAPTTYPE	0x15D4
> +
> +/* Adpat type for PA_TXHSADAPTTYPE attribute */
> +#define PA_REFRESH_ADAPT	0x00
> +#define PA_INITIAL_ADAPT	0x01
> +#define PA_NO_ADAPT		0x03
>   
>   #define PA_TACTIVATE_TIME_UNIT_US	10
>   #define PA_HIBERN8_TIME_UNIT_US		100
> @@ -192,6 +198,7 @@ enum ufs_hs_gear_tag {
>   	UFS_HS_G1,		/* HS Gear 1 (default for reset) */
>   	UFS_HS_G2,		/* HS Gear 2 */
>   	UFS_HS_G3,		/* HS Gear 3 */
> +	UFS_HS_G4,		/* HS Gear 4 */
>   };
>   
>   enum ufs_unipro_ver {
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk
  2020-01-23  7:25 ` [PATCH v4 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk Can Guo
@ 2020-01-24 18:04   ` Asutosh Das (asd)
  0 siblings, 0 replies; 24+ messages in thread
From: Asutosh Das (asd) @ 2020-01-24 18:04 UTC (permalink / raw)
  To: Can Guo, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn
  Cc: Andy Gross, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen,
	open list:ARM/QUALCOMM SUPPORT, open list

On 1/22/2020 11:25 PM, Can Guo wrote:
> After enter hibern8, as UFS JEDEC ver 3.0 requires, a specific gating wait
> time is required before disable the device reference clock. If it is not
> specified, use the old delay.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

>   drivers/scsi/ufs/ufs-qcom.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 85d7c17..3b5b2d9 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -833,6 +833,8 @@ static int ufs_qcom_bus_register(struct ufs_qcom_host *host)
>   
>   static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>   {
> +	unsigned long gating_wait;
> +
>   	if (host->dev_ref_clk_ctrl_mmio &&
>   	    (enable ^ host->is_dev_ref_clk_enabled)) {
>   		u32 temp = readl_relaxed(host->dev_ref_clk_ctrl_mmio);
> @@ -845,11 +847,16 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>   		/*
>   		 * If we are here to disable this clock it might be immediately
>   		 * after entering into hibern8 in which case we need to make
> -		 * sure that device ref_clk is active at least 1us after the
> +		 * sure that device ref_clk is active for specific time after
>   		 * hibern8 enter.
>   		 */
> -		if (!enable)
> -			udelay(1);
> +		if (!enable) {
> +			gating_wait = host->hba->dev_info.clk_gating_wait_us;
> +			if (!gating_wait)
> +				udelay(1);
> +			else
> +				usleep_range(gating_wait, gating_wait + 10);
> +		}
>   
>   		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);
>   
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support
  2020-01-23  7:25 ` [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support Can Guo
  2020-01-23 23:11   ` [EXT] " Bean Huo (beanhuo)
@ 2020-01-24 18:06   ` Asutosh Das (asd)
  2020-01-26  3:34   ` Bart Van Assche
  2 siblings, 0 replies; 24+ messages in thread
From: Asutosh Das (asd) @ 2020-01-24 18:06 UTC (permalink / raw)
  To: Can Guo, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Tomas Winkler, Stanley Chu,
	Venkat Gopalakrishnan, open list

On 1/22/2020 11:25 PM, Can Guo wrote:
> In UFS version 3.0, a newly added attribute bRefClkGatingWaitTime defines
> the minimum time for which the reference clock is required by device during
> transition to LS-MODE or HIBERN8 state. Make this change to reflect the new
> requirement by adding delays before turning off the clock.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

>   drivers/scsi/ufs/ufs.h    |  3 +++
>   drivers/scsi/ufs/ufshcd.c | 41 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 3327981..385bac8 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -168,6 +168,7 @@ enum attr_idn {
>   	QUERY_ATTR_IDN_FFU_STATUS		= 0x14,
>   	QUERY_ATTR_IDN_PSA_STATE		= 0x15,
>   	QUERY_ATTR_IDN_PSA_DATA_SIZE		= 0x16,
> +	QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME	= 0x17,
>   };
>   
>   /* Descriptor idn for Query requests */
> @@ -530,6 +531,8 @@ struct ufs_dev_info {
>   	bool f_power_on_wp_en;
>   	/* Keeps information if any of the LU is power on write protected */
>   	bool is_lu_power_on_wp;
> +	u16 spec_version;
> +	u32 clk_gating_wait_us;
>   };
>   
>   #define MAX_MODEL_LEN 16
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c316a07..1ee2187 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -91,6 +91,9 @@
>   /* default delay of autosuspend: 2000 ms */
>   #define RPM_AUTOSUSPEND_DELAY_MS 2000
>   
> +/* Default value of wait time before gating device ref clock */
> +#define UFSHCD_REF_CLK_GATING_WAIT_US 0xFF /* microsecs */
> +
>   #define ufshcd_toggle_vreg(_dev, _vreg, _on)				\
>   	({                                                              \
>   		int _ret;                                               \
> @@ -3357,6 +3360,37 @@ static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba,
>   				      param_offset, param_read_buf, param_size);
>   }
>   
> +static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba)
> +{
> +	int err = 0;
> +	u32 gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
> +
> +	if (hba->dev_info.spec_version >= 0x300) {
> +		err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +				QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME, 0, 0,
> +				&gating_wait);
> +		if (err)
> +			dev_err(hba->dev, "Failed reading bRefClkGatingWait. err = %d, use default %uus\n",
> +					 err, gating_wait);
> +
> +		if (gating_wait == 0) {
> +			gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
> +			dev_err(hba->dev, "Undefined ref clk gating wait time, use default %uus\n",
> +					 gating_wait);
> +		}
> +
> +		/*
> +		 * bRefClkGatingWaitTime defines the minimum time for which the
> +		 * reference clock is required by device during transition from
> +		 * HS-MODE to LS-MODE or HIBERN8 state. Give it more time to be
> +		 * on the safe side.
> +		 */
> +		hba->dev_info.clk_gating_wait_us = gating_wait + 50;
> +	}
> +
> +	return err;
> +}
> +
>   /**
>    * ufshcd_memory_alloc - allocate memory for host memory space data structures
>    * @hba: per adapter instance
> @@ -6628,6 +6662,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba,
>   	dev_desc->wmanufacturerid = desc_buf[DEVICE_DESC_PARAM_MANF_ID] << 8 |
>   				     desc_buf[DEVICE_DESC_PARAM_MANF_ID + 1];
>   
> +	/* getting Specification Version in big endian format */
> +	hba->dev_info.spec_version = desc_buf[DEVICE_DESC_PARAM_SPEC_VER] << 8 |
> +				      desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
> +
>   	model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
>   	err = ufshcd_read_string_desc(hba, model_index,
>   				      &dev_desc->model, SD_ASCII_STD);
> @@ -7046,6 +7084,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>   
>   		/* clear any previous UFS device information */
>   		memset(&hba->dev_info, 0, sizeof(hba->dev_info));
> +
> +		ufshcd_get_ref_clk_gating_wait(hba);
> +
>   		if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
>   				QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
>   			hba->dev_info.f_power_on_wp_en = flag;
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 5/8] scsi: ufs: Fix ufshcd_hold() caused scheduling while atomic
  2020-01-23  7:25 ` [PATCH v4 5/8] scsi: ufs: Fix ufshcd_hold() caused scheduling while atomic Can Guo
@ 2020-01-24 18:07   ` Asutosh Das (asd)
  0 siblings, 0 replies; 24+ messages in thread
From: Asutosh Das (asd) @ 2020-01-24 18:07 UTC (permalink / raw)
  To: Can Guo, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Venkat Gopalakrishnan,
	Tomas Winkler, open list

On 1/22/2020 11:25 PM, Can Guo wrote:
> The async version of ufshcd_hold(async == true), which is only called
> in queuecommand path as for now, is expected to work in atomic context,
> thus it should not sleep or schedule out. When it runs into the condition
> that clocks are ON but link is still in hibern8 state, it should bail out
> without flushing the clock ungate work.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Reviewed-by: Hongwu Su <hongwus@codeaurora.org>
> ---

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

>   drivers/scsi/ufs/ufshcd.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 4dfd705..c316a07 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1542,6 +1542,11 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
>   		 */
>   		if (ufshcd_can_hibern8_during_gating(hba) &&
>   		    ufshcd_is_link_hibern8(hba)) {
> +			if (async) {
> +				rc = -EAGAIN;
> +				hba->clk_gating.active_reqs--;
> +				break;
> +			}
>   			spin_unlock_irqrestore(hba->host->host_lock, flags);
>   			flush_work(&hba->clk_gating.ungate_work);
>   			spin_lock_irqsave(hba->host->host_lock, flags);
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend
  2020-01-23  7:25 ` [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend Can Guo
  2020-01-23 23:09   ` [EXT] " Bean Huo (beanhuo)
@ 2020-01-26  3:29   ` Bart Van Assche
  2020-02-03  6:23     ` Can Guo
  1 sibling, 1 reply; 24+ messages in thread
From: Bart Van Assche @ 2020-01-26  3:29 UTC (permalink / raw)
  To: Can Guo, asutoshd, nguyenb, hongwus, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn
  Cc: Sayali Lokhande, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen, Stanley Chu, Bean Huo,
	Venkat Gopalakrishnan, Tomas Winkler, open list

On 2020-01-22 23:25, Can Guo wrote:
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 1201578..c2de29f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4760,8 +4760,15 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>  			 * UFS device needs urgent BKOPs.
>  			 */
>  			if (!hba->pm_op_in_progress &&
> -			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
> -				schedule_work(&hba->eeh_work);
> +			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr)) {
> +				/*
> +				 * Prevent suspend once eeh_work is scheduled
> +				 * to avoid deadlock between ufshcd_suspend
> +				 * and exception event handler.
> +				 */
> +				if (schedule_work(&hba->eeh_work))
> +					pm_runtime_get_noresume(hba->dev);
> +			}

Please combine the two logical tests with "&&" instead of nesting two
if-statements inside each other.

>  			break;
>  		case UPIU_TRANSACTION_REJECT_UPIU:
>  			/* TODO: handle Reject UPIU Response */
> @@ -5215,7 +5222,14 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
>  
>  out:
>  	scsi_unblock_requests(hba->host);
> -	pm_runtime_put_sync(hba->dev);
> +	/*
> +	 * pm_runtime_get_noresume is called while scheduling
> +	 * eeh_work to avoid suspend racing with exception work.
> +	 * Hence decrement usage counter using pm_runtime_put_noidle
> +	 * to allow suspend on completion of exception event handler.
> +	 */
> +	pm_runtime_put_noidle(hba->dev);
> +	pm_runtime_put(hba->dev);
>  	return;
>  }
>  
> @@ -7901,6 +7915,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  			goto enable_gating;
>  	}
>  
> +	flush_work(&hba->eeh_work);
>  	ret = ufshcd_link_state_transition(hba, req_link_state, 1);
>  	if (ret)
>  		goto set_dev_active;

I think this patch introduces a new race condition, namely the following:
- ufshcd_slave_destroy() tests pm_op_in_progress and reads the value
  zero from that variable.
- ufshcd_suspend() sets hba->pm_op_in_progress to one.
- ufshcd_slave_destroy() calls schedule_work().

How about fixing this race condition by calling
pm_runtime_get_noresume() before checking pm_op_in_progress and by
reallowing resume if no work is scheduled?

Thanks,

Bart.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support
  2020-01-23  7:25 ` [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support Can Guo
  2020-01-23 23:11   ` [EXT] " Bean Huo (beanhuo)
  2020-01-24 18:06   ` Asutosh Das (asd)
@ 2020-01-26  3:34   ` Bart Van Assche
  2020-02-03  7:16     ` Can Guo
  2 siblings, 1 reply; 24+ messages in thread
From: Bart Van Assche @ 2020-01-26  3:34 UTC (permalink / raw)
  To: Can Guo, asutoshd, nguyenb, hongwus, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Tomas Winkler, Stanley Chu,
	Venkat Gopalakrishnan, open list

On 2020-01-22 23:25, Can Guo wrote:
> +	/* getting Specification Version in big endian format */
> +	hba->dev_info.spec_version = desc_buf[DEVICE_DESC_PARAM_SPEC_VER] << 8 |
> +				      desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];

Please use get_unaligned_be16() instead of open-coding it.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 8/8] scsi: ufs: Select INITIAL adapt for HS Gear4
  2020-01-23  7:25 ` [PATCH v4 8/8] scsi: ufs: Select INITIAL adapt for HS Gear4 Can Guo
  2020-01-24 18:03   ` Asutosh Das (asd)
@ 2020-01-26  3:38   ` Bart Van Assche
  1 sibling, 0 replies; 24+ messages in thread
From: Bart Van Assche @ 2020-01-26  3:38 UTC (permalink / raw)
  To: Can Guo, asutoshd, nguyenb, hongwus, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn
  Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Venkat Gopalakrishnan,
	Tomas Winkler, Bean Huo, Greg Kroah-Hartman, Thomas Gleixner,
	Allison Randal, open list

On 2020-01-22 23:25, Can Guo wrote:
> @@ -8422,7 +8433,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  	if ((hba->ufs_version != UFSHCI_VERSION_10) &&
>  	    (hba->ufs_version != UFSHCI_VERSION_11) &&
>  	    (hba->ufs_version != UFSHCI_VERSION_20) &&
> -	    (hba->ufs_version != UFSHCI_VERSION_21))
> +	    (hba->ufs_version != UFSHCI_VERSION_21) &&
> +	    (hba->ufs_version != UFSHCI_VERSION_30))
>  		dev_err(hba->dev, "invalid UFS version 0x%x\n",
>  			hba->ufs_version);

Is the UFS specification backwards compatible? Or in other words, does
the existing driver work fine for devices with a controller that
supports a newer version of the spec? I'm asking this because in Linux
kernel driver a version check that excludes newer controller versions is
very unusual. Can the above version check be removed in its entirety?

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend
  2020-01-26  3:29   ` Bart Van Assche
@ 2020-02-03  6:23     ` Can Guo
  2020-02-04  3:12       ` Bart Van Assche
  0 siblings, 1 reply; 24+ messages in thread
From: Can Guo @ 2020-02-03  6:23 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Sayali Lokhande, Alim Akhtar, Avri Altman,
	Pedro Sousa, James E.J. Bottomley, Martin K. Petersen,
	Stanley Chu, Bean Huo, Venkat Gopalakrishnan, Tomas Winkler,
	open list

On 2020-01-26 11:29, Bart Van Assche wrote:
> On 2020-01-22 23:25, Can Guo wrote:
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 1201578..c2de29f 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -4760,8 +4760,15 @@ static void ufshcd_slave_destroy(struct 
>> scsi_device *sdev)
>>  			 * UFS device needs urgent BKOPs.
>>  			 */
>>  			if (!hba->pm_op_in_progress &&
>> -			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
>> -				schedule_work(&hba->eeh_work);
>> +			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr)) {
>> +				/*
>> +				 * Prevent suspend once eeh_work is scheduled
>> +				 * to avoid deadlock between ufshcd_suspend
>> +				 * and exception event handler.
>> +				 */
>> +				if (schedule_work(&hba->eeh_work))
>> +					pm_runtime_get_noresume(hba->dev);
>> +			}
> 
> Please combine the two logical tests with "&&" instead of nesting two
> if-statements inside each other.
> 
>>  			break;
>>  		case UPIU_TRANSACTION_REJECT_UPIU:
>>  			/* TODO: handle Reject UPIU Response */
>> @@ -5215,7 +5222,14 @@ static void 
>> ufshcd_exception_event_handler(struct work_struct *work)
>> 
>>  out:
>>  	scsi_unblock_requests(hba->host);
>> -	pm_runtime_put_sync(hba->dev);
>> +	/*
>> +	 * pm_runtime_get_noresume is called while scheduling
>> +	 * eeh_work to avoid suspend racing with exception work.
>> +	 * Hence decrement usage counter using pm_runtime_put_noidle
>> +	 * to allow suspend on completion of exception event handler.
>> +	 */
>> +	pm_runtime_put_noidle(hba->dev);
>> +	pm_runtime_put(hba->dev);
>>  	return;
>>  }
>> 
>> @@ -7901,6 +7915,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, 
>> enum ufs_pm_op pm_op)
>>  			goto enable_gating;
>>  	}
>> 
>> +	flush_work(&hba->eeh_work);
>>  	ret = ufshcd_link_state_transition(hba, req_link_state, 1);
>>  	if (ret)
>>  		goto set_dev_active;
> 
> I think this patch introduces a new race condition, namely the 
> following:
> - ufshcd_slave_destroy() tests pm_op_in_progress and reads the value
>   zero from that variable.
> - ufshcd_suspend() sets hba->pm_op_in_progress to one.
> - ufshcd_slave_destroy() calls schedule_work().
> 
> How about fixing this race condition by calling
> pm_runtime_get_noresume() before checking pm_op_in_progress and by
> reallowing resume if no work is scheduled?
> 
> Thanks,
> 
> Bart.

Hi Bart,

If you apply this patch, you will find the change is not in
ufshcd_slave_destroy(), but in ufshcd_transfer_rsp_status().
So the racing you mentioned above does not exist.

Thanks,

Can Guo.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support
  2020-01-26  3:34   ` Bart Van Assche
@ 2020-02-03  7:16     ` Can Guo
  2020-02-03 22:30       ` Bart Van Assche
  0 siblings, 1 reply; 24+ messages in thread
From: Can Guo @ 2020-02-03  7:16 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen, Tomas Winkler,
	Stanley Chu, Venkat Gopalakrishnan, open list

On 2020-01-26 11:34, Bart Van Assche wrote:
> On 2020-01-22 23:25, Can Guo wrote:
>> +	/* getting Specification Version in big endian format */
>> +	hba->dev_info.spec_version = desc_buf[DEVICE_DESC_PARAM_SPEC_VER] << 
>> 8 |
>> +				      desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
> 
> Please use get_unaligned_be16() instead of open-coding it.
> 
> Thanks,
> 
> Bart.

I am just keeping symmetry with the other device descriptors,
for example w_manufacturer_id.

Thanks,

Can Guo.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [EXT] [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support
  2020-01-23 23:11   ` [EXT] " Bean Huo (beanhuo)
@ 2020-02-03  7:17     ` Can Guo
  0 siblings, 0 replies; 24+ messages in thread
From: Can Guo @ 2020-02-03  7:17 UTC (permalink / raw)
  To: Bean Huo (beanhuo)
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen, Tomas Winkler,
	Stanley Chu, Venkat Gopalakrishnan, open list

On 2020-01-24 07:11, Bean Huo (beanhuo) wrote:
> Hi,  Can
> 
>> 
>> In UFS version 3.0, a newly added attribute bRefClkGatingWaitTime 
>> defines the
>> minimum time for which the reference clock is required by device 
>> during
>> transition to LS-MODE or HIBERN8 state. Make this change to reflect 
>> the new
>> requirement by adding delays before turning off the clock.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufs.h    |  3 +++
>>  drivers/scsi/ufs/ufshcd.c | 41
>> +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>> 
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index
>> 3327981..385bac8 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -168,6 +168,7 @@ enum attr_idn {
>>  	QUERY_ATTR_IDN_FFU_STATUS		= 0x14,
>>  	QUERY_ATTR_IDN_PSA_STATE		= 0x15,
>>  	QUERY_ATTR_IDN_PSA_DATA_SIZE		= 0x16,
>> +	QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME	= 0x17,
>>  };
>> 
>>  /* Descriptor idn for Query requests */ @@ -530,6 +531,8 @@ struct
>> ufs_dev_info {
>>  	bool f_power_on_wp_en;
>>  	/* Keeps information if any of the LU is power on write protected */
>>  	bool is_lu_power_on_wp;
>> +	u16 spec_version;
>> +	u32 clk_gating_wait_us;
>>  };
>> 
> This one also need rebase
> 
> Thanks,
> 
> //Bean

Shall rebase this series.

Thanks,

Can Guo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support
  2020-02-03  7:16     ` Can Guo
@ 2020-02-03 22:30       ` Bart Van Assche
  0 siblings, 0 replies; 24+ messages in thread
From: Bart Van Assche @ 2020-02-03 22:30 UTC (permalink / raw)
  To: Can Guo
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Alim Akhtar, Avri Altman, Pedro Sousa,
	James E.J. Bottomley, Martin K. Petersen, Tomas Winkler,
	Stanley Chu, Venkat Gopalakrishnan, open list

On 2/2/20 11:16 PM, Can Guo wrote:
> On 2020-01-26 11:34, Bart Van Assche wrote:
>> On 2020-01-22 23:25, Can Guo wrote:
>>> +    /* getting Specification Version in big endian format */
>>> +    hba->dev_info.spec_version = 
>>> desc_buf[DEVICE_DESC_PARAM_SPEC_VER] << 8 |
>>> +                      desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
>>
>> Please use get_unaligned_be16() instead of open-coding it.
> 
> I am just keeping symmetry with the other device descriptors,
> for example w_manufacturer_id.

Hi Can,

How about adding an additional patch that refactors the existing code?

Please use get_unaligned_be16() in new code. That makes code easier to 
read compared to open-coding get_unaligned_be16().

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend
  2020-02-03  6:23     ` Can Guo
@ 2020-02-04  3:12       ` Bart Van Assche
  2020-02-04  6:28         ` Can Guo
  0 siblings, 1 reply; 24+ messages in thread
From: Bart Van Assche @ 2020-02-04  3:12 UTC (permalink / raw)
  To: Can Guo
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Sayali Lokhande, Alim Akhtar, Avri Altman,
	Pedro Sousa, James E.J. Bottomley, Martin K. Petersen,
	Stanley Chu, Bean Huo, Venkat Gopalakrishnan, Tomas Winkler,
	open list

On 2020-02-02 22:23, Can Guo wrote:
> On 2020-01-26 11:29, Bart Van Assche wrote:
>> On 2020-01-22 23:25, Can Guo wrote:
>>>              break;
>>>          case UPIU_TRANSACTION_REJECT_UPIU:
>>>              /* TODO: handle Reject UPIU Response */
>>> @@ -5215,7 +5222,14 @@ static void
>>> ufshcd_exception_event_handler(struct work_struct *work)
>>>
>>>  out:
>>>      scsi_unblock_requests(hba->host);
>>> -    pm_runtime_put_sync(hba->dev);
>>> +    /*
>>> +     * pm_runtime_get_noresume is called while scheduling
>>> +     * eeh_work to avoid suspend racing with exception work.
>>> +     * Hence decrement usage counter using pm_runtime_put_noidle
>>> +     * to allow suspend on completion of exception event handler.
>>> +     */
>>> +    pm_runtime_put_noidle(hba->dev);
>>> +    pm_runtime_put(hba->dev);
>>>      return;
>>>  }
>>>
>>> @@ -7901,6 +7915,7 @@ static int ufshcd_suspend(struct ufs_hba *hba,
>>> enum ufs_pm_op pm_op)
>>>              goto enable_gating;
>>>      }
>>>
>>> +    flush_work(&hba->eeh_work);
>>>      ret = ufshcd_link_state_transition(hba, req_link_state, 1);
>>>      if (ret)
>>>          goto set_dev_active;
>>
>> I think this patch introduces a new race condition, namely the following:
>> - ufshcd_slave_destroy() tests pm_op_in_progress and reads the value
>>   zero from that variable.
>> - ufshcd_suspend() sets hba->pm_op_in_progress to one.
>> - ufshcd_slave_destroy() calls schedule_work().
>>
>> How about fixing this race condition by calling
>> pm_runtime_get_noresume() before checking pm_op_in_progress and by
>> reallowing resume if no work is scheduled?
> 
> If you apply this patch, you will find the change is not in
> ufshcd_slave_destroy(), but in ufshcd_transfer_rsp_status().
> So the racing you mentioned above does not exist.

Hi Can,

Apparently I got a function name wrong. Can the following race condition
happen:
- ufshcd_transfer_rsp_status() tests pm_op_in_progress and reads the
  value zero from that variable.
- ufshcd_suspend() sets hba->pm_op_in_progress to one.
- ufshcd_suspend() calls flush_work(&hba->eeh_work).
- ufshcd_transfer_rsp_status() calls schedule_work(&hba->eeh_work).

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend
  2020-02-04  3:12       ` Bart Van Assche
@ 2020-02-04  6:28         ` Can Guo
  0 siblings, 0 replies; 24+ messages in thread
From: Can Guo @ 2020-02-04  6:28 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Sayali Lokhande, Alim Akhtar, Avri Altman,
	Pedro Sousa, James E.J. Bottomley, Martin K. Petersen,
	Stanley Chu, Bean Huo, Venkat Gopalakrishnan, Tomas Winkler,
	open list

On 2020-02-04 11:12, Bart Van Assche wrote:
> On 2020-02-02 22:23, Can Guo wrote:
>> On 2020-01-26 11:29, Bart Van Assche wrote:
>>> On 2020-01-22 23:25, Can Guo wrote:
>>>>              break;
>>>>          case UPIU_TRANSACTION_REJECT_UPIU:
>>>>              /* TODO: handle Reject UPIU Response */
>>>> @@ -5215,7 +5222,14 @@ static void
>>>> ufshcd_exception_event_handler(struct work_struct *work)
>>>> 
>>>>  out:
>>>>      scsi_unblock_requests(hba->host);
>>>> -    pm_runtime_put_sync(hba->dev);
>>>> +    /*
>>>> +     * pm_runtime_get_noresume is called while scheduling
>>>> +     * eeh_work to avoid suspend racing with exception work.
>>>> +     * Hence decrement usage counter using pm_runtime_put_noidle
>>>> +     * to allow suspend on completion of exception event handler.
>>>> +     */
>>>> +    pm_runtime_put_noidle(hba->dev);
>>>> +    pm_runtime_put(hba->dev);
>>>>      return;
>>>>  }
>>>> 
>>>> @@ -7901,6 +7915,7 @@ static int ufshcd_suspend(struct ufs_hba *hba,
>>>> enum ufs_pm_op pm_op)
>>>>              goto enable_gating;
>>>>      }
>>>> 
>>>> +    flush_work(&hba->eeh_work);
>>>>      ret = ufshcd_link_state_transition(hba, req_link_state, 1);
>>>>      if (ret)
>>>>          goto set_dev_active;
>>> 
>>> I think this patch introduces a new race condition, namely the 
>>> following:
>>> - ufshcd_slave_destroy() tests pm_op_in_progress and reads the value
>>>   zero from that variable.
>>> - ufshcd_suspend() sets hba->pm_op_in_progress to one.
>>> - ufshcd_slave_destroy() calls schedule_work().
>>> 
>>> How about fixing this race condition by calling
>>> pm_runtime_get_noresume() before checking pm_op_in_progress and by
>>> reallowing resume if no work is scheduled?
>> 
>> If you apply this patch, you will find the change is not in
>> ufshcd_slave_destroy(), but in ufshcd_transfer_rsp_status().
>> So the racing you mentioned above does not exist.
> 
> Hi Can,
> 
> Apparently I got a function name wrong. Can the following race 
> condition
> happen:
> - ufshcd_transfer_rsp_status() tests pm_op_in_progress and reads the
>   value zero from that variable.
> - ufshcd_suspend() sets hba->pm_op_in_progress to one.
> - ufshcd_suspend() calls flush_work(&hba->eeh_work).
> - ufshcd_transfer_rsp_status() calls schedule_work(&hba->eeh_work).
> 
> Thanks,
> 
> Bart.

Hi Bart,

The sequence you mentioned is not possible.

In normal cases, before ufshcd_transfer_rsp_status() returns,
ufshcd_suspend() would not be called (unless you intentionally call
ufshcd_suspend() to screw it). Because ufshcd_transfer_rsp_status() is
called from __ufshcd_transfer_req_compl(), which is being used by either
UFS IRQ handler or err handler. Meanwhile, in 
__ufshcd_transfer_req_compl(),
scsi_done() is called only after ufshcd_transfer_rsp_status() returns. 
When
we are here, it means UFS driver is still handling requests/tasks, so 
suspend
would not kick start at this moment, either runtime suspend or system 
suspend.

And this is why below lines work, calling pm_runtime_get_noresume() 
within
ufshcd_transfer_rsp_status() can prevent runtime suspend from happening
after we leave ufshcd_transfer_rsp_status().

+                if (schedule_work(&hba->eeh_work))
+                    pm_runtime_get_noresume(hba->dev);

Thanks,

Can Guo.

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2020-02-04  6:28 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1579764349-15578-1-git-send-email-cang@codeaurora.org>
2020-01-23  7:25 ` [PATCH v4 1/8] scsi: ufs: Flush exception event before suspend Can Guo
2020-01-23 23:09   ` [EXT] " Bean Huo (beanhuo)
2020-01-26  3:29   ` Bart Van Assche
2020-02-03  6:23     ` Can Guo
2020-02-04  3:12       ` Bart Van Assche
2020-02-04  6:28         ` Can Guo
2020-01-23  7:25 ` [PATCH v4 2/8] scsi: ufs: set load before setting voltage in regulators Can Guo
2020-01-23  7:50   ` hongwus
2020-01-23  7:25 ` [PATCH v4 3/8] scsi: ufs: Remove the check before call setup clock notify vops Can Guo
2020-01-23  7:25 ` [PATCH v4 4/8] scsi: ufs-qcom: Adjust bus bandwidth voting and unvoting Can Guo
2020-01-23  7:25 ` [PATCH v4 5/8] scsi: ufs: Fix ufshcd_hold() caused scheduling while atomic Can Guo
2020-01-24 18:07   ` Asutosh Das (asd)
2020-01-23  7:25 ` [PATCH v4 6/8] scsi: ufs: Add dev ref clock gating wait time support Can Guo
2020-01-23 23:11   ` [EXT] " Bean Huo (beanhuo)
2020-02-03  7:17     ` Can Guo
2020-01-24 18:06   ` Asutosh Das (asd)
2020-01-26  3:34   ` Bart Van Assche
2020-02-03  7:16     ` Can Guo
2020-02-03 22:30       ` Bart Van Assche
2020-01-23  7:25 ` [PATCH v4 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk Can Guo
2020-01-24 18:04   ` Asutosh Das (asd)
2020-01-23  7:25 ` [PATCH v4 8/8] scsi: ufs: Select INITIAL adapt for HS Gear4 Can Guo
2020-01-24 18:03   ` Asutosh Das (asd)
2020-01-26  3:38   ` Bart Van Assche

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).