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

[Nitin Rawat]

On 4/16/2025 5:43 PM, Dmitry Baryshkov wrote:
> On Wed, 16 Apr 2025 at 12:08, Nitin Rawat <quic_nitirawa@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > > 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