lkml.org 
[lkml]   [2016]   [Dec]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and active clocks.
    From
    Date


    On 12/23/2016 3:12 AM, Bjorn Andersson wrote:
    > 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.
    OK.
    >
    >> };
    >>
    >> 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
    OK.
    >> {
    >> - 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".
    OK.
    >> + 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".
    OK.
    >
    >> + 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.
    OK.
    >> 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.
    OK.
    >
    >> };
    >>
    >> 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
    OK
    >> };
    > 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.
    OK, will squash patches into functional units and send them.
    >
    >
    > Other than that and these small style comments I think this patch looks
    > good!
    Thanks.
    >
    > Regards,
    > Bjorn

    --
    Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
    a Linux Foundation Collaborative Project.

    \
     
     \ /
      Last update: 2016-12-30 11:50    [W:2.511 / U:0.252 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site