lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <812a9905-a8f6-40b2-a603-6c0be18239da@quicinc.com> Date: Sat, 10 May 2025 19:19:31 +0530 From: Nitin Rawat <quic_nitirawa@...cinc.com> To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, <vkoul@...nel.org>, <kishon@...nel.org>, <manivannan.sadhasivam@...aro.org>, <James.Bottomley@...senPartnership.com>, <martin.petersen@...cle.com>, <bvanassche@....org>, <andersson@...nel.org>, <neil.armstrong@...aro.org> CC: <quic_rdwivedi@...cinc.com>, <quic_cang@...cinc.com>, <linux-arm-msm@...r.kernel.org>, <linux-phy@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, <linux-scsi@...r.kernel.org> Subject: Re: [PATCH V4 09/11] scsi: ufs: qcom : Refactor phy_power_on/off calls 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@...cinc.com> >> Signed-off-by: Can Guo <quic_cang@...cinc.com> >> Signed-off-by: Nitin Rawat <quic_nitirawa@...cinc.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
Powered by blists - more mailing lists