[PATCH 2/3] scsi: ufs: qcom: call phy_init() before phy_power_on()

[Vladimir Oltean]

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@xxxxxxx>
---
Cc: "James E.J. Bottomley" <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
Cc: Manivannan Sadhasivam <mani@xxxxxxxxxx>
Cc: "Martin K. Petersen" <martin.petersen@xxxxxxxxxx>
Cc: Nitin Rawat <quic_nitirawa@xxxxxxxxxxx>

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


--5nqk7qqoimem4szp
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0003-scsi-ufs-qcom-make-use-of-QMP-PHY-dynamic-gear-switc.patch"