[PATCH 0/2] phy: qcom-qmp: pipe clock fixes

[PATCH 0/2] phy: qcom-qmp: pipe clock fixes

[Johan Hovold]

This series fixes a kernel doc typo and pipe clock imbalance on PHY
power on failures.

Johan


Johan Hovold (2):
  phy: qcom-qmp: fix phy-descriptor kernel-doc typo
  phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

 drivers/phy/qualcomm/phy-qcom-qmp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.35.1

[PATCH 0/2] phy: qcom-qmp: pipe clock fixes

[Johan Hovold]

This series fixes a kernel doc typo and pipe clock imbalance on PHY
power on failures.

Johan


Johan Hovold (2):
  phy: qcom-qmp: fix phy-descriptor kernel-doc typo
  phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

 drivers/phy/qualcomm/phy-qcom-qmp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.35.1


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

[PATCH 1/2] phy: qcom-qmp: fix phy-descriptor kernel-doc typo

[Johan Hovold]

Fix misspelled "clock" in the description of the pipe_clk field in the
PHY-descriptor kernel-doc comment.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 972f27c0e0a2..8c2300bfe489 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -3291,7 +3291,7 @@ struct qmp_phy_combo_cfg {
  * @tx2: iomapped memory space for second lane's tx (in dual lane PHYs)
  * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
  * @pcs_misc: iomapped memory space for lane's pcs_misc
- * @pipe_clk: pipe lock
+ * @pipe_clk: pipe clock
  * @index: lane index
  * @qmp: QMP phy to which this lane belongs
  * @lane_rst: lane's reset controller
-- 
2.35.1

[PATCH 1/2] phy: qcom-qmp: fix phy-descriptor kernel-doc typo

[Johan Hovold]

Fix misspelled "clock" in the description of the pipe_clk field in the
PHY-descriptor kernel-doc comment.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 972f27c0e0a2..8c2300bfe489 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -3291,7 +3291,7 @@ struct qmp_phy_combo_cfg {
  * @tx2: iomapped memory space for second lane's tx (in dual lane PHYs)
  * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
  * @pcs_misc: iomapped memory space for lane's pcs_misc
- * @pipe_clk: pipe lock
+ * @pipe_clk: pipe clock
  * @index: lane index
  * @qmp: QMP phy to which this lane belongs
  * @lane_rst: lane's reset controller
-- 
2.35.1


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

[PATCH 2/2] phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

[Johan Hovold]

Make sure to disable the pipe clock also if ufs-reset deassertion fails
during power on.

Note that the ufs-reset is asserted in qcom_qmp_phy_com_exit().

Fixes: c9b589791fc1 ("phy: qcom: Utilize UFS reset controller")
Cc: Evan Green <evgreen@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 8c2300bfe489..7d2d1ab061f7 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -5375,14 +5375,14 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 		if (ret) {
 			dev_err(qmp->dev, "lane%d reset deassert failed\n",
 				qphy->index);
-			goto err_lane_rst;
+			return ret;
 		}
 	}
 
 	ret = clk_prepare_enable(qphy->pipe_clk);
 	if (ret) {
 		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
-		goto err_clk_enable;
+		goto err_reset_lane;
 	}
 
 	/* Tx, Rx, and PCS configurations */
@@ -5433,7 +5433,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 
 	ret = reset_control_deassert(qmp->ufs_reset);
 	if (ret)
-		goto err_lane_rst;
+		goto err_disable_pipe_clk;
 
 	qcom_qmp_phy_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl,
 			       cfg->pcs_misc_tbl_num);
@@ -5472,17 +5472,17 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 					 PHY_INIT_COMPLETE_TIMEOUT);
 		if (ret) {
 			dev_err(qmp->dev, "phy initialization timed-out\n");
-			goto err_pcs_ready;
+			goto err_disable_pipe_clk;
 		}
 	}
 	return 0;
 
-err_pcs_ready:
+err_disable_pipe_clk:
 	clk_disable_unprepare(qphy->pipe_clk);
