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

[Can Guo <cang@codeaurora.org>]

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.