From: Taniya Das <tdas@codeaurora.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
Stephen Boyd <sboyd@kernel.org>,
Rajendra Nayak <rnayak@codeaurora.org>,
Amit Nischal <anischal@codeaurora.org>,
devicetree@vger.kernel.org, robh@kernel.org,
skannan@codeaurora.org, amit.kucheria@linaro.org,
evgreen@google.com
Subject: Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
Date: Tue, 17 Jul 2018 08:29:06 +0530 [thread overview]
Message-ID: <3163a20a-31ca-15a0-56e2-38e977b21ae2@codeaurora.org> (raw)
In-Reply-To: <20180716051219.36rdvxux3lz3rjed@vireshk-i7>
On 7/16/2018 10:42 AM, Viresh Kumar wrote:
> On 12-07-18, 23:35, Taniya Das wrote:
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> + struct device_node *np, unsigned int cpu)
>> +{
>> + struct cpufreq_qcom *c;
>> + struct resource res;
>> + struct device *dev = &pdev->dev;
>> + unsigned int offset, cpu_r;
>> + int ret;
>> +
>> + c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> + if (!c)
>> + return -ENOMEM;
>> +
>> + c->reg_offset = of_device_get_match_data(&pdev->dev);
>> + if (!c->reg_offset)
>> + return -EINVAL;
>> +
>> + if (of_address_to_resource(np, 0, &res))
>> + return -ENOMEM;
>> +
>> + c->base = devm_ioremap(dev, res.start, resource_size(&res));
>> + if (!c->base) {
>> + dev_err(dev, "Unable to map %s base\n", np->name);
>> + return -ENOMEM;
>> + }
>> +
>> + offset = c->reg_offset[REG_ENABLE];
>> +
>> + /* HW should be in enabled state to proceed */
>> + if (!(readl_relaxed(c->base + offset) & 0x1)) {
>> + dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
>> + return -ENODEV;
>> + }
>> +
>> + ret = qcom_get_related_cpus(np, &c->related_cpus);
>> + if (ret) {
>> + dev_err(dev, "%s failed to get related CPUs\n", np->name);
>> + return ret;
>> + }
>> +
>> + c->max_cores = cpumask_weight(&c->related_cpus);
>> + if (!c->max_cores)
>> + return -ENOENT;
>> +
>> + ret = qcom_read_lut(pdev, c);
>> + if (ret) {
>> + dev_err(dev, "%s failed to read LUT\n", np->name);
>> + return ret;
>> + }
>> +
>> + qcom_freq_domain_map[cpu] = c;
>> +
>> + /* Related CPUs to keep a single copy */
>> + cpu_r = cpumask_first(&c->related_cpus);
>> + if (cpu != cpu_r) {
>> + qcom_freq_domain_map[cpu] = qcom_freq_domain_map[cpu_r];
>> + devm_kfree(dev, c);
>> + }
>
> Sorry about missing this, you have actually worked on my comments.
>
> But I think this isn't the clever way of doing it. You allocate the
> structures, fill everything and then finally free them because we were
> related. Why can't we have similar check at the top of this routine
> and skip everything then ?
>
Thanks Viresh, I actually replied to all the reviewers with the changes
I have made in v5 series, sorry for not replying individually to all
reviewers. Yes, I agree I should have put this check at the beginning of
the function.
Please help review of the new series[v5] which takes care of the below.
- Remove mapping different register regions of perf/lut/enable,
instead map the entire HW region.
- Add reg_offset/cpufreq_qcom_std_offsets to be supplied as device data.
- Check of src == 0 during lut read.
- Add of_node_put(cpu_np) in qcom_get_related_cpus
- Update the qcom_cpu_resources_init for register offset data,
and cleanup the related cpus to keep a single copy of CPUfreq.
- Replace FW with HW, update Kconfig, rename filename qcom-cpufreq-hw.c
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
next prev parent reply other threads:[~2018-07-17 2:59 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-12 18:05 [PATCH v5 0/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
2018-07-12 18:05 ` [PATCH v5 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings Taniya Das
2018-07-12 23:49 ` Stephen Boyd
2018-07-12 18:05 ` [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
2018-07-12 23:44 ` Stephen Boyd
2018-07-18 5:35 ` Taniya Das
2018-07-13 0:19 ` Matthias Kaehlcke
2018-07-17 5:57 ` Taniya Das
2018-07-16 5:09 ` Viresh Kumar
2018-07-16 5:12 ` Viresh Kumar
2018-07-17 2:59 ` Taniya Das [this message]
2018-07-16 23:02 ` Evan Green
2018-07-17 2:50 ` Taniya Das
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=3163a20a-31ca-15a0-56e2-38e977b21ae2@codeaurora.org \
--to=tdas@codeaurora.org \
--cc=amit.kucheria@linaro.org \
--cc=anischal@codeaurora.org \
--cc=devicetree@vger.kernel.org \
--cc=evgreen@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@rjwysocki.net \
--cc=rnayak@codeaurora.org \
--cc=robh@kernel.org \
--cc=sboyd@kernel.org \
--cc=skannan@codeaurora.org \
--cc=viresh.kumar@linaro.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).