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: 1224 bytes --]

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@oss.qualcomm.com/

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?

Thanks!

[-- Attachment #2: 0001-scsi-ufs-qcom-don-t-call-phy_power_on-before-phy_ini.patch --]
[-- Type: text/x-diff, Size: 7996 bytes --]

From 50dfff3541566eb094e931bd56c80011f29b9817 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Thu, 26 Mar 2026 10:01:55 +0200
Subject: [PATCH] scsi: ufs: qcom: don't call phy_power_on() before phy_init()

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_down().

Because the UFS variant operations are not balanced, but the PHY API
calls need to be, create wrappers for all Generic PHY API calls, and
keep a "phy_initialized" and a "phy_powered_on" boolean, so that we call
these only once, and they properly get paired with their phy_exit()/
phy_power_off() counterparts rather than leave the phy->init_count and
phy->power_count elevated.

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 | 104 ++++++++++++++++++++++++++----------
 drivers/ufs/host/ufs-qcom.h |   2 +
 2 files changed, 79 insertions(+), 27 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 375fd24ba458..ed067247d72a 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -485,11 +485,70 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba)
 	return UFS_HS_G3;
 }
 
+static int ufs_qcom_phy_init(struct ufs_qcom_host *host)
+{
+	struct phy *phy = host->generic_phy;
+	int err;
+
+	if (host->phy_initialized)
+		return 0;
+
+	err = phy_init(phy);
+	if (err)
+		return err;
+
+	host->phy_initialized = true;
+
+	return 0;
+}
+
+static void ufs_qcom_phy_exit(struct ufs_qcom_host *host)
+{
+	if (host->phy_initialized) {
+		phy_exit(host->generic_phy);
+		host->phy_initialized = false;
+	}
+}
+
+static int ufs_qcom_phy_power_on(struct ufs_qcom_host *host)
+{
+	int err;
+
+	if (host->phy_powered_on)
+		return 0;
+
+	err = phy_power_on(host->generic_phy);
+	if (err)
+		return err;
+
+	host->phy_powered_on = true;
+
+	return 0;
+}
+
+static int ufs_qcom_phy_set_gear(struct ufs_qcom_host *host,
+				 enum phy_mode mode)
+{
+	return phy_set_mode_ext(host->generic_phy, mode, host->phy_gear);
+}
+
+static int ufs_qcom_phy_calibrate(struct ufs_qcom_host *host)
+{
+	return phy_calibrate(host->generic_phy);
+}
+
+static void ufs_qcom_phy_power_off(struct ufs_qcom_host *host)
+{
+	if (host->phy_powered_on) {
+		phy_power_off(host->generic_phy);
+		host->phy_powered_on = false;
+	}
+}
+
 static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufs_host_params *host_params = &host->host_params;
-	struct phy *phy = host->generic_phy;
 	enum phy_mode mode;
 	int ret;
 
@@ -508,31 +567,22 @@ 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_init(phy);
+	ret = ufs_qcom_phy_set_gear(host, mode);
 	if (ret) {
-		dev_err(hba->dev, "%s: phy init failed, ret = %d\n",
+		dev_err(hba->dev, "%s: phy_set_mode_ext() failed, ret = %d\n",
 			__func__, ret);
-		return ret;
-	}
-
-	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);
+	ret = ufs_qcom_phy_power_on(host);
 	if (ret) {
 		dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
 			__func__, ret);
 		goto out_disable_phy;
 	}
 
-	ret = phy_calibrate(phy);
+	ret = ufs_qcom_phy_calibrate(host);
 	if (ret) {
 		dev_err(hba->dev, "Failed to calibrate PHY: %d\n", ret);
 		goto out_disable_phy;
@@ -543,7 +593,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 	return 0;
 
 out_disable_phy:
-	phy_exit(phy);
+	ufs_qcom_phy_power_off(host);
 
 	return ret;
 }
@@ -1233,7 +1283,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 				 enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	struct phy *phy;
 	int err;
 
 	/*
@@ -1244,8 +1293,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 	if (!host)
 		return 0;
 
-	phy = host->generic_phy;
-
 	switch (status) {
 	case PRE_CHANGE:
 		if (on) {
@@ -1263,16 +1310,12 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 				ufs_qcom_dev_ref_clk_ctrl(host, false);
 			}
 
-			err = phy_power_off(phy);
-			if (err) {
-				dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
-				return err;
-			}
+			ufs_qcom_phy_power_off(host);
 		}
 		break;
 	case POST_CHANGE:
 		if (on) {
-			err = phy_power_on(phy);
+			err = ufs_qcom_phy_power_on(host);
 			if (err) {
 				dev_err(hba->dev, "phy power on failed, ret = %d\n", err);
 				return err;
@@ -1441,6 +1484,13 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	if (err)
 		goto out_variant_clear;
 
+	err = ufs_qcom_phy_init(host);
+	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);
@@ -1466,8 +1516,8 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 
 	ufs_qcom_disable_lane_clks(host);
-	phy_power_off(host->generic_phy);
-	phy_exit(host->generic_phy);
+	ufs_qcom_phy_power_off(host);
+	ufs_qcom_phy_exit(host);
 }
 
 static int ufs_qcom_fw_managed_init(struct ufs_hba *hba)
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 1111ab34da01..33b1b1521916 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -282,6 +282,8 @@ struct ufs_qcom_host {
 	struct clk_bulk_data *clks;
 	u32 num_clks;
 	bool is_lane_clks_enabled;
+	bool phy_initialized;
+	bool phy_powered_on;
 
 	struct icc_path *icc_ddr;
 	struct icc_path *icc_cpu;
-- 
2.34.1