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

Vladimir Oltean
2026-03-26T08:05:12Z

[-- 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

[-- Attachment #3: Type: text/plain, Size: 161 bytes --]