Re: [PATCH RFC v4 1/1] scsi: ufs: Fix ufs power down/on specs violation

On Mon 28 Dec 19:48 CST 2020, Can Guo wrote:

On 2020-12-29 09:18, Can Guo wrote:

quoted

On 2020-12-29 01:55, Bjorn Andersson wrote:

quoted

On Tue 22 Dec 07:49 CST 2020, Ziqi Chen wrote:

quoted

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 <redacted>
Cc: Stanley Chu <redacted>
Signed-off-by: Ziqi Chen <redacted>
---
 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.

quoted

quoted

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

Besides, currently this device reset vops is only registered by ufs-qcom.c
and ufs-mediatek.c, meaning any delays that we put in the common code are
not
necessary for those who do not have this vops registered, i.e ufs-exynos.c,
ufs-hisi.c.

Surely we can detect this in the common code and only sleep if the vops
is implemented - and successfully deasserted the reset.

Regards,
Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel