| Message ID | 20241016211154.2425403-1-bvanassche@acm.org |
|---|---|
| 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: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 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.
Hi Martin, This patch series includes several fixes and cleanup patches for the UFS driver. Please consider this patch series for the next merge window. Thanks, Bart. Bart Van Assche (7): scsi: ufs: core: Move the ufshcd_mcq_enable_esi() definition scsi: ufs: core: Remove goto statements from ufshcd_try_to_abort_task() scsi: ufs: core: Simplify ufshcd_try_to_abort_task() scsi: ufs: core: Fix ufshcd_exception_event_handler() scsi: ufs: core: Simplify ufshcd_err_handling_prepare() scsi: ufs: core: Fix ufshcd_mcq_sq_cleanup() scsi: ufs: core: Make DMA mask configuration more flexible drivers/ufs/core/ufs-mcq.c | 28 ++++++------- drivers/ufs/core/ufshcd.c | 76 ++++++++++------------------------ drivers/ufs/host/ufs-renesas.c | 9 +++- include/ufs/ufshcd.h | 13 ++---- 4 files changed, 45 insertions(+), 81 deletions(-)