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

[Nitin Rawat <quic_nitirawa@quicinc.com>]

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


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