| 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: <49139ae4-4373-9e70-02ad-80f7bbc4494c@codeaurora.org>
Date: Wed, 5 Dec 2018 09:07:00 +0530
From: Taniya Das <tdas@...eaurora.org>
To: Stephen Boyd <sboyd@...nel.org>,
Viresh Kumar <viresh.kumar@...aro.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
Rajendra Nayak <rnayak@...eaurora.org>,
devicetree@...r.kernel.org, robh@...nel.org,
skannan@...eaurora.org, linux-arm-msm@...r.kernel.org,
evgreen@...gle.com
Subject: Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW
driver
Hello Stephen, Viresh
Thanks for the code and suggestions.
Having a NR_DOMAINS '2' makes the driver not scalable for re-use.
This assumption is only true for the current version of the HW and do
not intend to update/clean-up this logic again. So want to stick keeping
current logic of having the *qcom_freq_domain_map[NR_CPUS].
On 12/5/2018 4:35 AM, Stephen Boyd wrote:
> Quoting Stephen Boyd (2018-12-04 14:28:11)
>> Quoting Viresh Kumar (2018-12-03 21:12:31)
>>> Hi Taniya,
>>>
>>> Sorry that I haven't been reviewing it much from last few iterations as I was
>>> letting others get this into a better shape. Thanks for your efforts..
>>>
>>> On 02-12-18, 09:25, Taniya Das wrote:
>>>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>>>
>>>> +struct cpufreq_qcom {
>>>> + struct cpufreq_frequency_table *table;
>>>> + void __iomem *perf_state_reg;
>>>> + cpumask_t related_cpus;
>>>> +};
>>>> +
>>>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>>>
>>> Now that the code is much more simplified, I am not sure if you need this
>>> per-cpu structure at all. The only place where you are using it is in
>>> qcom_cpufreq_hw_cpu_init() and probe(). Why not merge qcom_cpu_resources_init()
>>> completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
>>> entirely ?
>>>
>>
>> Good point. Here's an untested patch to handle that. It removes the
>> related functionality and makes an array of pointers to the domains that
>> are created each time qcom_cpu_resources_init() is called.
>>
>> ---8<----
>>
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> index 8dc6b73c2f22..04e7cfd70316 100644
>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -23,14 +23,14 @@
>> #define REG_LUT_TABLE 0x110
>> #define REG_PERF_STATE 0x920
>>
>> +#define MAX_FREQ_DOMAINS 2
>> +
>> struct cpufreq_qcom {
>> struct cpufreq_frequency_table *table;
>> void __iomem *perf_state_reg;
>> cpumask_t related_cpus;
>> };
>>
>> -static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> -
>> static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>> unsigned int index)
>> {
>> @@ -76,9 +76,14 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>>
>> static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>> {
>> + struct cpufreq_qcom **freq_domain_map;
>> struct cpufreq_qcom *c;
>>
>> - c = qcom_freq_domain_map[policy->cpu];
>> + freq_domain_map = cpufreq_get_driver_data();
>> + if (!freq_domain_map)
>> + return -ENODEV;
>> +
>> + c = freq_domain_map[policy->cpu];
>
> And this fails now because it indexes based on cpu number. We have to
> parse the frequency domain out of the cpunode again to fix that.
>
> Here's the updated (still untested) patch and I also just allocated the
> freq domain array up front instead of in each domain init routine to
> simplify some more.
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 8dc6b73c2f22..bc0d734f7e3c 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -23,14 +23,14 @@
> #define REG_LUT_TABLE 0x110
> #define REG_PERF_STATE 0x920
>
> +#define MAX_FREQ_DOMAINS 2
> +
> struct cpufreq_qcom {
> struct cpufreq_frequency_table *table;
> void __iomem *perf_state_reg;
> cpumask_t related_cpus;
> };
>
> -static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> -
> static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> unsigned int index)
> {
> @@ -76,9 +76,26 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>
> static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> {
> - struct cpufreq_qcom *c;
> + struct cpufreq_qcom *freq_domain_map, *c;
> + struct device_node *cpu_np;
> + struct of_phandle_args args;
> + int ret;
> +
> + freq_domain_map = cpufreq_get_driver_data();
> + if (!freq_domain_map)
> + return -ENODEV;
> +
> + cpu_np = of_cpu_device_node_get(policy->cpu);
> + if (!cpu_np)
> + return -ENODEV;
> +
> + ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
> + "#freq-domain-cells", 0, &args);
> + of_node_put(cpu_np);
> + if (ret)
> + return ret;
>
> - c = qcom_freq_domain_map[policy->cpu];
> + c = &freq_domain_map[args.args[0]];
> if (!c) {
> pr_err("No scaling support for CPU%d\n", policy->cpu);
> return -ENODEV;
> @@ -171,43 +188,15 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
> return 0;
> }
>
> -static void qcom_get_related_cpus(int index, struct cpumask *m)
> -{
> - struct device_node *cpu_np;
> - struct of_phandle_args args;
> - int cpu, ret;
> -
> - for_each_possible_cpu(cpu) {
> - cpu_np = of_cpu_device_node_get(cpu);
> - if (!cpu_np)
> - continue;
> -
> - ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
> - "#freq-domain-cells", 0,
> - &args);
> - of_node_put(cpu_np);
> - if (ret < 0)
> - continue;
> -
> - if (index == args.args[0])
> - cpumask_set_cpu(cpu, m);
> - }
> -}
> -
> static int qcom_cpu_resources_init(struct platform_device *pdev,
> - unsigned int cpu, int index,
> - unsigned long xo_rate,
> - unsigned long cpu_hw_rate)
> + int index, unsigned long xo_rate,
> + unsigned long cpu_hw_rate,
> + struct cpufreq_qcom *c)
> {
> - struct cpufreq_qcom *c;
> struct resource *res;
> struct device *dev = &pdev->dev;
> void __iomem *base;
> - int ret, cpu_r;
> -
> - c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> - if (!c)
> - return -ENOMEM;
> + int ret;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> base = devm_ioremap_resource(dev, res);
> @@ -220,12 +209,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
> return -ENODEV;
> }
>
> - qcom_get_related_cpus(index, &c->related_cpus);
> - if (!cpumask_weight(&c->related_cpus)) {
> - dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
> - return -ENOENT;
> - }
> -
> c->perf_state_reg = base + REG_PERF_STATE;
>
> ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
> @@ -234,9 +217,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
> return ret;
> }
>
> - for_each_cpu(cpu_r, &c->related_cpus)
> - qcom_freq_domain_map[cpu_r] = c;
> -
> return 0;
> }
>
> @@ -245,9 +225,16 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
> struct device_node *cpu_np;
> struct of_phandle_args args;
> struct clk *clk;
> - unsigned int cpu;
> + unsigned int cpu, domain;
> unsigned long xo_rate, cpu_hw_rate;
> int ret;
> + struct cpufreq_qcom *freq_domain_map, *freq_domain;
> +
> + freq_domain_map = devm_kcalloc(&pdev->dev, MAX_FREQ_DOMAINS,
> + sizeof(*freq_domain_map), GFP_KERNEL);
> + cpufreq_qcom_hw_driver.driver_data = freq_domain_map;
> + if (!freq_domain_map)
> + return -ENOMEM;
>
> clk = clk_get(&pdev->dev, "xo");
> if (IS_ERR(clk))
> @@ -273,16 +260,26 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>
> ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
> "#freq-domain-cells", 0,
> - &args);
> + &args);
> of_node_put(cpu_np);
> if (ret)
> return ret;
>
> - if (qcom_freq_domain_map[cpu])
> + domain = args.args[0];
> + if (WARN_ON(domain >= MAX_FREQ_DOMAINS))
> + continue;
> +
> + freq_domain = &freq_domain_map[domain];
> + cpumask_set_cpu(cpu, &freq_domain->related_cpus);
> + /*
> + * If we've already populated the frequency table for this domain
> + * just mark it related and get out of here
> + */
> + if (cpumask_weight(&freq_domain->related_cpus) > 1)
> continue;
>
> - ret = qcom_cpu_resources_init(pdev, cpu, args.args[0],
> - xo_rate, cpu_hw_rate);
> + ret = qcom_cpu_resources_init(pdev, domain, xo_rate,
> + cpu_hw_rate, freq_domain);
> if (ret)
> return ret;
> }
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
Powered by blists - more mailing lists