[0/4] scsi: ufs-mediatek: Fixes for kernel v5.10

Hi Martin, Avri,

This series fix some defects and introduce host reset mechanism in MediaTek UFS platforms.
Please consider this patch series for kernel v5.10.

Thanks,

Stanley Chu

Stanley Chu (4):
  scsi: ufs-mediatek: Eliminate error message for unbound mphy
  scsi: ufs-mediatek: Fix HOST_PA_TACTIVATE quirk
  scsi: ufs-mediatek: Fix flag of unipro low-power mode
  scsi: ufs-mediatek: Add host reset mechanism

 drivers/scsi/ufs/ufs-mediatek.c | 79 ++++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufs-mediatek.h |  3 ++
 2 files changed, 67 insertions(+), 15 deletions(-)

-- 
2.18.0

Stanley,

> This series fix some defects and introduce host reset mechanism in
> MediaTek UFS platforms.  Please consider this patch series for kernel
> v5.10.

Applied to the 5.10 SCSI staging tree. Thanks!

> 
> Forcibly leave UniPro low-power mode if UIC commands is failed.
> This makes hba_enable_delay_us as correct (default) value for
> re-enabling the host.
> 
> At the same time, change type of parameter "lpm" in function
> ufs_mtk_unipro_set_pm() to "bool".
Semantically, better leave it u32 as its eventually assigned to the arg3 of the uic command 

> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufs-mediatek.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> index 887c03e8bcc0..feba74a72309 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -419,7 +419,7 @@ static int ufs_mtk_pwr_change_notify(struct ufs_hba
> *hba,
>         return ret;
>  }
> 
> -static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, u32 lpm)
> +static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, bool lpm)
>  {
>         int ret;
>         struct ufs_mtk_host *host = ufshcd_get_variant(hba);
> @@ -427,8 +427,14 @@ static int ufs_mtk_unipro_set_pm(struct ufs_hba
> *hba, u32 lpm)
>         ret = ufshcd_dme_set(hba,
>                              UIC_ARG_MIB_SEL(VS_UNIPROPOWERDOWNCONTROL, 0),
>                              lpm);
> -       if (!ret)
> +       if (!ret || !lpm) {
> +               /*
> +                * Forcibly set as non-LPM mode if UIC commands is failed
> +                * to use default hba_enable_delay_us value for re-enabling
> +                * the host.
> +                */
>                 host->unipro_lpm = lpm;
Maybe just host->unipro_lpm = false; instead

> 
> 
> Add host reset mechanism to try to recover host-side errors
> during recovery flow.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by Avri Altman <avri.altman@wdc.com>

See some nit below as well.
Thanks,
Avri

> +static void ufs_mtk_init_reset_control(struct ufs_hba *hba,
> +                                      struct reset_control **rc,
> +                                      char *str)
> +{
> +       *rc = devm_reset_control_get(hba->dev, str);
> +       if (IS_ERR(*rc)) {
How about verifying that the line is not shared as well?
Although this might not be an issue on your current platforms,
it might save you aggravation in the future..

> +               dev_info(hba->dev, "Failed to get reset control %s: %d\n",
> +                        str, PTR_ERR(*rc));
> +               *rc = NULL;
> +       }
> +}

Hi Avri,

On Sat, 2020-09-19 at 08:08 +0000, Avri Altman wrote:
> > 

> > Forcibly leave UniPro low-power mode if UIC commands is failed.

> > This makes hba_enable_delay_us as correct (default) value for

> > re-enabling the host.

> > 

> > At the same time, change type of parameter "lpm" in function

> > ufs_mtk_unipro_set_pm() to "bool".

> Semantically, better leave it u32 as its eventually assigned to the arg3 of the uic command 

> 


Thanks for reminding.
I will use specific u32 values while sending arg3 to fix this.

> > 

> > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>

> > ---

> >  drivers/scsi/ufs/ufs-mediatek.c | 20 ++++++++++++++------

> >  1 file changed, 14 insertions(+), 6 deletions(-)

> > 

> > diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c

> > index 887c03e8bcc0..feba74a72309 100644

> > --- a/drivers/scsi/ufs/ufs-mediatek.c

> > +++ b/drivers/scsi/ufs/ufs-mediatek.c

> > @@ -419,7 +419,7 @@ static int ufs_mtk_pwr_change_notify(struct ufs_hba

> > *hba,

> >         return ret;

> >  }

> > 

> > -static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, u32 lpm)

> > +static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, bool lpm)

> >  {

> >         int ret;

> >         struct ufs_mtk_host *host = ufshcd_get_variant(hba);

> > @@ -427,8 +427,14 @@ static int ufs_mtk_unipro_set_pm(struct ufs_hba

> > *hba, u32 lpm)

> >         ret = ufshcd_dme_set(hba,

> >                              UIC_ARG_MIB_SEL(VS_UNIPROPOWERDOWNCONTROL, 0),

> >                              lpm);

> > -       if (!ret)

> > +       if (!ret || !lpm) {

> > +               /*

> > +                * Forcibly set as non-LPM mode if UIC commands is failed

> > +                * to use default hba_enable_delay_us value for re-enabling

> > +                * the host.

> > +                */

> >                 host->unipro_lpm = lpm;

> Maybe just host->unipro_lpm = false; instead


This statement shall stay like this for case: "lpm = true"

Thanks,
Stanley Chu