[PATCH V4 00/11] Refactor ufs phy powerup sequence

[PATCH V4 00/11] Refactor ufs phy powerup sequence

[Nitin Rawat]

In Current code regulators enable, clks enable, calibrating UFS PHY,
start_serdes and polling PCS_ready_status are part of phy_power_on.

UFS PHY registers are retained after power collapse, meaning calibrating
UFS PHY, start_serdes and polling PCS_ready_status can be done only when
hba is powered_on, and not needed every time when phy_power_on is called
during resume. Hence keep the code which enables PHY's regulators & clks
in phy_power_on and move the rest steps into phy_calibrate function.

Since phy_power_on is separated out from phy calibrate, make separate calls
to phy_power_on and phy_calibrate calls from ufs qcom driver.

Also for better power saving, remove the phy_power_on/off calls from
resume/suspend path and put them to ufs_qcom_setup_clocks, so that
PHY's regulators & clks can be turned on/off along with UFS's clocks.

This patch series is tested on SM8550 QRD, SM8650 MTP , SM8750 MTP.

Note: Patch 1 of this series is a requirement for the rest of the PHY
      patches for the functional dependency.

Changes in v4:
1. Addressed Dmitry's comment to update cover letter to mention patch1
   as a requirement for the rest of the PHY patches.
2. Addressed Dmitry's comment to move parsing UFS PHY reset handle logic
   to init from probe to avoid probe failure.
3. Addressed Dmitry's comment to update commit text to reflect reason
   to remove qmp_ufs_com_init() (Patch 7 of current series)
4. Addrssed Konrad's comment to return failure from power up sequence when
    phy_calibrate return failure and modify the debug print.

Changes in v3:
1. Addresed neil and bjorn comment to align the order of the patch to
   maintain the bisectability compliance within the patch.
2. Addressed neil comment to move qmp_ufs_get_phy_reset() in a separate
   patch, inline qmp_ufs_com_init() inline.

Changes in v2:
1. Addressed vinod koul and manivannan comment to split the phy patch
   into multiple patches.
2. Addressed vinod's comment to reuse SW_PWRDN instead of creating
   new macros SW_PWRUP in phy-qcom-qmp-ufs.c.
3. Addressed Konrad's comment to optimize mutex lock in ufs-qcom.c
4. Addressed konrad and Manivannan comment to clean debug print in
   ufs-qcom.c


Nitin Rawat (11):
  scsi: ufs: qcom: add a new phy calibrate API call
  phy: qcom-qmp-ufs: Rename qmp_ufs_enable and qmp_ufs_power_on
  phy: qcom-qmp-ufs: Refactor phy_power_on and phy_calibrate callbacks
  phy: qcom-qmp-ufs: Refactor UFS PHY reset
  phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()
  phy: qcom-qmp-ufs: Rename qmp_ufs_power_off
  phy: qcom-qmp-ufs: Remove qmp_ufs_exit() and Inline qmp_ufs_com_exit()
  phy: qcom-qmp-ufs: refactor qmp_ufs_power_off
  scsi: ufs: qcom : Refactor phy_power_on/off calls
  scsi: ufs: qcom : Introduce phy_power_on/off wrapper function
  scsi: ufs: qcom: Prevent calling phy_exit before phy_init

 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 145 ++++++++----------------
 drivers/ufs/host/ufs-qcom.c             |  96 ++++++++++------
 drivers/ufs/host/ufs-qcom.h             |   4 +
 3 files changed, 111 insertions(+), 134 deletions(-)

--
2.48.1

[PATCH V4 01/11] scsi: ufs: qcom: add a new phy calibrate API call

[Nitin Rawat]

Introduce a new phy calibrate API call in the UFS Qualcomm driver to
separate phy calibration from phy power-on. This change is a precursor
to the next patchset in this series, which requires these two operations
to be distinct.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 1b37449fbffc..2cd44ee522b8 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -473,6 +473,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 		goto out_disable_phy;
 	}

+	ret = phy_calibrate(phy);
+	if (ret) {
+		dev_err(hba->dev, "Failed to calibrate PHY: %d\n", ret);
+		goto out_disable_phy;
+	}
+
 	ufs_qcom_select_unipro_mode(host);

 	return 0;
--
2.48.1

[PATCH V4 02/11] phy: qcom-qmp-ufs: Rename qmp_ufs_enable and qmp_ufs_power_on

[Nitin Rawat]

Rename qmp_ufs_enable to qmp_ufs_power_on and qmp_ufs_power_on to
qmp_ufs_phy_calibrate to better reflect their functionality. Also
update function calls and structure assignments accordingly.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Co-developed-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 45b3b792696e..bb836bc0f736 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1837,7 +1837,7 @@ static int qmp_ufs_init(struct phy *phy)
 	return 0;
 }

-static int qmp_ufs_power_on(struct phy *phy)
+static int qmp_ufs_phy_calibrate(struct phy *phy)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
@@ -1898,7 +1898,7 @@ static int qmp_ufs_exit(struct phy *phy)
 	return 0;
 }

-static int qmp_ufs_enable(struct phy *phy)
+static int qmp_ufs_power_on(struct phy *phy)
 {
 	int ret;

@@ -1906,7 +1906,7 @@ static int qmp_ufs_enable(struct phy *phy)
 	if (ret)
 		return ret;

-	ret = qmp_ufs_power_on(phy);
+	ret = qmp_ufs_phy_calibrate(phy);
 	if (ret)
 		qmp_ufs_exit(phy);

@@ -1940,7 +1940,7 @@ static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 }

 static const struct phy_ops qcom_qmp_ufs_phy_ops = {
-	.power_on	= qmp_ufs_enable,
+	.power_on	= qmp_ufs_power_on,
 	.power_off	= qmp_ufs_disable,
 	.set_mode	= qmp_ufs_set_mode,
 	.owner		= THIS_MODULE,
--
2.48.1

[PATCH V4 03/11] phy: qcom-qmp-ufs: Refactor phy_power_on and phy_calibrate callbacks

[Nitin Rawat]

Commit 052553af6a31 ("ufs/phy: qcom: Refactor to use phy_init call")
puts enabling regulators & clks, calibrating UFS PHY, starting serdes
and polling PCS ready status into phy_power_on.

In Current code regulators enable, clks enable, calibrating UFS PHY,
start_serdes and polling PCS_ready_status are part of phy_power_on.

UFS PHY registers are retained after power collapse, meaning calibrating
UFS PHY, start_serdes and polling PCS_ready_status can be done only when
hba is powered_on, and not needed every time when phy_power_on is called
during resume. Hence keep the code which enables PHY's regulators & clks
in phy_power_on and move the rest steps into phy_calibrate function.

Refactor the code to retain PHY regulators & clks in phy_power_on and
move out rest of the code to new phy_calibrate function.

Also move reset_control_assert to qmp_ufs_phy_calibrate to align
with Hardware programming guide.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 26 ++++++-------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index bb836bc0f736..636dc3dc3ea8 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1796,7 +1796,7 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
 	return 0;
 }

-static int qmp_ufs_init(struct phy *phy)
+static int qmp_ufs_power_on(struct phy *phy)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
@@ -1824,10 +1824,6 @@ static int qmp_ufs_init(struct phy *phy)
 				return ret;
 			}
 		}
-
-		ret = reset_control_assert(qmp->ufs_reset);
-		if (ret)
-			return ret;
 	}

 	ret = qmp_ufs_com_init(qmp);
@@ -1846,6 +1842,10 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
 	unsigned int val;
 	int ret;

