Date: Thu, 22 Dec 2016 13:42:07 -0800
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
Cc: sboyd@codeaurora.org, agross@codeaurora.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-remoteproc@vger.kernel.org
Subject: Re: [PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and
 active clocks.
Message-ID: <20161222214207.GA10519@minitux>
References: <1481804490-8583-1-git-send-email-akdwived@codeaurora.org>
 <1481804490-8583-3-git-send-email-akdwived@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1481804490-8583-3-git-send-email-akdwived@codeaurora.org>
User-Agent: Mutt/1.7.2 (2016-11-26)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4773
Lines: 178

On Thu 15 Dec 04:21 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 respectively, whose handle is being obtained
> by supplying proxy and active clock name string.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 65 +++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index d875448..8c8b239 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -95,6 +95,8 @@
>  
>  struct rproc_hexagon_res {
>  	const char *hexagon_mba_image;
> +	char **proxy_clk_string;
> +	char **active_clk_string;

Use "name" instead of "string" in these variable names - i.e.
proxy_clk_names and active_clk_names.

>  };
>  
>  struct q6v5 {
> @@ -114,6 +116,11 @@ struct q6v5 {
>  	struct qcom_smem_state *state;
>  	unsigned stop_bit;
>  
> +	struct clk *active_clks[8];
> +	struct clk *proxy_clks[4];
> +	int active_clk_count;
> +	int proxy_clk_count;
> +
>  	struct regulator_bulk_data supply[4];
>  
>  	struct clk *ahb_clk;
> @@ -706,27 +713,33 @@ 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)

clk_names

>  {
> -	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 count = 0;
> +	int i;
>  
> -	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[count])
> +		count++;
> +
> +	for (i = 0; i < count; i++) {

You can squash these two loops into one, e.g. replace them with:

for (i = 0; clk_str[i]; i++) {}

and then return "i".

> +		clks[i] = devm_clk_get(dev, clk_str[i]);
> +		if (IS_ERR(clks[i])) {
> +
> +			int rc = PTR_ERR(clks[i]);
> +
> +			if (rc != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get %s clock\n",
> +					clk_str[i]);
> +			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 count;
>  }
>  
>  static int q6v5_init_reset(struct q6v5 *qproc)
> @@ -843,9 +856,21 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> -	ret = q6v5_init_clocks(qproc);
> -	if (ret)
> +	ret = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks,
> +					desc->proxy_clk_string);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to setup proxy clocks.\n");

Please replace "setup" with "acquire" or "get".

> +		goto free_rproc;
> +	}
> +	qproc->proxy_clk_count = ret;
> +
> +	ret = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
> +					desc->active_clk_string);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to setup active clocks.\n");

Dito.

>  		goto free_rproc;
> +	}
> +	qproc->active_clk_count = ret;
>  
>  	ret = q6v5_regulator_init(qproc);
>  	if (ret)
> @@ -901,10 +926,14 @@ static int q6v5_remove(struct platform_device *pdev)
>  
>  static const struct rproc_hexagon_res msm8916_mss = {
>  	.hexagon_mba_image = "mba.mbn",
> +	.proxy_clk_string = (char*[]){"xo", NULL},
> +	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},

Line wrap these list of clock names, like:

	.active_clk_string = (char*[]){
		"iface",
		"bus",
		"mem",
		NULL
	},

Makes it consistent with the regulator
patch and it's easier for me to read.


>  };
>  
>  static const struct rproc_hexagon_res msm8974_mss = {
>  	.hexagon_mba_image = "mba.b00",
> +	.proxy_clk_string = (char*[]){"xo", NULL},
> +	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},

Dito

>  };

If I apply this patch without patch 5 (Modify clock enable and disable
interface) we will successfully probe with the new clocks, but we will
not be able to boot because ahb_clk, axi_clk and rom_clk are NULL.

When you're sending patches you should make sure that the code works
(before and) after each patch that you introduce.

So please squash patch 5 into this patch.


Other than that and these small style comments I think this patch looks
good!

Regards,
Bjorn