As explained in the similar ufs-exynos.c change, PHY consumer drivers
should not look at the phy->power_count, because in the general case
there might also be other consumers who have called phy_power_on() too,
so the fact that the power_count is non-zero does not mean that we did.
Moreover, struct phy will become opaque soon, so the qcom UFS driver
will not be able to apply this pattern. Keep parallel track of the PHY
power state, instead of looking at a field which will become unavailable
(phy->power_count).
About treating the phy_power_off() return code: from an API perspective,
this should have probably returned void, otherwise consumers would be
stuck in a state they can't escape. The provider, phy-qcom-qmp-ufs.c,
does return 0 in its power_off() implementation. I consider it safe to
discard potential errors from phy_power_off() instead of complicating
the phy_powered_on logic.
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>
v4->v5: patch is new
---
drivers/ufs/host/ufs-qcom.c | 9 +++++++--
drivers/ufs/host/ufs-qcom.h | 1 +
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 375fd24ba458..3b8bd9968235 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -508,9 +508,10 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
if (ret)
return ret;
- if (phy->power_count)
+ if (host->phy_powered_on) {
phy_power_off(phy);
-
+ host->phy_powered_on = false;
+ }
/* phy initialization - calibrate the phy */
ret = phy_init(phy);
@@ -531,6 +532,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
__func__, ret);
goto out_disable_phy;
}
+ host->phy_powered_on = true;
ret = phy_calibrate(phy);
if (ret) {
@@ -1268,6 +1270,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
return err;
}
+ host->phy_powered_on = false;
}
break;
case POST_CHANGE:
@@ -1277,6 +1280,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
dev_err(hba->dev, "phy power on failed, ret = %d\n", err);
return err;
}
+ host->phy_powered_on = true;
/* enable the device ref clock for HS mode*/
if (ufshcd_is_hs_mode(&hba->pwr_info))
@@ -1467,6 +1471,7 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
ufs_qcom_disable_lane_clks(host);
phy_power_off(host->generic_phy);
+ host->phy_powered_on = false;
phy_exit(host->generic_phy);
}
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 1111ab34da01..72ce0687fa42 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -282,6 +282,7 @@ struct ufs_qcom_host {
struct clk_bulk_data *clks;
u32 num_clks;
bool is_lane_clks_enabled;
+ bool phy_powered_on;
struct icc_path *icc_ddr;
struct icc_path *icc_cpu;
--
2.43.0On Fri, Mar 20, 2026 at 12:32:24AM +0200, Vladimir Oltean wrote:
> As explained in the similar ufs-exynos.c change, PHY consumer drivers
> should not look at the phy->power_count, because in the general case
> there might also be other consumers who have called phy_power_on() too,
> so the fact that the power_count is non-zero does not mean that we did.
>
> Moreover, struct phy will become opaque soon, so the qcom UFS driver
> will not be able to apply this pattern. Keep parallel track of the PHY
> power state, instead of looking at a field which will become unavailable
> (phy->power_count).
>
> About treating the phy_power_off() return code: from an API perspective,
> this should have probably returned void, otherwise consumers would be
> stuck in a state they can't escape. The provider, phy-qcom-qmp-ufs.c,
> does return 0 in its power_off() implementation. I consider it safe to
> discard potential errors from phy_power_off() instead of complicating
> the phy_powered_on logic.
>
You could even simplify the code by getting rid of the 'phy_powered_on' check
altogether. There is no real need to track the PHY power state in this driver.
It is safe to call phy_power_off() without any checks.
- Mani
> 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>
>
> v4->v5: patch is new
> ---
> drivers/ufs/host/ufs-qcom.c | 9 +++++++--
> drivers/ufs/host/ufs-qcom.h | 1 +
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 375fd24ba458..3b8bd9968235 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -508,9 +508,10 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> if (ret)
> return ret;
>
> - if (phy->power_count)
> + if (host->phy_powered_on) {
> phy_power_off(phy);
> -
> + host->phy_powered_on = false;
> + }
>
> /* phy initialization - calibrate the phy */
> ret = phy_init(phy);
> @@ -531,6 +532,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> __func__, ret);
> goto out_disable_phy;
> }
> + host->phy_powered_on = true;
>
> ret = phy_calibrate(phy);
> if (ret) {
> @@ -1268,6 +1270,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
> return err;
> }
> + host->phy_powered_on = false;
> }
> break;
> case POST_CHANGE:
> @@ -1277,6 +1280,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> dev_err(hba->dev, "phy power on failed, ret = %d\n", err);
> return err;
> }
> + host->phy_powered_on = true;
>
> /* enable the device ref clock for HS mode*/
> if (ufshcd_is_hs_mode(&hba->pwr_info))
> @@ -1467,6 +1471,7 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
>
> ufs_qcom_disable_lane_clks(host);
> phy_power_off(host->generic_phy);
> + host->phy_powered_on = false;
> phy_exit(host->generic_phy);
> }
>
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 1111ab34da01..72ce0687fa42 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -282,6 +282,7 @@ struct ufs_qcom_host {
> struct clk_bulk_data *clks;
> u32 num_clks;
> bool is_lane_clks_enabled;
> + bool phy_powered_on;
>
> struct icc_path *icc_ddr;
> struct icc_path *icc_cpu;
> --
> 2.43.0
>
--
மணிவண்ணன் சதாசிவம்
On Tue, Mar 24, 2026 at 11:00:10AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Mar 20, 2026 at 12:32:24AM +0200, Vladimir Oltean wrote:
> > As explained in the similar ufs-exynos.c change, PHY consumer drivers
> > should not look at the phy->power_count, because in the general case
> > there might also be other consumers who have called phy_power_on() too,
> > so the fact that the power_count is non-zero does not mean that we did.
> >
> > Moreover, struct phy will become opaque soon, so the qcom UFS driver
> > will not be able to apply this pattern. Keep parallel track of the PHY
> > power state, instead of looking at a field which will become unavailable
> > (phy->power_count).
> >
> > About treating the phy_power_off() return code: from an API perspective,
> > this should have probably returned void, otherwise consumers would be
> > stuck in a state they can't escape. The provider, phy-qcom-qmp-ufs.c,
> > does return 0 in its power_off() implementation. I consider it safe to
> > discard potential errors from phy_power_off() instead of complicating
> > the phy_powered_on logic.
> >
>
> You could even simplify the code by getting rid of the 'phy_powered_on' check
> altogether. There is no real need to track the PHY power state in this driver.
> It is safe to call phy_power_off() without any checks.
>
> - Mani
Ok.. as the author of commit 7bac65687510 ("scsi: ufs: qcom: Power off
the PHY if it was already powered on in ufs_qcom_power_up_sequence()"),
I assume you have hardware to test. Would you mind writing a patch that
I could pick up to replace this one with?
I suppose that the power_count test is somehow no longer necessary after
commit 77d2fa54a945 ("scsi: ufs: qcom : Refactor phy_power_on/off
calls"), but frankly I don't see it - the ufshcd state machine is a bit
too complicated for me to just statically analyze.
On Wed, Mar 25, 2026 at 01:43:09PM +0200, Vladimir Oltean wrote:
> On Tue, Mar 24, 2026 at 11:00:10AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Mar 20, 2026 at 12:32:24AM +0200, Vladimir Oltean wrote:
> > > As explained in the similar ufs-exynos.c change, PHY consumer drivers
> > > should not look at the phy->power_count, because in the general case
> > > there might also be other consumers who have called phy_power_on() too,
> > > so the fact that the power_count is non-zero does not mean that we did.
> > >
> > > Moreover, struct phy will become opaque soon, so the qcom UFS driver
> > > will not be able to apply this pattern. Keep parallel track of the PHY
> > > power state, instead of looking at a field which will become unavailable
> > > (phy->power_count).
> > >
> > > About treating the phy_power_off() return code: from an API perspective,
> > > this should have probably returned void, otherwise consumers would be
> > > stuck in a state they can't escape. The provider, phy-qcom-qmp-ufs.c,
> > > does return 0 in its power_off() implementation. I consider it safe to
> > > discard potential errors from phy_power_off() instead of complicating
> > > the phy_powered_on logic.
> > >
> >
> > You could even simplify the code by getting rid of the 'phy_powered_on' check
> > altogether. There is no real need to track the PHY power state in this driver.
> > It is safe to call phy_power_off() without any checks.
> >
> > - Mani
>
> Ok.. as the author of commit 7bac65687510 ("scsi: ufs: qcom: Power off
> the PHY if it was already powered on in ufs_qcom_power_up_sequence()"),
> I assume you have hardware to test. Would you mind writing a patch that
> I could pick up to replace this one with?
>
Sure, will do.
> I suppose that the power_count test is somehow no longer necessary after
> commit 77d2fa54a945 ("scsi: ufs: qcom : Refactor phy_power_on/off
> calls"), but frankly I don't see it - the ufshcd state machine is a bit
> too complicated for me to just statically analyze.
I believe I added the power_count check for phy_exit(). But since that got
moved, the check becomes no longer necessary.
- Mani
--
மணிவண்ணன் சதாசிவம்
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.
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!
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@oss.qualcomm.com/
>
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
--
மணிவண்ணன் சதாசிவம்
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)?
© 2016 - 2026 Red Hat, Inc.