| Message ID | 1479981638-32069-4-git-send-email-akdwived@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote: > Regulator enable routine will get additional input parameter of > regulator info and count, It will read regulator info and will do > appropriate voltage and load configuration before turning them up. > Also separate out disable interface into proxy and active disable > so that on arrival of handover interrupt proxy regulators alone > could be voted out. > > Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> > --- > drivers/remoteproc/qcom_q6v5_pil.c | 113 ++++++++++++++++++++++++++++--------- > 1 file changed, 86 insertions(+), 27 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c > index b0f0fcf..06d5bb2 100644 > --- a/drivers/remoteproc/qcom_q6v5_pil.c > +++ b/drivers/remoteproc/qcom_q6v5_pil.c > @@ -126,7 +126,6 @@ struct q6v5 { > struct qcom_smem_state *state; > unsigned stop_bit; > > - struct regulator_bulk_data supply[4]; > > struct clk *ahb_clk; > struct clk *axi_clk; > @@ -160,13 +159,6 @@ enum { > Q5V56_1_5_0, /*hexagon on msm8996*/ > }; > > -enum { > - Q6V5_SUPPLY_CX, > - Q6V5_SUPPLY_MX, > - Q6V5_SUPPLY_MSS, > - Q6V5_SUPPLY_PLL, > -}; > - > static int q6v5_regulator_init(struct device *dev, > struct reg_info *regs, char **reg_str, int volatage_load[][2]) > { > @@ -206,35 +198,93 @@ static int q6v5_regulator_init(struct device *dev, > return reg_count; > } > > -static int q6v5_regulator_enable(struct q6v5 *qproc) > +static int q6v5_regulator_enable(struct q6v5 *qproc, > + struct reg_info *regs, int count) > { > - struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer; > - struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer; > - int ret; > + int i, rc = 0; One variable per line, no need to replace "ret" with "rc" and no need to initialize it to 0 as it won't be read before written in any code path. > + > + for (i = 0; i < count; i++) { > + if (regs[i].uV > 0) { > + rc = regulator_set_voltage(regs[i].reg, > + regs[i].uV, INT_MAX); > + if (rc) { > + dev_err(qproc->dev, > + "Failed to request voltage for %d.\n", > + i); > + goto err; > + } > + } > > - /* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */ > + if (regs[i].uA > 0) { > + rc = regulator_set_load(regs[i].reg, > + regs[i].uA); > + if (rc < 0) { > + dev_err(qproc->dev, "Failed to set regulator mode\n"); > + goto err; > + } > + } > > - ret = regulator_set_voltage(mx, 1050000, INT_MAX); > - if (ret) > - return ret; > + rc = regulator_enable(regs[i].reg); > + if (rc) { > + dev_err(qproc->dev, "Regulator enable failed\n"); > + goto err; > + } > + } > + > + return 0; > +err: > + for (; i >= 0; i--) { > + if (regs[i].uV > 0) > + regulator_set_voltage(regs[i].reg, 0, INT_MAX); > > - regulator_set_voltage(mss, 1000000, 1150000); > + if (regs[i].uA > 0) > + regulator_set_load(regs[i].reg, 0); > + > + regulator_disable(regs[i].reg); > + } > > - return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply); > + return rc; > } > > -static void q6v5_regulator_disable(struct q6v5 *qproc) > +static void q6v5_proxy_regulator_disable(struct q6v5 *qproc) Please have follow the same scheme as for enable, with a function taking the list of regulators and count. > { > - struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer; > - struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer; > + int i; > + struct reg_info *regs = qproc->proxy_regs; > > - /* TODO: Q6V5_SUPPLY_CX corner votes should be released */ > + for (i = 0; i < qproc->proxy_reg_count; i++) { > + if (regs[i].uV > 0) > + regulator_set_voltage(regs[i].reg, 0, INT_MAX); > > - regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply); > - regulator_set_voltage(mx, 0, INT_MAX); > - regulator_set_voltage(mss, 0, 1150000); > + if (regs[i].uA > 0) > + regulator_set_load(regs[i].reg, 0); > + > + regulator_disable(regs[i].reg); > + } > } > > +static void q6v5_active_regulator_disable(struct q6v5 *qproc) > +{ > + int i; > + struct reg_info *regs = qproc->active_regs; > + > + for (i = 0; i < qproc->active_reg_count; i++) { > + if (regs[i].uV > 0) > + regulator_set_voltage(regs[i].reg, 0, INT_MAX); > + > + if (regs[i].uA > 0) > + regulator_set_load(regs[i].reg, 0); > + > + regulator_disable(regs[i].reg); > + } > +} > + > +static void q6v5_regulator_disable(struct q6v5 *qproc) > +{ > + q6v5_proxy_regulator_disable(qproc); > + q6v5_active_regulator_disable(qproc); > +} > + > + > static int q6v5_load(struct rproc *rproc, const struct firmware *fw) > { > struct q6v5 *qproc = rproc->priv; > @@ -524,12 +574,19 @@ static int q6v5_start(struct rproc *rproc) > struct q6v5 *qproc = (struct q6v5 *)rproc->priv; > int ret; > > - ret = q6v5_regulator_enable(qproc); > + ret = q6v5_regulator_enable(qproc, qproc->proxy_regs, > + qproc->proxy_reg_count); Align indentation with parenthesis on previous line. > if (ret) { > - dev_err(qproc->dev, "failed to enable supplies\n"); > + dev_err(qproc->dev, "failed to enable proxy supplies\n"); > return ret; > } > > + ret = q6v5_regulator_enable(qproc, qproc->active_regs, > + qproc->active_reg_count); > + if (ret) { > + dev_err(qproc->dev, "failed to enable supplies\n"); > + goto disable_proxy_reg; > + } > ret = reset_control_deassert(qproc->mss_restart); > if (ret) { > dev_err(qproc->dev, "failed to deassert mss restart\n"); > @@ -600,6 +657,8 @@ static int q6v5_start(struct rproc *rproc) > disable_vdd: > q6v5_regulator_disable(qproc); Here you disable both active and proxy regulators, then you call through... > > +disable_proxy_reg: > + q6v5_proxy_regulator_disable(qproc); ...and once again disable proxy regulators. Remove q6v5_regulator_disable() and just call the active and proxy disable functions directly. > return ret; > } > Regards, Bjorn -- 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 12/9/2016 8:14 AM, Bjorn Andersson wrote: > On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote: > >> Regulator enable routine will get additional input parameter of >> regulator info and count, It will read regulator info and will do >> appropriate voltage and load configuration before turning them up. >> Also separate out disable interface into proxy and active disable >> so that on arrival of handover interrupt proxy regulators alone >> could be voted out. >> >> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> >> --- >> drivers/remoteproc/qcom_q6v5_pil.c | 113 ++++++++++++++++++++++++++++--------- >> 1 file changed, 86 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index b0f0fcf..06d5bb2 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c >> @@ -126,7 +126,6 @@ struct q6v5 { >> struct qcom_smem_state *state; >> unsigned stop_bit; >> >> - struct regulator_bulk_data supply[4]; >> >> struct clk *ahb_clk; >> struct clk *axi_clk; >> @@ -160,13 +159,6 @@ enum { >> Q5V56_1_5_0, /*hexagon on msm8996*/ >> }; >> >> -enum { >> - Q6V5_SUPPLY_CX, >> - Q6V5_SUPPLY_MX, >> - Q6V5_SUPPLY_MSS, >> - Q6V5_SUPPLY_PLL, >> -}; >> - >> static int q6v5_regulator_init(struct device *dev, >> struct reg_info *regs, char **reg_str, int volatage_load[][2]) >> { >> @@ -206,35 +198,93 @@ static int q6v5_regulator_init(struct device *dev, >> return reg_count; >> } >> >> -static int q6v5_regulator_enable(struct q6v5 *qproc) >> +static int q6v5_regulator_enable(struct q6v5 *qproc, >> + struct reg_info *regs, int count) >> { >> - struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer; >> - struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer; >> - int ret; >> + int i, rc = 0; > One variable per line, no need to replace "ret" with "rc" and no need to > initialize it to 0 as it won't be read before written in any code path. OK. > >> + >> + for (i = 0; i < count; i++) { >> + if (regs[i].uV > 0) { >> + rc = regulator_set_voltage(regs[i].reg, >> + regs[i].uV, INT_MAX); >> + if (rc) { >> + dev_err(qproc->dev, >> + "Failed to request voltage for %d.\n", >> + i); >> + goto err; >> + } >> + } >> >> - /* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */ >> + if (regs[i].uA > 0) { >> + rc = regulator_set_load(regs[i].reg, >> + regs[i].uA); >> + if (rc < 0) { >> + dev_err(qproc->dev, "Failed to set regulator mode\n"); >> + goto err; >> + } >> + } >> >> - ret = regulator_set_voltage(mx, 1050000, INT_MAX); >> - if (ret) >> - return ret; >> + rc = regulator_enable(regs[i].reg); >> + if (rc) { >> + dev_err(qproc->dev, "Regulator enable failed\n"); >> + goto err; >> + } >> + } >> + >> + return 0; >> +err: >> + for (; i >= 0; i--) { >> + if (regs[i].uV > 0) >> + regulator_set_voltage(regs[i].reg, 0, INT_MAX); >> >> - regulator_set_voltage(mss, 1000000, 1150000); >> + if (regs[i].uA > 0) >> + regulator_set_load(regs[i].reg, 0); >> + >> + regulator_disable(regs[i].reg); >> + } >> >> - return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply); >> + return rc; >> } >> >> -static void q6v5_regulator_disable(struct q6v5 *qproc) >> +static void q6v5_proxy_regulator_disable(struct q6v5 *qproc) > Please have follow the same scheme as for enable, with a function taking > the list of regulators and count. Ok. > >> { >> - struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer; >> - struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer; >> + int i; >> + struct reg_info *regs = qproc->proxy_regs; >> >> - /* TODO: Q6V5_SUPPLY_CX corner votes should be released */ >> + for (i = 0; i < qproc->proxy_reg_count; i++) { >> + if (regs[i].uV > 0) >> + regulator_set_voltage(regs[i].reg, 0, INT_MAX); >> >> - regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply); >> - regulator_set_voltage(mx, 0, INT_MAX); >> - regulator_set_voltage(mss, 0, 1150000); >> + if (regs[i].uA > 0) >> + regulator_set_load(regs[i].reg, 0); >> + >> + regulator_disable(regs[i].reg); >> + } >> } >> >> +static void q6v5_active_regulator_disable(struct q6v5 *qproc) >> +{ >> + int i; >> + struct reg_info *regs = qproc->active_regs; >> + >> + for (i = 0; i < qproc->active_reg_count; i++) { >> + if (regs[i].uV > 0) >> + regulator_set_voltage(regs[i].reg, 0, INT_MAX); >> + >> + if (regs[i].uA > 0) >> + regulator_set_load(regs[i].reg, 0); >> + >> + regulator_disable(regs[i].reg); >> + } >> +} >> + >> +static void q6v5_regulator_disable(struct q6v5 *qproc) >> +{ >> + q6v5_proxy_regulator_disable(qproc); >> + q6v5_active_regulator_disable(qproc); >> +} >> + >> + >> static int q6v5_load(struct rproc *rproc, const struct firmware *fw) >> { >> struct q6v5 *qproc = rproc->priv; >> @@ -524,12 +574,19 @@ static int q6v5_start(struct rproc *rproc) >> struct q6v5 *qproc = (struct q6v5 *)rproc->priv; >> int ret; >> >> - ret = q6v5_regulator_enable(qproc); >> + ret = q6v5_regulator_enable(qproc, qproc->proxy_regs, >> + qproc->proxy_reg_count); > Align indentation with parenthesis on previous line. Ok. > >> if (ret) { >> - dev_err(qproc->dev, "failed to enable supplies\n"); >> + dev_err(qproc->dev, "failed to enable proxy supplies\n"); >> return ret; >> } >> >> + ret = q6v5_regulator_enable(qproc, qproc->active_regs, >> + qproc->active_reg_count); >> + if (ret) { >> + dev_err(qproc->dev, "failed to enable supplies\n"); >> + goto disable_proxy_reg; >> + } >> ret = reset_control_deassert(qproc->mss_restart); >> if (ret) { >> dev_err(qproc->dev, "failed to deassert mss restart\n"); >> @@ -600,6 +657,8 @@ static int q6v5_start(struct rproc *rproc) >> disable_vdd: >> q6v5_regulator_disable(qproc); > Here you disable both active and proxy regulators, then you call > through... Ok. > >> >> +disable_proxy_reg: >> + q6v5_proxy_regulator_disable(qproc); > ...and once again disable proxy regulators. > > Remove q6v5_regulator_disable() and just call the active and proxy > disable functions directly. Ok. Using single function to disable regulator but passing proxy or active regulator info as necessary. > >> return ret; >> } >> > Regards, > Bjorn
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index b0f0fcf..06d5bb2 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -126,7 +126,6 @@ struct q6v5 { struct qcom_smem_state *state; unsigned stop_bit; - struct regulator_bulk_data supply[4]; struct clk *ahb_clk; struct clk *axi_clk; @@ -160,13 +159,6 @@ enum { Q5V56_1_5_0, /*hexagon on msm8996*/ }; -enum { - Q6V5_SUPPLY_CX, - Q6V5_SUPPLY_MX, - Q6V5_SUPPLY_MSS, - Q6V5_SUPPLY_PLL, -}; - static int q6v5_regulator_init(struct device *dev, struct reg_info *regs, char **reg_str, int volatage_load[][2]) { @@ -206,35 +198,93 @@ static int q6v5_regulator_init(struct device *dev, return reg_count; } -static int q6v5_regulator_enable(struct q6v5 *qproc) +static int q6v5_regulator_enable(struct q6v5 *qproc, + struct reg_info *regs, int count) { - struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer; - struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer; - int ret; + int i, rc = 0; + + for (i = 0; i < count; i++) { + if (regs[i].uV > 0) { + rc = regulator_set_voltage(regs[i].reg, + regs[i].uV, INT_MAX); + if (rc) { + dev_err(qproc->dev, + "Failed to request voltage for %d.\n", + i); + goto err; + } + } - /* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */ + if (regs[i].uA > 0) { + rc = regulator_set_load(regs[i].reg, + regs[i].uA); + if (rc < 0) { + dev_err(qproc->dev, "Failed to set regulator mode\n"); + goto err; + } + } - ret = regulator_set_voltage(mx, 1050000, INT_MAX); - if (ret) - return ret; + rc = regulator_enable(regs[i].reg); + if (rc) { + dev_err(qproc->dev, "Regulator enable failed\n"); + goto err; + } + } + + return 0; +err: + for (; i >= 0; i--) { + if (regs[i].uV > 0) + regulator_set_voltage(regs[i].reg, 0, INT_MAX); - regulator_set_voltage(mss, 1000000, 1150000); + if (regs[i].uA > 0) + regulator_set_load(regs[i].reg, 0); + + regulator_disable(regs[i].reg); + } - return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply); + return rc; } -static void q6v5_regulator_disable(struct q6v5 *qproc) +static void q6v5_proxy_regulator_disable(struct q6v5 *qproc) { - struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer; - struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer; + int i; + struct reg_info *regs = qproc->proxy_regs; - /* TODO: Q6V5_SUPPLY_CX corner votes should be released */ + for (i = 0; i < qproc->proxy_reg_count; i++) { + if (regs[i].uV > 0) + regulator_set_voltage(regs[i].reg, 0, INT_MAX); - regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply); - regulator_set_voltage(mx, 0, INT_MAX); - regulator_set_voltage(mss, 0, 1150000); + if (regs[i].uA > 0) + regulator_set_load(regs[i].reg, 0); + + regulator_disable(regs[i].reg); + } } +static void q6v5_active_regulator_disable(struct q6v5 *qproc) +{ + int i; + struct reg_info *regs = qproc->active_regs; + + for (i = 0; i < qproc->active_reg_count; i++) { + if (regs[i].uV > 0) + regulator_set_voltage(regs[i].reg, 0, INT_MAX); + + if (regs[i].uA > 0) + regulator_set_load(regs[i].reg, 0); + + regulator_disable(regs[i].reg); + } +} + +static void q6v5_regulator_disable(struct q6v5 *qproc) +{ + q6v5_proxy_regulator_disable(qproc); + q6v5_active_regulator_disable(qproc); +} + + static int q6v5_load(struct rproc *rproc, const struct firmware *fw) { struct q6v5 *qproc = rproc->priv; @@ -524,12 +574,19 @@ static int q6v5_start(struct rproc *rproc) struct q6v5 *qproc = (struct q6v5 *)rproc->priv; int ret; - ret = q6v5_regulator_enable(qproc); + ret = q6v5_regulator_enable(qproc, qproc->proxy_regs, + qproc->proxy_reg_count); if (ret) { - dev_err(qproc->dev, "failed to enable supplies\n"); + dev_err(qproc->dev, "failed to enable proxy supplies\n"); return ret; } + ret = q6v5_regulator_enable(qproc, qproc->active_regs, + qproc->active_reg_count); + if (ret) { + dev_err(qproc->dev, "failed to enable supplies\n"); + goto disable_proxy_reg; + } ret = reset_control_deassert(qproc->mss_restart); if (ret) { dev_err(qproc->dev, "failed to deassert mss restart\n"); @@ -600,6 +657,8 @@ static int q6v5_start(struct rproc *rproc) disable_vdd: q6v5_regulator_disable(qproc); +disable_proxy_reg: + q6v5_proxy_regulator_disable(qproc); return ret; }
Regulator enable routine will get additional input parameter of regulator info and count, It will read regulator info and will do appropriate voltage and load configuration before turning them up. Also separate out disable interface into proxy and active disable so that on arrival of handover interrupt proxy regulators alone could be voted out. Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> --- drivers/remoteproc/qcom_q6v5_pil.c | 113 ++++++++++++++++++++++++++++--------- 1 file changed, 86 insertions(+), 27 deletions(-)