| Message ID | 9c7ccbfb8fe05c29ab3e31d9cd14e6b91065b8b0.1690939662.git.kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | change UIC command handling | expand |
On 2/08/23 04:28, Kiwoong Kim wrote: > With auto hibern8 enabled, UIC could be working > for a while to process a hibern8 operation and HCI > reports UIC not ready for a short term through HCS.UCRDY. > And UFS driver can't recognize the operation. > UFSHCI spec specifies UCRDY like this: > whether the host controller is ready to process UIC COMMAND > > The 'ready' could be seen as many different meanings. If the meaning > includes not processing any request from HCI, processing a hibern8 > operation can be 'not ready'. In this situation, the driver needs to > wait until the operations is completed. > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > --- > drivers/ufs/core/ufshcd.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index a89d39a..10ccc85 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -22,6 +22,7 @@ > #include <linux/module.h> > #include <linux/regulator/consumer.h> > #include <linux/sched/clock.h> > +#include <linux/iopoll.h> > #include <scsi/scsi_cmnd.h> > #include <scsi/scsi_dbg.h> > #include <scsi/scsi_driver.h> > @@ -2365,7 +2366,11 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba) > */ > static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba) > { > - return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY; > + u32 val; > + int ret = read_poll_timeout(ufshcd_readl, val, val & UIC_COMMAND_READY, > + 500, UIC_CMD_TIMEOUT * 1000, false, hba, > + REG_CONTROLLER_STATUS); > + return ret == 0 ? true : false; Could use a comment in the code. And perhaps the following is neater: u32 val; return !read_poll_timeout(ufshcd_readl, val, val & UIC_COMMAND_READY, 500, UIC_CMD_TIMEOUT * 1000, false, hba, REG_CONTROLLER_STATUS); > } > > /**
On 8/14/23 04:26, Adrian Hunter wrote: > And perhaps the following is neater: > > u32 val; > > return !read_poll_timeout(ufshcd_readl, val, val & UIC_COMMAND_READY, > 500, UIC_CMD_TIMEOUT * 1000, false, hba, > REG_CONTROLLER_STATUS); Would the above make readers of that code wonder whether read_poll_timeout() perhaps returns a boolean? Wouldn't it be better to test the read_poll_timeout() return value as follows? return read_poll_timeout(ufshcd_readl, val, val & UIC_COMMAND_READY, 500, UIC_CMD_TIMEOUT * 1000, false, hba, REG_CONTROLLER_STATUS) == 0; Thanks, Bart.
On 17/08/23 18:02, Bart Van Assche wrote: > On 8/14/23 04:26, Adrian Hunter wrote: >> And perhaps the following is neater: >> >> u32 val; >> >> return !read_poll_timeout(ufshcd_readl, val, val & UIC_COMMAND_READY, >> 500, UIC_CMD_TIMEOUT * 1000, false, hba, >> REG_CONTROLLER_STATUS); > > Would the above make readers of that code wonder whether read_poll_timeout() > perhaps returns a boolean? Wouldn't it be better to test the > read_poll_timeout() return value as follows? > > return read_poll_timeout(ufshcd_readl, val, val & UIC_COMMAND_READY, > 500, UIC_CMD_TIMEOUT * 1000, false, hba, > REG_CONTROLLER_STATUS) == 0; > Either is fine, otherwise: Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index a89d39a..10ccc85 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -22,6 +22,7 @@ #include <linux/module.h> #include <linux/regulator/consumer.h> #include <linux/sched/clock.h> +#include <linux/iopoll.h> #include <scsi/scsi_cmnd.h> #include <scsi/scsi_dbg.h> #include <scsi/scsi_driver.h> @@ -2365,7 +2366,11 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba) */ static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba) { - return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY; + u32 val; + int ret = read_poll_timeout(ufshcd_readl, val, val & UIC_COMMAND_READY, + 500, UIC_CMD_TIMEOUT * 1000, false, hba, + REG_CONTROLLER_STATUS); + return ret == 0 ? true : false; } /**
With auto hibern8 enabled, UIC could be working for a while to process a hibern8 operation and HCI reports UIC not ready for a short term through HCS.UCRDY. And UFS driver can't recognize the operation. UFSHCI spec specifies UCRDY like this: whether the host controller is ready to process UIC COMMAND The 'ready' could be seen as many different meanings. If the meaning includes not processing any request from HCI, processing a hibern8 operation can be 'not ready'. In this situation, the driver needs to wait until the operations is completed. Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> --- drivers/ufs/core/ufshcd.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)