Re: [PATCH v2 0/3] scsi: ufs: Add error handling of Auto-Hibernate
Hi Avri, Alim, Pedro, Gentle ping for this patch. On Wed, 2019-05-15 at 17:36 +0800, Stanley Chu wrote: > Currently auto-hibernate is activated if host supports > auto-hibern8 capability. However error-handling is not implemented, > which makes the feature somewhat risky. > > If either "Hibernate Enter" or "Hibernate Exit" fail during > auto-hibernate flow, the corresponding interrupt > "UIC_HIBERNATE_ENTER" or "UIC_HIBERNATE_EXIT" shall be raised > according to UFS specification. > > This patch adds auto-hibernate error-handling: > > - Monitor "Hibernate Enter" and "Hibernate Exit" interrupts after > auto-hibernate feature is activated. > > - If fail happens, trigger error-handling just like "manual-hibernate" > fail and apply the same recovery flow: schedule UFS error handler in > ufshcd_check_errors(), and then do host reset and restore > in UFS error handler. > > v2: > - Fix sentences in commit message (Marc Gonzalez) > - Make "Auto-Hibernate" error detection more precise (Bean Huo) > > Stanley Chu (3): > scsi: ufs: Do not overwrite Auto-Hibernate timer > scsi: ufs: Add error-handling of Auto-Hibernate > scsi: ufs: Use re-factored Auto-Hibernate function > > drivers/scsi/ufs/ufshcd.c | 33 - > drivers/scsi/ufs/ufshcd.h | 5 + > drivers/scsi/ufs/ufshci.h | 3 +++ > 3 files changed, 40 insertions(+), 1 deletion(-) > Thanks, Stanley
Re: [PATCH v2 1/3] scsi: ufs: Do not overwrite Auto-Hibernate timer
Hello Stanley,
On 5/15/19 3:06 PM, Stanley Chu wrote:
> Some vendor-specific initialization flow may set its own
> auto-hibernate timer. In this case, do not overwrite timer value
> as "default value" in ufshcd_init().
>
> Signed-off-by: Stanley Chu
> ---
> drivers/scsi/ufs/ufshcd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e040f9dd9ff3..1665820c22fd 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8309,7 +8309,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> *mmio_base, unsigned int irq)
> UIC_LINK_HIBERN8_STATE);
>
> /* Set the default auto-hiberate idle timer value to 150 ms */
> - if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT) {
> + if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT & !hba->ahit) {
> hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK, 150) |
> FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, 3);
> }
>
Looks good to me,
Reviewed-by: Alim Akhtar
RE: [PATCH v2 1/3] scsi: ufs: Do not overwrite Auto-Hibernate timer
>
> Hello Stanley,
>
> On 5/15/19 3:06 PM, Stanley Chu wrote:
> > Some vendor-specific initialization flow may set its own
> > auto-hibernate timer. In this case, do not overwrite timer value
> > as "default value" in ufshcd_init().
> >
> > Signed-off-by: Stanley Chu
> > ---
> > drivers/scsi/ufs/ufshcd.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index e040f9dd9ff3..1665820c22fd 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -8309,7 +8309,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> *mmio_base, unsigned int irq)
> > UIC_LINK_HIBERN8_STATE);
> >
> > /* Set the default auto-hiberate idle timer value to 150 ms */
> > - if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT) {
> > + if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT & !hba->ahit) {
A typo?
> > hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK,
> 150) |
> > FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, 3);
> > }
> >
> Looks good to me,
> Reviewed-by: Alim Akhtar
RE: [PATCH v2 1/3] scsi: ufs: Do not overwrite Auto-Hibernate timer
Hi Avri,
On Mon, 2019-05-20 at 05:47 +, Avri Altman wrote:
> >
> > Hello Stanley,
> >
> > On 5/15/19 3:06 PM, Stanley Chu wrote:
> > > Some vendor-specific initialization flow may set its own
> > > auto-hibernate timer. In this case, do not overwrite timer value
> > > as "default value" in ufshcd_init().
> > >
> > > Signed-off-by: Stanley Chu
> > > ---
> > > drivers/scsi/ufs/ufshcd.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index e040f9dd9ff3..1665820c22fd 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -8309,7 +8309,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> > *mmio_base, unsigned int irq)
> > > UIC_LINK_HIBERN8_STATE);
> > >
> > > /* Set the default auto-hiberate idle timer value to 150 ms */
> > > - if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT) {
> > > + if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT & !hba->ahit) {
> A typo?
Yes! Thanks for remind this.
Will fix it in next version.
>
>
> > > hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK,
> > 150) |
> > > FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, 3);
> > > }
> > >
> > Looks good to me,
> > Reviewed-by: Alim Akhtar
Thanks.
Stanley
Re: [PATCH v2 2/3] scsi: ufs: Add error-handling of Auto-Hibernate
Hi Stanley,
On 5/15/19 3:06 PM, Stanley Chu wrote:
> Currently auto-hibernate is activated if host supports
> auto-hibern8 capability. However error-handling is not implemented,
> which makes the feature somewhat risky.
>
> If either "Hibernate Enter" or "Hibernate Exit" fail during
> auto-hibernate flow, the corresponding interrupt
> "UIC_HIBERNATE_ENTER" or "UIC_HIBERNATE_EXIT" shall be raised
> according to UFS specification.
>
> This patch adds auto-hibernate error-handling:
>
> - Monitor "Hibernate Enter" and "Hibernate Exit" interrupts after
>auto-hibernate feature is activated.
>
> - If fail happens, trigger error-handling just like "manual-hibernate"
>fail and apply the same recovery flow: schedule UFS error handler in
>ufshcd_check_errors(), and then do host reset and restore
>in UFS error handler.
>
> Signed-off-by: Stanley Chu
> ---
> drivers/scsi/ufs/ufshcd.c | 31 +++
> drivers/scsi/ufs/ufshcd.h | 5 +
> drivers/scsi/ufs/ufshci.h | 3 +++
> 3 files changed, 39 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 1665820c22fd..e6a86223a0d4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5254,6 +5254,7 @@ static void ufshcd_err_handler(struct work_struct *work)
> goto skip_err_handling;
> }
> if ((hba->saved_err & INT_FATAL_ERRORS) ||
> + (hba->saved_err & UFSHCD_UIC_AH8_ERROR_MASK) ||
> ((hba->saved_err & UIC_ERROR) &&
> (hba->saved_uic_err & (UFSHCD_UIC_DL_PA_INIT_ERROR |
> UFSHCD_UIC_DL_NAC_RECEIVED_ERROR |
> @@ -5413,6 +5414,23 @@ static void ufshcd_update_uic_error(struct ufs_hba
> *hba)
> __func__, hba->uic_error);
> }
>
> +static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
> + u32 intr_mask)
> +{
> + if (!ufshcd_is_auto_hibern8_supported(hba))
> + return false;
> +
> + if (!(intr_mask & UFSHCD_UIC_AH8_ERROR_MASK))
> + return false;
> +
> + if (hba->active_uic_cmd &&
> + (hba->active_uic_cmd->command == UIC_CMD_DME_HIBER_ENTER ||
> + hba->active_uic_cmd->command == UIC_CMD_DME_HIBER_EXIT))
> + return false;
> +
> + return true;
> +}
> +
> /**
>* ufshcd_check_errors - Check for errors that need s/w attention
>* @hba: per-adapter instance
> @@ -5431,6 +5449,15 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
> queue_eh_work = true;
> }
>
> + if (hba->errors & UFSHCD_UIC_AH8_ERROR_MASK) {
> + dev_err(hba->dev,
> + "%s: Auto Hibern8 %s failed - status: 0x%08x, upmcrs:
> 0x%08x\n",
> + __func__, (hba->errors & UIC_HIBERNATE_ENTER) ?
> + "Enter" : "Exit",
> + hba->errors, ufshcd_get_upmcrs(hba));
> + queue_eh_work = true;
> + }
> +
> if (queue_eh_work) {
> /*
>* update the transfer error masks to sticky bits, let's do this
> @@ -5493,6 +5520,10 @@ static void ufshcd_tmc_handler(struct ufs_hba *hba)
> static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> {
> hba->errors = UFSHCD_ERROR_MASK & intr_status;
> +
> + if (ufshcd_is_auto_hibern8_error(hba, intr_status))
> + hba->errors |= (UFSHCD_UIC_AH8_ERROR_MASK & intr_status);
> +
> if (hba->errors)
> ufshcd_check_errors(hba);
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index ecfa898b9ccc..994d73d03207 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -740,6 +740,11 @@ return true;
> #endif
> }
>
> +static inline bool ufshcd_is_auto_hibern8_supported(struct ufs_hba *hba)
> +{
> + return (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT);
> +}
> +
> #define ufshcd_writel(hba, val, reg)\
> writel((val), (hba)->mmio_base + (reg))
> #define ufshcd_readl(hba, reg) \
> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
> index 6fa889de5ee5..4bcb205f2077 100644
> --- a/drivers/scsi/ufs/ufshci.h
> +++ b/drivers/scsi/ufs/ufshci.h
> @@ -148,6 +148,9 @@ enum {
> UIC_HIBERNATE_EXIT |\
> UIC_POWER_MODE)
>
> +#define UFSHCD_UIC_AH8_ERROR_MASK(UIC_HIBERNATE_ENTER |\
> + UIC_HIBERNATE_EXIT)
> +
> #define UFSHCD_UIC_MASK (UIC_COMMAND_COMPL |
> UFSHCD_UIC_PWR_MASK)
>
> #define UFSHCD_ERROR_MASK (UIC_ERROR |\
>
I don't have a way to test this patch, as my current platform does not
support Auto Hibern8, over all this look ok to me.
Thanks,
Reviewed-by: Alim Akhtar
RE: [PATCH v1 0/3] scsi: ufs: add error handlings of auto-hibern8
> > On 13/05/2019 16:36, Stanley Chu wrote: > > > Currently auto-hibern8 is activated if host supports > > auto-hibern8 capability. However no error handlings are existed thus > > this feature is kind of risky. > > This last sentence is not very idiomatic. > > I would suggest: > "However, error-handling is not implemented, which makes the feature > somewhat risky." > > > If "Hibernate Enter" or "Hibernate Exit" fail happens > > I would suggest: > If either "Hibernate Enter" or "Hibernate Exit" fail during ... > > > during auto-hibern8 flow, the corresponding interrupt > > "UIC_HIBERNATE_ENTER" or "UIC_HIBERNATE_EXIT" shall be raised > > according to UFS specification. > > > > This patch adds auto-hibern8 error handlings: > > error-handling > > > - Monitor "Hibernate Enter" and "Hibernate Exit" interrupts after > > auto-hibern8 feature is activated. > > I just want to take this opportunity to ask a rhetorical question. > > Who in the Great Heavens thought it would be a good idea to call the > feature "auto-hibern8" ? > > Was it really worth it to save 2 characters by writing "8" instead > of "ate" ? > > This bugs me so much that I just might send a patch to fix it up. As strange as it may be, this is not the product of the creative mind Of the original driver's authors, nor even JEDEC guys which uses it in their specs (both UFS & HCI). This strange amalgam dates back to the mipi-unipro terminology. Thanks, Avri
Re: [PATCH v2 3/3] scsi: ufs: Use re-factored Auto-Hibernate function
On 5/15/19 3:06 PM, Stanley Chu wrote:
> Use re-factored ufshcd_is_auto_hibern8_supported() function
> in ufshcd_init() instead to make code more cleaner.
>
> Signed-off-by: Stanley Chu
> ---
> drivers/scsi/ufs/ufshcd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e6a86223a0d4..17af157c2cc1 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8340,7 +8340,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> *mmio_base, unsigned int irq)
> UIC_LINK_HIBERN8_STATE);
>
> /* Set the default auto-hiberate idle timer value to 150 ms */
> - if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT & !hba->ahit) {
> + if (ufshcd_is_auto_hibern8_supported(hba) & !hba->ahit) {
After fixing the typo in above line (as pointed by Avri in patch 1),
feel free to add
Reviewed-by: Alim Akhtar
> hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK, 150) |
> FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, 3);
> }
>
