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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <77b12f94-e496-ff44-fe61-75b21a1a4e76@codeaurora.org>
Date:   Fri, 12 Jan 2018 14:16:19 +0530
From:   Manu Gautam <mgautam@...eaurora.org>
To:     Vivek Gautam <vivek.gautam@...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

Hi Vivek,


On 1/12/2018 2:14 PM, Vivek Gautam wrote:
> 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.

We need to follow an order for assert and opposite order for
de-assert, hence cant use 'bulk reset' APIs.

>
> 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
>
>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project