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

From: Manu Gautam
Date: Fri Jan 12 2018 - 03:46:34 EST


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