| Message ID | 20241016211154.2425403-7-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | UFS driver fixes and cleanups | expand |
> - /* Poll SQRTSy.CUS = 1. Return result from SQRTSy.RTC */ > - reg = opr_sqd_base + REG_SQRTS; > - err = read_poll_timeout(readl, val, val & SQ_CUS, 20, > - MCQ_POLL_US, false, reg); > + /* Wait until SQRTSy.CUS = 1. */ > + err = read_poll_timeout(readl, val, val & SQ_CUS, 20, MCQ_POLL_US, > + false, opr_sqd_base + REG_SQRTS); > if (err) Can remove the if (err) > - dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%ld\n", > - __func__, id, task_tag, > - FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg))); > + dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%d\n", > + __func__, id, task_tag, err); And report RTC on success or err otherwise: + __func__, id, task_tag, err ? : FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(opr_sqd_base + REG_SQRTS)); Thanks, Avri > > if (ufshcd_mcq_sq_start(hba, hwq)) > err = -ETIMEDOUT;
On 10/16/24 11:55 PM, Avri Altman wrote: > >> - /* Poll SQRTSy.CUS = 1. Return result from SQRTSy.RTC */ >> - reg = opr_sqd_base + REG_SQRTS; >> - err = read_poll_timeout(readl, val, val & SQ_CUS, 20, >> - MCQ_POLL_US, false, reg); >> + /* Wait until SQRTSy.CUS = 1. */ >> + err = read_poll_timeout(readl, val, val & SQ_CUS, 20, MCQ_POLL_US, >> + false, opr_sqd_base + REG_SQRTS); >> if (err) > Can remove the if (err) > >> - dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%ld\n", >> - __func__, id, task_tag, >> - FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg))); >> + dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%d\n", >> + __func__, id, task_tag, err); > And report RTC on success or err otherwise: > + __func__, id, task_tag, err ? : FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(opr_sqd_base + REG_SQRTS)); Hi Avri, From the UFSCHI standard about RTC: 0 : Success 1 : Fail – Task Not found 2 : Fail – SQ not stopped 3 : Fail – SQ is disabled Others : Reserved Do you agree with changing ufshcd_mcq_sq_cleanup() such that it fails if RTC is not zero? Thanks, Bart.
> On 10/16/24 11:55 PM, Avri Altman wrote: > > > >> - /* Poll SQRTSy.CUS = 1. Return result from SQRTSy.RTC */ > >> - reg = opr_sqd_base + REG_SQRTS; > >> - err = read_poll_timeout(readl, val, val & SQ_CUS, 20, > >> - MCQ_POLL_US, false, reg); > >> + /* Wait until SQRTSy.CUS = 1. */ > >> + err = read_poll_timeout(readl, val, val & SQ_CUS, 20, MCQ_POLL_US, > >> + false, opr_sqd_base + REG_SQRTS); > >> if (err) > > Can remove the if (err) > > > >> - dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%ld\n", > >> - __func__, id, task_tag, > >> - FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg))); > >> + dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%d\n", > >> + __func__, id, task_tag, err); > > And report RTC on success or err otherwise: > > + __func__, id, task_tag, err ? : > > + FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(opr_sqd_base + > REG_SQRTS)); > > Hi Avri, > > From the UFSCHI standard about RTC: > > 0 : Success > 1 : Fail – Task Not found > 2 : Fail – SQ not stopped > 3 : Fail – SQ is disabled > Others : Reserved > > Do you agree with changing ufshcd_mcq_sq_cleanup() such that it fails if RTC > is not zero? I was just pointing out that after your change, the extra info of RTC will no longer be available, And proposed a way in which we can still retain it. Thanks, Avri > > Thanks, > > Bart.
On 10/17/24 11:07 AM, Avri Altman wrote: > I was just pointing out that after your change, the extra info of RTC will no longer be available, > And proposed a way in which we can still retain it. Something like this change? diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 57ced1729b73..988400500560 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -572,14 +572,18 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag) /* SQRTCy.ICU = 1 */ writel(SQ_ICU, opr_sqd_base + REG_SQRTC); - /* Poll SQRTSy.CUS = 1. Return result from SQRTSy.RTC */ + /* Wait until SQRTSy.CUS = 1. Report SQRTSy.RTC. */ reg = opr_sqd_base + REG_SQRTS; err = read_poll_timeout(readl, val, val & SQ_CUS, 20, MCQ_POLL_US, false, reg); if (err) - dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%ld\n", - __func__, id, task_tag, - FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg))); + dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%d\n", + __func__, id, task_tag, err); + else + dev_info(hba->dev, + "%s, hwq %d: cleanup return code (RTC) %ld\n", + __func__, id, + FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg))); if (ufshcd_mcq_sq_start(hba, hwq)) err = -ETIMEDOUT; Thanks, Bart.
> > On 10/17/24 11:07 AM, Avri Altman wrote: > > I was just pointing out that after your change, the extra info of RTC > > will no longer be available, And proposed a way in which we can still retain > it. > > Something like this change? > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index > 57ced1729b73..988400500560 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -572,14 +572,18 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, > int > task_tag) > /* SQRTCy.ICU = 1 */ > writel(SQ_ICU, opr_sqd_base + REG_SQRTC); > > - /* Poll SQRTSy.CUS = 1. Return result from SQRTSy.RTC */ > + /* Wait until SQRTSy.CUS = 1. Report SQRTSy.RTC. */ > reg = opr_sqd_base + REG_SQRTS; > err = read_poll_timeout(readl, val, val & SQ_CUS, 20, > MCQ_POLL_US, false, reg); > if (err) > - dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%ld\n", > - __func__, id, task_tag, > - FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg))); > + dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%d\n", > + __func__, id, task_tag, err); > + else > + dev_info(hba->dev, > + "%s, hwq %d: cleanup return code (RTC) %ld\n", > + __func__, id, > + FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg))); Yes. Thanks, Avri > > if (ufshcd_mcq_sq_start(hba, hwq)) > err = -ETIMEDOUT; > > Thanks, > > Bart.
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 57ced1729b73..bef4c53f9c06 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -538,7 +538,7 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag) struct ufshcd_lrb *lrbp = &hba->lrb[task_tag]; struct scsi_cmnd *cmd = lrbp->cmd; struct ufs_hw_queue *hwq; - void __iomem *reg, *opr_sqd_base; + void __iomem *opr_sqd_base; u32 nexus, id, val; int err; @@ -572,14 +572,12 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag) /* SQRTCy.ICU = 1 */ writel(SQ_ICU, opr_sqd_base + REG_SQRTC); - /* Poll SQRTSy.CUS = 1. Return result from SQRTSy.RTC */ - reg = opr_sqd_base + REG_SQRTS; - err = read_poll_timeout(readl, val, val & SQ_CUS, 20, - MCQ_POLL_US, false, reg); + /* Wait until SQRTSy.CUS = 1. */ + err = read_poll_timeout(readl, val, val & SQ_CUS, 20, MCQ_POLL_US, + false, opr_sqd_base + REG_SQRTS); if (err) - dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%ld\n", - __func__, id, task_tag, - FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg))); + dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%d\n", + __func__, id, task_tag, err); if (ufshcd_mcq_sq_start(hba, hwq)) err = -ETIMEDOUT;
From the UFSHCI specification: "CleanUp Command Return Code (RTC): host controller sets this return code to provide more details of the cleanup process. It is valid only when CUS is 1." Hence, do not read RTC if the CUS bitfield is zero. Cc: Bao D. Nguyen <quic_nguyenb@quicinc.com> Fixes: 8d7290348992 ("scsi: ufs: mcq: Add supporting functions for MCQ abort") Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufs-mcq.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)