Re: [PATCH] scsi: ufs: qcom: Perform read back after writing reset bit

From: Manivannan Sadhasivam <mani@kernel.org>
To: Andrew Halaney <ahalaney@redhat.com>
Cc: 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>,
	Will Deacon <will@kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-scsi@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] scsi: ufs: qcom: Perform read back after writing reset bit
Date: Mon, 11 Dec 2023 15:06:30 +0530	[thread overview]
Message-ID: <20231211093630.GA2894@thinkpad> (raw)
In-Reply-To: <20231208-ufs-reset-ensure-effect-before-delay-v1-1-8a0f82d7a09e@redhat.com>

On Fri, Dec 08, 2023 at 02:19:44PM -0600, Andrew Halaney wrote:
> 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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

This patch is perfectly fine. I did similar cleanups earlier, but missed this
one. Thanks!

> 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

Yeah, this inspired me too :)

- Mani

> --->  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>
> 

-- 
மணிவண்ணன் சதாசிவம்