[PATCH V3 0/9] Refactor phy powerup sequence

[PATCH V3 0/9] Refactor 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.

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 (9):
  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: Refactor qmp_ufs_exit callback.
  scsi: ufs: qcom : Refactor phy_power_on/off calls
  scsi: ufs: qcom : Introduce phy_power_on/off wrapper function
  scsi: ufs: qcom: Prevent calling phy_exit before phy_init

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

--
2.48.1

[PATCH V3 1/9] scsi: ufs: qcom: add a new phy calibrate API call

[Nitin Rawat]

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

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

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 1b37449fbffc..4998656e9267 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -473,6 +473,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 		goto out_disable_phy;
 	}
 
+	ret = phy_calibrate(phy);
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed to calibrate PHY %d\n",
+			__func__, ret);
+	}
+
 	ufs_qcom_select_unipro_mode(host);
 
 	return 0;
-- 
2.48.1

Re: [PATCH V3 1/9] scsi: ufs: qcom: add a new phy calibrate API call

[Konrad Dybcio]

On 4/10/25 11:00 AM, 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(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 1b37449fbffc..4998656e9267 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -473,6 +473,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  		goto out_disable_phy;
>  	}
>  
> +	ret = phy_calibrate(phy);
> +	if (ret) {
> +		dev_err(hba->dev, "%s: Failed to calibrate PHY %d\n",

Please add a colon, so that it becomes "..PHY: %d\n"

> +			__func__, ret);
Avoid __func__, this print is fine without it

Shouldn't we fail the power-on if this can't succeed?

Konrad

Re: [PATCH V3 1/9] scsi: ufs: qcom: add a new phy calibrate API call

[Nitin Rawat]

On 4/23/2025 4:12 PM, Konrad Dybcio wrote:
> On 4/10/25 11:00 AM, 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(+)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 1b37449fbffc..4998656e9267 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -473,6 +473,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>>   		goto out_disable_phy;
>>   	}
>>   
>> +	ret = phy_calibrate(phy);
>> +	if (ret) {
>> +		dev_err(hba->dev, "%s: Failed to calibrate PHY %d\n",
> 
> Please add a colon, so that it becomes "..PHY: %d\n"
> 
>> +			__func__, ret);
> Avoid __func__, this print is fine without it

Sure will update this in next patchset.

> 
> Shouldn't we fail the power-on if this can't succeed?

Thanks for the catch. Yes we should return power-on failure if calibrate 
fails.
Even if there is calibrate phy ops registered, it will return 0. So for 
so nonzero return value we should treat failure and fail poweron.
Sure will this in next patchset.

Thanks,
Nitin

> 
> Konrad

[PATCH V3 2/9] 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.

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

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 45b3b792696e..bb836bc0f736 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1837,7 +1837,7 @@ static int qmp_ufs_init(struct phy *phy)
 	return 0;
 }
 
-static int qmp_ufs_power_on(struct phy *phy)
+static int qmp_ufs_phy_calibrate(struct phy *phy)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
@@ -1898,7 +1898,7 @@ static int qmp_ufs_exit(struct phy *phy)
 	return 0;
 }
 
-static int qmp_ufs_enable(struct phy *phy)
+static int qmp_ufs_power_on(struct phy *phy)
 {
 	int ret;
 
@@ -1906,7 +1906,7 @@ static int qmp_ufs_enable(struct phy *phy)
 	if (ret)
 		return ret;
 
-	ret = qmp_ufs_power_on(phy);
+	ret = qmp_ufs_phy_calibrate(phy);
 	if (ret)
 		qmp_ufs_exit(phy);
 
@@ -1940,7 +1940,7 @@ static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 }
 
 static const struct phy_ops qcom_qmp_ufs_phy_ops = {
-	.power_on	= qmp_ufs_enable,
+	.power_on	= qmp_ufs_power_on,
 	.power_off	= qmp_ufs_disable,
 	.set_mode	= qmp_ufs_set_mode,
 	.owner		= THIS_MODULE,
-- 
2.48.1

Re: [PATCH V3 2/9] phy: qcom-qmp-ufs: Rename qmp_ufs_enable and qmp_ufs_power_on

[Dmitry Baryshkov]

On Thu, Apr 10, 2025 at 02:30:55PM +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.
> 
> 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(-)
> 

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

-- 
With best wishes
Dmitry

[PATCH V3 3/9] 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.

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

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

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

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

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

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

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

Re: [PATCH V3 3/9] phy: qcom-qmp-ufs: Refactor phy_power_on and phy_calibrate callbacks

[Dmitry Baryshkov]

On Thu, Apr 10, 2025 at 02:30:56PM +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.
> 
> 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(-)
> 

With the cover letter updated to note the dependencies:


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


-- 
With best wishes
Dmitry

[PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Nitin Rawat]

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

Move the UFS PHY reset parsing logic from qmp_phy_power_on to
qmp_ufs_probe to avoid unnecessary parsing during resume.

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

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

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

 static int qmp_ufs_phy_calibrate(struct phy *phy)
@@ -2088,6 +2061,34 @@ static int qmp_ufs_parse_dt(struct qmp_ufs *qmp)
 	return 0;
 }

