diff mbox series

[v4] scsi: ufs-mediatek: fix ufs power down specs violation

Message ID 1620813706-25331-1-git-send-email-peter.wang@mediatek.com (mailing list archive)
State Superseded
Headers show
Series [v4] scsi: ufs-mediatek: fix ufs power down specs violation | expand

Commit Message

Peter Wang (王信友) May 12, 2021, 10:01 a.m. UTC
From: Peter Wang <peter.wang@mediatek.com>

As per specs, e.g, JESD220E chapter 7.2, while powering off
the ufs device, RST_N signal should be between VSS(Ground)
and VCCQ/VCCQ2. The power down sequence after fixing as below:

Power down:
1. Assert RST_N low
2. Turn-off VCC
3. Turn-off VCCQ/VCCQ2

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/scsi/ufs/ufs-mediatek.c |    4 ++++
 1 file changed, 4 insertions(+)

Comments

Stanley Chu May 12, 2021, 10:11 a.m. UTC | #1
Hi Peter,

On Wed, 2021-05-12 at 18:01 +0800, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> As per specs, e.g, JESD220E chapter 7.2, while powering off
> the ufs device, RST_N signal should be between VSS(Ground)
> and VCCQ/VCCQ2. The power down sequence after fixing as below:
> 
> Power down:
> 1. Assert RST_N low
> 2. Turn-off VCC
> 3. Turn-off VCCQ/VCCQ2
> 
> Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> ---
>  drivers/scsi/ufs/ufs-mediatek.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> index c55202b..47b4066 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -922,6 +922,7 @@ static void ufs_mtk_vreg_set_lpm(struct ufs_hba *hba, bool lpm)
>  static int ufs_mtk_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  {
>  	int err;
> +	struct arm_smccc_res res;
>  
>  	if (ufshcd_is_link_hibern8(hba)) {
>  		err = ufs_mtk_link_set_lpm(hba);
> @@ -941,6 +942,9 @@ static int ufs_mtk_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  			goto fail;
>  	}
>  
> +	if (ufshcd_is_link_off(hba))
> +		ufs_mtk_device_reset_ctrl(0, res);
> +

For the case "linkoff @ suspend", HW reset pin can be de-asserted via
ufshcd_resume() -> ufshcd_reset_and_restore() ->
ufshcd_vops_device_reset(), so this looks good to me.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Avri Altman May 13, 2021, 6:45 a.m. UTC | #2
> From: Peter Wang <peter.wang@mediatek.com>
> 
> As per specs, e.g, JESD220E chapter 7.2, while powering off
> the ufs device, RST_N signal should be between VSS(Ground)
> and VCCQ/VCCQ2. The power down sequence after fixing as below:
> 
> Power down:
> 1. Assert RST_N low
> 2. Turn-off VCC
> 3. Turn-off VCCQ/VCCQ2
> 
> Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> ---
>  drivers/scsi/ufs/ufs-mediatek.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> index c55202b..47b4066 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -922,6 +922,7 @@ static void ufs_mtk_vreg_set_lpm(struct ufs_hba
> *hba, bool lpm)
>  static int ufs_mtk_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  {
>         int err;
> +       struct arm_smccc_res res;
> 
>         if (ufshcd_is_link_hibern8(hba)) {
>                 err = ufs_mtk_link_set_lpm(hba);
> @@ -941,6 +942,9 @@ static int ufs_mtk_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>                         goto fail;
>         }
> 
> +       if (ufshcd_is_link_off(hba))
> +               ufs_mtk_device_reset_ctrl(0, res);
According to your commit log, you should call reset just before 
ufs_mtk_vreg_set_lpm, or turn phy off, whichever turn off vcc - 
Few lines above.

Thanks,
Avri

> +
>         return 0;
>  fail:
>         /*
> --
> 1.7.9.5
Peter Wang (王信友) May 13, 2021, 7:58 a.m. UTC | #3
On Thu, 2021-05-13 at 06:45 +0000, Avri Altman wrote:
> > From: Peter Wang <peter.wang@mediatek.com>
> > 
> > As per specs, e.g, JESD220E chapter 7.2, while powering off
> > the ufs device, RST_N signal should be between VSS(Ground)
> > and VCCQ/VCCQ2. The power down sequence after fixing as below:
> > 
> > Power down:
> > 1. Assert RST_N low
> > 2. Turn-off VCC
> > 3. Turn-off VCCQ/VCCQ2
> > 
> > Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> > ---
> >  drivers/scsi/ufs/ufs-mediatek.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> > index c55202b..47b4066 100644
> > --- a/drivers/scsi/ufs/ufs-mediatek.c
> > +++ b/drivers/scsi/ufs/ufs-mediatek.c
> > @@ -922,6 +922,7 @@ static void ufs_mtk_vreg_set_lpm(struct ufs_hba
> > *hba, bool lpm)
> >  static int ufs_mtk_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >  {
> >         int err;
> > +       struct arm_smccc_res res;
> > 
> >         if (ufshcd_is_link_hibern8(hba)) {
> >                 err = ufs_mtk_link_set_lpm(hba);
> > @@ -941,6 +942,9 @@ static int ufs_mtk_suspend(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> >                         goto fail;
> >         }
> > 
> > +       if (ufshcd_is_link_off(hba))
> > +               ufs_mtk_device_reset_ctrl(0, res);
> According to your commit log, you should call reset just before 
> ufs_mtk_vreg_set_lpm, or turn phy off, whichever turn off vcc - 
> Few lines above.
> 
> Thanks,
> Avri

ufs_mtk_vreg_set_lpm only set vccq2 power mode to lpm, dose't trun off
vccq2.And turn phy off is also turn off vcc, dose't turn off vccq2.
RST_N keep high is no problem when we trun off vcc and vccq2 keep on.
But RST_N will got problem if we trun off vccq2.
(RST_N signal should be between VSS(Ground) and VCCQ/VCCQ2)
Here set RST_N to low is after shut down pm set link off, and before
shutdown pm turn off vccq2.

Thanks
Peter

> 
> > +
> >         return 0;
> >  fail:
> >         /*
> > --
> > 1.7.9.5
>
Avri Altman May 13, 2021, 8:15 a.m. UTC | #4
> > > +       if (ufshcd_is_link_off(hba))
> > > +               ufs_mtk_device_reset_ctrl(0, res);
> > According to your commit log, you should call reset just before
> > ufs_mtk_vreg_set_lpm, or turn phy off, whichever turn off vcc -
> > Few lines above.
> >
> > Thanks,
> > Avri
> 
> ufs_mtk_vreg_set_lpm only set vccq2 power mode to lpm, dose't trun off
> vccq2.And turn phy off is also turn off vcc, dose't turn off vccq2.
> RST_N keep high is no problem when we trun off vcc and vccq2 keep on.
> But RST_N will got problem if we trun off vccq2.
> (RST_N signal should be between VSS(Ground) and VCCQ/VCCQ2)
> Here set RST_N to low is after shut down pm set link off, and before
> shutdown pm turn off vccq2.
OK. Thanks for clarifying this.

Thanks,
Avri

> 
> Thanks
> Peter
> 
> >
> > > +
> > >         return 0;
> > >  fail:
> > >         /*
> > > --
> > > 1.7.9.5
> >
Martin K. Petersen May 15, 2021, 3:13 a.m. UTC | #5
On Wed, 12 May 2021 18:01:45 +0800, peter.wang@mediatek.com wrote:

> As per specs, e.g, JESD220E chapter 7.2, while powering off
> the ufs device, RST_N signal should be between VSS(Ground)
> and VCCQ/VCCQ2. The power down sequence after fixing as below:
> 
> Power down:
> 1. Assert RST_N low
> 2. Turn-off VCC
> 3. Turn-off VCCQ/VCCQ2

Applied to 5.13/scsi-fixes, thanks!

[1/1] scsi: ufs-mediatek: fix ufs power down specs violation
      https://git.kernel.org/mkp/scsi/c/c625b80b9d00
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index c55202b..47b4066 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -922,6 +922,7 @@  static void ufs_mtk_vreg_set_lpm(struct ufs_hba *hba, bool lpm)
 static int ufs_mtk_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 {
 	int err;
+	struct arm_smccc_res res;
 
 	if (ufshcd_is_link_hibern8(hba)) {
 		err = ufs_mtk_link_set_lpm(hba);
@@ -941,6 +942,9 @@  static int ufs_mtk_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 			goto fail;
 	}
 
+	if (ufshcd_is_link_off(hba))
+		ufs_mtk_device_reset_ctrl(0, res);
+
 	return 0;
 fail:
 	/*