mbox series

[V3,0/9] Refactor phy powerup sequence

Message ID 20250410090102.20781-1-quic_nitirawa@quicinc.com
Headers show
Series Refactor phy powerup sequence | expand

Message

Nitin Rawat April 10, 2025, 9 a.m. UTC
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.

This patch series is tested on SM8550 QRD, SM8650 MTP , SM8750 MTP.

Changes in v3:
1. Addresed neil and bjorn comment to align the order of the patch to
   maintain the bisectability compliance within the patch.
2. Addressed neil comment to move qmp_ufs_get_phy_reset() in a separate
   patch, inline qmp_ufs_com_init() inline.

Changes in v2:
1. Addressed vinod koul and manivannan comment to split the phy patch
   into multiple patches.
2. Addressed vinod's comment to reuse SW_PWRDN instead of creating
   new macros SW_PWRUP in phy-qcom-qmp-ufs.c.
3. Addressed Konrad's comment to optimize mutex lock in ufs-qcom.c
4. Addressed konrad and Manivannan comment to clean debug print in
   ufs-qcom.c

Nitin Rawat (9):
  scsi: ufs: qcom: add a new phy calibrate API call
  phy: qcom-qmp-ufs: Rename qmp_ufs_enable and qmp_ufs_power_on
  phy: qcom-qmp-ufs: Refactor phy_power_on and phy_calibrate callbacks
  phy: qcom-qmp-ufs: Refactor UFS PHY reset
  phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()
  phy: qcom-qmp-ufs: Refactor qmp_ufs_exit callback.
  scsi: ufs: qcom : Refactor phy_power_on/off calls
  scsi: ufs: qcom : Introduce phy_power_on/off wrapper function
  scsi: ufs: qcom: Prevent calling phy_exit before phy_init

 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 145 ++++++++----------------
 drivers/ufs/host/ufs-qcom.c             |  96 ++++++++++------
 drivers/ufs/host/ufs-qcom.h             |   4 +
 3 files changed, 111 insertions(+), 134 deletions(-)

--
2.48.1

Comments

Nitin Rawat April 11, 2025, 10:35 a.m. UTC | #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.
>>
>
Dmitry Baryshkov April 11, 2025, 11:08 a.m. UTC | #2
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
> >>
> >
>
Nitin Rawat April 14, 2025, 8:34 p.m. UTC | #3
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
>>>>
>>>
>>
> 
>
Dmitry Baryshkov April 15, 2025, 9:29 a.m. UTC | #4
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
>>>>>
>>>>
>>>
>>
>>
>
Nitin Rawat April 16, 2025, 9:08 a.m. UTC | #5
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
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
>
Dmitry Baryshkov April 16, 2025, 12:13 p.m. UTC | #6
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
Nitin Rawat April 16, 2025, 12:26 p.m. UTC | #7
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
>