+static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp)
+{
+	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 int qmp_ufs_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -2114,6 +2115,10 @@ static int qmp_ufs_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;

+	ret = qmp_ufs_get_phy_reset(qmp);
+	if (ret)
+		return ret;
+
 	/* Check for legacy binding with child node. */
 	np = of_get_next_available_child(dev->of_node, NULL);
 	if (np) {
--
2.48.1

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Dmitry Baryshkov]

On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
> Refactor the UFS PHY reset handling to parse the reset logic only once
> during probe, instead of every resume.
> 
> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
> qmp_ufs_probe to avoid unnecessary parsing during resume.

How did you solve the circular dependency issue being noted below?

> 
> 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 | 61 +++++++++++++------------
>  1 file changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 636dc3dc3ea8..12dad28cc1bd 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1799,38 +1799,11 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>  static int qmp_ufs_power_on(struct phy *phy)
>  {
>  	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> -	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  	int ret;
>  	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> 
> -	if (cfg->no_pcs_sw_reset) {
> -		/*
> -		 * Get UFS reset, which is delayed until now to avoid a
> -		 * circular dependency where UFS needs its PHY, but the PHY
> -		 * needs this UFS reset.
> -		 */
> -		if (!qmp->ufs_reset) {
> -			qmp->ufs_reset =
> -				devm_reset_control_get_exclusive(qmp->dev,
> -								 "ufsphy");
> -
> -			if (IS_ERR(qmp->ufs_reset)) {
> -				ret = PTR_ERR(qmp->ufs_reset);
> -				dev_err(qmp->dev,
> -					"failed to get UFS reset: %d\n",
> -					ret);
> -
> -				qmp->ufs_reset = NULL;
> -				return ret;
> -			}
> -		}
> -	}
> -
>  	ret = qmp_ufs_com_init(qmp);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return ret;
>  }
> 
>  static int qmp_ufs_phy_calibrate(struct phy *phy)
> @@ -2088,6 +2061,34 @@ static int qmp_ufs_parse_dt(struct qmp_ufs *qmp)
>  	return 0;
>  }
> 
> +static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp)
> +{
> +	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");

Strange indentation.

> +
> +		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 int qmp_ufs_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -2114,6 +2115,10 @@ static int qmp_ufs_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
> 
> +	ret = qmp_ufs_get_phy_reset(qmp);
> +	if (ret)
> +		return ret;
> +
>  	/* Check for legacy binding with child node. */
>  	np = of_get_next_available_child(dev->of_node, NULL);
>  	if (np) {
> --
> 2.48.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Nitin Rawat]

On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>> Refactor the UFS PHY reset handling to parse the reset logic only once
>> during probe, instead of every resume.
>>
>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>> qmp_ufs_probe to avoid unnecessary parsing during resume.
> 
> How did you solve the circular dependency issue being noted below?

Hi Dmitry,
As part of my patch, I moved the parsing logic from qmp_phy_power_on to 
qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain 
about the circular dependency issue and whether if it still exists.

Regards,
Nitin


> 
>>
>> 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 | 61 +++++++++++++------------
>>   1 file changed, 33 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> index 636dc3dc3ea8..12dad28cc1bd 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> @@ -1799,38 +1799,11 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>>   static int qmp_ufs_power_on(struct phy *phy)
>>   {
>>   	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>> -	const struct qmp_phy_cfg *cfg = qmp->cfg;
>>   	int ret;
>>   	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
>>
>> -	if (cfg->no_pcs_sw_reset) {
>> -		/*
>> -		 * Get UFS reset, which is delayed until now to avoid a
>> -		 * circular dependency where UFS needs its PHY, but the PHY
>> -		 * needs this UFS reset.
>> -		 */
>> -		if (!qmp->ufs_reset) {
>> -			qmp->ufs_reset =
>> -				devm_reset_control_get_exclusive(qmp->dev,
>> -								 "ufsphy");
>> -
>> -			if (IS_ERR(qmp->ufs_reset)) {
>> -				ret = PTR_ERR(qmp->ufs_reset);
>> -				dev_err(qmp->dev,
>> -					"failed to get UFS reset: %d\n",
>> -					ret);
>> -
>> -				qmp->ufs_reset = NULL;
>> -				return ret;
>> -			}
>> -		}
>> -	}
>> -
>>   	ret = qmp_ufs_com_init(qmp);
>> -	if (ret)
>> -		return ret;
>> -
>> -	return 0;
>> +	return ret;
>>   }
>>
>>   static int qmp_ufs_phy_calibrate(struct phy *phy)
>> @@ -2088,6 +2061,34 @@ static int qmp_ufs_parse_dt(struct qmp_ufs *qmp)
>>   	return 0;
>>   }
>>
>> +static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp)
>> +{
>> +	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");
> 
> Strange indentation.
> 
>> +
>> +		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 int qmp_ufs_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> @@ -2114,6 +2115,10 @@ static int qmp_ufs_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		return ret;
>>
>> +	ret = qmp_ufs_get_phy_reset(qmp);
>> +	if (ret)
>> +		return ret;
>> +
>>   	/* Check for legacy binding with child node. */
>>   	np = of_get_next_available_child(dev->of_node, NULL);
>>   	if (np) {
>> --
>> 2.48.1
>>
> 

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Dmitry Baryshkov]

On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>
>
>
> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
> > On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
> >> Refactor the UFS PHY reset handling to parse the reset logic only once
> >> during probe, instead of every resume.
> >>
> >> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
> >> qmp_ufs_probe to avoid unnecessary parsing during resume.
> >
> > How did you solve the circular dependency issue being noted below?
>
> Hi Dmitry,
> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
> about the circular dependency issue and whether if it still exists.

It surely does. The reset controller is registered in the beginning of
ufs_qcom_init() and the PHY is acquired only a few lines below. It
creates a very small window for PHY driver to probe.
Which means, NAK, this patch doesn't look acceptable.

>
> Regards,
> Nitin
>
>
> >
> >>
> >> 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 | 61 +++++++++++++------------
> >>   1 file changed, 33 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> >> index 636dc3dc3ea8..12dad28cc1bd 100644
> >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> >> @@ -1799,38 +1799,11 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
> >>   static int qmp_ufs_power_on(struct phy *phy)
> >>   {
> >>      struct qmp_ufs *qmp = phy_get_drvdata(phy);
> >> -    const struct qmp_phy_cfg *cfg = qmp->cfg;
> >>      int ret;
> >>      dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> >>
> >> -    if (cfg->no_pcs_sw_reset) {
> >> -            /*
> >> -             * Get UFS reset, which is delayed until now to avoid a
> >> -             * circular dependency where UFS needs its PHY, but the PHY
> >> -             * needs this UFS reset.
> >> -             */
> >> -            if (!qmp->ufs_reset) {
> >> -                    qmp->ufs_reset =
> >> -                            devm_reset_control_get_exclusive(qmp->dev,
> >> -                                                             "ufsphy");
> >> -
> >> -                    if (IS_ERR(qmp->ufs_reset)) {
> >> -                            ret = PTR_ERR(qmp->ufs_reset);
> >> -                            dev_err(qmp->dev,
> >> -                                    "failed to get UFS reset: %d\n",
> >> -                                    ret);
> >> -
> >> -                            qmp->ufs_reset = NULL;
> >> -                            return ret;
> >> -                    }
> >> -            }
> >> -    }
> >> -
> >>      ret = qmp_ufs_com_init(qmp);
> >> -    if (ret)
> >> -            return ret;
> >> -
> >> -    return 0;
> >> +    return ret;
> >>   }
> >>
> >>   static int qmp_ufs_phy_calibrate(struct phy *phy)
> >> @@ -2088,6 +2061,34 @@ static int qmp_ufs_parse_dt(struct qmp_ufs *qmp)
> >>      return 0;
> >>   }
> >>
> >> +static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp)
> >> +{
> >> +    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");
> >
> > Strange indentation.
> >
> >> +
> >> +            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 int qmp_ufs_probe(struct platform_device *pdev)
> >>   {
> >>      struct device *dev = &pdev->dev;
> >> @@ -2114,6 +2115,10 @@ static int qmp_ufs_probe(struct platform_device *pdev)
> >>      if (ret)
> >>              return ret;
> >>
> >> +    ret = qmp_ufs_get_phy_reset(qmp);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >>      /* Check for legacy binding with child node. */
> >>      np = of_get_next_available_child(dev->of_node, NULL);
> >>      if (np) {
> >> --
> >> 2.48.1
> >>
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Nitin Rawat]

On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote:
> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>>
>>
>>
>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>>>> Refactor the UFS PHY reset handling to parse the reset logic only once
>>>> during probe, instead of every resume.
>>>>
>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
>>>
>>> How did you solve the circular dependency issue being noted below?
>>
>> Hi Dmitry,
>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
>> about the circular dependency issue and whether if it still exists.
> 
> It surely does. The reset controller is registered in the beginning of
> ufs_qcom_init() and the PHY is acquired only a few lines below. It
> creates a very small window for PHY driver to probe.
> Which means, NAK, this patch doesn't look acceptable.

Hi Dmitry,

Thanks for pointing this out. I agree that it leaves very little time 
for the PHY to probe, which may cause issues with targets where 
no_pcs_sw_reset is set to true.

As an experiment, I kept no_pcs_sw_reset set to true for the SM8750, and 
it caused bootup probe issues in some of the iterations I ran.

To address this, I propose updating the patch to move the 
qmp_ufs_get_phy_reset call to phy_calibrate, just before the 
reset_control_assert call.

This change will delay the UFS PHY reset as much as possible in the 
code. Additionally, moving it from phy_power_on to calibrate will ensure 
that qmp_ufs_get_phy_reset is called only once during boot, rather than 
during each phy_power_on call.

Please let me know your thoughts.
=====================================================================================================
  static int qmp_ufs_phy_calibrate(struct phy *phy)
  {
         struct qmp_ufs *qmp = phy_get_drvdata(phy);
@@ -1793,6 +1826,12 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
         unsigned int val;
         int ret;

+       pr_err("%s %d\n", __func__, __LINE__);
+
+       ret = qmp_ufs_get_phy_reset(qmp);
+        if (ret)
+                return ret;
+
         ret = reset_control_assert(qmp->ufs_reset);
         if (ret)
                 return ret;
@@ -1817,7 +1856,7 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
                 dev_err(qmp->dev, "phy initialization timed-out\n");
                 return ret;
=====================================================================================================


Regards.
Nitin
> 
>>
>> Regards,
>> Nitin
>>
>>
>>>
>>>>
>>>> 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 | 61 +++++++++++++------------
>>>>    1 file changed, 33 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> index 636dc3dc3ea8..12dad28cc1bd 100644
>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> @@ -1799,38 +1799,11 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>>>>    static int qmp_ufs_power_on(struct phy *phy)
>>>>    {
>>>>       struct qmp_ufs *qmp = phy_get_drvdata(phy);
>>>> -    const struct qmp_phy_cfg *cfg = qmp->cfg;
>>>>       int ret;
>>>>       dev_vdbg(qmp->dev, "Initializing QMP phy\n");
>>>>
>>>> -    if (cfg->no_pcs_sw_reset) {
>>>> -            /*
>>>> -             * Get UFS reset, which is delayed until now to avoid a
>>>> -             * circular dependency where UFS needs its PHY, but the PHY
>>>> -             * needs this UFS reset.
>>>> -             */
>>>> -            if (!qmp->ufs_reset) {
>>>> -                    qmp->ufs_reset =
>>>> -                            devm_reset_control_get_exclusive(qmp->dev,
>>>> -                                                             "ufsphy");
>>>> -
>>>> -                    if (IS_ERR(qmp->ufs_reset)) {
>>>> -                            ret = PTR_ERR(qmp->ufs_reset);
>>>> -                            dev_err(qmp->dev,
>>>> -                                    "failed to get UFS reset: %d\n",
>>>> -                                    ret);
>>>> -
>>>> -                            qmp->ufs_reset = NULL;
>>>> -                            return ret;
>>>> -                    }
>>>> -            }
>>>> -    }
>>>> -
>>>>       ret = qmp_ufs_com_init(qmp);
>>>> -    if (ret)
>>>> -            return ret;
>>>> -
>>>> -    return 0;
>>>> +    return ret;
>>>>    }
>>>>
>>>>    static int qmp_ufs_phy_calibrate(struct phy *phy)
>>>> @@ -2088,6 +2061,34 @@ static int qmp_ufs_parse_dt(struct qmp_ufs *qmp)
>>>>       return 0;
>>>>    }
>>>>
>>>> +static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp)
>>>> +{
>>>> +    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");
>>>
>>> Strange indentation.
>>>
>>>> +
>>>> +            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 int qmp_ufs_probe(struct platform_device *pdev)
>>>>    {
>>>>       struct device *dev = &pdev->dev;
>>>> @@ -2114,6 +2115,10 @@ static int qmp_ufs_probe(struct platform_device *pdev)
>>>>       if (ret)
>>>>               return ret;
>>>>
>>>> +    ret = qmp_ufs_get_phy_reset(qmp);
>>>> +    if (ret)
>>>> +            return ret;
>>>> +
>>>>       /* Check for legacy binding with child node. */
>>>>       np = of_get_next_available_child(dev->of_node, NULL);
>>>>       if (np) {
>>>> --
>>>> 2.48.1
>>>>
>>>
>>
> 
> 

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Dmitry Baryshkov]

On 14/04/2025 23:34, Nitin Rawat wrote:
> 
> 
> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote:
>> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com> 
>> wrote:
>>>
>>>
>>>
>>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
>>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>>>>> Refactor the UFS PHY reset handling to parse the reset logic only once
>>>>> during probe, instead of every resume.
>>>>>
>>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
>>>>
>>>> How did you solve the circular dependency issue being noted below?
>>>
>>> Hi Dmitry,
>>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
>>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
>>> about the circular dependency issue and whether if it still exists.
>>
>> It surely does. The reset controller is registered in the beginning of
>> ufs_qcom_init() and the PHY is acquired only a few lines below. It
>> creates a very small window for PHY driver to probe.
>> Which means, NAK, this patch doesn't look acceptable.
> 
> Hi Dmitry,
> 
> Thanks for pointing this out. I agree that it leaves very little time 
> for the PHY to probe, which may cause issues with targets where 
> no_pcs_sw_reset is set to true.
> 
> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750, and 
> it caused bootup probe issues in some of the iterations I ran.
> 
> To address this, I propose updating the patch to move the 
> qmp_ufs_get_phy_reset call to phy_calibrate, just before the 
> reset_control_assert call.

Will it cause an issue if we move it to phy_init() instead of 
phy_calibrate()?

> 
> This change will delay the UFS PHY reset as much as possible in the 
> code. Additionally, moving it from phy_power_on to calibrate will ensure 
> that qmp_ufs_get_phy_reset is called only once during boot, rather than 
> during each phy_power_on call.
> 
> Please let me know your thoughts.
> =====================================================================================================
>   static int qmp_ufs_phy_calibrate(struct phy *phy)
>   {
>          struct qmp_ufs *qmp = phy_get_drvdata(phy);
> @@ -1793,6 +1826,12 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
>          unsigned int val;
>          int ret;
> 
> +       pr_err("%s %d\n", __func__, __LINE__);
> +
> +       ret = qmp_ufs_get_phy_reset(qmp);
> +        if (ret)
> +                return ret;
> +
>          ret = reset_control_assert(qmp->ufs_reset);
>          if (ret)
>                  return ret;
> @@ -1817,7 +1856,7 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
>                  dev_err(qmp->dev, "phy initialization timed-out\n");
>                  return ret;
> =====================================================================================================
> 
> 
> Regards.
> Nitin
>>
>>>
>>> Regards,
>>> Nitin
>>>
>>>
>>>>
>>>>>
>>>>> 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 | 61 ++++++++++++ 
>>>>> +------------
>>>>>    1 file changed, 33 insertions(+), 28 deletions(-)
>>>>>
>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/ 
>>>>> qualcomm/phy-qcom-qmp-ufs.c
>>>>> index 636dc3dc3ea8..12dad28cc1bd 100644
>>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>>> @@ -1799,38 +1799,11 @@ static int qmp_ufs_com_exit(struct qmp_ufs 
>>>>> *qmp)
>>>>>    static int qmp_ufs_power_on(struct phy *phy)
>>>>>    {
>>>>>       struct qmp_ufs *qmp = phy_get_drvdata(phy);
>>>>> -    const struct qmp_phy_cfg *cfg = qmp->cfg;
>>>>>       int ret;
>>>>>       dev_vdbg(qmp->dev, "Initializing QMP phy\n");
>>>>>
>>>>> -    if (cfg->no_pcs_sw_reset) {
>>>>> -            /*
>>>>> -             * Get UFS reset, which is delayed until now to avoid a
>>>>> -             * circular dependency where UFS needs its PHY, but 
>>>>> the PHY
>>>>> -             * needs this UFS reset.
>>>>> -             */
>>>>> -            if (!qmp->ufs_reset) {
>>>>> -                    qmp->ufs_reset =
>>>>> -                            devm_reset_control_get_exclusive(qmp- 
>>>>> >dev,
>>>>> -                                                             
>>>>> "ufsphy");
>>>>> -
>>>>> -                    if (IS_ERR(qmp->ufs_reset)) {
>>>>> -                            ret = PTR_ERR(qmp->ufs_reset);
>>>>> -                            dev_err(qmp->dev,
>>>>> -                                    "failed to get UFS reset: %d\n",
>>>>> -                                    ret);
>>>>> -
>>>>> -                            qmp->ufs_reset = NULL;
>>>>> -                            return ret;
>>>>> -                    }
>>>>> -            }
>>>>> -    }
>>>>> -
>>>>>       ret = qmp_ufs_com_init(qmp);
>>>>> -    if (ret)
>>>>> -            return ret;
>>>>> -
>>>>> -    return 0;
>>>>> +    return ret;
>>>>>    }
>>>>>
>>>>>    static int qmp_ufs_phy_calibrate(struct phy *phy)
>>>>> @@ -2088,6 +2061,34 @@ static int qmp_ufs_parse_dt(struct qmp_ufs 
>>>>> *qmp)
>>>>>       return 0;
>>>>>    }
>>>>>
>>>>> +static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp)
>>>>> +{
>>>>> +    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");
>>>>
>>>> Strange indentation.
>>>>
>>>>> +
>>>>> +            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 int qmp_ufs_probe(struct platform_device *pdev)
>>>>>    {
>>>>>       struct device *dev = &pdev->dev;
>>>>> @@ -2114,6 +2115,10 @@ static int qmp_ufs_probe(struct 
>>>>> platform_device *pdev)
>>>>>       if (ret)
>>>>>               return ret;
>>>>>
>>>>> +    ret = qmp_ufs_get_phy_reset(qmp);
>>>>> +    if (ret)
>>>>> +            return ret;
>>>>> +
>>>>>       /* Check for legacy binding with child node. */
>>>>>       np = of_get_next_available_child(dev->of_node, NULL);
>>>>>       if (np) {
>>>>> -- 
>>>>> 2.48.1
>>>>>
>>>>
>>>
>>
>>
> 


-- 
With best wishes
Dmitry

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Nitin Rawat]

On 4/15/2025 2:59 PM, Dmitry Baryshkov wrote:
> On 14/04/2025 23:34, Nitin Rawat wrote:
>>
>>
>> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote:
>>> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com> 
>>> wrote:
>>>>
>>>>
>>>>
>>>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
>>>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>>>>>> Refactor the UFS PHY reset handling to parse the reset logic only 
>>>>>> once
>>>>>> during probe, instead of every resume.
>>>>>>
>>>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
>>>>>
>>>>> How did you solve the circular dependency issue being noted below?
>>>>
>>>> Hi Dmitry,
>>>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
>>>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
>>>> about the circular dependency issue and whether if it still exists.
>>>
>>> It surely does. The reset controller is registered in the beginning of
>>> ufs_qcom_init() and the PHY is acquired only a few lines below. It
>>> creates a very small window for PHY driver to probe.
>>> Which means, NAK, this patch doesn't look acceptable.
>>
>> Hi Dmitry,
>>
>> Thanks for pointing this out. I agree that it leaves very little time 
>> for the PHY to probe, which may cause issues with targets where 
>> no_pcs_sw_reset is set to true.
>>
>> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750, 
>> and it caused bootup probe issues in some of the iterations I ran.
>>
>> To address this, I propose updating the patch to move the 
>> qmp_ufs_get_phy_reset call to phy_calibrate, just before the 
>> reset_control_assert call.
> 
> Will it cause an issue if we move it to phy_init() instead of 
> phy_calibrate()?

Hi Dmitry,

Thanks for suggestion.
Phy_init is invoked before phy_set_mode_ext and ufs_qcom_phy_power_on, 
whereas calibrate is called after ufs_qcom_phy_power_on. Keeping the UFS 
PHY reset in phy_calibrate introduces relatively more delay, providing 
more buffer time for the PHY driver probe, ensuring the UFS PHY reset is 
handled correctly the first time.

Moving the calibration to phy_init shouldn't cause any issues. However, 
since we currently don't have an initialization operations registered 
for init, we would need to add a new PHY initialization ops if we decide 
to move it to phy_init.

Please let me know if this looks fine to you, or if you have any 
suggestions. I am open to your suggestions.

Regards,
Nitin


> 
>>
>> This change will delay the UFS PHY reset as much as possible in the 
>> code. Additionally, moving it from phy_power_on to calibrate will 
>> ensure that qmp_ufs_get_phy_reset is called only once during boot, 
>> rather than during each phy_power_on call.
>>
>> Please let me know your thoughts.
>> =====================================================================================================
>>   static int qmp_ufs_phy_calibrate(struct phy *phy)
>>   {
>>          struct qmp_ufs *qmp = phy_get_drvdata(phy);
>> @@ -1793,6 +1826,12 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
>>          unsigned int val;
>>          int ret;
>>
>> +       pr_err("%s %d\n", __func__, __LINE__);
>> +
>> +       ret = qmp_ufs_get_phy_reset(qmp);
>> +        if (ret)
>> +                return ret;
>> +
>>          ret = reset_control_assert(qmp->ufs_reset);
>>          if (ret)
>>                  return ret;
>> @@ -1817,7 +1856,7 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
>>                  dev_err(qmp->dev, "phy initialization timed-out\n");
>>                  return ret;
>> =====================================================================================================
>>
>>
>> Regards.
>> Nitin
>>>
>>>>
>>>> Regards,
>>>> Nitin
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> 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 | 61 ++++++++++++ 
>>>>>> +------------
>>>>>>    1 file changed, 33 insertions(+), 28 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/ 
>>>>>> phy/ qualcomm/phy-qcom-qmp-ufs.c
>>>>>> index 636dc3dc3ea8..12dad28cc1bd 100644
>>>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>>>> @@ -1799,38 +1799,11 @@ static int qmp_ufs_com_exit(struct qmp_ufs 
>>>>>> *qmp)
>>>>>>    static int qmp_ufs_power_on(struct phy *phy)
>>>>>>    {
>>>>>>       struct qmp_ufs *qmp = phy_get_drvdata(phy);
>>>>>> -    const struct qmp_phy_cfg *cfg = qmp->cfg;
>>>>>>       int ret;
>>>>>>       dev_vdbg(qmp->dev, "Initializing QMP phy\n");
>>>>>>
>>>>>> -    if (cfg->no_pcs_sw_reset) {
>>>>>> -            /*
>>>>>> -             * Get UFS reset, which is delayed until now to avoid a
>>>>>> -             * circular dependency where UFS needs its PHY, but 
>>>>>> the PHY
>>>>>> -             * needs this UFS reset.
>>>>>> -             */
>>>>>> -            if (!qmp->ufs_reset) {
>>>>>> -                    qmp->ufs_reset =
>>>>>> -                            devm_reset_control_get_exclusive(qmp- 
>>>>>> >dev,
>>>>>> - "ufsphy");
>>>>>> -
>>>>>> -                    if (IS_ERR(qmp->ufs_reset)) {
>>>>>> -                            ret = PTR_ERR(qmp->ufs_reset);
>>>>>> -                            dev_err(qmp->dev,
>>>>>> -                                    "failed to get UFS reset: %d\n",
>>>>>> -                                    ret);
>>>>>> -
>>>>>> -                            qmp->ufs_reset = NULL;
>>>>>> -                            return ret;
>>>>>> -                    }
>>>>>> -            }
>>>>>> -    }
>>>>>> -
>>>>>>       ret = qmp_ufs_com_init(qmp);
>>>>>> -    if (ret)
>>>>>> -            return ret;
>>>>>> -
>>>>>> -    return 0;
>>>>>> +    return ret;
>>>>>>    }
>>>>>>
>>>>>>    static int qmp_ufs_phy_calibrate(struct phy *phy)
>>>>>> @@ -2088,6 +2061,34 @@ static int qmp_ufs_parse_dt(struct qmp_ufs 
>>>>>> *qmp)
>>>>>>       return 0;
>>>>>>    }
>>>>>>
>>>>>> +static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp)
>>>>>> +{
>>>>>> +    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");
>>>>>
>>>>> Strange indentation.
>>>>>
>>>>>> +
>>>>>> +            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 int qmp_ufs_probe(struct platform_device *pdev)
>>>>>>    {
>>>>>>       struct device *dev = &pdev->dev;
>>>>>> @@ -2114,6 +2115,10 @@ static int qmp_ufs_probe(struct 
>>>>>> platform_device *pdev)
>>>>>>       if (ret)
>>>>>>               return ret;
>>>>>>
>>>>>> +    ret = qmp_ufs_get_phy_reset(qmp);
>>>>>> +    if (ret)
>>>>>> +            return ret;
>>>>>> +
>>>>>>       /* Check for legacy binding with child node. */
>>>>>>       np = of_get_next_available_child(dev->of_node, NULL);
>>>>>>       if (np) {
>>>>>> -- 
>>>>>> 2.48.1
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Dmitry Baryshkov]

On Wed, 16 Apr 2025 at 12:08, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>
>
>
> On 4/15/2025 2:59 PM, Dmitry Baryshkov wrote:
> > On 14/04/2025 23:34, Nitin Rawat wrote:
> >>
> >>
> >> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote:
> >>> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com>
> >>> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
> >>>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
> >>>>>> Refactor the UFS PHY reset handling to parse the reset logic only
> >>>>>> once
> >>>>>> during probe, instead of every resume.
> >>>>>>
> >>>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
> >>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
> >>>>>
> >>>>> How did you solve the circular dependency issue being noted below?
> >>>>
> >>>> Hi Dmitry,
> >>>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
> >>>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
> >>>> about the circular dependency issue and whether if it still exists.
> >>>
> >>> It surely does. The reset controller is registered in the beginning of
> >>> ufs_qcom_init() and the PHY is acquired only a few lines below. It
> >>> creates a very small window for PHY driver to probe.
> >>> Which means, NAK, this patch doesn't look acceptable.
> >>
> >> Hi Dmitry,
> >>
> >> Thanks for pointing this out. I agree that it leaves very little time
> >> for the PHY to probe, which may cause issues with targets where
> >> no_pcs_sw_reset is set to true.
> >>
> >> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750,
> >> and it caused bootup probe issues in some of the iterations I ran.
> >>
> >> To address this, I propose updating the patch to move the
> >> qmp_ufs_get_phy_reset call to phy_calibrate, just before the
> >> reset_control_assert call.
> >
> > Will it cause an issue if we move it to phy_init() instead of
> > phy_calibrate()?
>
> Hi Dmitry,
>
> Thanks for suggestion.
> Phy_init is invoked before phy_set_mode_ext and ufs_qcom_phy_power_on,
> whereas calibrate is called after ufs_qcom_phy_power_on. Keeping the UFS
> PHY reset in phy_calibrate introduces relatively more delay, providing
> more buffer time for the PHY driver probe, ensuring the UFS PHY reset is
> handled correctly the first time.

We are requesting the PHY anyway, so the PHY driver should have probed
well before phy_init() call. I don't get this comment.

>
> Moving the calibration to phy_init shouldn't cause any issues. However,
> since we currently don't have an initialization operations registered
> for init, we would need to add a new PHY initialization ops if we decide
> to move it to phy_init.

Yes. I don't see it as a problem. Is there any kind of an issue there?

>
> Please let me know if this looks fine to you, or if you have any
> suggestions. I am open to your suggestions.

phy_init() callback

-- 
With best wishes
Dmitry

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Nitin Rawat]

On 4/16/2025 5:43 PM, Dmitry Baryshkov wrote:
> On Wed, 16 Apr 2025 at 12:08, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>>
>>
>>
>> On 4/15/2025 2:59 PM, Dmitry Baryshkov wrote:
>>> On 14/04/2025 23:34, Nitin Rawat wrote:
>>>>
>>>>
>>>> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote:
>>>>> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>>>>>>>> Refactor the UFS PHY reset handling to parse the reset logic only
>>>>>>>> once
>>>>>>>> during probe, instead of every resume.
>>>>>>>>
>>>>>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>>>>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
>>>>>>>
>>>>>>> How did you solve the circular dependency issue being noted below?
>>>>>>
>>>>>> Hi Dmitry,
>>>>>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
>>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
>>>>>> about the circular dependency issue and whether if it still exists.
>>>>>
>>>>> It surely does. The reset controller is registered in the beginning of
>>>>> ufs_qcom_init() and the PHY is acquired only a few lines below. It
>>>>> creates a very small window for PHY driver to probe.
>>>>> Which means, NAK, this patch doesn't look acceptable.
>>>>
>>>> Hi Dmitry,
>>>>
>>>> Thanks for pointing this out. I agree that it leaves very little time
>>>> for the PHY to probe, which may cause issues with targets where
>>>> no_pcs_sw_reset is set to true.
>>>>
>>>> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750,
>>>> and it caused bootup probe issues in some of the iterations I ran.
>>>>
>>>> To address this, I propose updating the patch to move the
>>>> qmp_ufs_get_phy_reset call to phy_calibrate, just before the
>>>> reset_control_assert call.
>>>
>>> Will it cause an issue if we move it to phy_init() instead of
>>> phy_calibrate()?
>>
>> Hi Dmitry,
>>
>> Thanks for suggestion.
>> Phy_init is invoked before phy_set_mode_ext and ufs_qcom_phy_power_on,
>> whereas calibrate is called after ufs_qcom_phy_power_on. Keeping the UFS
>> PHY reset in phy_calibrate introduces relatively more delay, providing
>> more buffer time for the PHY driver probe, ensuring the UFS PHY reset is
>> handled correctly the first time.
> 
> We are requesting the PHY anyway, so the PHY driver should have probed
> well before phy_init() call. I don't get this comment.
> 
>>
>> Moving the calibration to phy_init shouldn't cause any issues. However,
>> since we currently don't have an initialization operations registered
>> for init, we would need to add a new PHY initialization ops if we decide
>> to move it to phy_init.
> 
> Yes. I don't see it as a problem. Is there any kind of an issue there?

No issues. In my next patchset, I would add a new init ops registered 
for qcom phy and move get ufs phy reset handler to it.

Regards,
Nitin

> 
>>
>> Please let me know if this looks fine to you, or if you have any
>> suggestions. I am open to your suggestions.
> 
> phy_init() callback
> 

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Konrad Dybcio]

On 4/16/25 2:26 PM, Nitin Rawat wrote:
> 
> 
> On 4/16/2025 5:43 PM, Dmitry Baryshkov wrote:
>> On Wed, 16 Apr 2025 at 12:08, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 4/15/2025 2:59 PM, Dmitry Baryshkov wrote:
>>>> On 14/04/2025 23:34, Nitin Rawat wrote:
>>>>>
>>>>>
>>>>> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote:
>>>>>> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
>>>>>>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>>>>>>>>> Refactor the UFS PHY reset handling to parse the reset logic only
>>>>>>>>> once
>>>>>>>>> during probe, instead of every resume.
>>>>>>>>>
>>>>>>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>>>>>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
>>>>>>>>
>>>>>>>> How did you solve the circular dependency issue being noted below?
>>>>>>>
>>>>>>> Hi Dmitry,
>>>>>>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
>>>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
>>>>>>> about the circular dependency issue and whether if it still exists.
>>>>>>
>>>>>> It surely does. The reset controller is registered in the beginning of
>>>>>> ufs_qcom_init() and the PHY is acquired only a few lines below. It
>>>>>> creates a very small window for PHY driver to probe.
>>>>>> Which means, NAK, this patch doesn't look acceptable.
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Thanks for pointing this out. I agree that it leaves very little time
>>>>> for the PHY to probe, which may cause issues with targets where
>>>>> no_pcs_sw_reset is set to true.
>>>>>
>>>>> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750,
>>>>> and it caused bootup probe issues in some of the iterations I ran.
>>>>>
>>>>> To address this, I propose updating the patch to move the
>>>>> qmp_ufs_get_phy_reset call to phy_calibrate, just before the
>>>>> reset_control_assert call.
>>>>
>>>> Will it cause an issue if we move it to phy_init() instead of
>>>> phy_calibrate()?
>>>
>>> Hi Dmitry,
>>>
>>> Thanks for suggestion.
>>> Phy_init is invoked before phy_set_mode_ext and ufs_qcom_phy_power_on,
>>> whereas calibrate is called after ufs_qcom_phy_power_on. Keeping the UFS
>>> PHY reset in phy_calibrate introduces relatively more delay, providing
>>> more buffer time for the PHY driver probe, ensuring the UFS PHY reset is
>>> handled correctly the first time.
>>
>> We are requesting the PHY anyway, so the PHY driver should have probed
>> well before phy_init() call. I don't get this comment.
>>
>>>
>>> Moving the calibration to phy_init shouldn't cause any issues. However,
>>> since we currently don't have an initialization operations registered
>>> for init, we would need to add a new PHY initialization ops if we decide
>>> to move it to phy_init.
>>
>> Yes. I don't see it as a problem. Is there any kind of an issue there?
> 
> No issues. In my next patchset, I would add a new init ops registered for qcom phy and move get ufs phy reset handler to it.

I don't really like this circular dependency.

So I took a peek at the docs and IIUC, they say that the reset coming
from the UFS controller effectively does the same thing as QPHY_SW_RESET.

Moreover, the docs mention the controller-side reset should not be used
anymore (as of at least X1E & SM8550). Docs for MSM8996 (one of the
oldest platforms that this driver supports) also don't really mention a
hard dependency on it.

So we can get rid of this mess entirely, without affecting backwards
compatibility even, as this is all contained within the PHYs register
space and driver.

Konrad

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Konrad Dybcio]

On 4/23/25 1:09 PM, Konrad Dybcio wrote:
> On 4/16/25 2:26 PM, Nitin Rawat wrote:
>>
>>
>> On 4/16/2025 5:43 PM, Dmitry Baryshkov wrote:
>>> On Wed, 16 Apr 2025 at 12:08, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 4/15/2025 2:59 PM, Dmitry Baryshkov wrote:
>>>>> On 14/04/2025 23:34, Nitin Rawat wrote:
>>>>>>
>>>>>>
>>>>>> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote:
>>>>>>> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>>>>>>>>>> Refactor the UFS PHY reset handling to parse the reset logic only
>>>>>>>>>> once
>>>>>>>>>> during probe, instead of every resume.
>>>>>>>>>>
>>>>>>>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>>>>>>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
>>>>>>>>>
>>>>>>>>> How did you solve the circular dependency issue being noted below?
>>>>>>>>
>>>>>>>> Hi Dmitry,
>>>>>>>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
>>>>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
>>>>>>>> about the circular dependency issue and whether if it still exists.
>>>>>>>
>>>>>>> It surely does. The reset controller is registered in the beginning of
>>>>>>> ufs_qcom_init() and the PHY is acquired only a few lines below. It
>>>>>>> creates a very small window for PHY driver to probe.
>>>>>>> Which means, NAK, this patch doesn't look acceptable.
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Thanks for pointing this out. I agree that it leaves very little time
>>>>>> for the PHY to probe, which may cause issues with targets where
>>>>>> no_pcs_sw_reset is set to true.
>>>>>>
>>>>>> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750,
>>>>>> and it caused bootup probe issues in some of the iterations I ran.
>>>>>>
>>>>>> To address this, I propose updating the patch to move the
>>>>>> qmp_ufs_get_phy_reset call to phy_calibrate, just before the
>>>>>> reset_control_assert call.
>>>>>
>>>>> Will it cause an issue if we move it to phy_init() instead of
>>>>> phy_calibrate()?
>>>>
>>>> Hi Dmitry,
>>>>
>>>> Thanks for suggestion.
>>>> Phy_init is invoked before phy_set_mode_ext and ufs_qcom_phy_power_on,
>>>> whereas calibrate is called after ufs_qcom_phy_power_on. Keeping the UFS
>>>> PHY reset in phy_calibrate introduces relatively more delay, providing
>>>> more buffer time for the PHY driver probe, ensuring the UFS PHY reset is
>>>> handled correctly the first time.
>>>
>>> We are requesting the PHY anyway, so the PHY driver should have probed
>>> well before phy_init() call. I don't get this comment.
>>>
>>>>
>>>> Moving the calibration to phy_init shouldn't cause any issues. However,
>>>> since we currently don't have an initialization operations registered
>>>> for init, we would need to add a new PHY initialization ops if we decide
>>>> to move it to phy_init.
>>>
>>> Yes. I don't see it as a problem. Is there any kind of an issue there?
>>
>> No issues. In my next patchset, I would add a new init ops registered for qcom phy and move get ufs phy reset handler to it.
> 
> I don't really like this circular dependency.
> 
> So I took a peek at the docs and IIUC, they say that the reset coming
> from the UFS controller effectively does the same thing as QPHY_SW_RESET.
> 
> Moreover, the docs mention the controller-side reset should not be used
> anymore (as of at least X1E & SM8550). Docs for MSM8996 (one of the
> oldest platforms that this driver supports) also don't really mention a
> hard dependency on it.
> 
> So we can get rid of this mess entirely, without affecting backwards
> compatibility even, as this is all contained within the PHYs register
> space and driver.

Well hmm, certain platforms (with no_pcs_sw_reset) don't agree.. some
have GCC-sourced resets, but I'm not 100% sure how they affect the CSR
state

Konrad

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Nitin Rawat]

On 4/23/2025 4:51 PM, Konrad Dybcio wrote:
> On 4/23/25 1:09 PM, Konrad Dybcio wrote:
>> On 4/16/25 2:26 PM, Nitin Rawat wrote:
>>>
>>>
>>> On 4/16/2025 5:43 PM, Dmitry Baryshkov wrote:
>>>> On Wed, 16 Apr 2025 at 12:08, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 4/15/2025 2:59 PM, Dmitry Baryshkov wrote:
>>>>>> On 14/04/2025 23:34, Nitin Rawat wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>>>>>>>>>>> Refactor the UFS PHY reset handling to parse the reset logic only
>>>>>>>>>>> once
>>>>>>>>>>> during probe, instead of every resume.
>>>>>>>>>>>
>>>>>>>>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>>>>>>>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
>>>>>>>>>>
>>>>>>>>>> How did you solve the circular dependency issue being noted below?
>>>>>>>>>
>>>>>>>>> Hi Dmitry,
>>>>>>>>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
>>>>>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
>>>>>>>>> about the circular dependency issue and whether if it still exists.
>>>>>>>>
>>>>>>>> It surely does. The reset controller is registered in the beginning of
>>>>>>>> ufs_qcom_init() and the PHY is acquired only a few lines below. It
>>>>>>>> creates a very small window for PHY driver to probe.
>>>>>>>> Which means, NAK, this patch doesn't look acceptable.
>>>>>>>
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> Thanks for pointing this out. I agree that it leaves very little time
>>>>>>> for the PHY to probe, which may cause issues with targets where
>>>>>>> no_pcs_sw_reset is set to true.
>>>>>>>
>>>>>>> As an experiment, I kept no_pcs_sw_reset set to true for the SM8750,
>>>>>>> and it caused bootup probe issues in some of the iterations I ran.
>>>>>>>
>>>>>>> To address this, I propose updating the patch to move the
>>>>>>> qmp_ufs_get_phy_reset call to phy_calibrate, just before the
>>>>>>> reset_control_assert call.
>>>>>>
>>>>>> Will it cause an issue if we move it to phy_init() instead of
>>>>>> phy_calibrate()?
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Thanks for suggestion.
>>>>> Phy_init is invoked before phy_set_mode_ext and ufs_qcom_phy_power_on,
>>>>> whereas calibrate is called after ufs_qcom_phy_power_on. Keeping the UFS
>>>>> PHY reset in phy_calibrate introduces relatively more delay, providing
>>>>> more buffer time for the PHY driver probe, ensuring the UFS PHY reset is
>>>>> handled correctly the first time.
>>>>
>>>> We are requesting the PHY anyway, so the PHY driver should have probed
>>>> well before phy_init() call. I don't get this comment.
>>>>
>>>>>
>>>>> Moving the calibration to phy_init shouldn't cause any issues. However,
>>>>> since we currently don't have an initialization operations registered
>>>>> for init, we would need to add a new PHY initialization ops if we decide
>>>>> to move it to phy_init.
>>>>
>>>> Yes. I don't see it as a problem. Is there any kind of an issue there?
>>>
>>> No issues. In my next patchset, I would add a new init ops registered for qcom phy and move get ufs phy reset handler to it.
>>
>> I don't really like this circular dependency.
>>
>> So I took a peek at the docs and IIUC, they say that the reset coming
>> from the UFS controller effectively does the same thing as QPHY_SW_RESET.
>>
>> Moreover, the docs mention the controller-side reset should not be used
>> anymore (as of at least X1E & SM8550). Docs for MSM8996 (one of the
>> oldest platforms that this driver supports) also don't really mention a
>> hard dependency on it.
>>
>> So we can get rid of this mess entirely, without affecting backwards
>> compatibility even, as this is all contained within the PHYs register
>> space and driver.
> 
> Well hmm, certain platforms (with no_pcs_sw_reset) don't agree.. some
> have GCC-sourced resets, but I'm not 100% sure how they affect the CSR
> state

Hi Konrad,

I agree with you, but there are still some targets (Sdm845, SM7150, 
SM6125, and MSM8996) that have upstream support and require a 
controller-side GCC reset. Therefore to align with HPG we can't remove
gcc reset for these targets.
Please let me know your opinions.

Regards,
nitin


> 
> Konrad

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Konrad Dybcio]

