| Message ID | 20250410090102.20781-1-quic_nitirawa@quicinc.com |
|---|---|
| Headers | show |
| Series | Refactor phy powerup sequence | expand |
On 4/11/2025 1:35 AM, Dmitry Baryshkov wrote: > On Thu, Apr 10, 2025 at 02:30:53PM +0530, Nitin Rawat wrote: >> In Current code regulators enable, clks enable, calibrating UFS PHY, >> start_serdes and polling PCS_ready_status are part of phy_power_on. >> >> UFS PHY registers are retained after power collapse, meaning calibrating >> UFS PHY, start_serdes and polling PCS_ready_status can be done only when >> hba is powered_on, and not needed every time when phy_power_on is called >> during resume. Hence keep the code which enables PHY's regulators & clks >> in phy_power_on and move the rest steps into phy_calibrate function. >> >> Since phy_power_on is separated out from phy calibrate, make separate calls >> to phy_power_on and phy_calibrate calls from ufs qcom driver. >> >> Also for better power saving, remove the phy_power_on/off calls from >> resume/suspend path and put them to ufs_qcom_setup_clocks, so that >> PHY's regulators & clks can be turned on/off along with UFS's clocks. > > Please add an explicit note that patch1 is a requirement for the rest of > the PHY patches. It might make sense to merge it through the PHY tree > too (or to use an immutable branch). > Hi Dmitry, Thanks for the suggestion. Sure I would mention this in the cover letter when I post next patchset. Thanks, Nitin >> >> This patch series is tested on SM8550 QRD, SM8650 MTP , SM8750 MTP. >> >
On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com> wrote: > > > > On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote: > > On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote: > >> Refactor the UFS PHY reset handling to parse the reset logic only once > >> during probe, instead of every resume. > >> > >> Move the UFS PHY reset parsing logic from qmp_phy_power_on to > >> qmp_ufs_probe to avoid unnecessary parsing during resume. > > > > How did you solve the circular dependency issue being noted below? > > Hi Dmitry, > As part of my patch, I moved the parsing logic from qmp_phy_power_on to > qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain > about the circular dependency issue and whether if it still exists. It surely does. The reset controller is registered in the beginning of ufs_qcom_init() and the PHY is acquired only a few lines below. It creates a very small window for PHY driver to probe. Which means, NAK, this patch doesn't look acceptable. > > Regards, > Nitin > > > > > >> > >> Co-developed-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com> > >> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com> > >> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> > >> --- > >> drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 61 +++++++++++++------------ > >> 1 file changed, 33 insertions(+), 28 deletions(-) > >> > >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c > >> index 636dc3dc3ea8..12dad28cc1bd 100644 > >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c > >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c > >> @@ -1799,38 +1799,11 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp) > >> static int qmp_ufs_power_on(struct phy *phy) > >> { > >> struct qmp_ufs *qmp = phy_get_drvdata(phy); > >> - const struct qmp_phy_cfg *cfg = qmp->cfg; > >> int ret; > >> dev_vdbg(qmp->dev, "Initializing QMP phy\n"); > >> > >> - if (cfg->no_pcs_sw_reset) { > >> - /* > >> - * Get UFS reset, which is delayed until now to avoid a > >> - * circular dependency where UFS needs its PHY, but the PHY > >> - * needs this UFS reset. > >> - */ > >> - if (!qmp->ufs_reset) { > >> - qmp->ufs_reset = > >> - devm_reset_control_get_exclusive(qmp->dev, > >> - "ufsphy"); > >> - > >> - if (IS_ERR(qmp->ufs_reset)) { > >> - ret = PTR_ERR(qmp->ufs_reset); > >> - dev_err(qmp->dev, > >> - "failed to get UFS reset: %d\n", > >> - ret); > >> - > >> - qmp->ufs_reset = NULL; > >> - return ret; > >> - } > >> - } > >> - } > >> - > >> ret = qmp_ufs_com_init(qmp); > >> - if (ret) > >> - return ret; > >> - > >> - return 0; > >> + return ret; > >> } > >> > >> static int qmp_ufs_phy_calibrate(struct phy *phy) > >> @@ -2088,6 +2061,34 @@ static int qmp_ufs_parse_dt(struct qmp_ufs *qmp) > >> return 0; > >> } > >> > >> +static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp) > >> +{ > >> + const struct qmp_phy_cfg *cfg = qmp->cfg; > >> + int ret; > >> + > >> + if (!cfg->no_pcs_sw_reset) > >> + return 0; > >> + > >> + /* > >> + * Get UFS reset, which is delayed until now to avoid a > >> + * circular dependency where UFS needs its PHY, but the PHY > >> + * needs this UFS reset. > >> + */ > >> + if (!qmp->ufs_reset) { > >> + qmp->ufs_reset = > >> + devm_reset_control_get_exclusive(qmp->dev, "ufsphy"); > > > > Strange indentation. > > > >> + > >> + if (IS_ERR(qmp->ufs_reset)) { > >> + ret = PTR_ERR(qmp->ufs_reset); > >> + dev_err(qmp->dev, "failed to get PHY reset: %d\n", ret); > >> + qmp->ufs_reset = NULL; > >> + return ret; > >> + } > >> + } > >> + > >> + return 0; > >> +} > >> + > >> static int qmp_ufs_probe(struct platform_device *pdev) > >> { > >> struct device *dev = &pdev->dev; > >> @@ -2114,6 +2115,10 @@ static int qmp_ufs_probe(struct platform_device *pdev) > >> if (ret) > >> return ret; > >> > >> + ret = qmp_ufs_get_phy_reset(qmp); > >> + if (ret) > >> + return ret; > >> + > >> /* Check for legacy binding with child node. */ > >> np = of_get_next_available_child(dev->of_node, NULL); > >> if (np) { > >> -- > >> 2.48.1 > >> > > >