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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