-err_clk_enable:
+err_reset_lane:
 	if (cfg->has_lane_rst)
 		reset_control_assert(qphy->lane_rst);
-err_lane_rst:
+
 	return ret;
 }
 
-- 
2.35.1

[PATCH 2/2] phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

[Johan Hovold]

Make sure to disable the pipe clock also if ufs-reset deassertion fails
during power on.

Note that the ufs-reset is asserted in qcom_qmp_phy_com_exit().

Fixes: c9b589791fc1 ("phy: qcom: Utilize UFS reset controller")
Cc: Evan Green <evgreen@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 8c2300bfe489..7d2d1ab061f7 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -5375,14 +5375,14 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 		if (ret) {
 			dev_err(qmp->dev, "lane%d reset deassert failed\n",
 				qphy->index);
-			goto err_lane_rst;
+			return ret;
 		}
 	}
 
 	ret = clk_prepare_enable(qphy->pipe_clk);
 	if (ret) {
 		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
-		goto err_clk_enable;
+		goto err_reset_lane;
 	}
 
 	/* Tx, Rx, and PCS configurations */
@@ -5433,7 +5433,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 
 	ret = reset_control_deassert(qmp->ufs_reset);
 	if (ret)
-		goto err_lane_rst;
+		goto err_disable_pipe_clk;
 
 	qcom_qmp_phy_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl,
 			       cfg->pcs_misc_tbl_num);
@@ -5472,17 +5472,17 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 					 PHY_INIT_COMPLETE_TIMEOUT);
 		if (ret) {
 			dev_err(qmp->dev, "phy initialization timed-out\n");
-			goto err_pcs_ready;
+			goto err_disable_pipe_clk;
 		}
 	}
 	return 0;
 
-err_pcs_ready:
+err_disable_pipe_clk:
 	clk_disable_unprepare(qphy->pipe_clk);
-err_clk_enable:
+err_reset_lane:
 	if (cfg->has_lane_rst)
 		reset_control_assert(qphy->lane_rst);
-err_lane_rst:
+
 	return ret;
 }
 
-- 
2.35.1


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

Re: [PATCH 2/2] phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

[Vinod Koul]

On 20-04-22, 17:23, Johan Hovold wrote:
> Make sure to disable the pipe clock also if ufs-reset deassertion fails
> during power on.
> 
> Note that the ufs-reset is asserted in qcom_qmp_phy_com_exit().
> 
> Fixes: c9b589791fc1 ("phy: qcom: Utilize UFS reset controller")
> Cc: Evan Green <evgreen@chromium.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 8c2300bfe489..7d2d1ab061f7 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -5375,14 +5375,14 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  		if (ret) {
>  			dev_err(qmp->dev, "lane%d reset deassert failed\n",
>  				qphy->index);
> -			goto err_lane_rst;
> +			return ret;

This can be skipped if we retain the err_lane_rst label

>  		}
>  	}
>  
>  	ret = clk_prepare_enable(qphy->pipe_clk);
>  	if (ret) {
>  		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> -		goto err_clk_enable;
> +		goto err_reset_lane;
>  	}
>  
>  	/* Tx, Rx, and PCS configurations */
> @@ -5433,7 +5433,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  
>  	ret = reset_control_deassert(qmp->ufs_reset);
>  	if (ret)
> -		goto err_lane_rst;
> +		goto err_disable_pipe_clk;

this is the actual fix...

>  
>  	qcom_qmp_phy_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl,
>  			       cfg->pcs_misc_tbl_num);
> @@ -5472,17 +5472,17 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  					 PHY_INIT_COMPLETE_TIMEOUT);
>  		if (ret) {
>  			dev_err(qmp->dev, "phy initialization timed-out\n");
> -			goto err_pcs_ready;
> +			goto err_disable_pipe_clk;

same rename here

>  		}
>  	}
>  	return 0;
>  
> -err_pcs_ready:
> +err_disable_pipe_clk:
>  	clk_disable_unprepare(qphy->pipe_clk);
> -err_clk_enable:
> +err_reset_lane:
>  	if (cfg->has_lane_rst)
>  		reset_control_assert(qphy->lane_rst);
> -err_lane_rst:
> +
>  	return ret;

