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/[email protected]/ 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!
>From 50dfff3541566eb094e931bd56c80011f29b9817 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean <[email protected]> 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 <[email protected]> --- Cc: "James E.J. Bottomley" <[email protected]> Cc: Manivannan Sadhasivam <[email protected]> Cc: "Martin K. Petersen" <[email protected]> Cc: Nitin Rawat <[email protected]> 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
