All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Halaney <ahalaney@redhat.com>
To: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	 Konrad Dybcio <konrad.dybcio@linaro.org>,
	 Manivannan Sadhasivam <mani@kernel.org>,
	 "James E.J. Bottomley" <jejb@linux.ibm.com>,
	 "Martin K. Petersen" <martin.petersen@oracle.com>,
	 Yaniv Gardi <ygardi@codeaurora.org>,
	Dov Levenglick <dovl@codeaurora.org>
Cc: Will Deacon <will@kernel.org>,
	linux-arm-msm@vger.kernel.org,  linux-scsi@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	 Andrew Halaney <ahalaney@redhat.com>
Subject: [PATCH] scsi: ufs: qcom: Perform read back after writing reset bit
Date: Fri, 08 Dec 2023 14:19:44 -0600	[thread overview]
Message-ID: <20231208-ufs-reset-ensure-effect-before-delay-v1-1-8a0f82d7a09e@redhat.com> (raw)

Currently, the reset bit for the UFS provided reset controller (used by
its phy) is written to, and then a mb() happens to try and ensure that
hit the device. Immediately afterwards a usleep_range() occurs.

mb() ensure that the write completes, but completion doesn't mean that
it isn't stored in a buffer somewhere. The recommendation for
ensuring this bit has taken effect on the device is to perform a read
back to force it to make it all the way to the device. This is
documented in device-io.rst and a talk by Will Deacon on this can
be seen over here:

    https://youtu.be/i6DayghhA8Q?si=MiyxB5cKJXSaoc01&t=1678

Let's do that to ensure the bit hits the device. By doing so and
guaranteeing the ordering against the immediately following
usleep_range(), the mb() can safely be removed.

Fixes: 81c0fc51b7a7 ("ufs-qcom: add support for Qualcomm Technologies Inc platforms")
Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
This is based on top of:

    https://lore.kernel.org/linux-arm-msm/20231208065902.11006-1-manivannan.sadhasivam@linaro.org/T/#ma6bf749cc3d08ab8ce05be98401ebce099fa92ba

Since it mucks with the reset as well, and looks like it will go in
soon.

I'm unsure if this is totally correct. The goal of this
seems to be "ensure the device reset bit has taken effect before
delaying afterwards". As I describe in the commit message, mb()
doesn't guarantee that, the read back does... if it's against a udelay().
I can't quite totally 100% convince myself that applies to usleep_range(),
but I think it should be.

In either case, I think the read back makes sense, the question is "is
it safe to remove the mb()?".

Sorry, Will's talk over has inspired me to poke the bear whenever I see
a memory barrier in a driver I play with :)

    https://youtu.be/i6DayghhA8Q?si=12B0wCqImx1lz8QX&t=1677
---
 drivers/ufs/host/ufs-qcom.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index cdceeb795e70..c8cd59b1b8a8 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -147,10 +147,10 @@ static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
 	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
 
 	/*
-	 * Make sure assertion of ufs phy reset is written to
-	 * register before returning
+	 * Dummy read to ensure the write takes effect before doing any sort
+	 * of delay
 	 */
-	mb();
+	ufshcd_readl(hba, REG_UFS_CFG1);
 }
 
 static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
@@ -158,10 +158,10 @@ static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
 	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, 0, REG_UFS_CFG1);
 
 	/*
-	 * Make sure de-assertion of ufs phy reset is written to
-	 * register before returning
+	 * Dummy read to ensure the write takes effect before doing any sort
+	 * of delay
 	 */
-	mb();
+	ufshcd_readl(hba, REG_UFS_CFG1);
 }
 
 /* Host controller hardware version: major.minor.step */

---
base-commit: 8fdfb333a099b142b49510f2e55778d654a5b224
change-id: 20231208-ufs-reset-ensure-effect-before-delay-6e06899d5419

Best regards,
-- 
Andrew Halaney <ahalaney@redhat.com>


             reply	other threads:[~2023-12-08 20:20 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-08 20:19 Andrew Halaney [this message]
2023-12-11  9:36 ` [PATCH] scsi: ufs: qcom: Perform read back after writing reset bit Manivannan Sadhasivam
2023-12-11 17:59 ` Bart Van Assche
2023-12-11 19:51   ` Andrew Halaney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231208-ufs-reset-ensure-effect-before-delay-v1-1-8a0f82d7a09e@redhat.com \
    --to=ahalaney@redhat.com \
    --cc=agross@kernel.org \
    --cc=andersson@kernel.org \
    --cc=dovl@codeaurora.org \
    --cc=jejb@linux.ibm.com \
    --cc=konrad.dybcio@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=mani@kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=will@kernel.org \
    --cc=ygardi@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.