| Message ID | 20250503162440.2954-11-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: > Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper > functions with mutex protection to ensure safe usage of is_phy_pwr_on > and prevent possible race conditions. > > 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> > --- The PHY framework does the same thing internally already, this seems unnecessary Konrad
On 5/9/2025 5:07 PM, Konrad Dybcio wrote: > On 5/3/25 6:24 PM, Nitin Rawat wrote: >> Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper >> functions with mutex protection to ensure safe usage of is_phy_pwr_on >> and prevent possible race conditions. >> >> 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> >> --- > > The PHY framework does the same thing internally already, this seems > unnecessary Hi Konrad, Thanks for the review. There are scenarios where ufshcd_link_startup() can call ufshcd_vops_link_startup_notify() multiple times during retries. This leads to the PHY reference count increasing continuously, preventing proper re-initialization of the PHY. Recently, this issue was addressed with patch 7bac65687510 ("scsi: ufs: qcom: Power off the PHY if it was already powered on in ufs_qcom_power_up_sequence()"). However, I still want to maintain a reference count (ref_cnt) to safeguard against similar conditions in the code. Additionally, this approach helps avoid unnecessary phy_power_on and phy_power_off calls. Please let me know your thoughts. Regards, Nitin > > Konrad
On 5/9/25 1:49 PM, Nitin Rawat wrote: > > > On 5/9/2025 5:07 PM, Konrad Dybcio wrote: >> On 5/3/25 6:24 PM, Nitin Rawat wrote: >>> Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper >>> functions with mutex protection to ensure safe usage of is_phy_pwr_on >>> and prevent possible race conditions. >>> >>> 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> >>> --- >> >> The PHY framework does the same thing internally already, this seems >> unnecessary > > Hi Konrad, > > Thanks for the review. There are scenarios where ufshcd_link_startup() can call ufshcd_vops_link_startup_notify() multiple times during retries. This leads to the PHY reference count increasing continuously, preventing proper re-initialization of the PHY. I'm assuming you're talking about the scenario where it jumps into ufs_qcom_power_up_sequence() - you have a label in there called `out_disable_phy` - add a phy_power_off() after phy_calibrate if things fail and you should be good to go if I'm reading things right. Please include something resembling a call stack in the commit message, as currently everyone reviewing this has to make guesses about why this needs to be done > Recently, this issue was addressed with patch 7bac65687510 ("scsi: ufs: > qcom: Power off the PHY if it was already powered on in ufs_qcom_power_up_sequence()"). However, I still want to maintain a reference count (ref_cnt) to safeguard against similar conditions in the code. Additionally, this approach helps avoid unnecessary phy_power_on and phy_power_off calls. Please let me know your thoughts. These unnecessary calls only amount to a couple of jumps and compares, just like your wrappers, as the framework keeps track of the enable count as well Konrad
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index ff35cd15c72f..a7e9e06847f8 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -421,6 +421,38 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba) return UFS_HS_G3; } +static int ufs_qcom_phy_power_on(struct ufs_hba *hba) +{ + struct ufs_qcom_host *host = ufshcd_get_variant(hba); + struct phy *phy = host->generic_phy; + int ret = 0; + + guard(mutex)(&host->phy_mutex); + if (!host->is_phy_pwr_on) { + ret = phy_power_on(phy); + if (!ret) + host->is_phy_pwr_on = true; + } + + return ret; +} + +static int ufs_qcom_phy_power_off(struct ufs_hba *hba) +{ + struct ufs_qcom_host *host = ufshcd_get_variant(hba); + struct phy *phy = host->generic_phy; + int ret = 0; + + guard(mutex)(&host->phy_mutex); + if (host->is_phy_pwr_on) { + ret = phy_power_off(phy); + if (!ret) + host->is_phy_pwr_on = false; + } + + return ret; +} + static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); @@ -449,7 +481,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) return ret; if (phy->power_count) { - phy_power_off(phy); + ufs_qcom_phy_power_off(hba); phy_exit(phy); } @@ -466,7 +498,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) goto out_disable_phy; /* power on phy - start serdes and phy's power and clocks */ - ret = phy_power_on(phy); + ret = ufs_qcom_phy_power_on(hba); if (ret) { dev_err(hba->dev, "%s: phy power on failed, ret = %d\n", __func__, ret); @@ -1018,7 +1050,6 @@ 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; /* @@ -1038,7 +1069,7 @@ 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); + err = ufs_qcom_phy_power_off(hba); if (err) { dev_err(hba->dev, "%s: phy power off failed, ret=%d\n", __func__, err); @@ -1048,7 +1079,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on, break; case POST_CHANGE: if (on) { - err = phy_power_on(phy); + err = ufs_qcom_phy_power_on(hba); if (err) { dev_err(hba->dev, "%s: phy power on failed, ret = %d\n", __func__, err); @@ -1236,10 +1267,9 @@ 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(phy); + ufs_qcom_phy_power_off(hba); phy_exit(host->generic_phy); } diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h index d0e6ec9128e7..3db29fbcd40b 100644 --- a/drivers/ufs/host/ufs-qcom.h +++ b/drivers/ufs/host/ufs-qcom.h @@ -252,6 +252,10 @@ struct ufs_qcom_host { u32 phy_gear; bool esi_enabled; + /* flag to check if phy is powered on */ + bool is_phy_pwr_on; + /* Protect the usage of is_phy_pwr_on against racing */ + struct mutex phy_mutex; }; struct ufs_qcom_drvdata {