Re: [RESEND PATCH v4 2/7] remoteproc: qcom: Initialize proxy and active clock's and regulator's

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Certain regulators and clocks need voting by rproc on behalf of hexagon
> only during restart operation but certain clocks and voltage need to be
> voted till hexagon is up, these regulators and clocks are identified as
> proxy and active resource whose handle is being obtained by supplying
> private proxy and active regulator and clock string.
> 

Please split this patch out into the regulator and clock patches
respectively, making each a clear functional change.

> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 148 +++++++++++++++++++++++++++----------
>  1 file changed, 107 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 3360312..b0f0fcf 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -37,7 +37,6 @@
>  
>  #include <linux/qcom_scm.h>
>  
> -#define MBA_FIRMWARE_NAME		"mba.b00"

Please move this to patch 1.

>  #define MPSS_FIRMWARE_NAME		"modem.mdt"
>  
>  #define MPSS_CRASH_REASON_SMEM		421
> @@ -132,6 +131,14 @@ struct q6v5 {
>  	struct clk *ahb_clk;
>  	struct clk *axi_clk;
>  	struct clk *rom_clk;
> +	struct clk *active_clks[8];
> +	struct clk *proxy_clks[4];
> +	struct reg_info active_regs[1];
> +	struct reg_info proxy_regs[3];
> +	int active_reg_count;
> +	int proxy_reg_count;
> +	int active_clk_count;
> +	int proxy_clk_count;

Group clocks members together and regulators together.

>  
>  	struct completion start_done;
>  	struct completion stop_done;
> @@ -160,27 +167,43 @@ enum {
>  	Q6V5_SUPPLY_PLL,
>  };
>  
> -static int q6v5_regulator_init(struct q6v5 *qproc)
> +static int q6v5_regulator_init(struct device *dev,
> +	struct reg_info *regs, char **reg_str, int volatage_load[][2])
>  {
> -	int ret;
> +	int reg_count = 0, i;

Please, one variable per line, sorted (descending) by line length.

>  
> -	qproc->supply[Q6V5_SUPPLY_CX].supply = "cx";
> -	qproc->supply[Q6V5_SUPPLY_MX].supply = "mx";
> -	qproc->supply[Q6V5_SUPPLY_MSS].supply = "mss";
> -	qproc->supply[Q6V5_SUPPLY_PLL].supply = "pll";
> +	if (!reg_str)
> +		return 0;
>  
> -	ret = devm_regulator_bulk_get(qproc->dev,
> -				      ARRAY_SIZE(qproc->supply), qproc->supply);
> -	if (ret < 0) {
> -		dev_err(qproc->dev, "failed to get supplies\n");
> -		return ret;
> -	}
> +	while (reg_str[reg_count] != NULL)

Drop "!= NULL" from your condition.

> +		reg_count++;
>  
> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 100000);
> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_MSS].consumer, 100000);
> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 10000);
> +	if (!reg_count)
> +		return reg_count;

You can omit this check, as the for loop below will iterate 0 times and
we will then return 0.

>  
> -	return 0;
> +	if (!regs)
> +		return -ENOMEM;

This will not happen, please drop.

> +
> +	for (i = 0; i < reg_count; i++) {
> +		const char *reg_name;

Please keep local variables at the top of the function.

And generally try to use short but clear variable names; in line with my
suggestion in patch 1 name it "supply" (or in this case where the
original variable is quite short just use it directly).

> +
> +		reg_name = reg_str[i];
> +		regs[i].reg = devm_regulator_get(dev, reg_name);
> +		if (IS_ERR(regs[i].reg)) {
> +
> +			int rc = PTR_ERR(regs[i].reg);
> +
> +			if (rc != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get %s\n regulator",
> +								reg_name);
> +			return rc;
> +		}
> +
> +		regs[i].uV = volatage_load[i][0];
> +		regs[i].uA = volatage_load[i][1];
> +	}
> +
> +	return reg_count;
>  }
>  
>  static int q6v5_regulator_enable(struct q6v5 *qproc)
> @@ -725,27 +748,41 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int q6v5_init_clocks(struct q6v5 *qproc)
> +static int q6v5_init_clocks(struct device *dev, struct clk **clks,
> +		char **clk_str)
>  {
> -	qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
> -	if (IS_ERR(qproc->ahb_clk)) {
> -		dev_err(qproc->dev, "failed to get iface clock\n");
> -		return PTR_ERR(qproc->ahb_clk);
> -	}
> +	int clk_count = 0, i;

One variable per line, please.  And "count" is unambiguous enough.

>  
> -	qproc->axi_clk = devm_clk_get(qproc->dev, "bus");
> -	if (IS_ERR(qproc->axi_clk)) {
> -		dev_err(qproc->dev, "failed to get bus clock\n");
> -		return PTR_ERR(qproc->axi_clk);
> -	}
> +	if (!clk_str)
> +		return 0;
> +
> +	while (clk_str[clk_count] != NULL)

Drop "!= NULL" part

> +		clk_count++;
> +
> +	if (!clk_count)
> +		return clk_count;

This is not needed.

> +
> +	if (!clks)
> +		return -ENOMEM;

This can't happen.

> +
> +	for (i = 0; i < clk_count; i++) {
> +		const char *clock_name;
> +
> +		clock_name = clk_str[i];
> +		clks[i] = devm_clk_get(dev, clock_name);
> +		if (IS_ERR(clks[i])) {
> +
> +			int rc = PTR_ERR(clks[i]);
> +
> +			if (rc != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get %s clock\n",
> +					clock_name);
> +			return rc;
> +		}
>  
> -	qproc->rom_clk = devm_clk_get(qproc->dev, "mem");
> -	if (IS_ERR(qproc->rom_clk)) {
> -		dev_err(qproc->dev, "failed to get mem clock\n");
> -		return PTR_ERR(qproc->rom_clk);
>  	}
>  
> -	return 0;
> +	return clk_count;
>  }
>  
>  static int q6v5_init_reset(struct q6v5 *qproc)
> @@ -830,10 +867,15 @@ static int q6v5_probe(struct platform_device *pdev)
>  {
>  	struct q6v5 *qproc;
>  	struct rproc *rproc;
> -	int ret;
> +	const struct rproc_hexagon_res *desc;
> +	int ret, count;

One variable per line please, and sort (descending) on line length.

> +
> +	desc = of_device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
>  
>  	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
> -			    MBA_FIRMWARE_NAME, sizeof(*qproc));
> +			    desc->hexagon_mba_image, sizeof(*qproc));

Please move this to patch 1.

>  	if (!rproc) {
>  		dev_err(&pdev->dev, "failed to allocate rproc\n");
>  		return -ENOMEM;
> @@ -857,18 +899,42 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> -	if (ret)
> -		goto free_rproc;
> +	count = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks,
> +		desc->proxy_clk_string);

Your "count" serves the same purpose as "ret", so just use "ret".
And align broken lines with the parenthesis.

> +	if (count < 0) {
> +		dev_err(&pdev->dev, "Failed to setup proxy clocks.\n");
> +		return count;

And using "ret" instead of "count" makes it easy to goto free_rproc,
which you must do after calling rproc_alloc().

> +	}
> +	qproc->proxy_clk_count = count;
>  
> -	ret = q6v5_regulator_init(qproc);
> -	if (ret)
> -		goto free_rproc;
> +	count = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
> +		desc->active_clk_string);
> +	if (count < 0) {
> +		dev_err(&pdev->dev, "Failed to setup active clocks.\n");
> +		return count;
> +	}
> +	qproc->active_clk_count = count;
>  
>  	ret = q6v5_init_reset(qproc);
>  	if (ret)
>  		goto free_rproc;
>  
> +	count = q6v5_regulator_init(&pdev->dev, qproc->proxy_regs,
> +		desc->proxy_reg_string, (int (*)[2])desc->proxy_uV_uA);

Moving proxy_reg_string and proxy_uV_uA into a struct (as suggested in
patch 1) will make this cleaner and we get rid of that typecast.

> +	if (count < 0) {
> +		dev_err(&pdev->dev, "Failed to setup active regulators.\n");
> +		return count;
> +	}
> +	qproc->proxy_reg_count = count;
> +
> +	count = q6v5_regulator_init(&pdev->dev, qproc->active_regs,
> +		desc->active_reg_string, (int (*)[2])desc->active_uV_uA);
> +	if (count < 0) {
> +		dev_err(&pdev->dev, "Failed to setup proxy regulators.\n");
> +		return count;
> +	}
> +	qproc->active_reg_count = count;
> +
>  	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
>  	if (ret < 0)
>  		goto free_rproc;

Regards,
Bjorn