| Message ID | 1511256206-1587-4-git-send-email-mgautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 11/21/2017 01:23 AM, Manu Gautam wrote: > PHY must be powered on before turning ON clocks and > attempting to initialize it. Driver is exposing > separate init and power_on routines for this. > Apparently USB dwc3 core driver performs power-on after > init. Also, poweron and init for QMP PHY need to be Why does dwc3 driver power on after init? Seems backwards. > executed together always, hence remove poweron callback > from phy_ops and explicitly perform this from com_init, Why do they need to be executed together? > similar changes needed for poweroff. On similar lines move > clk_enable from init to com_init which can be called once > for multi lane PHYs. Please add parenthesis, clk_enable() for example, to functions so we know they're functions. > > Signed-off-by: Manu Gautam <mgautam@codeaurora.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp.c | 61 +++++++++++++------------------------ > 1 file changed, 21 insertions(+), 40 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c > index 90794dd..2f427e3 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > @@ -720,33 +720,6 @@ static void qcom_qmp_phy_configure(void __iomem *base, > } > } > > -static int qcom_qmp_phy_poweron(struct phy *phy) > -{ > - struct qmp_phy *qphy = phy_get_drvdata(phy); > - struct qcom_qmp *qmp = qphy->qmp; > - int num = qmp->cfg->num_vregs; > - int ret; > - > - dev_vdbg(&phy->dev, "Powering on QMP phy\n"); > - > - /* turn on regulator supplies */ > - ret = regulator_bulk_enable(num, qmp->vregs); > - if (ret) > - dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret); > - > - return ret; > -} > - > -static int qcom_qmp_phy_poweroff(struct phy *phy) > -{ > - struct qmp_phy *qphy = phy_get_drvdata(phy); > - struct qcom_qmp *qmp = qphy->qmp; > - > - regulator_bulk_disable(qmp->cfg->num_vregs, qmp->vregs); > - > - return 0; > -} > - > static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) > { > const struct qmp_phy_cfg *cfg = qmp->cfg; > @@ -759,6 +732,19 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) > return 0; > } > > + /* turn on regulator supplies */ > + ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); > + if (ret) { > + mutex_unlock(&qmp->phy_mutex); > + return ret; This could also be a goto. > + } > + > + 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_deassert(qmp->resets[i]); > if (ret) {
Hi, On 11/22/2017 11:33 PM, Stephen Boyd wrote: > On 11/21/2017 01:23 AM, Manu Gautam wrote: >> PHY must be powered on before turning ON clocks and >> attempting to initialize it. Driver is exposing >> separate init and power_on routines for this. >> Apparently USB dwc3 core driver performs power-on after >> init. Also, poweron and init for QMP PHY need to be > Why does dwc3 driver power on after init? Seems backwards. There are not many PHY drivers implementing power_on, rather they rely on init to take care of complete initialization. However though the name indicates power_on, but PHY drivers are not using it to turn on power supplies but rather PHY register operations to enable/ start PHY - somewhat like init only. > >> executed together always, hence remove poweron callback >> from phy_ops and explicitly perform this from com_init, > Why do they need to be executed together? Hardware programming guide requires PHY supplies to be ON before it is initialized. And if PHY supplies were turned-off, then it must be reset after turning them ON. > >> similar changes needed for poweroff. On similar lines move >> clk_enable from init to com_init which can be called once >> for multi lane PHYs. > Please add parenthesis, clk_enable() for example, to functions so we > know they're functions. Ok. > >> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp.c | 61 +++++++++++++------------------------ >> 1 file changed, 21 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c >> index 90794dd..2f427e3 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c >> @@ -720,33 +720,6 @@ static void qcom_qmp_phy_configure(void __iomem *base, >> } >> } >> >> -static int qcom_qmp_phy_poweron(struct phy *phy) >> -{ >> - struct qmp_phy *qphy = phy_get_drvdata(phy); >> - struct qcom_qmp *qmp = qphy->qmp; >> - int num = qmp->cfg->num_vregs; >> - int ret; >> - >> - dev_vdbg(&phy->dev, "Powering on QMP phy\n"); >> - >> - /* turn on regulator supplies */ >> - ret = regulator_bulk_enable(num, qmp->vregs); >> - if (ret) >> - dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret); >> - >> - return ret; >> -} >> - >> -static int qcom_qmp_phy_poweroff(struct phy *phy) >> -{ >> - struct qmp_phy *qphy = phy_get_drvdata(phy); >> - struct qcom_qmp *qmp = qphy->qmp; >> - >> - regulator_bulk_disable(qmp->cfg->num_vregs, qmp->vregs); >> - >> - return 0; >> -} >> - >> static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) >> { >> const struct qmp_phy_cfg *cfg = qmp->cfg; >> @@ -759,6 +732,19 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) >> return 0; >> } >> >> + /* turn on regulator supplies */ >> + ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); >> + if (ret) { >> + mutex_unlock(&qmp->phy_mutex); >> + return ret; > This could also be a goto. Yes, I can replace with goto here. > >> + } >> + >> + 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_deassert(qmp->resets[i]); >> if (ret) {
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c index 90794dd..2f427e3 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c @@ -720,33 +720,6 @@ static void qcom_qmp_phy_configure(void __iomem *base, } } -static int qcom_qmp_phy_poweron(struct phy *phy) -{ - struct qmp_phy *qphy = phy_get_drvdata(phy); - struct qcom_qmp *qmp = qphy->qmp; - int num = qmp->cfg->num_vregs; - int ret; - - dev_vdbg(&phy->dev, "Powering on QMP phy\n"); - - /* turn on regulator supplies */ - ret = regulator_bulk_enable(num, qmp->vregs); - if (ret) - dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret); - - return ret; -} - -static int qcom_qmp_phy_poweroff(struct phy *phy) -{ - struct qmp_phy *qphy = phy_get_drvdata(phy); - struct qcom_qmp *qmp = qphy->qmp; - - regulator_bulk_disable(qmp->cfg->num_vregs, qmp->vregs); - - return 0; -} - static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) { const struct qmp_phy_cfg *cfg = qmp->cfg; @@ -759,6 +732,19 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) return 0; } + /* turn on regulator supplies */ + ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); + if (ret) { + mutex_unlock(&qmp->phy_mutex); + return ret; + } + + 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_deassert(qmp->resets[i]); if (ret) { @@ -803,6 +789,9 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) err_rst: while (--i >= 0) reset_control_assert(qmp->resets[i]); + clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks); +err_clk_enable: + regulator_bulk_disable(cfg->num_vregs, qmp->vregs); mutex_unlock(&qmp->phy_mutex); return ret; @@ -832,6 +821,10 @@ static int qcom_qmp_phy_com_exit(struct qcom_qmp *qmp) while (--i >= 0) reset_control_assert(qmp->resets[i]); + clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks); + + regulator_bulk_disable(cfg->num_vregs, qmp->vregs); + mutex_unlock(&qmp->phy_mutex); return 0; @@ -852,15 +845,9 @@ static int qcom_qmp_phy_init(struct phy *phy) dev_vdbg(qmp->dev, "Initializing QMP phy\n"); - ret = clk_bulk_prepare_enable(cfg->num_clks, qmp->clks); - if (ret) { - dev_err(qmp->dev, "failed to enable clks, err=%d\n", ret); - return ret; - } - ret = qcom_qmp_phy_com_init(qmp); if (ret) - goto err_com_init; + return ret; if (cfg->has_lane_rst) { ret = reset_control_deassert(qphy->lane_rst); @@ -915,8 +902,6 @@ static int qcom_qmp_phy_init(struct phy *phy) reset_control_assert(qphy->lane_rst); err_lane_rst: qcom_qmp_phy_com_exit(qmp); -err_com_init: - clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks); return ret; } @@ -943,8 +928,6 @@ static int qcom_qmp_phy_exit(struct phy *phy) qcom_qmp_phy_com_exit(qmp); - clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks); - return 0; } @@ -1057,8 +1040,6 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np) static const struct phy_ops qcom_qmp_phy_gen_ops = { .init = qcom_qmp_phy_init, .exit = qcom_qmp_phy_exit, - .power_on = qcom_qmp_phy_poweron, - .power_off = qcom_qmp_phy_poweroff, .owner = THIS_MODULE, };
PHY must be powered on before turning ON clocks and attempting to initialize it. Driver is exposing separate init and power_on routines for this. Apparently USB dwc3 core driver performs power-on after init. Also, poweron and init for QMP PHY need to be executed together always, hence remove poweron callback from phy_ops and explicitly perform this from com_init, similar changes needed for poweroff. On similar lines move clk_enable from init to com_init which can be called once for multi lane PHYs. Signed-off-by: Manu Gautam <mgautam@codeaurora.org> --- drivers/phy/qualcomm/phy-qcom-qmp.c | 61 +++++++++++++------------------------ 1 file changed, 21 insertions(+), 40 deletions(-)