Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

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
>>>>>
>>>>
>>>
>>
>>
> 


-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy