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

[PATCH V5 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 v5:
1. Addressed Dmitry's comment to inline inline qmp_ufs_exit into
   qmp_ufs_power_off
2. Addrsssed Konrad's comment to improve code identation and updating
   kernel doc to update the info regarding controller and phy clocks
   being managed together.
3. Addrsssed Konrad's comment to update commit text for patch #10 to
   improve explanation to maintain separate ref_count in ufs controller
   driver.


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             | 102 +++++++++++------
 drivers/ufs/host/ufs-qcom.h             |   4 +
 3 files changed, 117 insertions(+), 134 deletions(-)

--
2.48.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V5 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 37887ec68412..23b4fc84dc8c 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -531,6 +531,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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V5 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 b33e2e2b5014..a67cf0a64f74 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1838,7 +1838,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;
@@ -1899,7 +1899,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;
 
@@ -1907,7 +1907,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);
 
@@ -1941,7 +1941,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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V5 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 a67cf0a64f74..ade8e9c4b9ae 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1797,7 +1797,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;
@@ -1825,10 +1825,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);
@@ -1847,6 +1843,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);
@@ -1899,21 +1899,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;
@@ -1943,6 +1928,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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V5 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>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.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 ade8e9c4b9ae..33d238cf49aa 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1800,38 +1800,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)
@@ -1925,7 +1898,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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V5 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 | 43 ++++++++++---------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 33d238cf49aa..d3f9ee490a32 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1758,12 +1758,28 @@ 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)
+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);
+	const struct qmp_phy_cfg *cfg = qmp->cfg;
 	void __iomem *pcs = qmp->pcs;
 	int ret;
 
+	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
+
 	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
 	if (ret) {
 		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
@@ -1775,35 +1791,10 @@ static int qmp_ufs_com_init(struct qmp_ufs *qmp)
 		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)
-{
-	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);
-	int ret;
-	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
-
-	ret = qmp_ufs_com_init(qmp);
 	return ret;
 }
 
-- 
2.48.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V5 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, inline qmp_ufs_exit into qmp_ufs_power_off
function 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 | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index d3f9ee490a32..a5974a1fb5bb 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1851,28 +1851,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);

 	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 +1904,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

[PATCH V5 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>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index a5974a1fb5bb..fca47e5e8bf0 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1758,19 +1758,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);
@@ -1851,7 +1838,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_com_exit(qmp);
+	/* 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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V5 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>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.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 fca47e5e8bf0..abfebf0435d8 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1827,13 +1827,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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V5 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 | 61 ++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 23b4fc84dc8c..3ee4ab90dfba 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -697,26 +697,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);
 }
@@ -724,26 +715,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);
 }
@@ -1133,12 +1109,20 @@ static void ufs_qcom_set_caps(struct ufs_hba *hba)
  * @on: If true, enable clocks else disable them.
  * @status: PRE_CHANGE or POST_CHANGE notify
  *
+ * There are certain clocks which comes from the PHY so it needs
+ * to be managed together along with controller clocks which also
+ * provides a better power saving. Hence keep phy_power_off/on calls
+ * in ufs_qcom_setup_clocks, so that PHY's regulators & clks can be
+ * turned on/off along with UFS's clocks.
+ *
  * Return: 0 on success, non-zero on failure.
  */
 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.
@@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err);
+				return err;
+			}
 		}
 		break;
 	case POST_CHANGE:
 		if (on) {
+			err = phy_power_on(phy);
+			if (err) {
+				dev_err(hba->dev, "phy power on failed, ret = %d\n", 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);
@@ -1343,9 +1339,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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

There can be scenarios where phy_power_on is called when PHY is
already on (phy_count=1). For instance, ufs_qcom_power_up_sequence
can be called multiple times from ufshcd_link_startup as part of
ufshcd_hba_enable call for each link startup retries(max retries =3),
causing the PHY reference count to increase and leading to inconsistent
phy behavior.

Similarly, there can be scenarios where phy_power_on or phy_power_off
might be called directly from the UFS controller driver when the PHY
is already powered on or off respectiely, causing similar issues.

To fix this, introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off
wrappers for phy_power_on and phy_power_off. These wrappers will use an
is_phy_pwr_on flag to check if the PHY is already powered on or off,
avoiding redundant calls. Protect the is_phy_pwr_on flag with a mutex
to ensure safe usage and prevent 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 3ee4ab90dfba..583db910efd4 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -479,6 +479,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);
@@ -507,7 +539,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);
 	}
 
@@ -524,7 +556,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);
@@ -1121,7 +1153,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;
 
 	/*
@@ -1142,7 +1173,7 @@ 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);
+			err = ufs_qcom_phy_power_off(hba);
 			if (err) {
 				dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
 				return err;
@@ -1151,7 +1182,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, "phy power on failed, ret = %d\n", err);
 				return err;
@@ -1339,10 +1370,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 0a5cfc2dd4f7..f51b774e17be 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -281,6 +281,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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V5 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>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.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 583db910efd4..bd7f65500db7 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -540,7 +540,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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Dmitry Baryshkov]

On Thu, May 15, 2025 at 09:57:17PM +0530, Nitin Rawat wrote:
> Rename qmp_ufs_disable to qmp_ufs_power_off to better represent its
> functionality. Additionally, inline qmp_ufs_exit into qmp_ufs_power_off
> function 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 | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 

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

-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Dmitry Baryshkov]

On Thu, May 15, 2025 at 09:57:12PM +0530, 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>
> ---
>  drivers/ufs/host/ufs-qcom.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

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

-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[neil.armstrong]

On 15/05/2025 18:27, Nitin Rawat wrote:
> Rename qmp_ufs_disable to qmp_ufs_power_off to better represent its
> functionality. Additionally, inline qmp_ufs_exit into qmp_ufs_power_off
> function 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 | 19 +------------------
>   1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index d3f9ee490a32..a5974a1fb5bb 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1851,28 +1851,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);
> 
>   	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 +1904,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
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V5 00/11] Refactor ufs phy powerup sequence

[Martin K. Petersen]

Hi Nitin!

> 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

What is your intent wrt. getting this series merged? Can the phy: and
scsi: patches be merged independently?

-- 
Martin K. Petersen

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:12PM +0530, 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

'next patchset in the series' wouldn't make sense once this patch gets merged.
They all become commits. So you should mention 'successive commits'.

> to be distinct.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  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 37887ec68412..23b4fc84dc8c 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -531,6 +531,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
> 

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:13PM +0530, Nitin Rawat wrote:
> 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  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 b33e2e2b5014..a67cf0a64f74 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1838,7 +1838,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;
> @@ -1899,7 +1899,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;
>  
> @@ -1907,7 +1907,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);
>  
> @@ -1941,7 +1941,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
> 

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V5 00/11] Refactor ufs phy powerup sequence

[Dmitry Baryshkov]

On Tue, May 20, 2025 at 09:45:40PM -0400, Martin K. Petersen wrote:
> 
> Hi Nitin!
> 
> > 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
> 
> What is your intent wrt. getting this series merged? Can the phy: and
> scsi: patches be merged independently?

Unfortunately PHY patches depend on the first scsi patch.

-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:14PM +0530, Nitin Rawat wrote:
> 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  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 a67cf0a64f74..ade8e9c4b9ae 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1797,7 +1797,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;
> @@ -1825,10 +1825,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);
> @@ -1847,6 +1843,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);
> @@ -1899,21 +1899,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;
> @@ -1943,6 +1928,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
> 

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:15PM +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.
> 

More importantly, you are introducing the phy_ops::init callback, which
should've been mentioned.

> 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>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.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 ade8e9c4b9ae..33d238cf49aa 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1800,38 +1800,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;

This can't be:
	return qmp_ufs_com_init; ?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:16PM +0530, Nitin Rawat wrote:
> 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 | 43 ++++++++++---------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 33d238cf49aa..d3f9ee490a32 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1758,12 +1758,28 @@ 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)
> +static int qmp_ufs_com_exit(struct qmp_ufs *qmp)

Since there is no qmp_ufs_com_init() now, qmp_ufs_com_exit() lacks symmetry.
Maybe you should rename it too.

>  {
>  	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);
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  	void __iomem *pcs = qmp->pcs;
>  	int ret;
>  
> +	dev_vdbg(qmp->dev, "Initializing QMP phy\n");

Now, this debug print doesn't make sense. You can drop it.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:18PM +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.
> 

Okay, so you are doing it now...

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

Why are you dropping the reset_assert()?

> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index a5974a1fb5bb..fca47e5e8bf0 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1758,19 +1758,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);
> @@ -1851,7 +1838,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_com_exit(qmp);
> +	/* Turn off all the phy clocks */

You should drop this and below comment. They add no value.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:19PM +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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.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 fca47e5e8bf0..abfebf0435d8 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1827,13 +1827,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
> 

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:20PM +0530, 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

s/removes/moved

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

This patch is not calling phy_calibrate().

> 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 | 61 ++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 32 deletions(-)
> 

[...]

>  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.
> @@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err);
> +				return err;
> +			}
>  		}
>  		break;
>  	case POST_CHANGE:
>  		if (on) {
> +			err = phy_power_on(phy);
> +			if (err) {
> +				dev_err(hba->dev, "phy power on failed, ret = %d\n", 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);
> @@ -1343,9 +1339,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);

This change is not at all needed.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:21PM +0530, Nitin Rawat wrote:

Subject should mention ufs_qcom_phy_power_{on/off} as phy_power_{on/off} are
generic APIs.

> There can be scenarios where phy_power_on is called when PHY is
> already on (phy_count=1). For instance, ufs_qcom_power_up_sequence
> can be called multiple times from ufshcd_link_startup as part of
> ufshcd_hba_enable call for each link startup retries(max retries =3),
> causing the PHY reference count to increase and leading to inconsistent
> phy behavior.
> 
> Similarly, there can be scenarios where phy_power_on or phy_power_off
> might be called directly from the UFS controller driver when the PHY
> is already powered on or off respectiely, causing similar issues.
> 
> To fix this, introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off
> wrappers for phy_power_on and phy_power_off. These wrappers will use an
> is_phy_pwr_on flag to check if the PHY is already powered on or off,
> avoiding redundant calls. Protect the is_phy_pwr_on flag with a mutex
> to ensure safe usage and prevent race conditions.
> 

This smells like the phy_power_{on/off} calls are not balanced and you are
trying to workaround that in the UFS driver.

- Mani

> 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 3ee4ab90dfba..583db910efd4 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -479,6 +479,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);
> @@ -507,7 +539,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);
>  	}
>  
> @@ -524,7 +556,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);
> @@ -1121,7 +1153,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;
>  
>  	/*
> @@ -1142,7 +1173,7 @@ 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);
> +			err = ufs_qcom_phy_power_off(hba);
>  			if (err) {
>  				dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
>  				return err;
> @@ -1151,7 +1182,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, "phy power on failed, ret = %d\n", err);
>  				return err;
> @@ -1339,10 +1370,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 0a5cfc2dd4f7..f51b774e17be 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -281,6 +281,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
> 

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 15, 2025 at 09:57:22PM +0530, 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>

You should move this fix to the start of the series so that it can be applied
separately if needed and also to be backported cleanly.

> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.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 583db910efd4..bd7f65500db7 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -540,7 +540,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 don't need braces now.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/21/2025 7:27 PM, Manivannan Sadhasivam wrote:
> On Thu, May 15, 2025 at 09:57:20PM +0530, 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
> 
> s/removes/moved

Sure, will address in next patchset.

> 
>> 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.
>>
> 
> This patch is not calling phy_calibrate().

updated commit text to remove phy_Calibrate from commit text

> 
>> 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 | 61 ++++++++++++++++++-------------------
>>   1 file changed, 29 insertions(+), 32 deletions(-)
>>
> 
> [...]
> 
>>   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.
>> @@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err);
>> +				return err;
>> +			}
>>   		}
>>   		break;
>>   	case POST_CHANGE:
>>   		if (on) {
>> +			err = phy_power_on(phy);
>> +			if (err) {
>> +				dev_err(hba->dev, "phy power on failed, ret = %d\n", 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);
>> @@ -1343,9 +1339,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);
> 
> This change is not at all needed.

In the current code, PHY resources (clocks and rails) remain active even 
when the link is in a hibernate state and all controller clocks are off. 
This results in a significant power penalty and can prevent CX power 
collapse.

These rails and clocks are only turned off when a system suspend is 
triggered, and even then, only at SPM level 5 where the link is turned 
off. This approach is not power-efficient.

As part of this series, the code that enables/disables the PHY's 
regulators and clocks is now confined to the phy_power_on and 
phy_power_off calls, with the rest moved to the calibration phase.

Therefore, we are invoking the phy_power_off and phy_power_on calls from 
ufs_qcom_setup_clocks, ensuring that the PHY's regulators and clocks can 
be turned on/off along with the UFS clocks, thereby achieving power savings.

Therefore, this patch is the cornerstone of this series.

Regards,
Nitin


> 
> - Mani
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/21/2025 6:38 PM, Manivannan Sadhasivam wrote:
> On Thu, May 15, 2025 at 09:57:12PM +0530, 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
> 
> 'next patchset in the series' wouldn't make sense once this patch gets merged.
> They all become commits. So you should mention 'successive commits'.

Sure, will address in next patchset.

> 
>> to be distinct.
>>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> - Mani
> 
>> ---
>>   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 37887ec68412..23b4fc84dc8c 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -531,6 +531,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
>>
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/21/2025 6:56 PM, Manivannan Sadhasivam wrote:
> On Thu, May 15, 2025 at 09:57:15PM +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.
>>

Sure. I'll update the commit text in next patchset.

> 
> More importantly, you are introducing the phy_ops::init callback, which
> should've been mentioned.
> 
>> 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>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.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 ade8e9c4b9ae..33d238cf49aa 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> @@ -1800,38 +1800,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;
> 
> This can't be:
> 	return qmp_ufs_com_init; ?

This is already taken care in next patch (#6) of this series.

Thanks,
Nitin


> 
> - Mani
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/21/2025 7:00 PM, Manivannan Sadhasivam wrote:
> On Thu, May 15, 2025 at 09:57:16PM +0530, Nitin Rawat wrote:
>> 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 | 43 ++++++++++---------------
>>   1 file changed, 17 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> index 33d238cf49aa..d3f9ee490a32 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> @@ -1758,12 +1758,28 @@ 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)
>> +static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
> 
> Since there is no qmp_ufs_com_init() now, qmp_ufs_com_exit() lacks symmetry.
> Maybe you should rename it too.


Yes this is already taken care in differnt patch of this series.

> 
>>   {
>>   	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);
>> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
>>   	void __iomem *pcs = qmp->pcs;
>>   	int ret;
>>   
>> +	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> 
> Now, this debug print doesn't make sense. You can drop it.

Sure, wil remove.

> 
> - Mani
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/21/2025 7:31 PM, Manivannan Sadhasivam wrote:
> On Thu, May 15, 2025 at 09:57:21PM +0530, Nitin Rawat wrote:
> 
> Subject should mention ufs_qcom_phy_power_{on/off} as phy_power_{on/off} are
> generic APIs.
> 
>> There can be scenarios where phy_power_on is called when PHY is
>> already on (phy_count=1). For instance, ufs_qcom_power_up_sequence
>> can be called multiple times from ufshcd_link_startup as part of
>> ufshcd_hba_enable call for each link startup retries(max retries =3),
>> causing the PHY reference count to increase and leading to inconsistent
>> phy behavior.
>>
>> Similarly, there can be scenarios where phy_power_on or phy_power_off
>> might be called directly from the UFS controller driver when the PHY
>> is already powered on or off respectiely, causing similar issues.
>>
>> To fix this, introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off
>> wrappers for phy_power_on and phy_power_off. These wrappers will use an
>> is_phy_pwr_on flag to check if the PHY is already powered on or off,
>> avoiding redundant calls. Protect the is_phy_pwr_on flag with a mutex
>> to ensure safe usage and prevent race conditions.
>>
> 
> This smells like the phy_power_{on/off} calls are not balanced and you are
> trying to workaround that in the UFS driver.

Hi Mani,

Yes, there can be scenarios that were not previously encountered because 
phy_power_on and phy_power_off were only called during system suspend 
(spm_lvl = 5). However, with phy_power_on now moved to 
ufs_qcom_setup_clocks, there is a slightly more probability of 
phy_power_on being called twice, i.e., phy_power_on being invoked when 
the PHY is already on.

For instance, if the PHY power is already on and the UFS driver calls 
ufs_qcom_setup_clocks from an error handling context, phy_power_on could 
be called again which may increase phy_count and can cause inconsistent 
phy bheaviour . Therefore, we need to have a flag, is_phy_pwr_on, in the 
controller driver, protected by a mutex, to indicate the state of 
phy_power_on and phy_power_off.

This approach is also present in Qualcomm downstream UFS driver and 
similiar solution in mtk ufs driver to have flag in controller 
indictring phy power state in their upstream UFS drivers.

Regards,
Nitin



>> +			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);
>> @@ -507,7 +539,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);
>>   	}
>>   
>> @@ -524,7 +556,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);
>> @@ -1121,7 +1153,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;
>>   
>>   	/*
>> @@ -1142,7 +1173,7 @@ 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);
>> +			err = ufs_qcom_phy_power_off(hba);
>>   			if (err) {
>>   				dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
>>   				return err;
>> @@ -1151,7 +1182,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, "phy power on failed, ret = %d\n", err);
>>   				return err;
>> @@ -1339,10 +1370,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 0a5cfc2dd4f7..f51b774e17be 100644
>> --- a/drivers/ufs/host/ufs-qcom.h
>> +++ b/drivers/ufs/host/ufs-qcom.h
>> @@ -281,6 +281,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
>>
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/21/2025 7:19 PM, Manivannan Sadhasivam wrote:
> On Thu, May 15, 2025 at 09:57:18PM +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.
>>
> 
> Okay, so you are doing it now...

Yes

> 
>> Additonally partial Inline(dropping the reset assert) qmp_ufs_com_exit
>> into qmp_ufs_power_off function to avoid unnecessary function call.
>>
> 
> Why are you dropping the reset_assert()?

This was not aligning to Phy programming guide .


> 
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 19 +++++--------------
>>   1 file changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> index a5974a1fb5bb..fca47e5e8bf0 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> @@ -1758,19 +1758,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);
>> @@ -1851,7 +1838,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_com_exit(qmp);
>> +	/* Turn off all the phy clocks */
> 
> You should drop this and below comment. They add no value.

Comments are actually provided for each operation within 
qmp_ufs_power_off which actually facilitate understanding of all actions 
performed by the function which may not be fully clear by code. Hence
I thought to keep the comments. But If you insist i'll remove.

Thanks.
Nitin


> 
> - Mani
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/21/2025 7:35 PM, Manivannan Sadhasivam wrote:
> On Thu, May 15, 2025 at 09:57:22PM +0530, 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>
> 
> You should move this fix to the start of the series so that it can be applied
> separately if needed and also to be backported cleanly.

sure , will move

> 
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.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 583db910efd4..bd7f65500db7 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -540,7 +540,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 don't need braces now.
will remove.
> 
> - Mani
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 22, 2025 at 03:49:12AM +0530, Nitin Rawat wrote:
> 
> 
> On 5/21/2025 7:19 PM, Manivannan Sadhasivam wrote:
> > On Thu, May 15, 2025 at 09:57:18PM +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.
> > > 
> > 
> > Okay, so you are doing it now...
> 
> Yes
> 
> > 
> > > Additonally partial Inline(dropping the reset assert) qmp_ufs_com_exit
> > > into qmp_ufs_power_off function to avoid unnecessary function call.
> > > 
> > 
> > Why are you dropping the reset_assert()?
> 
> This was not aligning to Phy programming guide .
> 

You should mention it in the description.

> 
> > 
> > > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > > ---
> > >   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 19 +++++--------------
> > >   1 file changed, 5 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > > index a5974a1fb5bb..fca47e5e8bf0 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > > @@ -1758,19 +1758,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);
> > > @@ -1851,7 +1838,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_com_exit(qmp);
> > > +	/* Turn off all the phy clocks */
> > 
> > You should drop this and below comment. They add no value.
> 
> Comments are actually provided for each operation within qmp_ufs_power_off
> which actually facilitate understanding of all actions performed by the
> function which may not be fully clear by code. Hence
> I thought to keep the comments. But If you insist i'll remove.
> 

For complex code, comment should be added indeed. But for
clk_bulk_disable_unprepare() and regulator_bulk_disable(), NO. It is obvious
that they turn off clock and regulators.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 22, 2025 at 03:10:48AM +0530, Nitin Rawat wrote:
> 
> 
> On 5/21/2025 7:27 PM, Manivannan Sadhasivam wrote:
> > On Thu, May 15, 2025 at 09:57:20PM +0530, 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
> > 
> > s/removes/moved
> 
> Sure, will address in next patchset.
> 
> > 
> > > 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.
> > > 
> > 
> > This patch is not calling phy_calibrate().
> 
> updated commit text to remove phy_Calibrate from commit text
> 
> > 
> > > 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 | 61 ++++++++++++++++++-------------------
> > >   1 file changed, 29 insertions(+), 32 deletions(-)
> > > 
> > 
> > [...]
> > 
> > >   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.
> > > @@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err);
> > > +				return err;
> > > +			}
> > >   		}
> > >   		break;
> > >   	case POST_CHANGE:
> > >   		if (on) {
> > > +			err = phy_power_on(phy);
> > > +			if (err) {
> > > +				dev_err(hba->dev, "phy power on failed, ret = %d\n", 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);
> > > @@ -1343,9 +1339,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);
> > 
> > This change is not at all needed.
> 
> In the current code, PHY resources (clocks and rails) remain active even
> when the link is in a hibernate state and all controller clocks are off.
> This results in a significant power penalty and can prevent CX power
> collapse.
> 
> These rails and clocks are only turned off when a system suspend is
> triggered, and even then, only at SPM level 5 where the link is turned off.
> This approach is not power-efficient.
> 
> As part of this series, the code that enables/disables the PHY's regulators
> and clocks is now confined to the phy_power_on and phy_power_off calls, with
> the rest moved to the calibration phase.
> 
> Therefore, we are invoking the phy_power_off and phy_power_on calls from
> ufs_qcom_setup_clocks, ensuring that the PHY's regulators and clocks can be
> turned on/off along with the UFS clocks, thereby achieving power savings.
> 
> Therefore, this patch is the cornerstone of this series.
> 

I did not question the patch, but just the change that you did in the
ufs_qcom_exit() function. You added a local variable for 'host->generic_phy',
which is not related to this patch.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/22/2025 2:25 PM, Manivannan Sadhasivam wrote:
> On Thu, May 22, 2025 at 03:10:48AM +0530, Nitin Rawat wrote:
>>
>>
>> On 5/21/2025 7:27 PM, Manivannan Sadhasivam wrote:
>>> On Thu, May 15, 2025 at 09:57:20PM +0530, 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
>>>
>>> s/removes/moved
>>
>> Sure, will address in next patchset.
>>
>>>
>>>> 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.
>>>>
>>>
>>> This patch is not calling phy_calibrate().
>>
>> updated commit text to remove phy_Calibrate from commit text
>>
>>>
>>>> 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 | 61 ++++++++++++++++++-------------------
>>>>    1 file changed, 29 insertions(+), 32 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>>    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.
>>>> @@ -1157,10 +1141,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, "phy power off failed, ret=%d\n", err);
>>>> +				return err;
>>>> +			}
>>>>    		}
>>>>    		break;
>>>>    	case POST_CHANGE:
>>>>    		if (on) {
>>>> +			err = phy_power_on(phy);
>>>> +			if (err) {
>>>> +				dev_err(hba->dev, "phy power on failed, ret = %d\n", 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);
>>>> @@ -1343,9 +1339,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);
>>>
>>> This change is not at all needed.
>>
>> In the current code, PHY resources (clocks and rails) remain active even
>> when the link is in a hibernate state and all controller clocks are off.
>> This results in a significant power penalty and can prevent CX power
>> collapse.
>>
>> These rails and clocks are only turned off when a system suspend is
>> triggered, and even then, only at SPM level 5 where the link is turned off.
>> This approach is not power-efficient.
>>
>> As part of this series, the code that enables/disables the PHY's regulators
>> and clocks is now confined to the phy_power_on and phy_power_off calls, with
>> the rest moved to the calibration phase.
>>
>> Therefore, we are invoking the phy_power_off and phy_power_on calls from
>> ufs_qcom_setup_clocks, ensuring that the PHY's regulators and clocks can be
>> turned on/off along with the UFS clocks, thereby achieving power savings.
>>
>> Therefore, this patch is the cornerstone of this series.
>>
> 
> I did not question the patch, but just the change that you did in the
> ufs_qcom_exit() function. You added a local variable for 'host->generic_phy',
> which is not related to this patch.

Sorry about that, Mani. I misunderstood your comments. Sure, will 
address them.


> 
> - Mani
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/22/2025 2:23 PM, Manivannan Sadhasivam wrote:
> On Thu, May 22, 2025 at 03:49:12AM +0530, Nitin Rawat wrote:
>>
>>
>> On 5/21/2025 7:19 PM, Manivannan Sadhasivam wrote:
>>> On Thu, May 15, 2025 at 09:57:18PM +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.
>>>>
>>>
>>> Okay, so you are doing it now...
>>
>> Yes
>>
>>>
>>>> Additonally partial Inline(dropping the reset assert) qmp_ufs_com_exit
>>>> into qmp_ufs_power_off function to avoid unnecessary function call.
>>>>
>>>
>>> Why are you dropping the reset_assert()?
>>
>> This was not aligning to Phy programming guide .
>>
> 
> You should mention it in the description.

Sure, will update the commit text.

> 
>>
>>>
>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>> ---
>>>>    drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 19 +++++--------------
>>>>    1 file changed, 5 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> index a5974a1fb5bb..fca47e5e8bf0 100644
>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> @@ -1758,19 +1758,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);
>>>> @@ -1851,7 +1838,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_com_exit(qmp);
>>>> +	/* Turn off all the phy clocks */
>>>
>>> You should drop this and below comment. They add no value.
>>
>> Comments are actually provided for each operation within qmp_ufs_power_off
>> which actually facilitate understanding of all actions performed by the
>> function which may not be fully clear by code. Hence
>> I thought to keep the comments. But If you insist i'll remove.
>>
> 
> For complex code, comment should be added indeed. But for
> clk_bulk_disable_unprepare() and regulator_bulk_disable(), NO. It is obvious
> that they turn off clock and regulators.

ok sure, will remove it.

> 
> - Mani
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Manivannan Sadhasivam]

On Thu, May 22, 2025 at 03:48:29AM +0530, Nitin Rawat wrote:
> 
> 
> On 5/21/2025 7:31 PM, Manivannan Sadhasivam wrote:
> > On Thu, May 15, 2025 at 09:57:21PM +0530, Nitin Rawat wrote:
> > 
> > Subject should mention ufs_qcom_phy_power_{on/off} as phy_power_{on/off} are
> > generic APIs.
> > 
> > > There can be scenarios where phy_power_on is called when PHY is
> > > already on (phy_count=1). For instance, ufs_qcom_power_up_sequence
> > > can be called multiple times from ufshcd_link_startup as part of
> > > ufshcd_hba_enable call for each link startup retries(max retries =3),
> > > causing the PHY reference count to increase and leading to inconsistent
> > > phy behavior.
> > > 
> > > Similarly, there can be scenarios where phy_power_on or phy_power_off
> > > might be called directly from the UFS controller driver when the PHY
> > > is already powered on or off respectiely, causing similar issues.
> > > 
> > > To fix this, introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off
> > > wrappers for phy_power_on and phy_power_off. These wrappers will use an
> > > is_phy_pwr_on flag to check if the PHY is already powered on or off,
> > > avoiding redundant calls. Protect the is_phy_pwr_on flag with a mutex
> > > to ensure safe usage and prevent race conditions.
> > > 
> > 
> > This smells like the phy_power_{on/off} calls are not balanced and you are
> > trying to workaround that in the UFS driver.
> 
> Hi Mani,
> 
> Yes, there can be scenarios that were not previously encountered because
> phy_power_on and phy_power_off were only called during system suspend
> (spm_lvl = 5). However, with phy_power_on now moved to
> ufs_qcom_setup_clocks, there is a slightly more probability of phy_power_on
> being called twice, i.e., phy_power_on being invoked when the PHY is already
> on.
> 
> For instance, if the PHY power is already on and the UFS driver calls
> ufs_qcom_setup_clocks from an error handling context, phy_power_on could be
> called again which may increase phy_count and can cause inconsistent phy
> bheaviour . Therefore, we need to have a flag, is_phy_pwr_on, in the
> controller driver, protected by a mutex, to indicate the state of
> phy_power_on and phy_power_off.
> 

If phy_power_on() is called twice without phy_power_off(), there can be only 2
possibilities:

1. phy_power_off() is not balanced
2. phy_power_on() is called from a wrong place

> This approach is also present in Qualcomm downstream UFS driver and similiar
> solution in mtk ufs driver to have flag in controller indictring phy power
> state in their upstream UFS drivers.
> 

No, having this check in the host driver is clearly a workaround for a broken
behavior. I do not want to carry this mess all along.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Nitin Rawat]

On 5/22/2025 3:01 PM, Manivannan Sadhasivam wrote:
> On Thu, May 22, 2025 at 03:48:29AM +0530, Nitin Rawat wrote:
>>
>>
>> On 5/21/2025 7:31 PM, Manivannan Sadhasivam wrote:
>>> On Thu, May 15, 2025 at 09:57:21PM +0530, Nitin Rawat wrote:
>>>
>>> Subject should mention ufs_qcom_phy_power_{on/off} as phy_power_{on/off} are
>>> generic APIs.
>>>
>>>> There can be scenarios where phy_power_on is called when PHY is
>>>> already on (phy_count=1). For instance, ufs_qcom_power_up_sequence
>>>> can be called multiple times from ufshcd_link_startup as part of
>>>> ufshcd_hba_enable call for each link startup retries(max retries =3),
>>>> causing the PHY reference count to increase and leading to inconsistent
>>>> phy behavior.
>>>>
>>>> Similarly, there can be scenarios where phy_power_on or phy_power_off
>>>> might be called directly from the UFS controller driver when the PHY
>>>> is already powered on or off respectiely, causing similar issues.
>>>>
>>>> To fix this, introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off
>>>> wrappers for phy_power_on and phy_power_off. These wrappers will use an
>>>> is_phy_pwr_on flag to check if the PHY is already powered on or off,
>>>> avoiding redundant calls. Protect the is_phy_pwr_on flag with a mutex
>>>> to ensure safe usage and prevent race conditions.
>>>>
>>>
>>> This smells like the phy_power_{on/off} calls are not balanced and you are
>>> trying to workaround that in the UFS driver.
>>
>> Hi Mani,
>>
>> Yes, there can be scenarios that were not previously encountered because
>> phy_power_on and phy_power_off were only called during system suspend
>> (spm_lvl = 5). However, with phy_power_on now moved to
>> ufs_qcom_setup_clocks, there is a slightly more probability of phy_power_on
>> being called twice, i.e., phy_power_on being invoked when the PHY is already
>> on.
>>
>> For instance, if the PHY power is already on and the UFS driver calls
>> ufs_qcom_setup_clocks from an error handling context, phy_power_on could be
>> called again which may increase phy_count and can cause inconsistent phy
>> bheaviour . Therefore, we need to have a flag, is_phy_pwr_on, in the
>> controller driver, protected by a mutex, to indicate the state of
>> phy_power_on and phy_power_off.
>>
> 
> If phy_power_on() is called twice without phy_power_off(), there can be only 2
> possibilities:
> 
> 1. phy_power_off() is not balanced
> 2. phy_power_on() is called from a wrong place
> 
>> This approach is also present in Qualcomm downstream UFS driver and similiar
>> solution in mtk ufs driver to have flag in controller indictring phy power
>> state in their upstream UFS drivers.
>>
> 
> No, having this check in the host driver is clearly a workaround for a broken
> behavior. I do not want to carry this mess all along.
> 

Hi Mani,

I double checked the code again error handling scenarios which i mention 
is my earlier reply is actually under runtime suspend check so this 
issue won't occur there which means as of now we have no scenarios which 
exists as per our understanding where existing code will break w.r.t 
phy_on/_off , hence i can drop this patch from this series.

Regards,
Nitin


> - Mani
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V5 00/11] Refactor ufs phy powerup sequence

[Nitin Rawat]

On 5/21/2025 6:40 PM, Dmitry Baryshkov wrote:
> On Tue, May 20, 2025 at 09:45:40PM -0400, Martin K. Petersen wrote:
>>
>> Hi Nitin!
>>
>>> 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
>>
>> What is your intent wrt. getting this series merged? Can the phy: and
>> scsi: patches be merged independently?
> 
> Unfortunately PHY patches depend on the first scsi patch.

Thanks, Dmitry, for mentioning the dependency

Hi Martin,

After addressing the review comments for v5, there has been a change in 
the patch order.

In the latest patchset (v6), Patch 2 (SCSI patch) is now required for 
the functional dependency of the subsequent PHY patches (Patch 3 to 
Patch 9).

Patch 1 (SCSI patch) addresses an existing issue and does not depend on 
any other changes.

Regards,
Nitin



> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy