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

[Manu Gautam]

Hi Vivek,


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

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

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