| Message ID | 20250515162722.6933-10-quic_nitirawa@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor ufs phy powerup sequence | expand |
On Thu, May 15, 2025 at 09:57:20PM +0530, 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 s/removes/moved > 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. > This patch is not calling phy_calibrate(). > 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 | 61 ++++++++++++++++++------------------- > 1 file changed, 29 insertions(+), 32 deletions(-) > [...] > 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. > @@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err); > + return err; > + } > } > break; > case POST_CHANGE: > if (on) { > + err = phy_power_on(phy); > + if (err) { > + dev_err(hba->dev, "phy power on failed, ret = %d\n", 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); > @@ -1343,9 +1339,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); This change is not at all needed. - Mani
On 5/21/2025 7:27 PM, Manivannan Sadhasivam wrote: > On Thu, May 15, 2025 at 09:57:20PM +0530, 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 > > s/removes/moved Sure, will address in next patchset. > >> 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. >> > > This patch is not calling phy_calibrate(). updated commit text to remove phy_Calibrate from commit text > >> 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 | 61 ++++++++++++++++++------------------- >> 1 file changed, 29 insertions(+), 32 deletions(-) >> > > [...] > >> 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. >> @@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err); >> + return err; >> + } >> } >> break; >> case POST_CHANGE: >> if (on) { >> + err = phy_power_on(phy); >> + if (err) { >> + dev_err(hba->dev, "phy power on failed, ret = %d\n", 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); >> @@ -1343,9 +1339,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); > > This change is not at all needed. In the current code, PHY resources (clocks and rails) remain active even when the link is in a hibernate state and all controller clocks are off. This results in a significant power penalty and can prevent CX power collapse. These rails and clocks are only turned off when a system suspend is triggered, and even then, only at SPM level 5 where the link is turned off. This approach is not power-efficient. As part of this series, the code that enables/disables the PHY's regulators and clocks is now confined to the phy_power_on and phy_power_off calls, with the rest moved to the calibration phase. Therefore, we are invoking the phy_power_off and phy_power_on calls from ufs_qcom_setup_clocks, ensuring that the PHY's regulators and clocks can be turned on/off along with the UFS clocks, thereby achieving power savings. Therefore, this patch is the cornerstone of this series. Regards, Nitin > > - Mani >
On Thu, May 22, 2025 at 03:10:48AM +0530, Nitin Rawat wrote: > > > On 5/21/2025 7:27 PM, Manivannan Sadhasivam wrote: > > On Thu, May 15, 2025 at 09:57:20PM +0530, 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 > > > > s/removes/moved > > Sure, will address in next patchset. > > > > > > 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. > > > > > > > This patch is not calling phy_calibrate(). > > updated commit text to remove phy_Calibrate from commit text > > > > > > 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 | 61 ++++++++++++++++++------------------- > > > 1 file changed, 29 insertions(+), 32 deletions(-) > > > > > > > [...] > > > > > 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. > > > @@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err); > > > + return err; > > > + } > > > } > > > break; > > > case POST_CHANGE: > > > if (on) { > > > + err = phy_power_on(phy); > > > + if (err) { > > > + dev_err(hba->dev, "phy power on failed, ret = %d\n", 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); > > > @@ -1343,9 +1339,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); > > > > This change is not at all needed. > > In the current code, PHY resources (clocks and rails) remain active even > when the link is in a hibernate state and all controller clocks are off. > This results in a significant power penalty and can prevent CX power > collapse. > > These rails and clocks are only turned off when a system suspend is > triggered, and even then, only at SPM level 5 where the link is turned off. > This approach is not power-efficient. > > As part of this series, the code that enables/disables the PHY's regulators > and clocks is now confined to the phy_power_on and phy_power_off calls, with > the rest moved to the calibration phase. > > Therefore, we are invoking the phy_power_off and phy_power_on calls from > ufs_qcom_setup_clocks, ensuring that the PHY's regulators and clocks can be > turned on/off along with the UFS clocks, thereby achieving power savings. > > Therefore, this patch is the cornerstone of this series. > I did not question the patch, but just the change that you did in the ufs_qcom_exit() function. You added a local variable for 'host->generic_phy', which is not related to this patch. - Mani
On 5/22/2025 2:25 PM, Manivannan Sadhasivam wrote: > On Thu, May 22, 2025 at 03:10:48AM +0530, Nitin Rawat wrote: >> >> >> On 5/21/2025 7:27 PM, Manivannan Sadhasivam wrote: >>> On Thu, May 15, 2025 at 09:57:20PM +0530, 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 >>> >>> s/removes/moved >> >> Sure, will address in next patchset. >> >>> >>>> 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. >>>> >>> >>> This patch is not calling phy_calibrate(). >> >> updated commit text to remove phy_Calibrate from commit text >> >>> >>>> 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 | 61 ++++++++++++++++++------------------- >>>> 1 file changed, 29 insertions(+), 32 deletions(-) >>>> >>> >>> [...] >>> >>>> 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. >>>> @@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err); >>>> + return err; >>>> + } >>>> } >>>> break; >>>> case POST_CHANGE: >>>> if (on) { >>>> + err = phy_power_on(phy); >>>> + if (err) { >>>> + dev_err(hba->dev, "phy power on failed, ret = %d\n", 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); >>>> @@ -1343,9 +1339,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); >>> >>> This change is not at all needed. >> >> In the current code, PHY resources (clocks and rails) remain active even >> when the link is in a hibernate state and all controller clocks are off. >> This results in a significant power penalty and can prevent CX power >> collapse. >> >> These rails and clocks are only turned off when a system suspend is >> triggered, and even then, only at SPM level 5 where the link is turned off. >> This approach is not power-efficient. >> >> As part of this series, the code that enables/disables the PHY's regulators >> and clocks is now confined to the phy_power_on and phy_power_off calls, with >> the rest moved to the calibration phase. >> >> Therefore, we are invoking the phy_power_off and phy_power_on calls from >> ufs_qcom_setup_clocks, ensuring that the PHY's regulators and clocks can be >> turned on/off along with the UFS clocks, thereby achieving power savings. >> >> Therefore, this patch is the cornerstone of this series. >> > > I did not question the patch, but just the change that you did in the > ufs_qcom_exit() function. You added a local variable for 'host->generic_phy', > which is not related to this patch. Sorry about that, Mani. I misunderstood your comments. Sure, will address them. > > - Mani >
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 23b4fc84dc8c..3ee4ab90dfba 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -697,26 +697,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); } @@ -724,26 +715,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); } @@ -1133,12 +1109,20 @@ static void ufs_qcom_set_caps(struct ufs_hba *hba) * @on: If true, enable clocks else disable them. * @status: PRE_CHANGE or POST_CHANGE notify * + * There are certain clocks which comes from the PHY so it needs + * to be managed together along with controller clocks which also + * provides a better power saving. Hence keep phy_power_off/on calls + * in ufs_qcom_setup_clocks, so that PHY's regulators & clks can be + * turned on/off along with UFS's clocks. + * * Return: 0 on success, non-zero on failure. */ 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. @@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err); + return err; + } } break; case POST_CHANGE: if (on) { + err = phy_power_on(phy); + if (err) { + dev_err(hba->dev, "phy power on failed, ret = %d\n", 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); @@ -1343,9 +1339,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 | 61 ++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 32 deletions(-)