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@xxxxxxxxxx> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> > --- > This is based on top of: > > https://lore.kernel.org/linux-arm-msm/20231208065902.11006-1-manivannan.sadhasivam@xxxxxxxxxx/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@xxxxxxxxxx> > -- மணிவண்ணன் சதாசிவம்
