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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

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