while I feel the names given by this patch are better, they should not
be in a fix patch. We should just add the one line fix here and do
renames later

>  }
>  
> -- 
> 2.35.1

-- 
~Vinod

Re: [PATCH 2/2] phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

[Vinod Koul]

On 20-04-22, 17:23, Johan Hovold wrote:
> Make sure to disable the pipe clock also if ufs-reset deassertion fails
> during power on.
> 
> Note that the ufs-reset is asserted in qcom_qmp_phy_com_exit().
> 
> Fixes: c9b589791fc1 ("phy: qcom: Utilize UFS reset controller")
> Cc: Evan Green <evgreen@chromium.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 8c2300bfe489..7d2d1ab061f7 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -5375,14 +5375,14 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  		if (ret) {
>  			dev_err(qmp->dev, "lane%d reset deassert failed\n",
>  				qphy->index);
> -			goto err_lane_rst;
> +			return ret;

This can be skipped if we retain the err_lane_rst label

>  		}
>  	}
>  
>  	ret = clk_prepare_enable(qphy->pipe_clk);
>  	if (ret) {
>  		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> -		goto err_clk_enable;
> +		goto err_reset_lane;
>  	}
>  
>  	/* Tx, Rx, and PCS configurations */
> @@ -5433,7 +5433,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  
>  	ret = reset_control_deassert(qmp->ufs_reset);
>  	if (ret)
> -		goto err_lane_rst;
> +		goto err_disable_pipe_clk;

this is the actual fix...

>  
>  	qcom_qmp_phy_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl,
>  			       cfg->pcs_misc_tbl_num);
> @@ -5472,17 +5472,17 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  					 PHY_INIT_COMPLETE_TIMEOUT);
>  		if (ret) {
>  			dev_err(qmp->dev, "phy initialization timed-out\n");
> -			goto err_pcs_ready;
> +			goto err_disable_pipe_clk;

same rename here

>  		}
>  	}
>  	return 0;
>  
> -err_pcs_ready:
> +err_disable_pipe_clk:
>  	clk_disable_unprepare(qphy->pipe_clk);
> -err_clk_enable:
> +err_reset_lane:
>  	if (cfg->has_lane_rst)
>  		reset_control_assert(qphy->lane_rst);
> -err_lane_rst:
> +
>  	return ret;

while I feel the names given by this patch are better, they should not
be in a fix patch. We should just add the one line fix here and do
renames later

>  }
>  
> -- 
> 2.35.1

-- 
~Vinod

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

Re: [PATCH 1/2] phy: qcom-qmp: fix phy-descriptor kernel-doc typo

[Vinod Koul]

On 20-04-22, 17:23, Johan Hovold wrote:
> Fix misspelled "clock" in the description of the pipe_clk field in the
> PHY-descriptor kernel-doc comment.

Applied, thanks

-- 
~Vinod

Re: [PATCH 1/2] phy: qcom-qmp: fix phy-descriptor kernel-doc typo

[Vinod Koul]

On 20-04-22, 17:23, Johan Hovold wrote:
> Fix misspelled "clock" in the description of the pipe_clk field in the
> PHY-descriptor kernel-doc comment.

Applied, thanks

-- 
~Vinod

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

Re: [PATCH 2/2] phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

[Johan Hovold]

On Mon, May 02, 2022 at 03:39:41PM +0530, Vinod Koul wrote:
> On 20-04-22, 17:23, Johan Hovold wrote:
> > Make sure to disable the pipe clock also if ufs-reset deassertion fails
> > during power on.
> > 
> > Note that the ufs-reset is asserted in qcom_qmp_phy_com_exit().
> > 
> > Fixes: c9b589791fc1 ("phy: qcom: Utilize UFS reset controller")
> > Cc: Evan Green <evgreen@chromium.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 8c2300bfe489..7d2d1ab061f7 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -5375,14 +5375,14 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> >  		if (ret) {
> >  			dev_err(qmp->dev, "lane%d reset deassert failed\n",
> >  				qphy->index);
> > -			goto err_lane_rst;
> > +			return ret;
> 
> This can be skipped if we retain the err_lane_rst label
> 
> >  		}
> >  	}
> >  
> >  	ret = clk_prepare_enable(qphy->pipe_clk);
> >  	if (ret) {
> >  		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> > -		goto err_clk_enable;
> > +		goto err_reset_lane;
> >  	}
> >  
> >  	/* Tx, Rx, and PCS configurations */
> > @@ -5433,7 +5433,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> >  
> >  	ret = reset_control_deassert(qmp->ufs_reset);
> >  	if (ret)
> > -		goto err_lane_rst;
> > +		goto err_disable_pipe_clk;
> 
> this is the actual fix...

Right, but with a one-line fix this would read

	goto err_pcs_ready;

which makes no sense at all. Is that really what you prefer?

Renaming just this one and the below one, would leave the error labels
named using two different schemes, which also isn't very nice, but
perhaps ok.

> >  
> >  	qcom_qmp_phy_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl,
> >  			       cfg->pcs_misc_tbl_num);
> > @@ -5472,17 +5472,17 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> >  					 PHY_INIT_COMPLETE_TIMEOUT);
> >  		if (ret) {
> >  			dev_err(qmp->dev, "phy initialization timed-out\n");
> > -			goto err_pcs_ready;
> > +			goto err_disable_pipe_clk;
> 
> same rename here
> 
> >  		}
> >  	}
> >  	return 0;
> >  
> > -err_pcs_ready:
> > +err_disable_pipe_clk:
> >  	clk_disable_unprepare(qphy->pipe_clk);
> > -err_clk_enable:
> > +err_reset_lane:
> >  	if (cfg->has_lane_rst)
> >  		reset_control_assert(qphy->lane_rst);
> > -err_lane_rst:
> > +
> >  	return ret;
> 
> while I feel the names given by this patch are better, they should not
> be in a fix patch. We should just add the one line fix here and do
> renames later

I'll respin if you prefer, just want to double check that you really
want a one line fix (i.e. goto err_pcs_ready).

Johan

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

Re: [PATCH 2/2] phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

[Johan Hovold]

On Mon, May 02, 2022 at 03:39:41PM +0530, Vinod Koul wrote:
> On 20-04-22, 17:23, Johan Hovold wrote:
> > Make sure to disable the pipe clock also if ufs-reset deassertion fails
> > during power on.
> > 
> > Note that the ufs-reset is asserted in qcom_qmp_phy_com_exit().
> > 
> > Fixes: c9b589791fc1 ("phy: qcom: Utilize UFS reset controller")
> > Cc: Evan Green <evgreen@chromium.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 8c2300bfe489..7d2d1ab061f7 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -5375,14 +5375,14 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> >  		if (ret) {
> >  			dev_err(qmp->dev, "lane%d reset deassert failed\n",
> >  				qphy->index);
> > -			goto err_lane_rst;
> > +			return ret;
> 
> This can be skipped if we retain the err_lane_rst label
> 
> >  		}
> >  	}
> >  
> >  	ret = clk_prepare_enable(qphy->pipe_clk);
> >  	if (ret) {
> >  		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> > -		goto err_clk_enable;
> > +		goto err_reset_lane;
> >  	}
> >  
> >  	/* Tx, Rx, and PCS configurations */
> > @@ -5433,7 +5433,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> >  
> >  	ret = reset_control_deassert(qmp->ufs_reset);
> >  	if (ret)
> > -		goto err_lane_rst;
> > +		goto err_disable_pipe_clk;
> 
> this is the actual fix...

Right, but with a one-line fix this would read

	goto err_pcs_ready;

which makes no sense at all. Is that really what you prefer?

Renaming just this one and the below one, would leave the error labels
named using two different schemes, which also isn't very nice, but
perhaps ok.

> >  
> >  	qcom_qmp_phy_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl,
> >  			       cfg->pcs_misc_tbl_num);
> > @@ -5472,17 +5472,17 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> >  					 PHY_INIT_COMPLETE_TIMEOUT);
> >  		if (ret) {
> >  			dev_err(qmp->dev, "phy initialization timed-out\n");
> > -			goto err_pcs_ready;
> > +			goto err_disable_pipe_clk;
> 
> same rename here
> 
> >  		}
> >  	}
> >  	return 0;
> >  
> > -err_pcs_ready:
> > +err_disable_pipe_clk:
> >  	clk_disable_unprepare(qphy->pipe_clk);
> > -err_clk_enable:
> > +err_reset_lane:
> >  	if (cfg->has_lane_rst)
> >  		reset_control_assert(qphy->lane_rst);
> > -err_lane_rst:
> > +
> >  	return ret;
> 
> while I feel the names given by this patch are better, they should not
> be in a fix patch. We should just add the one line fix here and do
> renames later

I'll respin if you prefer, just want to double check that you really
want a one line fix (i.e. goto err_pcs_ready).

Johan

Re: [PATCH 2/2] phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

[Vinod Koul]

On 02-05-22, 12:27, Johan Hovold wrote:
> On Mon, May 02, 2022 at 03:39:41PM +0530, Vinod Koul wrote:
> > On 20-04-22, 17:23, Johan Hovold wrote:
> > > Make sure to disable the pipe clock also if ufs-reset deassertion fails
> > > during power on.
> > > 
> > > Note that the ufs-reset is asserted in qcom_qmp_phy_com_exit().
> > > 
> > > Fixes: c9b589791fc1 ("phy: qcom: Utilize UFS reset controller")
> > > Cc: Evan Green <evgreen@chromium.org>
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > >  drivers/phy/qualcomm/phy-qcom-qmp.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > index 8c2300bfe489..7d2d1ab061f7 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > @@ -5375,14 +5375,14 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> > >  		if (ret) {
> > >  			dev_err(qmp->dev, "lane%d reset deassert failed\n",
> > >  				qphy->index);
> > > -			goto err_lane_rst;
> > > +			return ret;
> > 
> > This can be skipped if we retain the err_lane_rst label
> > 
> > >  		}
> > >  	}
> > >  
> > >  	ret = clk_prepare_enable(qphy->pipe_clk);
> > >  	if (ret) {
> > >  		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> > > -		goto err_clk_enable;
> > > +		goto err_reset_lane;
> > >  	}
> > >  
> > >  	/* Tx, Rx, and PCS configurations */
> > > @@ -5433,7 +5433,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> > >  
> > >  	ret = reset_control_deassert(qmp->ufs_reset);
> > >  	if (ret)
> > > -		goto err_lane_rst;
> > > +		goto err_disable_pipe_clk;
> > 
> > this is the actual fix...
> 
> Right, but with a one-line fix this would read
> 
> 	goto err_pcs_ready;
> 
> which makes no sense at all. Is that really what you prefer?

Yes I feel for a fix we should always have the minimal changes.. makes
porting easier

> 
> Renaming just this one and the below one, would leave the error labels
> named using two different schemes, which also isn't very nice, but
> perhaps ok.
> 
> > >  
> > >  	qcom_qmp_phy_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl,
> > >  			       cfg->pcs_misc_tbl_num);
> > > @@ -5472,17 +5472,17 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> > >  					 PHY_INIT_COMPLETE_TIMEOUT);
> > >  		if (ret) {
> > >  			dev_err(qmp->dev, "phy initialization timed-out\n");
> > > -			goto err_pcs_ready;
> > > +			goto err_disable_pipe_clk;
> > 
> > same rename here
> > 
> > >  		}
> > >  	}
> > >  	return 0;
> > >  
> > > -err_pcs_ready:
> > > +err_disable_pipe_clk:
> > >  	clk_disable_unprepare(qphy->pipe_clk);
> > > -err_clk_enable:
> > > +err_reset_lane:
> > >  	if (cfg->has_lane_rst)
> > >  		reset_control_assert(qphy->lane_rst);
> > > -err_lane_rst:
> > > +
> > >  	return ret;
> > 
> > while I feel the names given by this patch are better, they should not
> > be in a fix patch. We should just add the one line fix here and do
> > renames later
> 
> I'll respin if you prefer, just want to double check that you really
> want a one line fix (i.e. goto err_pcs_ready).

