Re: [PATCH v5 phy-next 10/27] scsi: ufs: qcom: keep parallel track of PHY power state

[Manivannan Sadhasivam]

On Thu, Mar 26, 2026 at 10:04:44AM +0200, Vladimir Oltean wrote:
> On Wed, Mar 25, 2026 at 01:57:31PM +0200, Vladimir Oltean wrote:
> > On Wed, Mar 25, 2026 at 05:21:14PM +0530, Manivannan Sadhasivam wrote:
> > > I believe I added the power_count check for phy_exit(). But since that got
> > > moved, the check becomes no longer necessary.
> > 
> > FYI, the power_count keeps track of the balance of phy_power_on() and
> > phy_power_off() calls, whereas it is the init_count keeps track of
> > phy_init() and phy_exit() calls. They are only related to the extent
> > that you must respect the phy_init() -> phy_power_on() -> phy_power_off()
> > -> phy_exit() sequence. But in any case, both should be considered
> > PHY-internal fields. The "Order of API calls" section from
> > Documentation/driver-api/phy/phy.rst mentions the order that I just
> > described above, and consumers should just ensure they follow that.
> 
> Ok, so we can close this topic of "checking the power_count not needed"
> by linking to the conversation which spun off here:
> https://lore.kernel.org/lkml/20260325120122.265973-1-manivannan.sadhasivam@xxxxxxxxxxxxxxxx/
> 

Sure.

> Mani, I spent some more time to figure out what's really going on with
> this unexpected phy_power_off() call. Do you think you could
> regression-test the patch attached?
> 

I tested the patch. But it fails ufs_qcom_power_up_sequence() if PHY was already
powered on:

[   31.513321] qcom-qmp-ufs-phy 1d87000.phy: phy initialization timed-out
[   31.513335] ufshcd-qcom 1d84000.ufshc: Failed to calibrate PHY: -110
[   31.565273] ufshcd-qcom 1d84000.ufshc: Enabling the controller failed

Funny thing is, it didn't affect the functionality since the UFS core retries
ufshcd_hba_enable() and in the error path of ufs_qcom_power_up_sequence(),
phy_power_off() gets called and that causes the next try to succeed. So it is
evident that, if PHY was already powered ON, it should be powered off before
ufs_qcom_phy_power_on(). And due to the UFS driver design,
ufs_qcom_power_up_sequence() can get called multiple times. So we cannot just
remove phy_power_off().

Below diff on top of your patch fixes the issue:

```
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index ed067247d72a..2c9fe03f349e 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -567,6 +567,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
        if (ret)
                return ret;
 
+       ufs_qcom_phy_power_off(host);
+
        ret = ufs_qcom_phy_set_gear(host, mode);
        if (ret) {
                dev_err(hba->dev, "%s: phy_set_mode_ext() failed, ret = %d\n",
```

- Mani

-- 
மணிவண்ணன் சதாசிவம்