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] [day] [month] [year] [list]
Date:   Fri, 12 Jan 2018 14:42:01 +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 Fri, Jan 12, 2018 at 2:16 PM, Manu Gautam <mgautam@...eaurora.org> wrote:
> 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.

Okay, you can add my review then.
Thanks.
>
>>
>> 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
>



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