Re: [PATCH v4 05/16] phy: qcom-qmp: Fix PHY block reset sequence

[Vivek Gautam]

On Wed, Jan 3, 2018 at 4:58 PM, Manu Gautam <mgautam@xxxxxxxxxxxxxx> wrote:
> PHY block or asynchronous reset requires signal
> to be asserted before de-asserting. Driver is only
> de-asserting signal which is already low, hence
> reset operation is a no-op. Fix this by asserting
> signal first. Also, resetting requires PHY clocks
> to be turned ON only after reset is finished. Fix
> that as well.
>
> Signed-off-by: Manu Gautam <mgautam@xxxxxxxxxxxxxx>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 1b82cea..ecff261 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -752,13 +752,16 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
>                 goto err_reg_enable;
>         }
>
> -       ret = clk_bulk_prepare_enable(cfg->num_clks, qmp->clks);
> -       if (ret) {
> -               dev_err(qmp->dev, "failed to enable clks, err=%d\n", ret);
> -               goto err_clk_enable;
> +       for (i = 0; i < cfg->num_resets; i++) {
> +               ret = reset_control_assert(qmp->resets[i]);
> +               if (ret) {
> +                       dev_err(qmp->dev, "%s reset assert failed\n",
> +                               cfg->reset_list[i]);
> +                       goto err_rst_assert;
> +               }
>         }
>
> -       for (i = 0; i < cfg->num_resets; i++) {
> +       for (i = cfg->num_resets - 1; i >= 0; i--) {

Do we a dependency on the order in which these resets are
applied?
If not then we can use the 'bulk reset' APIs as well.

With that bulk reset change you can add my review.

Reviewed-by: Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx>

Thanks
Vivek

>                 ret = reset_control_deassert(qmp->resets[i]);
>                 if (ret) {
>                         dev_err(qmp->dev, "%s reset deassert failed\n",
> @@ -767,6 +770,12 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
>                 }
>         }
>
> +       ret = clk_bulk_prepare_enable(cfg->num_clks, qmp->clks);
> +       if (ret) {
> +               dev_err(qmp->dev, "failed to enable clks, err=%d\n", ret);
> +               goto err_rst;
> +       }
> +
>         if (cfg->has_phy_com_ctrl)
>                 qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
>                              SW_PWRDN);
> @@ -791,7 +800,7 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
>                 if (ret) {
>                         dev_err(qmp->dev,
>                                 "phy common block init timed-out\n");
> -                       goto err_rst;
> +                       goto err_com_init;
>                 }
>         }
>
> @@ -799,11 +808,12 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
>
>         return 0;
>
> +err_com_init:
> +       clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
>  err_rst:
> -       while (--i >= 0)
> +       while (++i < cfg->num_resets)
>                 reset_control_assert(qmp->resets[i]);
> -       clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
> -err_clk_enable:
> +err_rst_assert:
>         regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>  err_reg_enable:
>         mutex_unlock(&qmp->phy_mutex);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation