| Message ID | 20250410090102.20781-1-quic_nitirawa@quicinc.com |
|---|---|
| Headers | show |
| Series | Refactor phy powerup sequence | expand |
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). > > This patch series is tested on SM8550 QRD, SM8650 MTP , SM8750 MTP. >
On Thu, Apr 10, 2025 at 02:30:55PM +0530, Nitin Rawat wrote: > Rename qmp_ufs_enable to qmp_ufs_power_on and qmp_ufs_power_on to > qmp_ufs_phy_calibrate to better reflect their functionality. Also > update function calls and structure assignments accordingly. > > 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 | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
On Thu, Apr 10, 2025 at 02:30:56PM +0530, Nitin Rawat wrote: > Commit 052553af6a31 ("ufs/phy: qcom: Refactor to use phy_init call") > puts enabling regulators & clks, calibrating UFS PHY, starting serdes > and polling PCS ready status into phy_power_on. > > 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. > > Refactor the code to retain PHY regulators & clks in phy_power_on and > move out rest of the code to new phy_calibrate function. > > Also move reset_control_assert to qmp_ufs_phy_calibrate to align > with Hardware programming guide. > > 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/phy/qualcomm/phy-qcom-qmp-ufs.c | 26 ++++++------------------- > 1 file changed, 6 insertions(+), 20 deletions(-) > With the cover letter updated to note the dependencies: Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
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? > > 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 >
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 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. 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 >> >
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 > >> > > >
On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote: > 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. Hi Dmitry, Thanks for pointing this out. I agree that it leaves very little time for the PHY to probe, which may cause issues with targets where no_pcs_sw_reset is set to true. As an experiment, I kept no_pcs_sw_reset set to true for the SM8750, and it caused bootup probe issues in some of the iterations I ran. To address this, I propose updating the patch to move the qmp_ufs_get_phy_reset call to phy_calibrate, just before the reset_control_assert call. This change will delay the UFS PHY reset as much as possible in the code. Additionally, moving it from phy_power_on to calibrate will ensure that qmp_ufs_get_phy_reset is called only once during boot, rather than during each phy_power_on call. Please let me know your thoughts. ===================================================================================================== static int qmp_ufs_phy_calibrate(struct phy *phy) { struct qmp_ufs *qmp = phy_get_drvdata(phy); @@ -1793,6 +1826,12 @@ static int qmp_ufs_phy_calibrate(struct phy *phy) unsigned int val; int ret; + pr_err("%s %d\n", __func__, __LINE__); + + ret = qmp_ufs_get_phy_reset(qmp); + if (ret) + return ret; + ret = reset_control_assert(qmp->ufs_reset); if (ret) return ret; @@ -1817,7 +1856,7 @@ static int qmp_ufs_phy_calibrate(struct phy *phy) dev_err(qmp->dev, "phy initialization timed-out\n"); return ret; ===================================================================================================== Regards. Nitin > >> >> 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 >>>> >>> >> > >
On 14/04/2025 23:34, Nitin Rawat wrote: > > > On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote: >> 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. > > Hi Dmitry, > > Thanks for pointing this out. I agree that it leaves very little time > for the PHY to probe, which may cause issues with targets where > no_pcs_sw_reset is set to true. > > As an experiment, I kept no_pcs_sw_reset set to true for the SM8750, and > it caused bootup probe issues in some of the iterations I ran. > > To address this, I propose updating the patch to move the > qmp_ufs_get_phy_reset call to phy_calibrate, just before the > reset_control_assert call. Will it cause an issue if we move it to phy_init() instead of phy_calibrate()? > > This change will delay the UFS PHY reset as much as possible in the > code. Additionally, moving it from phy_power_on to calibrate will ensure > that qmp_ufs_get_phy_reset is called only once during boot, rather than > during each phy_power_on call. > > Please let me know your thoughts. > ===================================================================================================== > static int qmp_ufs_phy_calibrate(struct phy *phy) > { > struct qmp_ufs *qmp = phy_get_drvdata(phy); > @@ -1793,6 +1826,12 @@ static int qmp_ufs_phy_calibrate(struct phy *phy) > unsigned int val; > int ret; > > + pr_err("%s %d\n", __func__, __LINE__); > + > + ret = qmp_ufs_get_phy_reset(qmp); > + if (ret) > + return ret; > + > ret = reset_control_assert(qmp->ufs_reset); > if (ret) > return ret; > @@ -1817,7 +1856,7 @@ static int qmp_ufs_phy_calibrate(struct phy *phy) > dev_err(qmp->dev, "phy initialization timed-out\n"); > return ret; > ===================================================================================================== > > > Regards. > Nitin >> >>> >>> 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 >>>>> >>>> >>> >> >> >
On 4/15/2025 2:59 PM, Dmitry Baryshkov wrote: > On 14/04/2025 23:34, Nitin Rawat wrote: >> >> >> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote: >>> 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. >> >> Hi Dmitry, >> >> Thanks for pointing this out. I agree that it leaves very little time >> for the PHY to probe, which may cause issues with targets where >> no_pcs_sw_reset is set to true. >> >> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750, >> and it caused bootup probe issues in some of the iterations I ran. >> >> To address this, I propose updating the patch to move the >> qmp_ufs_get_phy_reset call to phy_calibrate, just before the >> reset_control_assert call. > > Will it cause an issue if we move it to phy_init() instead of > phy_calibrate()? Hi Dmitry, Thanks for suggestion. Phy_init is invoked before phy_set_mode_ext and ufs_qcom_phy_power_on, whereas calibrate is called after ufs_qcom_phy_power_on. Keeping the UFS PHY reset in phy_calibrate introduces relatively more delay, providing more buffer time for the PHY driver probe, ensuring the UFS PHY reset is handled correctly the first time. Moving the calibration to phy_init shouldn't cause any issues. However, since we currently don't have an initialization operations registered for init, we would need to add a new PHY initialization ops if we decide to move it to phy_init. Please let me know if this looks fine to you, or if you have any suggestions. I am open to your suggestions. Regards, Nitin > >> >> This change will delay the UFS PHY reset as much as possible in the >> code. Additionally, moving it from phy_power_on to calibrate will >> ensure that qmp_ufs_get_phy_reset is called only once during boot, >> rather than during each phy_power_on call. >> >> Please let me know your thoughts. >> ===================================================================================================== >> static int qmp_ufs_phy_calibrate(struct phy *phy) >> { >> struct qmp_ufs *qmp = phy_get_drvdata(phy); >> @@ -1793,6 +1826,12 @@ static int qmp_ufs_phy_calibrate(struct phy *phy) >> unsigned int val; >> int ret; >> >> + pr_err("%s %d\n", __func__, __LINE__); >> + >> + ret = qmp_ufs_get_phy_reset(qmp); >> + if (ret) >> + return ret; >> + >> ret = reset_control_assert(qmp->ufs_reset); >> if (ret) >> return ret; >> @@ -1817,7 +1856,7 @@ static int qmp_ufs_phy_calibrate(struct phy *phy) >> dev_err(qmp->dev, "phy initialization timed-out\n"); >> return ret; >> ===================================================================================================== >> >> >> Regards. >> Nitin >>> >>>> >>>> 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 >>>>>> >>>>> >>>> >>> >>> >> > >
On Wed, 16 Apr 2025 at 12:08, Nitin Rawat <quic_nitirawa@quicinc.com> wrote: > > > > On 4/15/2025 2:59 PM, Dmitry Baryshkov wrote: > > On 14/04/2025 23:34, Nitin Rawat wrote: > >> > >> > >> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote: > >>> 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. > >> > >> Hi Dmitry, > >> > >> Thanks for pointing this out. I agree that it leaves very little time > >> for the PHY to probe, which may cause issues with targets where > >> no_pcs_sw_reset is set to true. > >> > >> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750, > >> and it caused bootup probe issues in some of the iterations I ran. > >> > >> To address this, I propose updating the patch to move the > >> qmp_ufs_get_phy_reset call to phy_calibrate, just before the > >> reset_control_assert call. > > > > Will it cause an issue if we move it to phy_init() instead of > > phy_calibrate()? > > Hi Dmitry, > > Thanks for suggestion. > Phy_init is invoked before phy_set_mode_ext and ufs_qcom_phy_power_on, > whereas calibrate is called after ufs_qcom_phy_power_on. Keeping the UFS > PHY reset in phy_calibrate introduces relatively more delay, providing > more buffer time for the PHY driver probe, ensuring the UFS PHY reset is > handled correctly the first time. We are requesting the PHY anyway, so the PHY driver should have probed well before phy_init() call. I don't get this comment. > > Moving the calibration to phy_init shouldn't cause any issues. However, > since we currently don't have an initialization operations registered > for init, we would need to add a new PHY initialization ops if we decide > to move it to phy_init. Yes. I don't see it as a problem. Is there any kind of an issue there? > > Please let me know if this looks fine to you, or if you have any > suggestions. I am open to your suggestions. phy_init() callback
On 4/16/2025 5:43 PM, Dmitry Baryshkov wrote: > On Wed, 16 Apr 2025 at 12:08, Nitin Rawat <quic_nitirawa@quicinc.com> wrote: >> >> >> >> On 4/15/2025 2:59 PM, Dmitry Baryshkov wrote: >>> On 14/04/2025 23:34, Nitin Rawat wrote: >>>> >>>> >>>> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote: >>>>> 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. >>>> >>>> Hi Dmitry, >>>> >>>> Thanks for pointing this out. I agree that it leaves very little time >>>> for the PHY to probe, which may cause issues with targets where >>>> no_pcs_sw_reset is set to true. >>>> >>>> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750, >>>> and it caused bootup probe issues in some of the iterations I ran. >>>> >>>> To address this, I propose updating the patch to move the >>>> qmp_ufs_get_phy_reset call to phy_calibrate, just before the >>>> reset_control_assert call. >>> >>> Will it cause an issue if we move it to phy_init() instead of >>> phy_calibrate()? >> >> Hi Dmitry, >> >> Thanks for suggestion. >> Phy_init is invoked before phy_set_mode_ext and ufs_qcom_phy_power_on, >> whereas calibrate is called after ufs_qcom_phy_power_on. Keeping the UFS >> PHY reset in phy_calibrate introduces relatively more delay, providing >> more buffer time for the PHY driver probe, ensuring the UFS PHY reset is >> handled correctly the first time. > > We are requesting the PHY anyway, so the PHY driver should have probed > well before phy_init() call. I don't get this comment. > >> >> Moving the calibration to phy_init shouldn't cause any issues. However, >> since we currently don't have an initialization operations registered >> for init, we would need to add a new PHY initialization ops if we decide >> to move it to phy_init. > > Yes. I don't see it as a problem. Is there any kind of an issue there? No issues. In my next patchset, I would add a new init ops registered for qcom phy and move get ufs phy reset handler to it. Regards, Nitin > >> >> Please let me know if this looks fine to you, or if you have any >> suggestions. I am open to your suggestions. > > phy_init() callback >