On 4/11/25 1:08 PM, Dmitry Baryshkov wrote:
> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>>
>>
>>
>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>>>> Refactor the UFS PHY reset handling to parse the reset logic only once
>>>> during probe, instead of every resume.
>>>>
>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
>>>
>>> How did you solve the circular dependency issue being noted below?
>>
>> Hi Dmitry,
>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
>> about the circular dependency issue and whether if it still exists.
> 
> It surely does. The reset controller is registered in the beginning of
> ufs_qcom_init() and the PHY is acquired only a few lines below. It
> creates a very small window for PHY driver to probe.
> Which means, NAK, this patch doesn't look acceptable.

devlink? EPROBE_DEFER? is this really an issue?

Konrad

Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset

[Dmitry Baryshkov]

On 23/04/2025 16:47, Konrad Dybcio wrote:
> On 4/11/25 1:08 PM, Dmitry Baryshkov wrote:
>> On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
>>>> On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
>>>>> Refactor the UFS PHY reset handling to parse the reset logic only once
>>>>> during probe, instead of every resume.
>>>>>
>>>>> Move the UFS PHY reset parsing logic from qmp_phy_power_on to
>>>>> qmp_ufs_probe to avoid unnecessary parsing during resume.
>>>>
>>>> How did you solve the circular dependency issue being noted below?
>>>
>>> Hi Dmitry,
>>> As part of my patch, I moved the parsing logic from qmp_phy_power_on to
>>> qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
>>> about the circular dependency issue and whether if it still exists.
>>
>> It surely does. The reset controller is registered in the beginning of
>> ufs_qcom_init() and the PHY is acquired only a few lines below. It
>> creates a very small window for PHY driver to probe.
>> Which means, NAK, this patch doesn't look acceptable.
> 
> devlink? EPROBE_DEFER? is this really an issue?

