Re: [PATCH v4 05/16] phy: qcom-qmp: Fix PHY block reset sequence
On Fri, Jan 12, 2018 at 2:16 PM, Manu Gautam wrote:
> Hi Vivek,
>
>
> On 1/12/2018 2:14 PM, Vivek Gautam wrote:
>> On Wed, Jan 3, 2018 at 4:58 PM, Manu Gautam 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
>>> ---
>>> 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
>>
>> 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 majord...@vger.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
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 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
>> ---
>> 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
>
> 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 majord...@vger.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
Re: [PATCH v4 05/16] phy: qcom-qmp: Fix PHY block reset sequence
On Wed, Jan 3, 2018 at 4:58 PM, Manu Gautam 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
> ---
> 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
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 majord...@vger.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
[PATCH v4 05/16] phy: qcom-qmp: Fix PHY block reset sequence
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
---
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--) {
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
