| Message ID | 1514978930-31341-6-git-send-email-mgautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Wed, Jan 3, 2018 at 4:58 PM, Manu Gautam <mgautam@codeaurora.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@codeaurora.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. With that bulk reset change you can add my review. Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vivek, On 1/12/2018 2:14 PM, Vivek Gautam wrote: > On Wed, Jan 3, 2018 at 4:58 PM, Manu Gautam <mgautam@codeaurora.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@codeaurora.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@codeaurora.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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
On Fri, Jan 12, 2018 at 2:16 PM, Manu Gautam <mgautam@codeaurora.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@codeaurora.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@codeaurora.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@codeaurora.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@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 >
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);
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@codeaurora.org> --- drivers/phy/qualcomm/phy-qcom-qmp.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)