| Message ID | 20220420152331.5527-3-johan+linaro@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: qcom-qmp: pipe clock fixes | expand |
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
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
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
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; }
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(-)