Yes, it is. No, devlink won't help.


-- 
With best wishes
Dmitry

[PATCH V3 5/9] phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()

[Nitin Rawat]

Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into
qmp_ufs_power_on(). This change removes unnecessary function calls and
ensures that the initialization logic is directly within the power-on
routine, maintaining the same functionality.

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

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

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

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

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

--
2.48.1

Re: [PATCH V3 5/9] phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()

[Dmitry Baryshkov]

On Thu, Apr 10, 2025 at 02:30:58PM +0530, Nitin Rawat wrote:
> Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into
> qmp_ufs_power_on(). This change removes unnecessary function calls and
> ensures that the initialization logic is directly within the power-on
> routine, maintaining the same functionality.

Which problem is this patch trying to solve?

> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 44 ++++++++++---------------
>  1 file changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 12dad28cc1bd..2cc819089d71 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -1757,31 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
>  	qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
>  }
> 
> -static int qmp_ufs_com_init(struct qmp_ufs *qmp)
> -{
> -	const struct qmp_phy_cfg *cfg = qmp->cfg;
> -	void __iomem *pcs = qmp->pcs;
> -	int ret;
> -
> -	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> -	if (ret) {
> -		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
> -	if (ret)
> -		goto err_disable_regulators;
> -
> -	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> -
> -	return 0;
> -
> -err_disable_regulators:
> -	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
> -
> -	return ret;
> -}
> 
>  static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>  {
> @@ -1799,10 +1774,27 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>  static int qmp_ufs_power_on(struct phy *phy)
>  {
>  	struct qmp_ufs *qmp = phy_get_drvdata(phy);
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	void __iomem *pcs = qmp->pcs;
>  	int ret;
> +
>  	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> 
> -	ret = qmp_ufs_com_init(qmp);
> +	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> +	if (ret) {
> +		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
> +	if (ret)
> +		goto err_disable_regulators;
> +
> +	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> +	return 0;
> +
> +err_disable_regulators:
> +	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>  	return ret;
>  }
> 
> --
> 2.48.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH V3 5/9] phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()

[Nitin Rawat]

On 4/11/2025 1:39 AM, Dmitry Baryshkov wrote:
> On Thu, Apr 10, 2025 at 02:30:58PM +0530, Nitin Rawat wrote:
>> Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into
>> qmp_ufs_power_on(). This change removes unnecessary function calls and
>> ensures that the initialization logic is directly within the power-on
>> routine, maintaining the same functionality.
> 
> Which problem is this patch trying to solve?

Hi Dmitry,

As part of the patch, I simplified the code by moving qmp_ufs_com_init 
inline to qmp_ufs_power_on, since qmp_ufs_power_on was merely calling 
qmp_ufs_com_init. This change eliminates unnecessary function call.

Regards,
Nitin



> 
>>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 44 ++++++++++---------------
>>   1 file changed, 18 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> index 12dad28cc1bd..2cc819089d71 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> @@ -1757,31 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
>>   	qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
>>   }
>>
>> -static int qmp_ufs_com_init(struct qmp_ufs *qmp)
>> -{
>> -	const struct qmp_phy_cfg *cfg = qmp->cfg;
>> -	void __iomem *pcs = qmp->pcs;
>> -	int ret;
>> -
>> -	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>> -	if (ret) {
>> -		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
>> -	if (ret)
>> -		goto err_disable_regulators;
>> -
>> -	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
>> -
>> -	return 0;
>> -
>> -err_disable_regulators:
>> -	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>> -
>> -	return ret;
>> -}
>>
>>   static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>>   {
>> @@ -1799,10 +1774,27 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>>   static int qmp_ufs_power_on(struct phy *phy)
>>   {
>>   	struct qmp_ufs *qmp = phy_get_drvdata(phy);
>> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
>> +	void __iomem *pcs = qmp->pcs;
>>   	int ret;
>> +
>>   	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
>>
>> -	ret = qmp_ufs_com_init(qmp);
>> +	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>> +	if (ret) {
>> +		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
>> +	if (ret)
>> +		goto err_disable_regulators;
>> +
>> +	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
>> +	return 0;
>> +
>> +err_disable_regulators:
>> +	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>>   	return ret;
>>   }
>>
>> --
>> 2.48.1
>>
> 

Re: [PATCH V3 5/9] phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()

[Dmitry Baryshkov]

On Fri, 11 Apr 2025 at 13:42, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>
>
>
> On 4/11/2025 1:39 AM, Dmitry Baryshkov wrote:
> > On Thu, Apr 10, 2025 at 02:30:58PM +0530, Nitin Rawat wrote:
> >> Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into
> >> qmp_ufs_power_on(). This change removes unnecessary function calls and
> >> ensures that the initialization logic is directly within the power-on
> >> routine, maintaining the same functionality.
> >
> > Which problem is this patch trying to solve?
>
> Hi Dmitry,
>
> As part of the patch, I simplified the code by moving qmp_ufs_com_init
> inline to qmp_ufs_power_on, since qmp_ufs_power_on was merely calling
> qmp_ufs_com_init. This change eliminates unnecessary function call.

You again are describing what you did. Please start by stating the
problem or the issue.

>
> Regards,
> Nitin
>
>
>
> >
> >>
> >> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> >> ---
> >>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 44 ++++++++++---------------
> >>   1 file changed, 18 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> >> index 12dad28cc1bd..2cc819089d71 100644
> >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> >> @@ -1757,31 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
> >>      qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
> >>   }
> >>
> >> -static int qmp_ufs_com_init(struct qmp_ufs *qmp)
> >> -{
> >> -    const struct qmp_phy_cfg *cfg = qmp->cfg;
> >> -    void __iomem *pcs = qmp->pcs;
> >> -    int ret;
> >> -
> >> -    ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> >> -    if (ret) {
> >> -            dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
> >> -            return ret;
> >> -    }
> >> -
> >> -    ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
> >> -    if (ret)
> >> -            goto err_disable_regulators;
> >> -
> >> -    qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> >> -
> >> -    return 0;
> >> -
> >> -err_disable_regulators:
> >> -    regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
> >> -
> >> -    return ret;
> >> -}
> >>
> >>   static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
> >>   {
> >> @@ -1799,10 +1774,27 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
> >>   static int qmp_ufs_power_on(struct phy *phy)
> >>   {
> >>      struct qmp_ufs *qmp = phy_get_drvdata(phy);
> >> +    const struct qmp_phy_cfg *cfg = qmp->cfg;
> >> +    void __iomem *pcs = qmp->pcs;
> >>      int ret;
> >> +
> >>      dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> >>
> >> -    ret = qmp_ufs_com_init(qmp);
> >> +    ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> >> +    if (ret) {
> >> +            dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
> >> +            return ret;
> >> +    }
> >> +
> >> +    ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
> >> +    if (ret)
> >> +            goto err_disable_regulators;
> >> +
> >> +    qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> >> +    return 0;
> >> +
> >> +err_disable_regulators:
> >> +    regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
> >>      return ret;
> >>   }
> >>
> >> --
> >> 2.48.1
> >>
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH V3 5/9] phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()

[Nitin Rawat]

On 4/11/2025 4:26 PM, Dmitry Baryshkov wrote:
> On Fri, 11 Apr 2025 at 13:42, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>>
>>
>>
>> On 4/11/2025 1:39 AM, Dmitry Baryshkov wrote:
>>> On Thu, Apr 10, 2025 at 02:30:58PM +0530, Nitin Rawat wrote:
>>>> Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into
>>>> qmp_ufs_power_on(). This change removes unnecessary function calls and
>>>> ensures that the initialization logic is directly within the power-on
>>>> routine, maintaining the same functionality.
>>>
>>> Which problem is this patch trying to solve?
>>
>> Hi Dmitry,
>>
>> As part of the patch, I simplified the code by moving qmp_ufs_com_init
>> inline to qmp_ufs_power_on, since qmp_ufs_power_on was merely calling
>> qmp_ufs_com_init. This change eliminates unnecessary function call.
> 
> You again are describing what you did. Please start by stating the
> problem or the issue.
> 
>>
Hi Dmitry,

Sure, will update the commit with "problem" first in the next patchset 
when I post.

Thanks,
Nitin

>> Regards,
>> Nitin
>>
>>
>>
>>>
>>>>
>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>> ---
>>>>    drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 44 ++++++++++---------------
>>>>    1 file changed, 18 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> index 12dad28cc1bd..2cc819089d71 100644
>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>> @@ -1757,31 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
>>>>       qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
>>>>    }
>>>>
>>>> -static int qmp_ufs_com_init(struct qmp_ufs *qmp)
>>>> -{
>>>> -    const struct qmp_phy_cfg *cfg = qmp->cfg;
>>>> -    void __iomem *pcs = qmp->pcs;
>>>> -    int ret;
>>>> -
>>>> -    ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>>>> -    if (ret) {
>>>> -            dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
>>>> -            return ret;
>>>> -    }
>>>> -
>>>> -    ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
>>>> -    if (ret)
>>>> -            goto err_disable_regulators;
>>>> -
>>>> -    qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
>>>> -
>>>> -    return 0;
>>>> -
>>>> -err_disable_regulators:
>>>> -    regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>>>> -
>>>> -    return ret;
>>>> -}
>>>>
>>>>    static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>>>>    {
>>>> @@ -1799,10 +1774,27 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>>>>    static int qmp_ufs_power_on(struct phy *phy)
>>>>    {
>>>>       struct qmp_ufs *qmp = phy_get_drvdata(phy);
>>>> +    const struct qmp_phy_cfg *cfg = qmp->cfg;
>>>> +    void __iomem *pcs = qmp->pcs;
>>>>       int ret;
>>>> +
>>>>       dev_vdbg(qmp->dev, "Initializing QMP phy\n");
>>>>
>>>> -    ret = qmp_ufs_com_init(qmp);
>>>> +    ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>>>> +    if (ret) {
>>>> +            dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
>>>> +    if (ret)
>>>> +            goto err_disable_regulators;
>>>> +
>>>> +    qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
>>>> +    return 0;
>>>> +
>>>> +err_disable_regulators:
>>>> +    regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>>>>       return ret;
>>>>    }
>>>>
>>>> --
>>>> 2.48.1
>>>>
>>>
>>
> 
> 

Re: [PATCH V3 5/9] phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()

[Dmitry Baryshkov]

On Mon, Apr 14, 2025 at 12:58:48PM +0530, Nitin Rawat wrote:
> 
> 
> On 4/11/2025 4:26 PM, Dmitry Baryshkov wrote:
> > On Fri, 11 Apr 2025 at 13:42, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
> > > 
> > > 
> > > 
> > > On 4/11/2025 1:39 AM, Dmitry Baryshkov wrote:
> > > > On Thu, Apr 10, 2025 at 02:30:58PM +0530, Nitin Rawat wrote:
> > > > > Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into
> > > > > qmp_ufs_power_on(). This change removes unnecessary function calls and
> > > > > ensures that the initialization logic is directly within the power-on
> > > > > routine, maintaining the same functionality.
> > > > 
> > > > Which problem is this patch trying to solve?
> > > 
> > > Hi Dmitry,
> > > 
> > > As part of the patch, I simplified the code by moving qmp_ufs_com_init
> > > inline to qmp_ufs_power_on, since qmp_ufs_power_on was merely calling
> > > qmp_ufs_com_init. This change eliminates unnecessary function call.
> > 
> > You again are describing what you did. Please start by stating the
> > problem or the issue.
> > 
> > > 
> Hi Dmitry,
> 
> Sure, will update the commit with "problem" first in the next patchset when
> I post.

Before posting the next iteration, maybe you can respond inline? It well
might be that there is no problem to solve.

> 
> Thanks,
> Nitin
> 
> > > Regards,
> > > Nitin
> > > 
> > > 
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> > > > > ---
> > > > >    drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 44 ++++++++++---------------
> > > > >    1 file changed, 18 insertions(+), 26 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > > > > index 12dad28cc1bd..2cc819089d71 100644
> > > > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > > > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > > > > @@ -1757,31 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
> > > > >       qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
> > > > >    }
> > > > > 
> > > > > -static int qmp_ufs_com_init(struct qmp_ufs *qmp)
> > > > > -{
> > > > > -    const struct qmp_phy_cfg *cfg = qmp->cfg;
> > > > > -    void __iomem *pcs = qmp->pcs;
> > > > > -    int ret;
> > > > > -
> > > > > -    ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> > > > > -    if (ret) {
> > > > > -            dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
> > > > > -            return ret;
> > > > > -    }
> > > > > -
> > > > > -    ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
> > > > > -    if (ret)
> > > > > -            goto err_disable_regulators;
> > > > > -
> > > > > -    qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> > > > > -
> > > > > -    return 0;
> > > > > -
> > > > > -err_disable_regulators:
> > > > > -    regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
> > > > > -
> > > > > -    return ret;
> > > > > -}
> > > > > 
> > > > >    static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
> > > > >    {
> > > > > @@ -1799,10 +1774,27 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
> > > > >    static int qmp_ufs_power_on(struct phy *phy)
> > > > >    {
> > > > >       struct qmp_ufs *qmp = phy_get_drvdata(phy);
> > > > > +    const struct qmp_phy_cfg *cfg = qmp->cfg;
> > > > > +    void __iomem *pcs = qmp->pcs;
> > > > >       int ret;
> > > > > +
> > > > >       dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> > > > > 
> > > > > -    ret = qmp_ufs_com_init(qmp);
> > > > > +    ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> > > > > +    if (ret) {
> > > > > +            dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
> > > > > +            return ret;
> > > > > +    }
> > > > > +
> > > > > +    ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
> > > > > +    if (ret)
> > > > > +            goto err_disable_regulators;
> > > > > +
> > > > > +    qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> > > > > +    return 0;
> > > > > +
> > > > > +err_disable_regulators:
> > > > > +    regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
> > > > >       return ret;
> > > > >    }
> > > > > 
> > > > > --
> > > > > 2.48.1
> > > > > 
> > > > 
> > > 
> > 
> > 
> 
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
With best wishes
Dmitry

Re: [PATCH V3 5/9] phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()

[Nitin Rawat]

On 4/14/2025 1:13 PM, Dmitry Baryshkov wrote:
> On Mon, Apr 14, 2025 at 12:58:48PM +0530, Nitin Rawat wrote:
>>
>>
>> On 4/11/2025 4:26 PM, Dmitry Baryshkov wrote:
>>> On Fri, 11 Apr 2025 at 13:42, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 4/11/2025 1:39 AM, Dmitry Baryshkov wrote:
>>>>> On Thu, Apr 10, 2025 at 02:30:58PM +0530, Nitin Rawat wrote:
>>>>>> Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into
>>>>>> qmp_ufs_power_on(). This change removes unnecessary function calls and
>>>>>> ensures that the initialization logic is directly within the power-on
>>>>>> routine, maintaining the same functionality.
>>>>>
>>>>> Which problem is this patch trying to solve?
>>>>
>>>> Hi Dmitry,
>>>>
>>>> As part of the patch, I simplified the code by moving qmp_ufs_com_init
>>>> inline to qmp_ufs_power_on, since qmp_ufs_power_on was merely calling
>>>> qmp_ufs_com_init. This change eliminates unnecessary function call.
>>>
>>> You again are describing what you did. Please start by stating the
>>> problem or the issue.
>>>
>>>>
>> Hi Dmitry,
>>
>> Sure, will update the commit with "problem" first in the next patchset when
>> I post.
> 
> Before posting the next iteration, maybe you can respond inline? It well
> might be that there is no problem to solve.a

Hi Dmitry,

Apologies for late reply , I just realized I missed responding to your 
comment on this patch.


There is no functional "problem" here.
===================================================================
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.
==================================================================


I agree with you that there isn't a significant issue here. If you 
insist, I'm okay with skipping this patch. Let me know your thoughts.


Regards,
Nitin


> 
>>
>> Thanks,
>> Nitin
>>
>>>> Regards,
>>>> Nitin
>>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>>>> ---
>>>>>>     drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 44 ++++++++++---------------
>>>>>>     1 file changed, 18 insertions(+), 26 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>>>> index 12dad28cc1bd..2cc819089d71 100644
>>>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>>>>>> @@ -1757,31 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
>>>>>>        qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
>>>>>>     }
>>>>>>
>>>>>> -static int qmp_ufs_com_init(struct qmp_ufs *qmp)
>>>>>> -{
>>>>>> -    const struct qmp_phy_cfg *cfg = qmp->cfg;
>>>>>> -    void __iomem *pcs = qmp->pcs;
>>>>>> -    int ret;
>>>>>> -
>>>>>> -    ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>>>>>> -    if (ret) {
>>>>>> -            dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
>>>>>> -            return ret;
>>>>>> -    }
>>>>>> -
>>>>>> -    ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
>>>>>> -    if (ret)
>>>>>> -            goto err_disable_regulators;
>>>>>> -
>>>>>> -    qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
>>>>>> -
>>>>>> -    return 0;
>>>>>> -
>>>>>> -err_disable_regulators:
>>>>>> -    regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>>>>>> -
>>>>>> -    return ret;
>>>>>> -}
>>>>>>
>>>>>>     static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>>>>>>     {
>>>>>> @@ -1799,10 +1774,27 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
>>>>>>     static int qmp_ufs_power_on(struct phy *phy)
>>>>>>     {
>>>>>>        struct qmp_ufs *qmp = phy_get_drvdata(phy);
>>>>>> +    const struct qmp_phy_cfg *cfg = qmp->cfg;
>>>>>> +    void __iomem *pcs = qmp->pcs;
>>>>>>        int ret;
>>>>>> +
>>>>>>        dev_vdbg(qmp->dev, "Initializing QMP phy\n");
>>>>>>
>>>>>> -    ret = qmp_ufs_com_init(qmp);
>>>>>> +    ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>>>>>> +    if (ret) {
>>>>>> +            dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
>>>>>> +            return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
>>>>>> +    if (ret)
>>>>>> +            goto err_disable_regulators;
>>>>>> +
>>>>>> +    qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
>>>>>> +    return 0;
>>>>>> +
>>>>>> +err_disable_regulators:
>>>>>> +    regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>>>>>>        return ret;
>>>>>>     }
>>>>>>
>>>>>> --
>>>>>> 2.48.1
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>
>> -- 
>> linux-phy mailing list
>> linux-phy@lists.infradead.org
>> https://lists.infradead.org/mailman/listinfo/linux-phy
> 

Re: [PATCH V3 5/9] phy: qcom-qmp-ufs: Remove qmp_ufs_com_init()

[Dmitry Baryshkov]

On Sun, Apr 20, 2025 at 01:38:40AM +0530, Nitin Rawat wrote:
> 
> 
> On 4/14/2025 1:13 PM, Dmitry Baryshkov wrote:
> > On Mon, Apr 14, 2025 at 12:58:48PM +0530, Nitin Rawat wrote:
> > > 
> > > 
> > > On 4/11/2025 4:26 PM, Dmitry Baryshkov wrote:
> > > > On Fri, 11 Apr 2025 at 13:42, Nitin Rawat <quic_nitirawa@quicinc.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 4/11/2025 1:39 AM, Dmitry Baryshkov wrote:
> > > > > > On Thu, Apr 10, 2025 at 02:30:58PM +0530, Nitin Rawat wrote:
> > > > > > > Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into
> > > > > > > qmp_ufs_power_on(). This change removes unnecessary function calls and
> > > > > > > ensures that the initialization logic is directly within the power-on
> > > > > > > routine, maintaining the same functionality.
> > > > > > 
> > > > > > Which problem is this patch trying to solve?
> > > > > 
> > > > > Hi Dmitry,
> > > > > 
> > > > > As part of the patch, I simplified the code by moving qmp_ufs_com_init
> > > > > inline to qmp_ufs_power_on, since qmp_ufs_power_on was merely calling
> > > > > qmp_ufs_com_init. This change eliminates unnecessary function call.
> > > > 
> > > > You again are describing what you did. Please start by stating the
> > > > problem or the issue.
> > > > 
> > > > > 
> > > Hi Dmitry,
> > > 
> > > Sure, will update the commit with "problem" first in the next patchset when
> > > I post.
> > 
> > Before posting the next iteration, maybe you can respond inline? It well
> > might be that there is no problem to solve.a
> 
> Hi Dmitry,
> 
> Apologies for late reply , I just realized I missed responding to your
> comment on this patch.
> 
> 
> There is no functional "problem" here.
> ===================================================================
> 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.
> ==================================================================
> 
> 
> I agree with you that there isn't a significant issue here. If you insist,
> I'm okay with skipping this patch. Let me know your thoughts.


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



-- 
With best wishes
Dmitry

[PATCH V3 6/9] phy: qcom-qmp-ufs: Refactor qmp_ufs_exit callback.

[Nitin Rawat]

Rename qmp_ufs_disable to qmp_ufs_power_off and refactor
the code to move all the power off sequence to qmp_ufs_power_off.

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 | 42 ++-----------------------
 1 file changed, 3 insertions(+), 39 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 2cc819089d71..7776c248ebd5 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1757,20 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
 	qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
 }
 
-
-static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
-{
-	const struct qmp_phy_cfg *cfg = qmp->cfg;
-
-	reset_control_assert(qmp->ufs_reset);
-
-	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
-
-	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
-
-	return 0;
-}
-
 static int qmp_ufs_power_on(struct phy *phy)
 {
 	struct qmp_ufs *qmp = phy_get_drvdata(phy);
@@ -1840,39 +1826,17 @@ 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);
 
-	return 0;
-}
-
-static int qmp_ufs_exit(struct phy *phy)
-{
-	struct qmp_ufs *qmp = phy_get_drvdata(phy);
+	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
 
-	qmp_ufs_com_exit(qmp);
+	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
 
 	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);
@@ -1891,7 +1855,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,
+	.power_off	= qmp_ufs_power_off,
 	.calibrate	= qmp_ufs_phy_calibrate,
 	.set_mode	= qmp_ufs_set_mode,
 	.owner		= THIS_MODULE,
-- 
2.48.1

Re: [PATCH V3 6/9] phy: qcom-qmp-ufs: Refactor qmp_ufs_exit callback.

[Konrad Dybcio]

On 4/10/25 11:00 AM, Nitin Rawat wrote:
> Rename qmp_ufs_disable to qmp_ufs_power_off and refactor
> the code to move all the power off sequence to qmp_ufs_power_off.
> 
> 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>
> ---

So this patch does quite a lot without explaining the context
that isn't visible in just the diff below

- .power_on is altered to no longer reset the PHY (but it only did so
   on docs with !no_pcs_sw_reset?)
- partially inlines com_exit (dropping the reset assert)
- removes .disable in favor of .power_off that we can't tell
  what it does just by looking at this patch in the middle of the
  series

Please improve the commit message and consider splitting this change
in two

Konrad

[PATCH V3 7/9] scsi: ufs: qcom : Refactor phy_power_on/off calls

[Nitin Rawat]

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

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

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

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

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

[PATCH V3 8/9] scsi: ufs: qcom : Introduce phy_power_on/off wrapper function

[Nitin Rawat]

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

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

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

[PATCH V3 9/9] scsi: ufs: qcom: Prevent calling phy_exit before phy_init

[Nitin Rawat]

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

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

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

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

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

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

Re: [PATCH V3 0/9] Refactor phy powerup sequence

[Dmitry Baryshkov]

On Thu, Apr 10, 2025 at 02:30:53PM +0530, Nitin Rawat wrote:
> 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.

Please add an explicit note that patch1 is a requirement for the rest of
the PHY patches. It might make sense to merge it through the PHY tree
too (or to use an immutable branch).

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

-- 
With best wishes
Dmitry

Re: [PATCH V3 0/9] Refactor phy powerup sequence

[Nitin Rawat]

On 4/11/2025 1:35 AM, Dmitry Baryshkov wrote:
> On Thu, Apr 10, 2025 at 02:30:53PM +0530, Nitin Rawat wrote:
>> 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.
> 
> Please add an explicit note that patch1 is a requirement for the rest of
> the PHY patches. It might make sense to merge it through the PHY tree
> too (or to use an immutable branch).
> 

Hi Dmitry,

Thanks for the suggestion. Sure I would mention this in the cover letter 
when I post next patchset.

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