All of lore.kernel.org
 help / color / mirror / Atom feed
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.
Date: Thu, 22 Dec 2016 13:42:07 -0800	[thread overview]
Message-ID: <20161222214207.GA10519@minitux> (raw)
In-Reply-To: <1481804490-8583-3-git-send-email-akdwived@codeaurora.org>

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

  reply	other threads:[~2016-12-22 21:42 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-15 12:21 [PATCH v5 0/7] remoteproc: qcom: Add support of mss rproc for msm8996 Avaneesh Kumar Dwivedi
2016-12-15 12:21 ` [PATCH v5 1/7] remoteproc: qcom: Add private resource struct and new compatible Avaneesh Kumar Dwivedi
2016-12-15 12:21 ` [PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and active clocks Avaneesh Kumar Dwivedi
2016-12-22 21:42   ` Bjorn Andersson [this message]
2016-12-30 10:50     ` Dwivedi, Avaneesh Kumar (avani)
2016-12-15 12:21 ` [PATCH v5 3/7] remoteproc: qcom: Add and initialize proxy and active regulators Avaneesh Kumar Dwivedi
2016-12-22 21:46   ` Bjorn Andersson
2016-12-30 11:02     ` Dwivedi, Avaneesh Kumar (avani)
2016-12-28 23:38   ` Stephen Boyd
2016-12-15 12:21 ` [PATCH v5 4/7] remoteproc: qcom: Modify regulator enable and disable interface Avaneesh Kumar Dwivedi
2016-12-23  0:15   ` Bjorn Andersson
2016-12-15 12:21 ` [PATCH v5 5/7] remoteproc: qcom: Modify clock " Avaneesh Kumar Dwivedi
2016-12-23  0:17   ` Bjorn Andersson
2016-12-15 12:21 ` [PATCH v5 6/7] remoteproc: qcom: Implement Hexagon core specific changes Avaneesh Kumar Dwivedi
2016-12-23  1:04   ` Bjorn Andersson
2016-12-23  1:04     ` Bjorn Andersson
2016-12-30 10:18     ` Dwivedi, Avaneesh Kumar (avani)
2016-12-15 12:21 ` [PATCH v5 7/7] clk: qcom: Add GCC_MSS_RESET support Avaneesh Kumar Dwivedi
2016-12-28 23:40   ` Stephen Boyd
2016-12-29 17:09     ` Andy Gross
2016-12-29 19:55       ` Bjorn Andersson
  -- strict thread matches above, loose matches on Subject: below --
2016-12-15 12:05 [PATCH v5 0/7] remoteproc: qcom: Add support of mss rproc for msm8996 Avaneesh Kumar Dwivedi
2016-12-15 12:05 ` [PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and active clocks Avaneesh Kumar Dwivedi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161222214207.GA10519@minitux \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@codeaurora.org \
    --cc=akdwived@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=sboyd@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.