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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 12 Jan 2018 14:14:00 +0530
From:   Vivek Gautam <vivek.gautam@...eaurora.org>
To:     Manu Gautam <mgautam@...eaurora.org>
Cc:     Kishon Vijay Abraham I <kishon@...com>,
        Felipe Balbi <balbi@...nel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Linux USB Mailing List <linux-usb@...r.kernel.org>,
        Varadarajan Narayanan <varada@...eaurora.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Wei Yongjun <weiyongjun1@...wei.com>,
        Fengguang Wu <fengguang.wu@...el.com>,
        "open list:GENERIC PHY FRAMEWORK" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 05/16] phy: qcom-qmp: Fix PHY block reset sequence

On Wed, Jan 3, 2018 at 4:58 PM, Manu Gautam <mgautam@...eaurora.org> 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@...eaurora.org>
> ---
>  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@...eaurora.org>

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@...r.kernel.org
> 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