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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

On Fri, Mar 27, 2026 at 12:22:46PM +0530, Manivannan Sadhasivam wrote:
> 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

Understood. Thanks for testing.

I'm still not satisfied with this level of complexity. If I get you
right, ufs_qcom_phy_power_off() is still needed because phy_calibrate()
expects a "fresh after power on" state, otherwise it fails? That would
be the second reason, apart from the first one I already identified
(undo a phy_power_on() done prior to phy_init()).

If so, could you please test the 3 patches attached (no relationship
with anything else we've exchanged thus far)?

[-- Attachment #2: 0001-phy-qcom-qmp-ufs-support-dynamic-gear-changing.patch --]
[-- Type: text/x-diff, Size: 2013 bytes --]

From 2d42c2d40e6ddfd0c73fc39601f93f7b81a42401 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 27 Mar 2026 12:41:00 +0200
Subject: [PATCH 1/3] phy: qcom-qmp-ufs: support dynamic gear changing

Currently, phy_set_mode_ext() on the QMP UFS PHY expects the PHY to be
powered down, and it makes no change to the hardware state, instead
phy_power_on() followed by phy_calibrate() must be run afterwards.

"Order of API calls" from Documentation/driver-api/phy/phy.rst has a
roundabout and not really clear way of saying that both calling
sequences should be supported. This was further discussed here,
documentation is pending an update:
https://lore.kernel.org/linux-phy/E1vo0mF-00000007kbg-1OeA@rmk-PC.armlinux.org.uk/

By absorbing the phy_power_off() -> ... -> phy_power_on() ->
phy_configure() surrounding sequence into phy_set_mode_ext(), consumer
drivers can be greatly simplified, and we also have a proper
self-standing phy_set_mode_ext() implementation which does not rely on
other calls to do its job.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index df138a5442eb..e75b059bf246 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -2004,15 +2004,24 @@ static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
+	bool powered_on = phy->power_count;
 
 	if (submode > cfg->max_supported_gear || submode == 0) {
 		dev_err(qmp->dev, "Invalid PHY submode %d\n", submode);
 		return -EINVAL;
 	}
 
+	if (powered_on)
+		qmp_ufs_power_off(phy);
+
 	qmp->mode = mode;
 	qmp->submode = submode;
 
+	if (powered_on) {
+		qmp_ufs_power_on(phy);
+		return qmp_ufs_phy_calibrate(phy);
+	}
+
 	return 0;
 }
 
-- 
2.34.1


[-- Attachment #3: 0002-scsi-ufs-qcom-call-phy_init-before-phy_power_on.patch --]
[-- Type: text/x-diff, Size: 3707 bytes --]

From 8d156781d38597865da37a86417f553143d74eaa Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 27 Mar 2026 13:14:39 +0200
Subject: [PATCH 2/3] scsi: ufs: qcom: call phy_init() before phy_power_on()

The Qualcomm UFS host controller driver violates the Generic PHY API
expectation, documented in section "Order of API calls" from
Documentation/driver-api/phy/phy.rst, and then tries to hide it.

The expectation is that calls must be made in the phy_init() ->
phy_power_on() -> phy_power_off() -> phy_exit() sequence.

What we actually have is:

ufshcd_init()
-> ufshcd_hba_init()
   -> ufshcd_setup_clocks(hba, true)
      -> ufshcd_vops_setup_clocks(hba, true, POST_CHANGE)
         -> ufs_qcom_setup_clocks(hba, true, POST_CHANGE)
            -> phy_power_on(phy)
   -> ufshcd_variant_hba_init()
      -> ufs_qcom_init()
         -> ufs_qcom_setup_clocks(hba, true, POST_CHANGE)
            -> phy_power_on(phy)
-> ufshcd_hba_enable()
   -> ufshcd_vops_hce_enable_notify()
      -> ufs_qcom_hce_enable_notify()
         -> ufs_qcom_power_up_sequence()
            -> if (phy->power_count) phy_power_off(phy)
            -> phy_init(phy)

This "works" because the way that the "phy_power_on was called before
phy_init\n" condition is detected in phy-core.c is if the power_count is
positive at the phy_init() call time.

By having that "if (phy->power_count) phy_power_off(phy)" logic, the
ufs-qcom.c technically sidesteps the test, but actually violates the
Generic PHY API even more (calls phy_power_on() *and* phy_power_off()
before phy_init()).

The reason why I stumbled upon this was that I was trying to remove
dereferences of phy->power_count from drivers. This is a PHY-internal
field, and using it from drivers is highly likely to be incorrect, as
this case showcases rather well.

As commit 77d2fa54a945 ("scsi: ufs: qcom : Refactor phy_power_on/off
calls") shows, this driver tries to couple the PHY power state with the
HBA clocks, for power saving reasons. I won't try to change that, I will
just move the phy_init() call earlier, to ufs_qcom_init().

After the phy_init() movement, ufs_qcom_power_up_sequence() should no
longer need to do either phy_init() nor the conditional phy_power_off().
However, phy_power_off() is still needed, for a separate reason which
will be dealt with separately.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Nitin Rawat <quic_nitirawa@quicinc.com>

v5->v6: rewrite after actually understanding the core issue
v4->v5: patch is new
---
 drivers/ufs/host/ufs-qcom.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 375fd24ba458..ffa70c6c7143 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -513,13 +513,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 
 
 	/* phy initialization - calibrate the phy */
-	ret = phy_init(phy);
-	if (ret) {
-		dev_err(hba->dev, "%s: phy init failed, ret = %d\n",
-			__func__, ret);
-		return ret;
-	}
-
 	ret = phy_set_mode_ext(phy, mode, host->phy_gear);
 	if (ret)
 		goto out_disable_phy;
@@ -1441,6 +1434,13 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	if (err)
 		goto out_variant_clear;
 
+	err = phy_init(host->generic_phy);
+	if (err) {
+		dev_err(hba->dev, "%s: phy_init failed, ret = %d\n",
+			__func__, err);
+		goto out_variant_clear;
+	}
+
 	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
 
 	ufs_qcom_get_default_testbus_cfg(host);
-- 
2.34.1


[-- Attachment #4: 0003-scsi-ufs-qcom-make-use-of-QMP-PHY-dynamic-gear-switc.patch --]
[-- Type: text/x-diff, Size: 1696 bytes --]

From 88f4bdfee770cd433a940a14e318d8c8b5dfa516 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 27 Mar 2026 13:18:05 +0200
Subject: [PATCH 3/3] scsi: ufs: qcom: make use of QMP PHY dynamic gear
 switching ability

The QMP UFS PHY can now tolerate having phy_set_mode_ext() being called
while the PHY is powered up. We no longer need to power it down, back up
and calibrate it.

Simplify ufs_qcom_power_up_sequence() by relying on just phy_set_mode_ext()
and let PHY power management be handled just by ufs_qcom_setup_clocks().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/ufs/host/ufs-qcom.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index ffa70c6c7143..cf7b67f2021e 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -508,37 +508,14 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 	if (ret)
 		return ret;
 
-	if (phy->power_count)
-		phy_power_off(phy);
-
-
 	/* phy initialization - calibrate the phy */
 	ret = phy_set_mode_ext(phy, mode, host->phy_gear);
 	if (ret)
-		goto out_disable_phy;
-
-	/* power on phy - start serdes and phy's power and clocks */
-	ret = phy_power_on(phy);
-	if (ret) {
-		dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
-			__func__, ret);
-		goto out_disable_phy;
-	}
-
-	ret = phy_calibrate(phy);
-	if (ret) {
-		dev_err(hba->dev, "Failed to calibrate PHY: %d\n", ret);
-		goto out_disable_phy;
-	}
+		return ret;
 
 	ufs_qcom_select_unipro_mode(host);
 
 	return 0;
-
-out_disable_phy:
-	phy_exit(phy);
-
-	return ret;
 }
 
 /*
-- 
2.34.1


[-- Attachment #5: Type: text/plain, Size: 112 bytes --]

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