| Message ID | 20250503162440.2954-10-quic_nitirawa@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor ufs phy powerup sequence | expand |
On 5/3/25 6:24 PM, Nitin Rawat wrote: > Commit 3f6d1767b1a0 ("phy: ufs-qcom: Refactor all init steps into > phy_poweron") removes the phy_power_on/off from ufs_qcom_setup_clocks > to suspend/resume func. > > To have a better power saving, remove the phy_power_on/off calls from > resume/suspend path and put them back to ufs_qcom_setup_clocks, so that > PHY regulators & clks can be turned on/off along with UFS's clocks. > > Since phy phy_power_on is separated out from phy calibrate, make > separate calls to phy_power_on and phy_calibrate calls from ufs qcom > driver. > > Co-developed-by: Can Guo <quic_cang@quicinc.com> > Signed-off-by: Can Guo <quic_cang@quicinc.com> > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 55 ++++++++++++++++--------------------- > 1 file changed, 23 insertions(+), 32 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 2cd44ee522b8..ff35cd15c72f 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -639,26 +639,17 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, > enum ufs_notify_change_status status) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > - struct phy *phy = host->generic_phy; > > if (status == PRE_CHANGE) > return 0; > > - if (ufs_qcom_is_link_off(hba)) { > - /* > - * Disable the tx/rx lane symbol clocks before PHY is > - * powered down as the PLL source should be disabled > - * after downstream clocks are disabled. > - */ > + if (!ufs_qcom_is_link_active(hba)) so is_link_off and !is_link_active are not the same thing - this also allows for disabling the resources when in hibern8/broken states - is that intended? > ufs_qcom_disable_lane_clks(host); > - phy_power_off(phy); > > - /* reset the connected UFS device during power down */ > - ufs_qcom_device_reset_ctrl(hba, true); > > - } else if (!ufs_qcom_is_link_active(hba)) { > - ufs_qcom_disable_lane_clks(host); > - } > + /* reset the connected UFS device during power down */ > + if (ufs_qcom_is_link_off(hba) && host->device_reset) > + ufs_qcom_device_reset_ctrl(hba, true); similarly this will not be allowed in hibern8/broken states now > > return ufs_qcom_ice_suspend(host); > } > @@ -666,26 +657,11 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, > static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > - struct phy *phy = host->generic_phy; > int err; > > - if (ufs_qcom_is_link_off(hba)) { > - err = phy_power_on(phy); > - if (err) { > - dev_err(hba->dev, "%s: failed PHY power on: %d\n", > - __func__, err); > - return err; > - } > - > - err = ufs_qcom_enable_lane_clks(host); > - if (err) > - return err; > - > - } else if (!ufs_qcom_is_link_active(hba)) { > - err = ufs_qcom_enable_lane_clks(host); > - if (err) > - return err; > - } > + err = ufs_qcom_enable_lane_clks(host); > + if (err) > + return err; > > return ufs_qcom_ice_resume(host); > } > @@ -1042,6 +1018,8 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on, > enum ufs_notify_change_status status) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > + struct phy *phy = host->generic_phy; > + int err; > > /* > * In case ufs_qcom_init() is not yet done, simply ignore. > @@ -1060,10 +1038,22 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on, > /* disable device ref_clk */ > ufs_qcom_dev_ref_clk_ctrl(host, false); > } > + err = phy_power_off(phy); a newline to separate the blocks would improve readability> + if (err) { > + dev_err(hba->dev, "%s: phy power off failed, ret=%d\n", > + __func__, err); > + return err; please indent the return statement a tab earlier so it doesn't look like it's an argument to dev_err() putting PHY calls in the function that promises to toggle clocks could also use a comment, maybe extending the kerneldoc to say that certain clocks come from the PHY so it needs to be managed together Konrad
On 5/9/2025 5:05 PM, Konrad Dybcio wrote: > On 5/3/25 6:24 PM, Nitin Rawat wrote: >> Commit 3f6d1767b1a0 ("phy: ufs-qcom: Refactor all init steps into >> phy_poweron") removes the phy_power_on/off from ufs_qcom_setup_clocks >> to suspend/resume func. >> >> To have a better power saving, remove the phy_power_on/off calls from >> resume/suspend path and put them back to ufs_qcom_setup_clocks, so that >> PHY regulators & clks can be turned on/off along with UFS's clocks. >> >> Since phy phy_power_on is separated out from phy calibrate, make >> separate calls to phy_power_on and phy_calibrate calls from ufs qcom >> driver. >> >> Co-developed-by: Can Guo <quic_cang@quicinc.com> >> Signed-off-by: Can Guo <quic_cang@quicinc.com> >> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> >> --- >> drivers/ufs/host/ufs-qcom.c | 55 ++++++++++++++++--------------------- >> 1 file changed, 23 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index 2cd44ee522b8..ff35cd15c72f 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -639,26 +639,17 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, >> enum ufs_notify_change_status status) >> { >> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> - struct phy *phy = host->generic_phy; >> >> if (status == PRE_CHANGE) >> return 0; >> >> - if (ufs_qcom_is_link_off(hba)) { >> - /* >> - * Disable the tx/rx lane symbol clocks before PHY is >> - * powered down as the PLL source should be disabled >> - * after downstream clocks are disabled. >> - */ >> + if (!ufs_qcom_is_link_active(hba)) > > so is_link_off and !is_link_active are not the same thing - this also allows > for disabling the resources when in hibern8/broken states - is that intended? Yes is_link_off and !is_link_Active is not same thing. !is_link_active also include link in hibern8 state where lane clock is intended to be disabled because PHY is powered down in hibern8. > >> ufs_qcom_disable_lane_clks(host); >> - phy_power_off(phy); >> >> - /* reset the connected UFS device during power down */ >> - ufs_qcom_device_reset_ctrl(hba, true); >> >> - } else if (!ufs_qcom_is_link_active(hba)) { >> - ufs_qcom_disable_lane_clks(host); >> - } >> + /* reset the connected UFS device during power down */ >> + if (ufs_qcom_is_link_off(hba) && host->device_reset) >> + ufs_qcom_device_reset_ctrl(hba, true); > > similarly this will not be allowed in hibern8/broken states now > >> >> return ufs_qcom_ice_suspend(host); >> } >> @@ -666,26 +657,11 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, >> static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) >> { >> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> - struct phy *phy = host->generic_phy; >> int err; >> >> - if (ufs_qcom_is_link_off(hba)) { >> - err = phy_power_on(phy); >> - if (err) { >> - dev_err(hba->dev, "%s: failed PHY power on: %d\n", >> - __func__, err); >> - return err; >> - } >> - >> - err = ufs_qcom_enable_lane_clks(host); >> - if (err) >> - return err; >> - >> - } else if (!ufs_qcom_is_link_active(hba)) { >> - err = ufs_qcom_enable_lane_clks(host); >> - if (err) >> - return err; >> - } >> + err = ufs_qcom_enable_lane_clks(host); >> + if (err) >> + return err; >> >> return ufs_qcom_ice_resume(host); >> } >> @@ -1042,6 +1018,8 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on, >> enum ufs_notify_change_status status) >> { >> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> + struct phy *phy = host->generic_phy; >> + int err; >> >> /* >> * In case ufs_qcom_init() is not yet done, simply ignore. >> @@ -1060,10 +1038,22 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on, >> /* disable device ref_clk */ >> ufs_qcom_dev_ref_clk_ctrl(host, false); >> } >> + err = phy_power_off(phy); > > a newline to separate the blocks would improve readability> + if (err) { >> + dev_err(hba->dev, "%s: phy power off failed, ret=%d\n", >> + __func__, err); >> + return err; Sure will add in next patchset. > > please indent the return statement a tab earlier so it doesn't look > like it's an argument to dev_err() Sure will add in next patchset. > > putting PHY calls in the function that promises to toggle clocks could > also use a comment, maybe extending the kerneldoc to say that certain > clocks come from the PHY so it needs to be managed together > Sure will add a comment or update in kernel doc in next patchset. > Konrad
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 2cd44ee522b8..ff35cd15c72f 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -639,26 +639,17 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, enum ufs_notify_change_status status) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); - struct phy *phy = host->generic_phy; if (status == PRE_CHANGE) return 0; - if (ufs_qcom_is_link_off(hba)) { - /* - * Disable the tx/rx lane symbol clocks before PHY is - * powered down as the PLL source should be disabled - * after downstream clocks are disabled. - */ + if (!ufs_qcom_is_link_active(hba)) ufs_qcom_disable_lane_clks(host); - phy_power_off(phy); - /* reset the connected UFS device during power down */ - ufs_qcom_device_reset_ctrl(hba, true); - } else if (!ufs_qcom_is_link_active(hba)) { - ufs_qcom_disable_lane_clks(host); - } + /* reset the connected UFS device during power down */ + if (ufs_qcom_is_link_off(hba) && host->device_reset) + ufs_qcom_device_reset_ctrl(hba, true); return ufs_qcom_ice_suspend(host); } @@ -666,26 +657,11 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); - struct phy *phy = host->generic_phy; int err; - if (ufs_qcom_is_link_off(hba)) { - err = phy_power_on(phy); - if (err) { - dev_err(hba->dev, "%s: failed PHY power on: %d\n", - __func__, err); - return err; - } - - err = ufs_qcom_enable_lane_clks(host); - if (err) - return err; - - } else if (!ufs_qcom_is_link_active(hba)) { - err = ufs_qcom_enable_lane_clks(host); - if (err) - return err; - } + err = ufs_qcom_enable_lane_clks(host); + if (err) + return err; return ufs_qcom_ice_resume(host); } @@ -1042,6 +1018,8 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on, enum ufs_notify_change_status status) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); + struct phy *phy = host->generic_phy; + int err; /* * In case ufs_qcom_init() is not yet done, simply ignore. @@ -1060,10 +1038,22 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on, /* disable device ref_clk */ ufs_qcom_dev_ref_clk_ctrl(host, false); } + err = phy_power_off(phy); + if (err) { + dev_err(hba->dev, "%s: phy power off failed, ret=%d\n", + __func__, err); + return err; + } } break; case POST_CHANGE: if (on) { + err = phy_power_on(phy); + if (err) { + dev_err(hba->dev, "%s: phy power on failed, ret = %d\n", + __func__, err); + return err; + } /* enable the device ref clock for HS mode*/ if (ufshcd_is_hs_mode(&hba->pwr_info)) ufs_qcom_dev_ref_clk_ctrl(host, true); @@ -1246,9 +1236,10 @@ static int ufs_qcom_init(struct ufs_hba *hba) static void ufs_qcom_exit(struct ufs_hba *hba) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); + struct phy *phy = host->generic_phy; ufs_qcom_disable_lane_clks(host); - phy_power_off(host->generic_phy); + phy_power_off(phy); phy_exit(host->generic_phy); }
Commit 3f6d1767b1a0 ("phy: ufs-qcom: Refactor all init steps into phy_poweron") removes the phy_power_on/off from ufs_qcom_setup_clocks to suspend/resume func. To have a better power saving, remove the phy_power_on/off calls from resume/suspend path and put them back to ufs_qcom_setup_clocks, so that PHY regulators & clks can be turned on/off along with UFS's clocks. Since phy phy_power_on is separated out from phy calibrate, make separate calls to phy_power_on and phy_calibrate calls from ufs qcom driver. Co-developed-by: Can Guo <quic_cang@quicinc.com> Signed-off-by: Can Guo <quic_cang@quicinc.com> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> --- drivers/ufs/host/ufs-qcom.c | 55 ++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 32 deletions(-)