+	ret = reset_control_assert(qmp->ufs_reset);
+	if (ret)
+		return ret;
+
 	qmp_ufs_init_registers(qmp, cfg);

 	ret = reset_control_deassert(qmp->ufs_reset);
@@ -1898,21 +1898,6 @@ static int qmp_ufs_exit(struct phy *phy)
 	return 0;
 }

-static int qmp_ufs_power_on(struct phy *phy)
-{
-	int ret;
-
-	ret = qmp_ufs_init(phy);
-	if (ret)
-		return ret;
-
-	ret = qmp_ufs_phy_calibrate(phy);
-	if (ret)
-		qmp_ufs_exit(phy);
-
-	return ret;
-}
-
 static int qmp_ufs_disable(struct phy *phy)
 {
 	int ret;
@@ -1942,6 +1927,7 @@ static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 static const struct phy_ops qcom_qmp_ufs_phy_ops = {
 	.power_on	= qmp_ufs_power_on,
 	.power_off	= qmp_ufs_disable,
+	.calibrate	= qmp_ufs_phy_calibrate,
 	.set_mode	= qmp_ufs_set_mode,
 	.owner		= THIS_MODULE,
 };
--
2.48.1

[PATCH V4 04/11] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Nitin Rawat]

Refactor the UFS PHY reset handling to parse the reset logic only once
during initialization, instead of every resume.

As part of this change, move the UFS PHY reset parsing logic from
qmp_phy_power_on to the new qmp_ufs_phy_init function.

Co-developed-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 59 +++++++++++++------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 636dc3dc3ea8..43d2d714f28b 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1799,38 +1799,11 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
 static int qmp_ufs_power_on(struct phy *phy)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
-	const struct qmp_phy_cfg *cfg = qmp->cfg;
 	int ret;
 	dev_vdbg(qmp->dev, "Initializing QMP phy\n");

-	if (cfg->no_pcs_sw_reset) {
-		/*
-		 * Get UFS reset, which is delayed until now to avoid a
-		 * circular dependency where UFS needs its PHY, but the PHY
-		 * needs this UFS reset.
-		 */
-		if (!qmp->ufs_reset) {
-			qmp->ufs_reset =
-				devm_reset_control_get_exclusive(qmp->dev,
-								 "ufsphy");
-
-			if (IS_ERR(qmp->ufs_reset)) {
-				ret = PTR_ERR(qmp->ufs_reset);
-				dev_err(qmp->dev,
-					"failed to get UFS reset: %d\n",
-					ret);
-
-				qmp->ufs_reset = NULL;
-				return ret;
-			}
-		}
-	}
-
 	ret = qmp_ufs_com_init(qmp);
-	if (ret)
-		return ret;
-
-	return 0;
+	return ret;
 }

 static int qmp_ufs_phy_calibrate(struct phy *phy)
@@ -1924,7 +1897,37 @@ static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 	return 0;
 }

+static int qmp_ufs_phy_init(struct phy *phy)
+{
+	struct qmp_ufs *qmp = phy_get_drvdata(phy);
+	const struct qmp_phy_cfg *cfg = qmp->cfg;
+	int ret;
+
+	if (!cfg->no_pcs_sw_reset)
+		return 0;
+
+	/*
+	 * Get UFS reset, which is delayed until now to avoid a
+	 * circular dependency where UFS needs its PHY, but the PHY
+	 * needs this UFS reset.
+	 */
+	if (!qmp->ufs_reset) {
+		qmp->ufs_reset =
+			devm_reset_control_get_exclusive(qmp->dev, "ufsphy");
+
+		if (IS_ERR(qmp->ufs_reset)) {
+			ret = PTR_ERR(qmp->ufs_reset);
+			dev_err(qmp->dev, "failed to get PHY reset: %d\n", ret);
+			qmp->ufs_reset = NULL;
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static const struct phy_ops qcom_qmp_ufs_phy_ops = {
+	.init		= qmp_ufs_phy_init,
 	.power_on	= qmp_ufs_power_on,
 	.power_off	= qmp_ufs_disable,
 	.calibrate	= qmp_ufs_phy_calibrate,
--
2.48.1

[PATCH V4 05/11] phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()

[Nitin Rawat]

The qmp_ufs_power_on() function acts as a wrapper, solely invoking
qmp_ufs_com_init(). Additionally, the code within qmp_ufs_com_init()
does not correspond well with its name.

Therefore, to enhance the readability and eliminate unnecessary
function call inline qmp_ufs_com_init() into qmp_ufs_power_on().

There is no change to the functionality.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 44 ++++++++++---------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 43d2d714f28b..94095393148c 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1757,31 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
 	qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
 }

-static int qmp_ufs_com_init(struct qmp_ufs *qmp)
-{
-	const struct qmp_phy_cfg *cfg = qmp->cfg;
-	void __iomem *pcs = qmp->pcs;
-	int ret;
-
-	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
-	if (ret) {
-		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
-		return ret;
-	}
-
-	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
-	if (ret)
-		goto err_disable_regulators;
-
-	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
-
-	return 0;
-
-err_disable_regulators:
-	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
-
-	return ret;
-}

 static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
 {
@@ -1799,10 +1774,27 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
 static int qmp_ufs_power_on(struct phy *phy)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
+	const struct qmp_phy_cfg *cfg = qmp->cfg;
+	void __iomem *pcs = qmp->pcs;
 	int ret;
+
 	dev_vdbg(qmp->dev, "Initializing QMP phy\n");

-	ret = qmp_ufs_com_init(qmp);
+	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
+	if (ret) {
+		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
+		return ret;
+	}
+
+	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
+	if (ret)
+		goto err_disable_regulators;
+
+	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
+	return 0;
+
+err_disable_regulators:
+	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
 	return ret;
 }

--
2.48.1

[PATCH V4 06/11] phy: qcom-qmp-ufs: Rename qmp_ufs_power_off

[Nitin Rawat]

Rename qmp_ufs_disable to qmp_ufs_power_off to better represent its
functionality. Additionally, move the qmp_ufs_exit() call inside
qmp_ufs_power_off to preserve the functionality of .power_off.

There is no functional change.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 30 +++++++++----------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 94095393148c..c501223fc5f9 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1835,6 +1835,15 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
 	return 0;
 }

+static int qmp_ufs_exit(struct phy *phy)
+{
+	struct qmp_ufs *qmp = phy_get_drvdata(phy);
+
+	qmp_ufs_com_exit(qmp);
+
+	return 0;
+}
+
 static int qmp_ufs_power_off(struct phy *phy)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
@@ -1851,28 +1860,11 @@ static int qmp_ufs_power_off(struct phy *phy)
 	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
 			SW_PWRDN);

-	return 0;
-}
-
-static int qmp_ufs_exit(struct phy *phy)
-{
-	struct qmp_ufs *qmp = phy_get_drvdata(phy);
-
-	qmp_ufs_com_exit(qmp);
+	qmp_ufs_exit(phy);

 	return 0;
 }

-static int qmp_ufs_disable(struct phy *phy)
-{
-	int ret;
-
-	ret = qmp_ufs_power_off(phy);
-	if (ret)
-		return ret;
-	return qmp_ufs_exit(phy);
-}
-
 static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
@@ -1921,7 +1913,7 @@ static int qmp_ufs_phy_init(struct phy *phy)
 static const struct phy_ops qcom_qmp_ufs_phy_ops = {
 	.init		= qmp_ufs_phy_init,
 	.power_on	= qmp_ufs_power_on,
-	.power_off	= qmp_ufs_disable,
+	.power_off	= qmp_ufs_power_off,
 	.calibrate	= qmp_ufs_phy_calibrate,
 	.set_mode	= qmp_ufs_set_mode,
 	.owner		= THIS_MODULE,
--
2.48.1

[PATCH V4 07/11] phy: qcom-qmp-ufs: Remove qmp_ufs_exit() and Inline qmp_ufs_com_exit()

[Nitin Rawat]

qmp_ufs_exit() is a wrapper function. It only calls qmp_ufs_com_exit().
Remove it to simplify the ufs phy driver.

Additonally partial Inline(dropping the reset assert) qmp_ufs_com_exit
into qmp_ufs_power_off function to avoid unnecessary function call.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 29 +++++--------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index c501223fc5f9..2df61ec68dc7 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1757,20 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
 	qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
 }

-
-static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
-{
-	const struct qmp_phy_cfg *cfg = qmp->cfg;
-
-	reset_control_assert(qmp->ufs_reset);
-
-	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
-
-	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
-
-	return 0;
-}
-
 static int qmp_ufs_power_on(struct phy *phy)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
@@ -1835,15 +1821,6 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
 	return 0;
 }

-static int qmp_ufs_exit(struct phy *phy)
-{
-	struct qmp_ufs *qmp = phy_get_drvdata(phy);
-
-	qmp_ufs_com_exit(qmp);
-
-	return 0;
-}
-
 static int qmp_ufs_power_off(struct phy *phy)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
@@ -1860,7 +1837,11 @@ static int qmp_ufs_power_off(struct phy *phy)
 	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
 			SW_PWRDN);

-	qmp_ufs_exit(phy);
+	/* Turn off all the phy clocks */
+	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
+
+	/* Turn off all the phy rails */
+	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);

 	return 0;
 }
--
2.48.1

[PATCH V4 08/11] phy: qcom-qmp-ufs: refactor qmp_ufs_power_off

[Nitin Rawat]

In qmp_ufs_power_off, the PHY is already powered down by asserting
QPHY_PCS_POWER_DOWN_CONTROL. Therefore, additional phy_reset and
stopping SerDes are unnecessary. Also this approach does not
align with the phy HW programming guide.

Thus, refactor qmp_ufs_power_off to remove the phy_reset and stop
SerDes calls to simplify the code and ensure alignment with the PHY
HW programming guide.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 2df61ec68dc7..1cbc255c7c74 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1826,13 +1826,6 @@ static int qmp_ufs_power_off(struct phy *phy)
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;

-	/* PHY reset */
-	if (!cfg->no_pcs_sw_reset)
-		qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
-
-	/* stop SerDes */
-	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_START_CTRL], SERDES_START);
-
 	/* Put PHY into POWER DOWN state: active low */
 	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
 			SW_PWRDN);
--
2.48.1

[PATCH V4 09/11] scsi: ufs: qcom : Refactor phy_power_on/off calls

[Nitin Rawat]

Commit 3f6d1767b1a0 ("phy: ufs-qcom: Refactor all init steps into
phy_poweron") removes the phy_power_on/off from ufs_qcom_setup_clocks
to suspend/resume func.

To have a better power saving, remove the phy_power_on/off calls from
resume/suspend path and put them back to ufs_qcom_setup_clocks, so that
PHY regulators & clks can be turned on/off along with UFS's clocks.

Since phy phy_power_on is separated out from phy calibrate, make
separate calls to phy_power_on and phy_calibrate calls from ufs qcom
driver.

Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 55 ++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 32 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 2cd44ee522b8..ff35cd15c72f 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -639,26 +639,17 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
 	enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	struct phy *phy = host->generic_phy;
 
 	if (status == PRE_CHANGE)
 		return 0;
 
-	if (ufs_qcom_is_link_off(hba)) {
-		/*
-		 * Disable the tx/rx lane symbol clocks before PHY is
-		 * powered down as the PLL source should be disabled
-		 * after downstream clocks are disabled.
-		 */
+	if (!ufs_qcom_is_link_active(hba))
 		ufs_qcom_disable_lane_clks(host);
-		phy_power_off(phy);
 
-		/* reset the connected UFS device during power down */
-		ufs_qcom_device_reset_ctrl(hba, true);
 
-	} else if (!ufs_qcom_is_link_active(hba)) {
-		ufs_qcom_disable_lane_clks(host);
-	}
+	/* reset the connected UFS device during power down */
+	if (ufs_qcom_is_link_off(hba) && host->device_reset)
+		ufs_qcom_device_reset_ctrl(hba, true);
 
 	return ufs_qcom_ice_suspend(host);
 }
@@ -666,26 +657,11 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
 static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	struct phy *phy = host->generic_phy;
 	int err;
 
-	if (ufs_qcom_is_link_off(hba)) {
-		err = phy_power_on(phy);
-		if (err) {
-			dev_err(hba->dev, "%s: failed PHY power on: %d\n",
-				__func__, err);
-			return err;
-		}
-
-		err = ufs_qcom_enable_lane_clks(host);
-		if (err)
-			return err;
-
-	} else if (!ufs_qcom_is_link_active(hba)) {
-		err = ufs_qcom_enable_lane_clks(host);
-		if (err)
-			return err;
-	}
+	err = ufs_qcom_enable_lane_clks(host);
+	if (err)
+		return err;
 
 	return ufs_qcom_ice_resume(host);
 }
@@ -1042,6 +1018,8 @@ 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 = host->generic_phy;
+	int err;
 
 	/*
 	 * In case ufs_qcom_init() is not yet done, simply ignore.
@@ -1060,10 +1038,22 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 				/* disable device ref_clk */
 				ufs_qcom_dev_ref_clk_ctrl(host, false);
 			}
+			err = phy_power_off(phy);
+			if (err) {
+				dev_err(hba->dev, "%s: phy power off failed, ret=%d\n",
+					__func__, err);
+					return err;
+			}
 		}
 		break;
 	case POST_CHANGE:
 		if (on) {
+			err = phy_power_on(phy);
+			if (err) {
+				dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
+					__func__, err);
+				return err;
+			}
 			/* enable the device ref clock for HS mode*/
 			if (ufshcd_is_hs_mode(&hba->pwr_info))
 				ufs_qcom_dev_ref_clk_ctrl(host, true);
@@ -1246,9 +1236,10 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 static void ufs_qcom_exit(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct phy *phy = host->generic_phy;
 
 	ufs_qcom_disable_lane_clks(host);
-	phy_power_off(host->generic_phy);
+	phy_power_off(phy);
 	phy_exit(host->generic_phy);
 }
 
-- 
2.48.1

[PATCH V4 10/11] scsi: ufs: qcom : Introduce phy_power_on/off wrapper function

[Nitin Rawat]

Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper
functions with mutex protection to ensure safe usage of is_phy_pwr_on
and prevent possible race conditions.

Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 44 +++++++++++++++++++++++++++++++------
 drivers/ufs/host/ufs-qcom.h |  4 ++++
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index ff35cd15c72f..a7e9e06847f8 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -421,6 +421,38 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba)
 	return UFS_HS_G3;
 }
 
+static int ufs_qcom_phy_power_on(struct ufs_hba *hba)
+{
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct phy *phy = host->generic_phy;
+	int ret = 0;
+
+	guard(mutex)(&host->phy_mutex);
+	if (!host->is_phy_pwr_on) {
+		ret = phy_power_on(phy);
+		if (!ret)
+			host->is_phy_pwr_on = true;
+	}
+
+	return ret;
+}
+
+static int ufs_qcom_phy_power_off(struct ufs_hba *hba)
+{
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct phy *phy = host->generic_phy;
+	int ret = 0;
+
+	guard(mutex)(&host->phy_mutex);
+	if (host->is_phy_pwr_on) {
+		ret = phy_power_off(phy);
+		if (!ret)
+			host->is_phy_pwr_on = false;
+	}
+
+	return ret;
+}
+
 static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
@@ -449,7 +481,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 		return ret;
 
 	if (phy->power_count) {
-		phy_power_off(phy);
+		ufs_qcom_phy_power_off(hba);
 		phy_exit(phy);
 	}
 
@@ -466,7 +498,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 		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(hba);
 	if (ret) {
 		dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
 			__func__, ret);
@@ -1018,7 +1050,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 = host->generic_phy;
 	int err;
 
 	/*
@@ -1038,7 +1069,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 				/* disable device ref_clk */
 				ufs_qcom_dev_ref_clk_ctrl(host, false);
 			}
-			err = phy_power_off(phy);
+			err = ufs_qcom_phy_power_off(hba);
 			if (err) {
 				dev_err(hba->dev, "%s: phy power off failed, ret=%d\n",
 					__func__, err);
@@ -1048,7 +1079,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 		break;
 	case POST_CHANGE:
 		if (on) {
-			err = phy_power_on(phy);
+			err = ufs_qcom_phy_power_on(hba);
 			if (err) {
 				dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
 					__func__, err);
@@ -1236,10 +1267,9 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 static void ufs_qcom_exit(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	struct phy *phy = host->generic_phy;
 
 	ufs_qcom_disable_lane_clks(host);
-	phy_power_off(phy);
+	ufs_qcom_phy_power_off(hba);
 	phy_exit(host->generic_phy);
 }
 
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index d0e6ec9128e7..3db29fbcd40b 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -252,6 +252,10 @@ struct ufs_qcom_host {
 	u32 phy_gear;
 
 	bool esi_enabled;
+	/* flag to check if phy is powered on */
+	bool is_phy_pwr_on;
+	/* Protect the usage of is_phy_pwr_on against racing */
+	struct mutex phy_mutex;
 };
 
 struct ufs_qcom_drvdata {
-- 
2.48.1

[PATCH V4 11/11] scsi: ufs: qcom: Prevent calling phy_exit before phy_init

[Nitin Rawat]

Prevent calling phy_exit before phy_init to avoid abnormal power
count and the following warning during boot up.

[5.146763] phy phy-1d80000.phy.0: phy_power_on was called before phy_init

Fixes: 7bac65687510 ("scsi: ufs: qcom: Power off the PHY if it was already powered on in ufs_qcom_power_up_sequence()")
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index a7e9e06847f8..db51e1e7d836 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -482,7 +482,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)

 	if (phy->power_count) {
 		ufs_qcom_phy_power_off(hba);
-		phy_exit(phy);
 	}

 	/* phy initialization - calibrate the phy */
--
2.48.1

Re: [PATCH V4 04/11] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Dmitry Baryshkov]

On Sat, May 03, 2025 at 09:54:33PM +0530, Nitin Rawat wrote:
> Refactor the UFS PHY reset handling to parse the reset logic only once
> during initialization, instead of every resume.
> 
> As part of this change, move the UFS PHY reset parsing logic from
> qmp_phy_power_on to the new qmp_ufs_phy_init function.
> 
> Co-developed-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 59 +++++++++++++------------
>  1 file changed, 31 insertions(+), 28 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

-- 
With best wishes
Dmitry

Re: [PATCH V4 06/11] phy: qcom-qmp-ufs: Rename qmp_ufs_power_off

[Dmitry Baryshkov]

On Sat, May 03, 2025 at 09:54:35PM +0530, Nitin Rawat wrote:
> Rename qmp_ufs_disable to qmp_ufs_power_off to better represent its
> functionality. Additionally, move the qmp_ufs_exit() call inside
> qmp_ufs_power_off to preserve the functionality of .power_off.
> 
> There is no functional change.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 30 +++++++++----------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 94095393148c..c501223fc5f9 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1835,6 +1835,15 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
>  	return 0;
>  }
> 
> +static int qmp_ufs_exit(struct phy *phy)
> +{
> +	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> +
> +	qmp_ufs_com_exit(qmp);

Just inline it, unless you have any other plans.

> +
> +	return 0;
> +}
> +
>  static int qmp_ufs_power_off(struct phy *phy)
>  {
>  	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> @@ -1851,28 +1860,11 @@ static int qmp_ufs_power_off(struct phy *phy)
>  	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
>  			SW_PWRDN);
> 
> -	return 0;
> -}
> -
> -static int qmp_ufs_exit(struct phy *phy)
> -{
> -	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> -
> -	qmp_ufs_com_exit(qmp);
> +	qmp_ufs_exit(phy);
> 
>  	return 0;
>  }
> 
> -static int qmp_ufs_disable(struct phy *phy)
> -{
> -	int ret;
> -
> -	ret = qmp_ufs_power_off(phy);
> -	if (ret)
> -		return ret;
> -	return qmp_ufs_exit(phy);
> -}
> -
>  static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>  {
>  	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> @@ -1921,7 +1913,7 @@ static int qmp_ufs_phy_init(struct phy *phy)
>  static const struct phy_ops qcom_qmp_ufs_phy_ops = {
>  	.init		= qmp_ufs_phy_init,
>  	.power_on	= qmp_ufs_power_on,
> -	.power_off	= qmp_ufs_disable,
> +	.power_off	= qmp_ufs_power_off,
>  	.calibrate	= qmp_ufs_phy_calibrate,
>  	.set_mode	= qmp_ufs_set_mode,
>  	.owner		= THIS_MODULE,
> --
> 2.48.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH V4 06/11] phy: qcom-qmp-ufs: Rename qmp_ufs_power_off

[Nitin Rawat]

On 5/4/2025 9:07 PM, Dmitry Baryshkov wrote:
> On Sat, May 03, 2025 at 09:54:35PM +0530, Nitin Rawat wrote:
>> Rename qmp_ufs_disable to qmp_ufs_power_off to better represent its
>> functionality. Additionally, move the qmp_ufs_exit() call inside
>> qmp_ufs_power_off to preserve the functionality of .power_off.
>>
>> There is no functional change.
>>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 30 +++++++++----------------
>>   1 file changed, 11 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> index 94095393148c..c501223fc5f9 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> @@ -1835,6 +1835,15 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
>>   	return 0;
>>   }
>>
>> +static int qmp_ufs_exit(struct phy *phy)
>> +{
>> +	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>> +
>> +	qmp_ufs_com_exit(qmp);
> 
> Just inline it, unless you have any other plans.

Hi Dmitry,

I have inlined qcom_ufs_com_exit in patch #7 of the same series. I 
separated it into a different patch to keep each patch simpler.

Could you please review patch #7 and share your thoughts.

[PATCH V4 07/11] phy: qcom-qmp-ufs: Remove qmp_ufs_exit() and Inline 
qmp_ufs_com_exit().


Regards,
Nitin


> 
>> +
>> +	return 0;
>> +}
>> +
>>   static int qmp_ufs_power_off(struct phy *phy)
>>   {
>>   	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>> @@ -1851,28 +1860,11 @@ static int qmp_ufs_power_off(struct phy *phy)
>>   	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
>>   			SW_PWRDN);
>>
>> -	return 0;
>> -}
>> -
>> -static int qmp_ufs_exit(struct phy *phy)
>> -{
>> -	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>> -
>> -	qmp_ufs_com_exit(qmp);
>> +	qmp_ufs_exit(phy);
>>
>>   	return 0;
>>   }
>>
>> -static int qmp_ufs_disable(struct phy *phy)
>> -{
>> -	int ret;
>> -
>> -	ret = qmp_ufs_power_off(phy);
>> -	if (ret)
>> -		return ret;
>> -	return qmp_ufs_exit(phy);
>> -}
>> -
>>   static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>>   {
>>   	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>> @@ -1921,7 +1913,7 @@ static int qmp_ufs_phy_init(struct phy *phy)
>>   static const struct phy_ops qcom_qmp_ufs_phy_ops = {
>>   	.init		= qmp_ufs_phy_init,
>>   	.power_on	= qmp_ufs_power_on,
>> -	.power_off	= qmp_ufs_disable,
>> +	.power_off	= qmp_ufs_power_off,
>>   	.calibrate	= qmp_ufs_phy_calibrate,
>>   	.set_mode	= qmp_ufs_set_mode,
>>   	.owner		= THIS_MODULE,
>> --
>> 2.48.1
>>
> 

Re: [PATCH V4 07/11] phy: qcom-qmp-ufs: Remove qmp_ufs_exit() and Inline qmp_ufs_com_exit()

[Dmitry Baryshkov]

On Sat, May 03, 2025 at 09:54:36PM +0530, Nitin Rawat wrote:
> qmp_ufs_exit() is a wrapper function. It only calls qmp_ufs_com_exit().
> Remove it to simplify the ufs phy driver.
> 
> Additonally partial Inline(dropping the reset assert) qmp_ufs_com_exit
> into qmp_ufs_power_off function to avoid unnecessary function call.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 29 +++++--------------------
>  1 file changed, 5 insertions(+), 24 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

-- 
With best wishes
Dmitry

Re: [PATCH V4 06/11] phy: qcom-qmp-ufs: Rename qmp_ufs_power_off

[Dmitry Baryshkov]

On Sun, May 04, 2025 at 06:37:41PM +0300, Dmitry Baryshkov wrote:
> On Sat, May 03, 2025 at 09:54:35PM +0530, Nitin Rawat wrote:
> > Rename qmp_ufs_disable to qmp_ufs_power_off to better represent its
> > functionality. Additionally, move the qmp_ufs_exit() call inside
> > qmp_ufs_power_off to preserve the functionality of .power_off.
> > 
> > There is no functional change.
> > 
> > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 30 +++++++++----------------
> >  1 file changed, 11 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > index 94095393148c..c501223fc5f9 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > @@ -1835,6 +1835,15 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
> >  	return 0;
> >  }
> > 
> > +static int qmp_ufs_exit(struct phy *phy)
> > +{
> > +	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> > +
> > +	qmp_ufs_com_exit(qmp);
> 
> Just inline it, unless you have any other plans.

I mean, inline a call to qmp_ufs_com_exit() into qmp_ufs_power_off().

> 
> > +
> > +	return 0;
> > +}
> > +
> >  static int qmp_ufs_power_off(struct phy *phy)
> >  {
> >  	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> > @@ -1851,28 +1860,11 @@ static int qmp_ufs_power_off(struct phy *phy)
> >  	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> >  			SW_PWRDN);
> > 
> > -	return 0;
> > -}
> > -
> > -static int qmp_ufs_exit(struct phy *phy)
> > -{
> > -	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> > -
> > -	qmp_ufs_com_exit(qmp);
> > +	qmp_ufs_exit(phy);
> > 
> >  	return 0;
> >  }
> > 
> > -static int qmp_ufs_disable(struct phy *phy)
> > -{
> > -	int ret;
> > -
> > -	ret = qmp_ufs_power_off(phy);
> > -	if (ret)
> > -		return ret;
> > -	return qmp_ufs_exit(phy);
> > -}
> > -
> >  static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> >  {
> >  	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> > @@ -1921,7 +1913,7 @@ static int qmp_ufs_phy_init(struct phy *phy)
> >  static const struct phy_ops qcom_qmp_ufs_phy_ops = {
> >  	.init		= qmp_ufs_phy_init,
> >  	.power_on	= qmp_ufs_power_on,
> > -	.power_off	= qmp_ufs_disable,
> > +	.power_off	= qmp_ufs_power_off,
> >  	.calibrate	= qmp_ufs_phy_calibrate,
> >  	.set_mode	= qmp_ufs_set_mode,
> >  	.owner		= THIS_MODULE,
> > --
> > 2.48.1
> > 
> 
> -- 
> With best wishes
> Dmitry
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
With best wishes
Dmitry

Re: [PATCH V4 08/11] phy: qcom-qmp-ufs: refactor qmp_ufs_power_off

[Dmitry Baryshkov]

On Sat, May 03, 2025 at 09:54:37PM +0530, Nitin Rawat wrote:
> In qmp_ufs_power_off, the PHY is already powered down by asserting
> QPHY_PCS_POWER_DOWN_CONTROL. Therefore, additional phy_reset and
> stopping SerDes are unnecessary. Also this approach does not
> align with the phy HW programming guide.
> 
> Thus, refactor qmp_ufs_power_off to remove the phy_reset and stop
> SerDes calls to simplify the code and ensure alignment with the PHY
> HW programming guide.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 7 -------
>  1 file changed, 7 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

-- 
With best wishes
Dmitry

Re: [PATCH V4 06/11] phy: qcom-qmp-ufs: Rename qmp_ufs_power_off

[Dmitry Baryshkov]

On Sun, May 04, 2025 at 09:22:06PM +0530, Nitin Rawat wrote:
> 
> 
> On 5/4/2025 9:07 PM, Dmitry Baryshkov wrote:
> > On Sat, May 03, 2025 at 09:54:35PM +0530, Nitin Rawat wrote:
> > > Rename qmp_ufs_disable to qmp_ufs_power_off to better represent its
> > > functionality. Additionally, move the qmp_ufs_exit() call inside
> > > qmp_ufs_power_off to preserve the functionality of .power_off.
> > > 
> > > There is no functional change.
> > > 
> > > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> > > ---
> > >   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 30 +++++++++----------------
> > >   1 file changed, 11 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > > index 94095393148c..c501223fc5f9 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > > @@ -1835,6 +1835,15 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
> > >   	return 0;
> > >   }
> > > 
> > > +static int qmp_ufs_exit(struct phy *phy)
> > > +{
> > > +	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> > > +
> > > +	qmp_ufs_com_exit(qmp);
> > 
> > Just inline it, unless you have any other plans.
> 
> Hi Dmitry,
> 
> I have inlined qcom_ufs_com_exit in patch #7 of the same series. I separated
> it into a different patch to keep each patch simpler.

You have inlined qmp_ufs_com_exit() contents. Here I've asked you to
inline qmp_ufs_exit(), keeping qmp_ufs_com_exit() as is.

> 
> Could you please review patch #7 and share your thoughts.
> 
> [PATCH V4 07/11] phy: qcom-qmp-ufs: Remove qmp_ufs_exit() and Inline
> qmp_ufs_com_exit().
> 
> 
> Regards,
> Nitin
> 
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static int qmp_ufs_power_off(struct phy *phy)
> > >   {
> > >   	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> > > @@ -1851,28 +1860,11 @@ static int qmp_ufs_power_off(struct phy *phy)
> > >   	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> > >   			SW_PWRDN);
> > > 
> > > -	return 0;
> > > -}
> > > -
> > > -static int qmp_ufs_exit(struct phy *phy)
> > > -{
> > > -	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> > > -
> > > -	qmp_ufs_com_exit(qmp);
> > > +	qmp_ufs_exit(phy);
> > > 
> > >   	return 0;
> > >   }
> > > 
> > > -static int qmp_ufs_disable(struct phy *phy)
> > > -{
> > > -	int ret;
> > > -
> > > -	ret = qmp_ufs_power_off(phy);
> > > -	if (ret)
> > > -		return ret;
> > > -	return qmp_ufs_exit(phy);
> > > -}
> > > -
> > >   static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> > >   {
> > >   	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> > > @@ -1921,7 +1913,7 @@ static int qmp_ufs_phy_init(struct phy *phy)
> > >   static const struct phy_ops qcom_qmp_ufs_phy_ops = {
> > >   	.init		= qmp_ufs_phy_init,
> > >   	.power_on	= qmp_ufs_power_on,
> > > -	.power_off	= qmp_ufs_disable,
> > > +	.power_off	= qmp_ufs_power_off,
> > >   	.calibrate	= qmp_ufs_phy_calibrate,
> > >   	.set_mode	= qmp_ufs_set_mode,
> > >   	.owner		= THIS_MODULE,
> > > --
> > > 2.48.1
> > > 
> > 
> 
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
With best wishes
Dmitry

Re: [PATCH V4 06/11] phy: qcom-qmp-ufs: Rename qmp_ufs_power_off

[Nitin Rawat]

On 5/6/2025 5:23 PM, Dmitry Baryshkov wrote:
> On Sun, May 04, 2025 at 09:22:06PM +0530, Nitin Rawat wrote:
>>
>>
>> On 5/4/2025 9:07 PM, Dmitry Baryshkov wrote:
>>> On Sat, May 03, 2025 at 09:54:35PM +0530, Nitin Rawat wrote:
>>>> Rename qmp_ufs_disable to qmp_ufs_power_off to better represent its
>>>> functionality. Additionally, move the qmp_ufs_exit() call inside
>>>> qmp_ufs_power_off to preserve the functionality of .power_off.
>>>>
>>>> There is no functional change.
>>>>
>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>> ---
>>>>    drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 30 +++++++++----------------
>>>>    1 file changed, 11 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> index 94095393148c..c501223fc5f9 100644
>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> @@ -1835,6 +1835,15 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
>>>>    	return 0;
>>>>    }
>>>>
>>>> +static int qmp_ufs_exit(struct phy *phy)
>>>> +{
>>>> +	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>>>> +
>>>> +	qmp_ufs_com_exit(qmp);
>>>
>>> Just inline it, unless you have any other plans.
>>
>> Hi Dmitry,
>>
>> I have inlined qcom_ufs_com_exit in patch #7 of the same series. I separated
>> it into a different patch to keep each patch simpler.
> 
> You have inlined qmp_ufs_com_exit() contents. Here I've asked you to
> inline qmp_ufs_exit(), keeping qmp_ufs_com_exit() as is.


Sure Dmitry. I'll update this in next patchset. Thanks
> 
>>
>> Could you please review patch #7 and share your thoughts.
>>
>> [PATCH V4 07/11] phy: qcom-qmp-ufs: Remove qmp_ufs_exit() and Inline
>> qmp_ufs_com_exit().
>>
>>
>> Regards,
>> Nitin
>>
>>
>>>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static int qmp_ufs_power_off(struct phy *phy)
>>>>    {
>>>>    	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>>>> @@ -1851,28 +1860,11 @@ static int qmp_ufs_power_off(struct phy *phy)
>>>>    	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
>>>>    			SW_PWRDN);
>>>>
>>>> -	return 0;
>>>> -}
>>>> -
>>>> -static int qmp_ufs_exit(struct phy *phy)
>>>> -{
>>>> -	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>>>> -
>>>> -	qmp_ufs_com_exit(qmp);
>>>> +	qmp_ufs_exit(phy);
>>>>
>>>>    	return 0;
>>>>    }
>>>>
>>>> -static int qmp_ufs_disable(struct phy *phy)
>>>> -{
>>>> -	int ret;
>>>> -
>>>> -	ret = qmp_ufs_power_off(phy);
>>>> -	if (ret)
>>>> -		return ret;
>>>> -	return qmp_ufs_exit(phy);
>>>> -}
>>>> -
>>>>    static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>>>>    {
>>>>    	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>>>> @@ -1921,7 +1913,7 @@ static int qmp_ufs_phy_init(struct phy *phy)
>>>>    static const struct phy_ops qcom_qmp_ufs_phy_ops = {
>>>>    	.init		= qmp_ufs_phy_init,
>>>>    	.power_on	= qmp_ufs_power_on,
>>>> -	.power_off	= qmp_ufs_disable,
>>>> +	.power_off	= qmp_ufs_power_off,
>>>>    	.calibrate	= qmp_ufs_phy_calibrate,
>>>>    	.set_mode	= qmp_ufs_set_mode,
>>>>    	.owner		= THIS_MODULE,
>>>> --
>>>> 2.48.1
>>>>
>>>
>>
>>
>> -- 
>> linux-phy mailing list
>> linux-phy@lists.infradead.org
>> https://lists.infradead.org/mailman/listinfo/linux-phy
> 

Re: [PATCH V4 09/11] scsi: ufs: qcom : Refactor phy_power_on/off calls

[Konrad Dybcio]

On 5/3/25 6:24 PM, Nitin Rawat wrote:
> Commit 3f6d1767b1a0 ("phy: ufs-qcom: Refactor all init steps into
> phy_poweron") removes the phy_power_on/off from ufs_qcom_setup_clocks
> to suspend/resume func.
> 
> To have a better power saving, remove the phy_power_on/off calls from
> resume/suspend path and put them back to ufs_qcom_setup_clocks, so that
> PHY regulators & clks can be turned on/off along with UFS's clocks.
> 
> Since phy phy_power_on is separated out from phy calibrate, make
> separate calls to phy_power_on and phy_calibrate calls from ufs qcom
> driver.
> 
> Co-developed-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 55 ++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 2cd44ee522b8..ff35cd15c72f 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -639,26 +639,17 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>  	enum ufs_notify_change_status status)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	struct phy *phy = host->generic_phy;
>  
>  	if (status == PRE_CHANGE)
>  		return 0;
>  
> -	if (ufs_qcom_is_link_off(hba)) {
> -		/*
> -		 * Disable the tx/rx lane symbol clocks before PHY is
> -		 * powered down as the PLL source should be disabled
> -		 * after downstream clocks are disabled.
> -		 */
> +	if (!ufs_qcom_is_link_active(hba))

so is_link_off and !is_link_active are not the same thing - this also allows
for disabling the resources when in hibern8/broken states - is that intended?

>  		ufs_qcom_disable_lane_clks(host);
> -		phy_power_off(phy);
>  
> -		/* reset the connected UFS device during power down */
> -		ufs_qcom_device_reset_ctrl(hba, true);
>  
> -	} else if (!ufs_qcom_is_link_active(hba)) {
> -		ufs_qcom_disable_lane_clks(host);
> -	}
> +	/* reset the connected UFS device during power down */
> +	if (ufs_qcom_is_link_off(hba) && host->device_reset)
> +		ufs_qcom_device_reset_ctrl(hba, true);

similarly this will not be allowed in hibern8/broken states now

>  
>  	return ufs_qcom_ice_suspend(host);
>  }
> @@ -666,26 +657,11 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>  static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	struct phy *phy = host->generic_phy;
>  	int err;
>  
> -	if (ufs_qcom_is_link_off(hba)) {
> -		err = phy_power_on(phy);
> -		if (err) {
> -			dev_err(hba->dev, "%s: failed PHY power on: %d\n",
> -				__func__, err);
> -			return err;
> -		}
> -
> -		err = ufs_qcom_enable_lane_clks(host);
> -		if (err)
> -			return err;
> -
> -	} else if (!ufs_qcom_is_link_active(hba)) {
> -		err = ufs_qcom_enable_lane_clks(host);
> -		if (err)
> -			return err;
> -	}
> +	err = ufs_qcom_enable_lane_clks(host);
> +	if (err)
> +		return err;
>  
>  	return ufs_qcom_ice_resume(host);
>  }
> @@ -1042,6 +1018,8 @@ 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 = host->generic_phy;
> +	int err;
>  
>  	/*
>  	 * In case ufs_qcom_init() is not yet done, simply ignore.
> @@ -1060,10 +1038,22 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  				/* disable device ref_clk */
>  				ufs_qcom_dev_ref_clk_ctrl(host, false);
>  			}
> +			err = phy_power_off(phy);

a newline to separate the blocks would improve readability> +			if (err) {
> +				dev_err(hba->dev, "%s: phy power off failed, ret=%d\n",
> +					__func__, err);
> +					return err;

please indent the return statement a tab earlier so it doesn't look
like it's an argument to dev_err()

putting PHY calls in the function that promises to toggle clocks could
also use a comment, maybe extending the kerneldoc to say that certain
clocks come from the PHY so it needs to be managed together

Konrad

Re: [PATCH V4 10/11] scsi: ufs: qcom : Introduce phy_power_on/off wrapper function

[Konrad Dybcio]

On 5/3/25 6:24 PM, Nitin Rawat wrote:
> Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper
> functions with mutex protection to ensure safe usage of is_phy_pwr_on
> and prevent possible race conditions.
> 
> Co-developed-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---

The PHY framework does the same thing internally already, this seems
unnecessary

Konrad

Re: [PATCH V4 11/11] scsi: ufs: qcom: Prevent calling phy_exit before phy_init

[Konrad Dybcio]

On 5/3/25 6:24 PM, Nitin Rawat wrote:
> Prevent calling phy_exit before phy_init to avoid abnormal power
> count and the following warning during boot up.
> 
> [5.146763] phy phy-1d80000.phy.0: phy_power_on was called before phy_init
> 
> Fixes: 7bac65687510 ("scsi: ufs: qcom: Power off the PHY if it was already powered on in ufs_qcom_power_up_sequence()")
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index a7e9e06847f8..db51e1e7d836 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -482,7 +482,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> 
>  	if (phy->power_count) {
>  		ufs_qcom_phy_power_off(hba);
> -		phy_exit(phy);
>  	}

You can also remove the {} now 

since this is a fix for existing issues, which I don't think has any dependencies
on your other changes, please post it as the first patch so that the maintainer
can pick it up more easily

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH V4 10/11] scsi: ufs: qcom : Introduce phy_power_on/off wrapper function

[Nitin Rawat]

On 5/9/2025 5:07 PM, Konrad Dybcio wrote:
> On 5/3/25 6:24 PM, Nitin Rawat wrote:
>> Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper
>> functions with mutex protection to ensure safe usage of is_phy_pwr_on
>> and prevent possible race conditions.
>>
>> Co-developed-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
> 
> The PHY framework does the same thing internally already, this seems
> unnecessary

Hi Konrad,

Thanks for the review. There are scenarios where ufshcd_link_startup() 
can call ufshcd_vops_link_startup_notify() multiple times during 
retries. This leads to the PHY reference count increasing continuously, 
preventing proper re-initialization of the PHY.

Recently, this issue was addressed with patch 7bac65687510 ("scsi: ufs:
qcom: Power off the PHY if it was already powered on in 
ufs_qcom_power_up_sequence()"). However, I still want to maintain a 
reference count (ref_cnt) to safeguard against similar conditions in the 
code. Additionally, this approach helps avoid unnecessary phy_power_on 
and phy_power_off calls. Please let me know your thoughts.

Regards,
Nitin


> 
> Konrad

Re: [PATCH V4 11/11] scsi: ufs: qcom: Prevent calling phy_exit before phy_init

[Nitin Rawat]

On 5/9/2025 5:08 PM, Konrad Dybcio wrote:
> On 5/3/25 6:24 PM, Nitin Rawat wrote:
>> Prevent calling phy_exit before phy_init to avoid abnormal power
>> count and the following warning during boot up.
>>
>> [5.146763] phy phy-1d80000.phy.0: phy_power_on was called before phy_init
>>
>> Fixes: 7bac65687510 ("scsi: ufs: qcom: Power off the PHY if it was already powered on in ufs_qcom_power_up_sequence()")
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index a7e9e06847f8..db51e1e7d836 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -482,7 +482,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>>
>>   	if (phy->power_count) {
>>   		ufs_qcom_phy_power_off(hba);
>> -		phy_exit(phy);
>>   	}
> 
> You can also remove the {} now
Sure will review while posting next patchset.
> 
> since this is a fix for existing issues, which I don't think has any dependencies
> on your other changes, please post it as the first patch so that the maintainer
> can pick it up more easily
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> Konrad

Re: [PATCH V4 10/11] scsi: ufs: qcom : Introduce phy_power_on/off wrapper function

[Konrad Dybcio]

On 5/9/25 1:49 PM, Nitin Rawat wrote:
> 
> 
> On 5/9/2025 5:07 PM, Konrad Dybcio wrote:
>> On 5/3/25 6:24 PM, Nitin Rawat wrote:
>>> Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper
>>> functions with mutex protection to ensure safe usage of is_phy_pwr_on
>>> and prevent possible race conditions.
>>>
>>> Co-developed-by: Can Guo <quic_cang@quicinc.com>
>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>> ---
>>
>> The PHY framework does the same thing internally already, this seems
>> unnecessary
> 
> Hi Konrad,
> 
> Thanks for the review. There are scenarios where ufshcd_link_startup() can call ufshcd_vops_link_startup_notify() multiple times during retries. This leads to the PHY reference count increasing continuously, preventing proper re-initialization of the PHY.

I'm assuming you're talking about the scenario where it jumps into
ufs_qcom_power_up_sequence() - you have a label in there called
`out_disable_phy` - add a phy_power_off() after phy_calibrate if
things fail and you should be good to go if I'm reading things right.

Please include something resembling a call stack in the commit message,
as currently everyone reviewing this has to make guesses about why this
needs to be done


> Recently, this issue was addressed with patch 7bac65687510 ("scsi: ufs:
> qcom: Power off the PHY if it was already powered on in ufs_qcom_power_up_sequence()"). However, I still want to maintain a reference count (ref_cnt) to safeguard against similar conditions in the code. Additionally, this approach helps avoid unnecessary phy_power_on and phy_power_off calls. Please let me know your thoughts.

These unnecessary calls only amount to a couple of jumps and compares,
just like your wrappers, as the framework keeps track of the enable
count as well

Konrad

Re: [PATCH V4 01/11] scsi: ufs: qcom: add a new phy calibrate API call

[Konrad Dybcio]

On 5/3/25 6:24 PM, Nitin Rawat wrote:
> Introduce a new phy calibrate API call in the UFS Qualcomm driver to
> separate phy calibration from phy power-on. This change is a precursor
> to the next patchset in this series, which requires these two operations
> to be distinct.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---

I would imagine this would all go through a single tree - be it phy or
ufs, up to the maintainers - unless you want to merge it right now,
Bart/Martin?

Konrad

Re: [PATCH V4 09/11] scsi: ufs: qcom : Refactor phy_power_on/off calls

[Nitin Rawat]

On 5/9/2025 5:05 PM, Konrad Dybcio wrote:
> On 5/3/25 6:24 PM, Nitin Rawat wrote:
>> Commit 3f6d1767b1a0 ("phy: ufs-qcom: Refactor all init steps into
>> phy_poweron") removes the phy_power_on/off from ufs_qcom_setup_clocks
>> to suspend/resume func.
>>
>> To have a better power saving, remove the phy_power_on/off calls from
>> resume/suspend path and put them back to ufs_qcom_setup_clocks, so that
>> PHY regulators & clks can be turned on/off along with UFS's clocks.
>>
>> Since phy phy_power_on is separated out from phy calibrate, make
>> separate calls to phy_power_on and phy_calibrate calls from ufs qcom
>> driver.
>>
>> Co-developed-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 55 ++++++++++++++++---------------------
>>   1 file changed, 23 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 2cd44ee522b8..ff35cd15c72f 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -639,26 +639,17 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>>   	enum ufs_notify_change_status status)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> -	struct phy *phy = host->generic_phy;
>>   
>>   	if (status == PRE_CHANGE)
>>   		return 0;
>>   
>> -	if (ufs_qcom_is_link_off(hba)) {
>> -		/*
>> -		 * Disable the tx/rx lane symbol clocks before PHY is
>> -		 * powered down as the PLL source should be disabled
>> -		 * after downstream clocks are disabled.
>> -		 */
>> +	if (!ufs_qcom_is_link_active(hba))
> 
> so is_link_off and !is_link_active are not the same thing - this also allows
> for disabling the resources when in hibern8/broken states - is that intended?

Yes is_link_off and !is_link_Active is not same thing. !is_link_active 
also include link in hibern8 state where lane clock is intended to be
disabled because PHY is powered down in hibern8.



> 
>>   		ufs_qcom_disable_lane_clks(host);
>> -		phy_power_off(phy);
>>   
>> -		/* reset the connected UFS device during power down */
>> -		ufs_qcom_device_reset_ctrl(hba, true);
>>   
>> -	} else if (!ufs_qcom_is_link_active(hba)) {
>> -		ufs_qcom_disable_lane_clks(host);
>> -	}
>> +	/* reset the connected UFS device during power down */
>> +	if (ufs_qcom_is_link_off(hba) && host->device_reset)
>> +		ufs_qcom_device_reset_ctrl(hba, true);
> 
> similarly this will not be allowed in hibern8/broken states now
> 
>>   
>>   	return ufs_qcom_ice_suspend(host);
>>   }
>> @@ -666,26 +657,11 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>>   static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> -	struct phy *phy = host->generic_phy;
>>   	int err;
>>   
>> -	if (ufs_qcom_is_link_off(hba)) {
>> -		err = phy_power_on(phy);
>> -		if (err) {
>> -			dev_err(hba->dev, "%s: failed PHY power on: %d\n",
>> -				__func__, err);
>> -			return err;
>> -		}
>> -
>> -		err = ufs_qcom_enable_lane_clks(host);
>> -		if (err)
>> -			return err;
>> -
>> -	} else if (!ufs_qcom_is_link_active(hba)) {
>> -		err = ufs_qcom_enable_lane_clks(host);
>> -		if (err)
>> -			return err;
>> -	}
>> +	err = ufs_qcom_enable_lane_clks(host);
>> +	if (err)
>> +		return err;
>>   
>>   	return ufs_qcom_ice_resume(host);
>>   }
>> @@ -1042,6 +1018,8 @@ 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 = host->generic_phy;
>> +	int err;
>>   
>>   	/*
>>   	 * In case ufs_qcom_init() is not yet done, simply ignore.
>> @@ -1060,10 +1038,22 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>>   				/* disable device ref_clk */
>>   				ufs_qcom_dev_ref_clk_ctrl(host, false);
>>   			}
>> +			err = phy_power_off(phy);
> 
> a newline to separate the blocks would improve readability> +			if (err) {
>> +				dev_err(hba->dev, "%s: phy power off failed, ret=%d\n",
>> +					__func__, err);
>> +					return err;

Sure will add in next patchset.

> 
> please indent the return statement a tab earlier so it doesn't look
> like it's an argument to dev_err()

Sure will add in next patchset.

> 
> putting PHY calls in the function that promises to toggle clocks could
> also use a comment, maybe extending the kerneldoc to say that certain
> clocks come from the PHY so it needs to be managed together
> 
Sure will add a comment or update in kernel doc in next patchset.

> Konrad

Re: [PATCH V4 10/11] scsi: ufs: qcom : Introduce phy_power_on/off wrapper function

[Nitin Rawat]

On 5/9/2025 5:30 PM, Konrad Dybcio wrote:
> On 5/9/25 1:49 PM, Nitin Rawat wrote:
>>
>>
>> On 5/9/2025 5:07 PM, Konrad Dybcio wrote:
>>> On 5/3/25 6:24 PM, Nitin Rawat wrote:
>>>> Introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off wrapper
>>>> functions with mutex protection to ensure safe usage of is_phy_pwr_on
>>>> and prevent possible race conditions.
>>>>
>>>> Co-developed-by: Can Guo <quic_cang@quicinc.com>
>>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>> ---
>>>
>>> The PHY framework does the same thing internally already, this seems
>>> unnecessary
>>
>> Hi Konrad,
>>
>> Thanks for the review. There are scenarios where ufshcd_link_startup() can call ufshcd_vops_link_startup_notify() multiple times during retries. This leads to the PHY reference count increasing continuously, preventing proper re-initialization of the PHY.
> 
> I'm assuming you're talking about the scenario where it jumps into
> ufs_qcom_power_up_sequence() - you have a label in there called
> `out_disable_phy` - add a phy_power_off() after phy_calibrate if
> things fail and you should be good to go if I'm reading things right.

Hi Konrad,

I meant, ufs_qcom_power_up_sequence can be called multiple times from 
ufshcd_link_startup as part of ufshcd_hba_enable calls for each 
retries(max retries =3) and each attempt of ufs_qcom_power_up_sequence 
is success increasing the power_count ref to value more than 1.
But this is handled using the patch 7bac65687510.

Similiar scenarios can be possible in ufs driver , where there can be 2
phy_power_on calls which may caused caused phy ref count to be more than 
1 and this inconsistent behaviour may cause issue.

Hence having is_phy_pwr_on flag can help here.

Thanks,
Nitin

> 
> Please include something resembling a call stack in the commit message,
> as currently everyone reviewing this has to make guesses about why this
> needs to be done
> 
> 
>> Recently, this issue was addressed with patch 7bac65687510 ("scsi: ufs:
>> qcom: Power off the PHY if it was already powered on in ufs_qcom_power_up_sequence()"). However, I still want to maintain a reference count (ref_cnt) to safeguard against similar conditions in the code. Additionally, this approach helps avoid unnecessary phy_power_on and phy_power_off calls. Please let me know your thoughts.
> 
> These unnecessary calls only amount to a couple of jumps and compares,
> just like your wrappers, as the framework keeps track of the enable
> count as well
> 
> Konrad