* [RFC PATCH v1] ufs: relocate turning off device power sources [not found] <CGME20201219063815epcas2p1ffd277f7e53d9680abb460f55a53c599@epcas2p1.samsung.com> @ 2020-12-19 6:27 ` Kiwoong Kim 2020-12-20 5:34 ` Can Guo 0 siblings, 1 reply; 4+ messages in thread From: Kiwoong Kim @ 2020-12-19 6:27 UTC (permalink / raw) To: linux-scsi, alim.akhtar, avri.altman, jejb, martin.petersen, beanhuo, asutoshd, cang, bvanassche, grant.jung, sc.suh, hy50.seo, sh425.lee, bhoon95.kim Cc: 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH v1] ufs: relocate turning off device power sources 2020-12-19 6:27 ` [RFC PATCH v1] ufs: relocate turning off device power sources Kiwoong Kim @ 2020-12-20 5:34 ` Can Guo 2020-12-21 1:44 ` Kiwoong Kim 0 siblings, 1 reply; 4+ messages in thread From: Can Guo @ 2020-12-20 5:34 UTC (permalink / raw) To: Kiwoong Kim Cc: linux-scsi, alim.akhtar, avri.altman, jejb, martin.petersen, beanhuo, asutoshd, bvanassche, grant.jung, sc.suh, hy50.seo, sh425.lee, bhoon95.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? Thanks, Can Guo. ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [RFC PATCH v1] ufs: relocate turning off device power sources 2020-12-20 5:34 ` Can Guo @ 2020-12-21 1:44 ` Kiwoong Kim 2020-12-21 2:37 ` Can Guo 0 siblings, 1 reply; 4+ messages in thread From: Kiwoong Kim @ 2020-12-21 1:44 UTC (permalink / raw) To: 'Can Guo' Cc: linux-scsi, alim.akhtar, avri.altman, jejb, martin.petersen, beanhuo, asutoshd, bvanassche, grant.jung, sc.suh, hy50.seo, sh425.lee, bhoon95.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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH v1] ufs: relocate turning off device power sources 2020-12-21 1:44 ` Kiwoong Kim @ 2020-12-21 2:37 ` Can Guo 0 siblings, 0 replies; 4+ messages in thread From: Can Guo @ 2020-12-21 2:37 UTC (permalink / raw) To: Kiwoong Kim Cc: linux-scsi, alim.akhtar, avri.altman, jejb, martin.petersen, beanhuo, asutoshd, bvanassche, grant.jung, sc.suh, hy50.seo, sh425.lee, bhoon95.kim 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-21 2:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20201219063815epcas2p1ffd277f7e53d9680abb460f55a53c599@epcas2p1.samsung.com> 2020-12-19 6:27 ` [RFC PATCH v1] ufs: relocate turning off device power sources Kiwoong Kim 2020-12-20 5:34 ` Can Guo 2020-12-21 1:44 ` Kiwoong Kim 2020-12-21 2:37 ` Can Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).