| 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 >
On 4/10/25 11:00 AM, Nitin Rawat wrote: > Introduce a new phy calibrate API call in the UFS Qualcomm driver to > separate phy calibration from phy power-on. This change is a precursor > to the next patchset in this series, which requires these two operations > to be distinct. > > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 1b37449fbffc..4998656e9267 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -473,6 +473,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) > goto out_disable_phy; > } > > + ret = phy_calibrate(phy); > + if (ret) { > + dev_err(hba->dev, "%s: Failed to calibrate PHY %d\n", Please add a colon, so that it becomes "..PHY: %d\n" > + __func__, ret); Avoid __func__, this print is fine without it Shouldn't we fail the power-on if this can't succeed? Konrad
On 4/23/2025 4:12 PM, Konrad Dybcio wrote: > On 4/10/25 11:00 AM, Nitin Rawat wrote: >> Introduce a new phy calibrate API call in the UFS Qualcomm driver to >> separate phy calibration from phy power-on. This change is a precursor >> to the next patchset in this series, which requires these two operations >> to be distinct. >> >> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> >> --- >> drivers/ufs/host/ufs-qcom.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index 1b37449fbffc..4998656e9267 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -473,6 +473,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) >> goto out_disable_phy; >> } >> >> + ret = phy_calibrate(phy); >> + if (ret) { >> + dev_err(hba->dev, "%s: Failed to calibrate PHY %d\n", > > Please add a colon, so that it becomes "..PHY: %d\n" > >> + __func__, ret); > Avoid __func__, this print is fine without it Sure will update this in next patchset. > > Shouldn't we fail the power-on if this can't succeed? Thanks for the catch. Yes we should return power-on failure if calibrate fails. Even if there is calibrate phy ops registered, it will return 0. So for so nonzero return value we should treat failure and fail poweron. Sure will this in next patchset. Thanks, Nitin > > Konrad
On 4/16/25 2:26 PM, Nitin Rawat wrote: > > > 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. I don't really like this circular dependency. So I took a peek at the docs and IIUC, they say that the reset coming from the UFS controller effectively does the same thing as QPHY_SW_RESET. Moreover, the docs mention the controller-side reset should not be used anymore (as of at least X1E & SM8550). Docs for MSM8996 (one of the oldest platforms that this driver supports) also don't really mention a hard dependency on it. So we can get rid of this mess entirely, without affecting backwards compatibility even, as this is all contained within the PHYs register space and driver. Konrad
On 4/23/25 1:09 PM, Konrad Dybcio wrote: > On 4/16/25 2:26 PM, Nitin Rawat wrote: >> >> >> 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. > > I don't really like this circular dependency. > > So I took a peek at the docs and IIUC, they say that the reset coming > from the UFS controller effectively does the same thing as QPHY_SW_RESET. > > Moreover, the docs mention the controller-side reset should not be used > anymore (as of at least X1E & SM8550). Docs for MSM8996 (one of the > oldest platforms that this driver supports) also don't really mention a > hard dependency on it. > > So we can get rid of this mess entirely, without affecting backwards > compatibility even, as this is all contained within the PHYs register > space and driver. Well hmm, certain platforms (with no_pcs_sw_reset) don't agree.. some have GCC-sourced resets, but I'm not 100% sure how they affect the CSR state Konrad
On 4/23/2025 4:51 PM, Konrad Dybcio wrote: > On 4/23/25 1:09 PM, Konrad Dybcio wrote: >> On 4/16/25 2:26 PM, Nitin Rawat wrote: >>> >>> >>> 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. >> >> I don't really like this circular dependency. >> >> So I took a peek at the docs and IIUC, they say that the reset coming >> from the UFS controller effectively does the same thing as QPHY_SW_RESET. >> >> Moreover, the docs mention the controller-side reset should not be used >> anymore (as of at least X1E & SM8550). Docs for MSM8996 (one of the >> oldest platforms that this driver supports) also don't really mention a >> hard dependency on it. >> >> So we can get rid of this mess entirely, without affecting backwards >> compatibility even, as this is all contained within the PHYs register >> space and driver. > > Well hmm, certain platforms (with no_pcs_sw_reset) don't agree.. some > have GCC-sourced resets, but I'm not 100% sure how they affect the CSR > state Hi Konrad, I agree with you, but there are still some targets (Sdm845, SM7150, SM6125, and MSM8996) that have upstream support and require a controller-side GCC reset. Therefore to align with HPG we can't remove gcc reset for these targets. Please let me know your opinions. Regards, nitin > > Konrad
On 4/11/25 1:08 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. devlink? EPROBE_DEFER? is this really an issue? Konrad
On 23/04/2025 16:47, Konrad Dybcio wrote: > On 4/11/25 1:08 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. > > devlink? EPROBE_DEFER? is this really an issue? Yes, it is. No, devlink won't help.