Yes please


-- 
~Vinod

Re: [PATCH 2/2] phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

[Vinod Koul]

On 02-05-22, 12:27, Johan Hovold wrote:
> On Mon, May 02, 2022 at 03:39:41PM +0530, Vinod Koul wrote:
> > On 20-04-22, 17:23, Johan Hovold wrote:
> > > Make sure to disable the pipe clock also if ufs-reset deassertion fails
> > > during power on.
> > > 
> > > Note that the ufs-reset is asserted in qcom_qmp_phy_com_exit().
> > > 
> > > Fixes: c9b589791fc1 ("phy: qcom: Utilize UFS reset controller")
> > > Cc: Evan Green <evgreen@chromium.org>
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > >  drivers/phy/qualcomm/phy-qcom-qmp.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > index 8c2300bfe489..7d2d1ab061f7 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > @@ -5375,14 +5375,14 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> > >  		if (ret) {
> > >  			dev_err(qmp->dev, "lane%d reset deassert failed\n",
> > >  				qphy->index);
> > > -			goto err_lane_rst;
> > > +			return ret;
> > 
> > This can be skipped if we retain the err_lane_rst label
> > 
> > >  		}
> > >  	}
> > >  
> > >  	ret = clk_prepare_enable(qphy->pipe_clk);
> > >  	if (ret) {
> > >  		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> > > -		goto err_clk_enable;
> > > +		goto err_reset_lane;
> > >  	}
> > >  
> > >  	/* Tx, Rx, and PCS configurations */
> > > @@ -5433,7 +5433,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> > >  
> > >  	ret = reset_control_deassert(qmp->ufs_reset);
> > >  	if (ret)
> > > -		goto err_lane_rst;
> > > +		goto err_disable_pipe_clk;
> > 
> > this is the actual fix...
> 
> Right, but with a one-line fix this would read
> 
> 	goto err_pcs_ready;
> 
> which makes no sense at all. Is that really what you prefer?

Yes I feel for a fix we should always have the minimal changes.. makes
porting easier

> 
> Renaming just this one and the below one, would leave the error labels
> named using two different schemes, which also isn't very nice, but
> perhaps ok.
> 
> > >  
> > >  	qcom_qmp_phy_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl,
> > >  			       cfg->pcs_misc_tbl_num);
> > > @@ -5472,17 +5472,17 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> > >  					 PHY_INIT_COMPLETE_TIMEOUT);
> > >  		if (ret) {
> > >  			dev_err(qmp->dev, "phy initialization timed-out\n");
> > > -			goto err_pcs_ready;
> > > +			goto err_disable_pipe_clk;
> > 
> > same rename here
> > 
> > >  		}
> > >  	}
> > >  	return 0;
> > >  
> > > -err_pcs_ready:
> > > +err_disable_pipe_clk:
> > >  	clk_disable_unprepare(qphy->pipe_clk);
> > > -err_clk_enable:
> > > +err_reset_lane:
> > >  	if (cfg->has_lane_rst)
> > >  		reset_control_assert(qphy->lane_rst);
> > > -err_lane_rst:
> > > +
> > >  	return ret;
> > 
> > while I feel the names given by this patch are better, they should not
> > be in a fix patch. We should just add the one line fix here and do
> > renames later
> 
> I'll respin if you prefer, just want to double check that you really
> want a one line fix (i.e. goto err_pcs_ready).

Yes please


-- 
~Vinod

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