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 <akdwi...@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
