| Message ID | 20241016211154.2425403-4-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UFS driver fixes and cleanups | expand |
> - /* Single Doorbell Mode */ > - reg = ufshcd_readl(hba, > REG_UTP_TRANSFER_REQ_DOOR_BELL); > - if (reg & (1 << tag)) { > - /* sleep for max. 200us to stabilize */ > - usleep_range(100, 200); > - continue; > - } If we no longer use the doorbell to determine the inflight requests, I think this should be your patch title, or at least a clear indication in your commit log. Thanks, Avri
On 10/16/24 11:31 PM, Avri Altman wrote: >> - /* Single Doorbell Mode */ >> - reg = ufshcd_readl(hba, >> REG_UTP_TRANSFER_REQ_DOOR_BELL); >> - if (reg & (1 << tag)) { >> - /* sleep for max. 200us to stabilize */ >> - usleep_range(100, 200); >> - continue; >> - } > > If we no longer use the doorbell to determine the inflight requests, > I think this should be your patch title, or at least a clear indication in your commit log. Hmm ... I see this as an implementation detail. To me, the key aspect of this patch is that the legacy and MCQ mode code paths are combined into a single code path. Thanks, Bart.
On Wed, 2024-10-16 at 14:11 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > The MCQ code is also valid for legacy mode. Hence, remove the legacy > mode code and retain the MCQ code. Since it is not an error if a > command > completes while ufshcd_try_to_abort_task() is in progress, use > dev_info() > instead of dev_err() to report this. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufshcd.c | 32 ++++++++------------------------ > 1 file changed, 8 insertions(+), 24 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > Hi Bart, As the DBR clear event occurs before !ufshcd_cmd_inflight, this could potentially cause an additional usleep_range wait. If an additional wait of 100~200 us is not a concern, then I think it should be fine. Thanks Peter
On 10/21/24 1:58 AM, Peter Wang (王信友) wrote: > As the DBR clear event occurs before !ufshcd_cmd_inflight, > this could potentially cause an additional usleep_range wait. > If an additional wait of 100~200 us is not a concern, then > I think it should be fine. Hi Peter, ufshcd_try_to_abort_task() is typically called if no completion will be received from the UFS device. I think that the potential additional loop iteration is acceptable since it is rare that a UFS device reports a completion while ufshcd_try_to_abort_task() is in progress and since this is an error path and not the hot path. Thanks, Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 57ce1910fda0..76884df580c3 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -7489,7 +7489,6 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag) int err; int poll_cnt; u8 resp = 0xF; - u32 reg; for (poll_cnt = 100; poll_cnt; poll_cnt--) { err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, @@ -7504,32 +7503,17 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag) * cmd not pending in the device, check if it is * in transition. */ - dev_err(hba->dev, "%s: cmd at tag %d not pending in the device.\n", + dev_info( + hba->dev, + "%s: cmd with tag %d not pending in the device.\n", __func__, tag); - if (hba->mcq_enabled) { - /* MCQ mode */ - if (ufshcd_cmd_inflight(lrbp->cmd)) { - /* sleep for max. 200us same delay as in SDB mode */ - usleep_range(100, 200); - continue; - } - /* command completed already */ - dev_err(hba->dev, "%s: cmd at tag=%d is cleared.\n", - __func__, tag); + if (!ufshcd_cmd_inflight(lrbp->cmd)) { + dev_info(hba->dev, + "%s: cmd with tag=%d completed.\n", + __func__, tag); return 0; } - - /* Single Doorbell Mode */ - reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); - if (reg & (1 << tag)) { - /* sleep for max. 200us to stabilize */ - usleep_range(100, 200); - continue; - } - /* command completed already */ - dev_err(hba->dev, "%s: cmd at tag %d successfully cleared from DB.\n", - __func__, tag); - return 0; + usleep_range(100, 200); } else { dev_err(hba->dev, "%s: no response from device. tag = %d, err %d\n",
The MCQ code is also valid for legacy mode. Hence, remove the legacy mode code and retain the MCQ code. Since it is not an error if a command completes while ufshcd_try_to_abort_task() is in progress, use dev_info() instead of dev_err() to report this. Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufshcd.c | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-)