[RFC PATCH v1] ufs: relocate turning off device power sources

[RFC PATCH v1] ufs: relocate turning off device power sources

[Kiwoong Kim]

UFS specification says that while powering off the device,
RST_n signal and REF_CLK signal should be between VSS and VCCQ.
One of ways to make it is to drive both RST_n and REF_CLK to low.

However, the current location of turning off them doesn't
guarantee that. ufshcd_link_state_transition make enter hibern8
but it's not supposed to adjust the level of REF_CLK. Adding
ufshcd_vops_device_reset isn't proper because it just drives the
level of RST_n to low and then to high, not keep low.
In this situation, it could make those levels be
diverged from those proper ranges with actual hardware situations,
especially right when the driver turns off power.

The easist way to make it is just to move the location of turning
off because lowering the levels can be enabled through
the callbacks named suspend and setup_clocks.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 92d433d..dab9840 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8633,8 +8633,6 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	if (ret)
 		goto set_dev_active;
 
-	ufshcd_vreg_set_lpm(hba);
-
 disable_clks:
 	/*
 	 * Call vendor specific suspend callback. As these callbacks may access
@@ -8662,6 +8660,13 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 					hba->clk_gating.state);
 	}
 
+	/*
+	 * It should follows driving RST_n and REF_CLK
+	 * in the range specified in UFS specification
+	 */
+	if (!ufshcd_is_ufs_dev_active(hba))
+		ufshcd_vreg_set_lpm(hba);
+
 	/* Put the host controller in low power mode if possible */
 	ufshcd_hba_vreg_set_lpm(hba);
 	goto out;
-- 
2.7.4

Re: [RFC PATCH v1] ufs: relocate turning off device power sources

[Can Guo]

On 2020-12-19 14:27, Kiwoong Kim wrote:
> UFS specification says that while powering off the device,
> RST_n signal and REF_CLK signal should be between VSS and VCCQ.
> One of ways to make it is to drive both RST_n and REF_CLK to low.
> 
> However, the current location of turning off them doesn't
> guarantee that. ufshcd_link_state_transition make enter hibern8
> but it's not supposed to adjust the level of REF_CLK. Adding
> ufshcd_vops_device_reset isn't proper because it just drives the
> level of RST_n to low and then to high, not keep low.
> In this situation, it could make those levels be
> diverged from those proper ranges with actual hardware situations,
> especially right when the driver turns off power.
> 
> The easist way to make it is just to move the location of turning
> off because lowering the levels can be enabled through
> the callbacks named suspend and setup_clocks.
> 
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 92d433d..dab9840 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8633,8 +8633,6 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	if (ret)
>  		goto set_dev_active;
> 
> -	ufshcd_vreg_set_lpm(hba);
> -
>  disable_clks:
>  	/*
>  	 * Call vendor specific suspend callback. As these callbacks may 
> access
> @@ -8662,6 +8660,13 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  					hba->clk_gating.state);
>  	}
> 
> +	/*
> +	 * It should follows driving RST_n and REF_CLK
> +	 * in the range specified in UFS specification
> +	 */
> +	if (!ufshcd_is_ufs_dev_active(hba))
> +		ufshcd_vreg_set_lpm(hba);
> +
>  	/* Put the host controller in low power mode if possible */
>  	ufshcd_hba_vreg_set_lpm(hba);
>  	goto out;

Ziqi Chen has a change to totally fix the UFS power-on/off spec 
violation,
see https://lore.kernel.org/patchwork/patch/1351118/ and he is working 
on V2,
can we wait for his change?

Thanks,

Can Guo.

RE: [RFC PATCH v1] ufs: relocate turning off device power sources

[Kiwoong Kim]

> On 2020-12-19 14:27, Kiwoong Kim wrote:
> > UFS specification says that while powering off the device, RST_n
> > signal and REF_CLK signal should be between VSS and VCCQ.
> > One of ways to make it is to drive both RST_n and REF_CLK to low.
> >
> > However, the current location of turning off them doesn't guarantee
> > that. ufshcd_link_state_transition make enter hibern8 but it's not
> > supposed to adjust the level of REF_CLK. Adding
> > ufshcd_vops_device_reset isn't proper because it just drives the level
> > of RST_n to low and then to high, not keep low.
> > In this situation, it could make those levels be diverged from those
> > proper ranges with actual hardware situations, especially right when
> > the driver turns off power.
> >
> > The easist way to make it is just to move the location of turning off
> > because lowering the levels can be enabled through the callbacks named
> > suspend and setup_clocks.
> >
> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 92d433d..dab9840 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -8633,8 +8633,6 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> >  	if (ret)
> >  		goto set_dev_active;
> >
> > -	ufshcd_vreg_set_lpm(hba);
> > -
> >  disable_clks:
> >  	/*
> >  	 * Call vendor specific suspend callback. As these callbacks may
> > access @@ -8662,6 +8660,13 @@ static int ufshcd_suspend(struct ufs_hba
> > *hba, enum ufs_pm_op pm_op)
> >  					hba->clk_gating.state);
> >  	}
> >
> > +	/*
> > +	 * It should follows driving RST_n and REF_CLK
> > +	 * in the range specified in UFS specification
> > +	 */
> > +	if (!ufshcd_is_ufs_dev_active(hba))
> > +		ufshcd_vreg_set_lpm(hba);
> > +
> >  	/* Put the host controller in low power mode if possible */
> >  	ufshcd_hba_vreg_set_lpm(hba);
> >  	goto out;
> 
> Ziqi Chen has a change to totally fix the UFS power-on/off spec violation,
> see https://lore.kernel.org/patchwork/patch/1351118/ and he is working on
> V2, can we wait for his change? 

Two questions.
1. If his patch covers what I said, it's okay.
2. What does he plan to post?
What do you think?

Thanks.
Kiwoong Kim
> 
> Thanks,
> 
> Can Guo.

Re: [RFC PATCH v1] ufs: relocate turning off device power sources

[Can Guo]

On 2020-12-21 09:44, Kiwoong Kim wrote:
>> On 2020-12-19 14:27, Kiwoong Kim wrote:
>> > UFS specification says that while powering off the device, RST_n
>> > signal and REF_CLK signal should be between VSS and VCCQ.
>> > One of ways to make it is to drive both RST_n and REF_CLK to low.
>> >
>> > However, the current location of turning off them doesn't guarantee
>> > that. ufshcd_link_state_transition make enter hibern8 but it's not
>> > supposed to adjust the level of REF_CLK. Adding
>> > ufshcd_vops_device_reset isn't proper because it just drives the level
>> > of RST_n to low and then to high, not keep low.
>> > In this situation, it could make those levels be diverged from those
>> > proper ranges with actual hardware situations, especially right when
>> > the driver turns off power.
>> >
>> > The easist way to make it is just to move the location of turning off
>> > because lowering the levels can be enabled through the callbacks named
>> > suspend and setup_clocks.
>> >
>> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
>> > ---
>> >  drivers/scsi/ufs/ufshcd.c | 9 +++++++--
>> >  1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> > index 92d433d..dab9840 100644
>> > --- a/drivers/scsi/ufs/ufshcd.c
>> > +++ b/drivers/scsi/ufs/ufshcd.c
>> > @@ -8633,8 +8633,6 @@ static int ufshcd_suspend(struct ufs_hba *hba,
>> > enum ufs_pm_op pm_op)
>> >  	if (ret)
>> >  		goto set_dev_active;
>> >
>> > -	ufshcd_vreg_set_lpm(hba);
>> > -
>> >  disable_clks:
>> >  	/*
>> >  	 * Call vendor specific suspend callback. As these callbacks may
>> > access @@ -8662,6 +8660,13 @@ static int ufshcd_suspend(struct ufs_hba
>> > *hba, enum ufs_pm_op pm_op)
>> >  					hba->clk_gating.state);
>> >  	}
>> >
>> > +	/*
>> > +	 * It should follows driving RST_n and REF_CLK
>> > +	 * in the range specified in UFS specification
>> > +	 */
>> > +	if (!ufshcd_is_ufs_dev_active(hba))
>> > +		ufshcd_vreg_set_lpm(hba);
>> > +
>> >  	/* Put the host controller in low power mode if possible */
>> >  	ufshcd_hba_vreg_set_lpm(hba);
>> >  	goto out;
>> 
>> Ziqi Chen has a change to totally fix the UFS power-on/off spec 
>> violation,
>> see https://lore.kernel.org/patchwork/patch/1351118/ and he is working 
>> on
>> V2, can we wait for his change?
> 
> Two questions.
> 1. If his patch covers what I said, it's okay.

Yes, it does cover your fix as you can tell from V1.

> 2. What does he plan to post?

Within today.

Thanks,

Can Guo.

> What do you think?
> 
> Thanks.
> Kiwoong Kim
>> 
>> Thanks,
>> 
>> Can Guo.