| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <89757411-df44-95c0-37a6-fb3395c6144c@codeaurora.org>
Date: Fri, 30 Dec 2016 16:20:16 +0530
From: "Dwivedi, Avaneesh Kumar (avani)" <akdwived@...eaurora.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: sboyd@...eaurora.org, agross@...eaurora.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and
active clocks.
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@...eaurora.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.
Powered by blists - more mailing lists