| Message ID | 20240329-ufs-reset-ensure-effect-before-delay-v5-11-181252004586@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: ufs: Remove overzealous memory barriers | expand |
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Fri, Mar 29, 2024 at 03:46:53PM -0500, Andrew Halaney wrote: > Currently a wmb() is used to ensure that writes to the > UTP_TASK_REQ_LIST_BASE* regs are completed prior to following writes to > the run/stop registers. > > wmb() ensure that the write completes, but completion doesn't mean that > it isn't stored in a buffer somewhere. The recommendation for > ensuring the bits have 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 > > But, none of that is necessary here. All of the writel()/readl()'s here > are to the same endpoint, so they will be ordered. There's no subsequent > delay() etc that requires it to have taken effect already, so no > readback is necessary here. > > For that reason just drop the wmb() altogether. > > Fixes: 897efe628d7e ("scsi: ufs: add missing memory barriers") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/ufs/core/ufshcd.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index a2f2941450fd..cf6a24e550f0 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4769,12 +4769,6 @@ int ufshcd_make_hba_operational(struct ufs_hba *hba) > ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr), > REG_UTP_TASK_REQ_LIST_BASE_H); > > - /* > - * Make sure base address and interrupt setup are updated before > - * enabling the run/stop registers below. > - */ > - wmb(); > - > /* > * UCRDY, UTMRLDY and UTRLRDY bits must be 1 > */ > > -- > 2.44.0 > >
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index a2f2941450fd..cf6a24e550f0 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -4769,12 +4769,6 @@ int ufshcd_make_hba_operational(struct ufs_hba *hba) ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr), REG_UTP_TASK_REQ_LIST_BASE_H); - /* - * Make sure base address and interrupt setup are updated before - * enabling the run/stop registers below. - */ - wmb(); - /* * UCRDY, UTMRLDY and UTRLRDY bits must be 1 */
Currently a wmb() is used to ensure that writes to the UTP_TASK_REQ_LIST_BASE* regs are completed prior to following writes to the run/stop registers. wmb() ensure that the write completes, but completion doesn't mean that it isn't stored in a buffer somewhere. The recommendation for ensuring the bits have 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 But, none of that is necessary here. All of the writel()/readl()'s here are to the same endpoint, so they will be ordered. There's no subsequent delay() etc that requires it to have taken effect already, so no readback is necessary here. For that reason just drop the wmb() altogether. Fixes: 897efe628d7e ("scsi: ufs: add missing memory barriers") Signed-off-by: Andrew Halaney <ahalaney@redhat.com> --- drivers/ufs/core/ufshcd.c | 6 ------ 1 file changed, 6 deletions(-)