* Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
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
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2018-07-12 23:44 UTC (permalink / raw)
To: Rafael J. Wysocki, Taniya Das, Viresh Kumar, linux-kernel, linux-pm
Cc: Rajendra Nayak, Amit Nischal, devicetree, robh, skannan,
amit.kucheria, evgreen, Taniya Das
Quoting Taniya Das (2018-07-12 11:05:45)
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 52f5f1a..141ec3e 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -312,3 +312,13 @@ config ARM_PXA2xx_CPUFREQ
> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
> If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_HW
> + bool "QCOM CPUFreq HW driver"
Why can't it be a module?
> + help
> + Support for the CPUFreq HW driver.
> + Some QCOM chipsets have a HW engine to offload the steps
> + necessary for changing the frequency of the CPUs. Firmware loaded
> + in this engine exposes a programming interafce to the High-level OS.
typo on interface. Why is High capitalized? Just say OS?
> + The driver implements the cpufreq driver interface for this HW engine.
So much 'driver'.
> + Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..fa25a95
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,344 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#define INIT_RATE 300000000UL
This doesn't need to be configured from DT? Or more likely be specified
as some sort of PLL that is part of the clocks property so we know what
the 'safe' or 'default' frequency is?
> +#define XO_RATE 19200000UL
This should come from DT via some clocks property.
> +#define LUT_MAX_ENTRIES 40U
> +#define CORE_COUNT_VAL(val) (((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE 32
> +
> +enum {
> + REG_ENABLE,
> + REG_LUT_TABLE,
> + REG_PERF_STATE,
> +
> + REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_qcom {
> + struct cpufreq_frequency_table *table;
> + struct device *dev;
> + const u16 *reg_offset;
> + void __iomem *base;
> + cpumask_t related_cpus;
> + unsigned int max_cores;
> +};
> +
> +static u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
const?
> + [REG_ENABLE] = 0x0,
> + [REG_LUT_TABLE] = 0x110,
> + [REG_PERF_STATE] = 0x920,
Is the register map going to change again for the next device? It may be
better to precalculate the offset for the fast switch so that the
addition isn't in the hotpath.
> +};
> +
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> +
> +static int
> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> + unsigned int index)
> +{
> + struct cpufreq_qcom *c = policy->driver_data;
> + unsigned int offset = c->reg_offset[REG_PERF_STATE];
> +
> + writel_relaxed(index, c->base + offset);
> +
> + return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> +{
> + struct cpufreq_qcom *c;
> + struct cpufreq_policy *policy;
> + unsigned int index, offset;
> +
> + policy = cpufreq_cpu_get_raw(cpu);
> + if (!policy)
> + return 0;
> +
> + c = policy->driver_data;
> + offset = c->reg_offset[REG_PERF_STATE];
> +
> + index = readl_relaxed(c->base + offset);
> + index = min(index, LUT_MAX_ENTRIES - 1);
> +
> + return policy->freq_table[index].frequency;
> +}
> +
> +static unsigned int
> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> + unsigned int target_freq)
> +{
> + struct cpufreq_qcom *c = policy->driver_data;
> + unsigned int offset;
> + int index;
> +
> + index = cpufreq_table_find_index_l(policy, target_freq);
It's unfortunate that we have to search the table in software again.
Why can't we use policy->cached_resolved_idx to avoid this search twice?
> + if (index < 0)
> + return 0;
> +
> + offset = c->reg_offset[REG_PERF_STATE];
> +
> + writel_relaxed(index, c->base + offset);
> +
> + return policy->freq_table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_qcom *c;
> +
> + c = qcom_freq_domain_map[policy->cpu];
> + if (!c) {
> + pr_err("No scaling support for CPU%d\n", policy->cpu);
> + return -ENODEV;
> + }
> +
> + cpumask_copy(policy->cpus, &c->related_cpus);
> +
> + policy->fast_switch_possible = true;
> + policy->freq_table = c->table;
> + policy->driver_data = c;
> +
> + return 0;
> +}
> +
> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + &cpufreq_freq_attr_scaling_boost_freqs,
> + NULL
> +};
> +
> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> + .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> + CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> + .verify = cpufreq_generic_frequency_table_verify,
> + .target_index = qcom_cpufreq_hw_target_index,
> + .get = qcom_cpufreq_hw_get,
> + .init = qcom_cpufreq_hw_cpu_init,
> + .fast_switch = qcom_cpufreq_hw_fast_switch,
> + .name = "qcom-cpufreq-hw",
> + .attr = qcom_cpufreq_hw_attr,
> + .boost_enabled = true,
> +};
> +
> +static int qcom_read_lut(struct platform_device *pdev,
> + struct cpufreq_qcom *c)
> +{
> + struct device *dev = &pdev->dev;
> + unsigned int offset;
> + u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> + c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> + sizeof(*c->table), GFP_KERNEL);
> + if (!c->table)
> + return -ENOMEM;
> +
> + offset = c->reg_offset[REG_LUT_TABLE];
> +
> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> + data = readl_relaxed(c->base + offset + i * LUT_ROW_SIZE);
> + src = ((data & GENMASK(31, 30)) >> 30);
One too many parenthesis.
> + lval = (data & GENMASK(7, 0));
One too many parenthesis.
> + core_count = CORE_COUNT_VAL(data);
> +
> + if (src == 0)
> + c->table[i].frequency = INIT_RATE / 1000;
> + else
> + c->table[i].frequency = XO_RATE * lval / 1000;
> +
> + cur_freq = c->table[i].frequency;
> +
> + dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> + i, c->table[i].frequency, core_count);
> +
> + if (core_count != c->max_cores)
> + cur_freq = CPUFREQ_ENTRY_INVALID;
> +
> + /*
> + * Two of the same frequencies with the same core counts means
> + * end of table.
> + */
> + if (i > 0 && c->table[i - 1].frequency ==
> + c->table[i].frequency && prev_cc == core_count) {
> + struct cpufreq_frequency_table *prev = &c->table[i - 1];
> +
> + if (prev_freq == CPUFREQ_ENTRY_INVALID)
> + prev->flags = CPUFREQ_BOOST_FREQ;
> + break;
> + }
> + prev_cc = core_count;
> + prev_freq = cur_freq;
> + }
> +
> + c->table[i].frequency = CPUFREQ_TABLE_END;
> +
> + return 0;
> +}
> +
> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
> +{
> + struct device_node *cpu_np, *freq_np;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + cpu_np = of_cpu_device_node_get(cpu);
> + if (!cpu_np)
> + continue;
> + freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
Put the of_node_put(cpu_np) here? And then remove it from the other two
places below?
> + if (!freq_np) {
> + of_node_put(cpu_np);
> + continue;
> + }
> + if (freq_np == np)
> + cpumask_set_cpu(cpu, m);
> +
> + of_node_put(cpu_np);
> + }
> +
> + return 0;
> +}
> +
> +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))
This is unfortunate that it can't use platform APIs.
> + return -ENOMEM;
> +
> + c->base = devm_ioremap(dev, res.start, resource_size(&res));
No devm_ioremap_resource? And we don't put the reg properties in the
top-level node?
> + if (!c->base) {
> + dev_err(dev, "Unable to map %s base\n", np->name);
We don't need error messages like this for mapping failures when it will
spew a kmalloc error.
> + 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);
qcom_cpufreq_hw_read_lut?
> + 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 */
What does this comment mean?
> + 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);
> + }
> +
> + return 0;
> +}
> +
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> + struct device_node *np, *cpu_np;
> + unsigned int cpu;
> + int ret;
> +
> + for_each_possible_cpu(cpu) {
> + cpu_np = of_cpu_device_node_get(cpu);
> + if (!cpu_np) {
> + dev_err(&pdev->dev, "Failed to get cpu %d device\n",
> + cpu);
> + continue;
An error, but we continue? Why not dev_dbg level?
> + }
> +
> + np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> + if (!np) {
> + dev_err(&pdev->dev, "Failed to get freq-domain device\n");
> + return -EINVAL;
> + }
> +
> + of_node_put(cpu_np);
> +
> + ret = qcom_cpu_resources_init(pdev, np, cpu);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
> +{
> + int rc;
> +
> + /* Get the bases of cpufreq for domains */
> + rc = qcom_resources_init(pdev);
> + if (rc) {
> + dev_err(&pdev->dev, "CPUFreq resource init failed\n");
> + return rc;
> + }
> +
> + rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
> + if (rc) {
> + dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
> + return rc;
> + }
> +
> + dev_info(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
Move to dev_dbg? We have other ways to know if a driver probes
successfully so the whole line isn't really needed.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id match_table[] = {
Please call it something besides 'match_table'. qcom_cpufreq_hw_match?
> + { .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
> + {}
> +};
> +
> +static struct platform_driver qcom_cpufreq_hw_driver = {
> + .probe = qcom_cpufreq_hw_driver_probe,
> + .driver = {
> + .name = "qcom-cpufreq-hw",
> + .of_match_table = match_table,
> + .owner = THIS_MODULE,
platform_driver_register() already assigns this. This should be dropped
from here.
> + },
> +};
> +
> +static int __init qcom_cpufreq_hw_init(void)
> +{
> + return platform_driver_register(&qcom_cpufreq_hw_driver);
> +}
> +subsys_initcall(qcom_cpufreq_hw_init);
> +
> +MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
> +MODULE_LICENSE("GPL v2");
It should be tristate then in the Kconfig.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
2018-07-12 23:44 ` Stephen Boyd
@ 2018-07-18 5:35 ` Taniya Das
0 siblings, 0 replies; 13+ messages in thread
From: Taniya Das @ 2018-07-18 5:35 UTC (permalink / raw)
To: Stephen Boyd, Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm
Cc: Rajendra Nayak, Amit Nischal, devicetree, robh, skannan,
amit.kucheria, evgreen
Hello Stephen,
Thanks for the review comments.
On 7/13/2018 5:14 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-07-12 11:05:45)
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 52f5f1a..141ec3e 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -312,3 +312,13 @@ config ARM_PXA2xx_CPUFREQ
>> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>> If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_HW
>> + bool "QCOM CPUFreq HW driver"
>
> Why can't it be a module?
>
I am of the opinion to keep it in-built.
>> + help
>> + Support for the CPUFreq HW driver.
>> + Some QCOM chipsets have a HW engine to offload the steps
>> + necessary for changing the frequency of the CPUs. Firmware loaded
>> + in this engine exposes a programming interafce to the High-level OS.
>
> typo on interface. Why is High capitalized? Just say OS?
> Taken care in the next patch.
>> + The driver implements the cpufreq driver interface for this HW engine.
>
> So much 'driver'.
>
Taken care in the next patch.
>> + Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..fa25a95
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,344 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/cpufreq.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +
>> +#define INIT_RATE 300000000UL
>
> This doesn't need to be configured from DT? Or more likely be specified
> as some sort of PLL that is part of the clocks property so we know what
> the 'safe' or 'default' frequency is?
>
The source is RCG which is pre-configured by HW and is not modeled in SW
code. That is the reason to keep it as a macro.
>> +#define XO_RATE 19200000UL
>
> This should come from DT via some clocks property.
>
This would be taken as an input from DT clocks.
>> +#define LUT_MAX_ENTRIES 40U
>> +#define CORE_COUNT_VAL(val) (((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE 32
>> +
>> +enum {
>> + REG_ENABLE,
>> + REG_LUT_TABLE,
>> + REG_PERF_STATE,
>> +
>> + REG_ARRAY_SIZE,
>> +};
>> +
>> +struct cpufreq_qcom {
>> + struct cpufreq_frequency_table *table;
>> + struct device *dev;
>> + const u16 *reg_offset;
>> + void __iomem *base;
>> + cpumask_t related_cpus;
>> + unsigned int max_cores;
>> +};
>> +
>> +static u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
>
> const?
>
Updated to use const.
>> + [REG_ENABLE] = 0x0,
>> + [REG_LUT_TABLE] = 0x110,
>> + [REG_PERF_STATE] = 0x920,
>
> Is the register map going to change again for the next device? It may be
> better to precalculate the offset for the fast switch so that the
> addition isn't in the hotpath.
>
Taken care in the next patch set.
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> +
>> +static int
>> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>> + unsigned int index)
>> +{
>> + struct cpufreq_qcom *c = policy->driver_data;
>> + unsigned int offset = c->reg_offset[REG_PERF_STATE];
>> +
>> + writel_relaxed(index, c->base + offset);
>> +
>> + return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>> +{
>> + struct cpufreq_qcom *c;
>> + struct cpufreq_policy *policy;
>> + unsigned int index, offset;
>> +
>> + policy = cpufreq_cpu_get_raw(cpu);
>> + if (!policy)
>> + return 0;
>> +
>> + c = policy->driver_data;
>> + offset = c->reg_offset[REG_PERF_STATE];
>> +
>> + index = readl_relaxed(c->base + offset);
>> + index = min(index, LUT_MAX_ENTRIES - 1);
>> +
>> + return policy->freq_table[index].frequency;
>> +}
>> +
>> +static unsigned int
>> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>> + unsigned int target_freq)
>> +{
>> + struct cpufreq_qcom *c = policy->driver_data;
>> + unsigned int offset;
>> + int index;
>> +
>> + index = cpufreq_table_find_index_l(policy, target_freq);
>
> It's unfortunate that we have to search the table in software again.
> Why can't we use policy->cached_resolved_idx to avoid this search twice?
>
Yeah, I just checked the call flow
get_next_freq(already determines the idx and keeps a cached copy of the
index) and then invokes the 'sugov_update_commit' for fast switch.
My understanding is we could use policy->cached_resolved_idx instead of
searching again.
>> + if (index < 0)
>> + return 0;
>> +
>> + offset = c->reg_offset[REG_PERF_STATE];
>> +
>> + writel_relaxed(index, c->base + offset);
>> +
>> + return policy->freq_table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>> +{
>> + struct cpufreq_qcom *c;
>> +
>> + c = qcom_freq_domain_map[policy->cpu];
>> + if (!c) {
>> + pr_err("No scaling support for CPU%d\n", policy->cpu);
>> + return -ENODEV;
>> + }
>> +
>> + cpumask_copy(policy->cpus, &c->related_cpus);
>> +
>> + policy->fast_switch_possible = true;
>> + policy->freq_table = c->table;
>> + policy->driver_data = c;
>> +
>> + return 0;
>> +}
>> +
>> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
>> + &cpufreq_freq_attr_scaling_available_freqs,
>> + &cpufreq_freq_attr_scaling_boost_freqs,
>> + NULL
>> +};
>> +
>> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>> + .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>> + CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> + .verify = cpufreq_generic_frequency_table_verify,
>> + .target_index = qcom_cpufreq_hw_target_index,
>> + .get = qcom_cpufreq_hw_get,
>> + .init = qcom_cpufreq_hw_cpu_init,
>> + .fast_switch = qcom_cpufreq_hw_fast_switch,
>> + .name = "qcom-cpufreq-hw",
>> + .attr = qcom_cpufreq_hw_attr,
>> + .boost_enabled = true,
>> +};
>> +
>> +static int qcom_read_lut(struct platform_device *pdev,
>> + struct cpufreq_qcom *c)
>> +{
>> + struct device *dev = &pdev->dev;
>> + unsigned int offset;
>> + u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
>> +
>> + c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> + sizeof(*c->table), GFP_KERNEL);
>> + if (!c->table)
>> + return -ENOMEM;
>> +
>> + offset = c->reg_offset[REG_LUT_TABLE];
>> +
>> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> + data = readl_relaxed(c->base + offset + i * LUT_ROW_SIZE);
>> + src = ((data & GENMASK(31, 30)) >> 30);
>
> One too many parenthesis.
>
Removed.
>> + lval = (data & GENMASK(7, 0));
>
> One too many parenthesis.
>
Removed.
>> + core_count = CORE_COUNT_VAL(data);
>> +
>> + if (src == 0)
>> + c->table[i].frequency = INIT_RATE / 1000;
>> + else
>> + c->table[i].frequency = XO_RATE * lval / 1000;
>> +
>> + cur_freq = c->table[i].frequency;
>> +
>> + dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> + i, c->table[i].frequency, core_count);
>> +
>> + if (core_count != c->max_cores)
>> + cur_freq = CPUFREQ_ENTRY_INVALID;
>> +
>> + /*
>> + * Two of the same frequencies with the same core counts means
>> + * end of table.
>> + */
>> + if (i > 0 && c->table[i - 1].frequency ==
>> + c->table[i].frequency && prev_cc == core_count) {
>> + struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> + if (prev_freq == CPUFREQ_ENTRY_INVALID)
>> + prev->flags = CPUFREQ_BOOST_FREQ;
>> + break;
>> + }
>> + prev_cc = core_count;
>> + prev_freq = cur_freq;
>> + }
>> +
>> + c->table[i].frequency = CPUFREQ_TABLE_END;
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
>> +{
>> + struct device_node *cpu_np, *freq_np;
>> + int cpu;
>> +
>> + for_each_possible_cpu(cpu) {
>> + cpu_np = of_cpu_device_node_get(cpu);
>> + if (!cpu_np)
>> + continue;
>> + freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>
> Put the of_node_put(cpu_np) here? And then remove it from the other two
> places below?
>
Fixed it in the next patch.
>> + if (!freq_np) {
>> + of_node_put(cpu_np);
>> + continue;
>> + }
>> + if (freq_np == np)
>> + cpumask_set_cpu(cpu, m);
>> +
>> + of_node_put(cpu_np);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +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))
>
> This is unfortunate that it can't use platform APIs.
>
>> + return -ENOMEM;
>> +
>> + c->base = devm_ioremap(dev, res.start, resource_size(&res));
>
> No devm_ioremap_resource? And we don't put the reg properties in the
> top-level node?
>
Moved to use devm_ioremap_resource. There is no reg property in the top
level node.
>> + if (!c->base) {
>> + dev_err(dev, "Unable to map %s base\n", np->name);
>
> We don't need error messages like this for mapping failures when it will
> spew a kmalloc error.
>
removed the error message.
>> + 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);
>
> qcom_cpufreq_hw_read_lut?
>
Renamed the function to 'qcom_cpufreq_hw_read_lut'.
>> + 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 */
>
> What does this comment mean?
>
All related CPUs to use first cpu of the cluster structure.
>> + 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);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_resources_init(struct platform_device *pdev)
>> +{
>> + struct device_node *np, *cpu_np;
>> + unsigned int cpu;
>> + int ret;
>> +
>> + for_each_possible_cpu(cpu) {
>> + cpu_np = of_cpu_device_node_get(cpu);
>> + if (!cpu_np) {
>> + dev_err(&pdev->dev, "Failed to get cpu %d device\n",
>> + cpu);
>> + continue;
>
> An error, but we continue? Why not dev_dbg level?
>
Moved to dev_dbg.
>> + }
>> +
>> + np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>> + if (!np) {
>> + dev_err(&pdev->dev, "Failed to get freq-domain device\n");
>> + return -EINVAL;
>> + }
>> +
>> + of_node_put(cpu_np);
>> +
>> + ret = qcom_cpu_resources_init(pdev, np, cpu);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>> +{
>> + int rc;
>> +
>> + /* Get the bases of cpufreq for domains */
>> + rc = qcom_resources_init(pdev);
>> + if (rc) {
>> + dev_err(&pdev->dev, "CPUFreq resource init failed\n");
>> + return rc;
>> + }
>> +
>> + rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
>> + if (rc) {
>> + dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
>> + return rc;
>> + }
>> +
>> + dev_info(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
>
> Move to dev_dbg? We have other ways to know if a driver probes
> successfully so the whole line isn't really needed.
>
Moved it to dev_dbg.
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id match_table[] = {
>
> Please call it something besides 'match_table'. qcom_cpufreq_hw_match?
>
Renamed to use 'qcom_cpufreq_hw_match'.
>> + { .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
>> + {}
>> +};
>> +
>> +static struct platform_driver qcom_cpufreq_hw_driver = {
>> + .probe = qcom_cpufreq_hw_driver_probe,
>> + .driver = {
>> + .name = "qcom-cpufreq-hw",
>> + .of_match_table = match_table,
>> + .owner = THIS_MODULE,
>
> platform_driver_register() already assigns this. This should be dropped
> from here.
>
Removed.
>> + },
>> +};
>> +
>> +static int __init qcom_cpufreq_hw_init(void)
>> +{
>> + return platform_driver_register(&qcom_cpufreq_hw_driver);
>> +}
>> +subsys_initcall(qcom_cpufreq_hw_init);
>> +
>> +MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
>> +MODULE_LICENSE("GPL v2");
>
> It should be tristate then in the Kconfig.
>
Removed the module license and want to keep it inbuilt.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
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-13 0:19 ` Matthias Kaehlcke
2018-07-17 5:57 ` Taniya Das
2018-07-16 5:09 ` Viresh Kumar
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Matthias Kaehlcke @ 2018-07-13 0:19 UTC (permalink / raw)
To: Taniya Das
Cc: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm,
Stephen Boyd, Rajendra Nayak, Amit Nischal, devicetree, robh,
skannan, amit.kucheria, evgreen
Hi,
On Thu, Jul 12, 2018 at 11:35:45PM +0530, Taniya Das wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
> drivers/cpufreq/Kconfig.arm | 10 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 355 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 52f5f1a..141ec3e 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -312,3 +312,13 @@ config ARM_PXA2xx_CPUFREQ
> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
> If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_HW
> + bool "QCOM CPUFreq HW driver"
> + help
> + Support for the CPUFreq HW driver.
> + Some QCOM chipsets have a HW engine to offload the steps
> + necessary for changing the frequency of the CPUs. Firmware loaded
> + in this engine exposes a programming interafce to the High-level OS.
> + The driver implements the cpufreq driver interface for this HW engine.
> + Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index fb4a2ec..1226a3e 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ) += tegra124-cpufreq.o
> obj-$(CONFIG_ARM_TEGRA186_CPUFREQ) += tegra186-cpufreq.o
> obj-$(CONFIG_ARM_TI_CPUFREQ) += ti-cpufreq.o
> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW) += qcom-cpufreq-hw.o
>
>
> ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..fa25a95
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,344 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#define INIT_RATE 300000000UL
> +#define XO_RATE 19200000UL
> +#define LUT_MAX_ENTRIES 40U
> +#define CORE_COUNT_VAL(val) (((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE 32
> +
> +enum {
> + REG_ENABLE,
> + REG_LUT_TABLE,
> + REG_PERF_STATE,
> +
> + REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_qcom {
> + struct cpufreq_frequency_table *table;
> + struct device *dev;
> + const u16 *reg_offset;
> + void __iomem *base;
> + cpumask_t related_cpus;
> + unsigned int max_cores;
Same comment as on v4:
Why *max*_cores? This seems to be the number of CPUs in a cluster and
qcom_read_lut() expects the core count read from the LUT to match
exactly. Maybe it's the name from the datasheet? Should it still be
'num_cores' or similer?
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
It would be an option to limit this to the number of CPU clusters and
allocate it dynamically when the driver is initialized (key = first
core in the cluster). Probably not worth the hassle with the limited
number of cores though.
> +static int qcom_read_lut(struct platform_device *pdev,
> + struct cpufreq_qcom *c)
> +{
> + struct device *dev = &pdev->dev;
> + unsigned int offset;
> + u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> + c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> + sizeof(*c->table), GFP_KERNEL);
> + if (!c->table)
> + return -ENOMEM;
> +
> + offset = c->reg_offset[REG_LUT_TABLE];
> +
> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> + data = readl_relaxed(c->base + offset + i * LUT_ROW_SIZE);
> + src = ((data & GENMASK(31, 30)) >> 30);
> + lval = (data & GENMASK(7, 0));
> + core_count = CORE_COUNT_VAL(data);
> +
> + if (src == 0)
> + c->table[i].frequency = INIT_RATE / 1000;
> + else
> + c->table[i].frequency = XO_RATE * lval / 1000;
You changed the condition from '!src' to 'src == 0'. My suggestion on
v4 was in part about a negative condition, but also about the
order. If it doesn't obstruct the code otherwise I think for an if-else
branch it is good practice to handle the more common case first and
then the 'exception'. I would expect most entries to have an actual
rate. Just a nit in any case, feel free to ignore if you prefer as is.
> +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;
If the general code structure remains as is (see my comment below)
the assignment could be done in a 'if (cpu == cpu_r)' branch instead
of first assigning and then overwriting it for 'cpu != cpu_r'.
> +
> + /* 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);
> + }
Couldn't we do this at the beginning of the function instead of going
through allocation, ioremap, read_lut for every core only to throw the
information away later for the 'related' CPUs?
qcom_cpu_resources_init() is called with increasing 'cpu' values, hence the
'first' CPU of the cluster is already initialized when the 'related'
ones are processed.
> + return 0;
> +}
> +
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> + struct device_node *np, *cpu_np;
> + unsigned int cpu;
> + int ret;
> +
> + for_each_possible_cpu(cpu) {
> + cpu_np = of_cpu_device_node_get(cpu);
> + if (!cpu_np) {
> + dev_err(&pdev->dev, "Failed to get cpu %d device\n",
> + cpu);
> + continue;
> + }
> +
> + np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> + if (!np) {
> + dev_err(&pdev->dev, "Failed to get freq-domain device\n");
of_node_put(cpu_np);
> + return -EINVAL;
> + }
> +
> + of_node_put(cpu_np);
> +
> + ret = qcom_cpu_resources_init(pdev, np, cpu);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
Thanks
Matthias
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
2018-07-13 0:19 ` Matthias Kaehlcke
@ 2018-07-17 5:57 ` Taniya Das
0 siblings, 0 replies; 13+ messages in thread
From: Taniya Das @ 2018-07-17 5:57 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm,
Stephen Boyd, Rajendra Nayak, Amit Nischal, devicetree, robh,
skannan, amit.kucheria, evgreen
Hello Matthias,
Thanks for your review comments.
On 7/13/2018 5:49 AM, Matthias Kaehlcke wrote:
> Hi,
>
> On Thu, Jul 12, 2018 at 11:35:45PM +0530, Taniya Das wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>> drivers/cpufreq/Kconfig.arm | 10 ++
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 355 insertions(+)
>> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 52f5f1a..141ec3e 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -312,3 +312,13 @@ config ARM_PXA2xx_CPUFREQ
>> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>> If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_HW
>> + bool "QCOM CPUFreq HW driver"
>> + help
>> + Support for the CPUFreq HW driver.
>> + Some QCOM chipsets have a HW engine to offload the steps
>> + necessary for changing the frequency of the CPUs. Firmware loaded
>> + in this engine exposes a programming interafce to the High-level OS.
>> + The driver implements the cpufreq driver interface for this HW engine.
>> + Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index fb4a2ec..1226a3e 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ) += tegra124-cpufreq.o
>> obj-$(CONFIG_ARM_TEGRA186_CPUFREQ) += tegra186-cpufreq.o
>> obj-$(CONFIG_ARM_TI_CPUFREQ) += ti-cpufreq.o
>> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW) += qcom-cpufreq-hw.o
>>
>>
>> ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..fa25a95
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,344 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/cpufreq.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +
>> +#define INIT_RATE 300000000UL
>> +#define XO_RATE 19200000UL
>> +#define LUT_MAX_ENTRIES 40U
>> +#define CORE_COUNT_VAL(val) (((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE 32
>> +
>> +enum {
>> + REG_ENABLE,
>> + REG_LUT_TABLE,
>> + REG_PERF_STATE,
>> +
>> + REG_ARRAY_SIZE,
>> +};
>> +
>> +struct cpufreq_qcom {
>> + struct cpufreq_frequency_table *table;
>> + struct device *dev;
>> + const u16 *reg_offset;
>> + void __iomem *base;
>> + cpumask_t related_cpus;
>> + unsigned int max_cores;
>
> Same comment as on v4:
>
> Why *max*_cores? This seems to be the number of CPUs in a cluster and
> qcom_read_lut() expects the core count read from the LUT to match
> exactly. Maybe it's the name from the datasheet? Should it still be
> 'num_cores' or similer?
>
Your understanding is correct. I would prefer to leave the naming as
'max_cores'.
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>
> It would be an option to limit this to the number of CPU clusters and
> allocate it dynamically when the driver is initialized (key = first
> core in the cluster). Probably not worth the hassle with the limited
> number of cores though.
>
>> +static int qcom_read_lut(struct platform_device *pdev,
>> + struct cpufreq_qcom *c)
>> +{
>> + struct device *dev = &pdev->dev;
>> + unsigned int offset;
>> + u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
>> +
>> + c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> + sizeof(*c->table), GFP_KERNEL);
>> + if (!c->table)
>> + return -ENOMEM;
>> +
>> + offset = c->reg_offset[REG_LUT_TABLE];
>> +
>> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> + data = readl_relaxed(c->base + offset + i * LUT_ROW_SIZE);
>> + src = ((data & GENMASK(31, 30)) >> 30);
>> + lval = (data & GENMASK(7, 0));
>> + core_count = CORE_COUNT_VAL(data);
>> +
>> + if (src == 0)
>> + c->table[i].frequency = INIT_RATE / 1000;
>> + else
>> + c->table[i].frequency = XO_RATE * lval / 1000;
>
> You changed the condition from '!src' to 'src == 0'. My suggestion on
> v4 was in part about a negative condition, but also about the
> order. If it doesn't obstruct the code otherwise I think for an if-else
> branch it is good practice to handle the more common case first and
> then the 'exception'. I would expect most entries to have an actual
> rate. Just a nit in any case, feel free to ignore if you prefer as is.
>
Thanks, Sure, I would take care of it in the next series.
>> +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;
>
> If the general code structure remains as is (see my comment below)
> the assignment could be done in a 'if (cpu == cpu_r)' branch instead
> of first assigning and then overwriting it for 'cpu != cpu_r'.
>
>> +
>> + /* 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);
>> + }
>
> Couldn't we do this at the beginning of the function instead of going
> through allocation, ioremap, read_lut for every core only to throw the
> information away later for the 'related' CPUs?
>
> qcom_cpu_resources_init() is called with increasing 'cpu' values, hence the
> 'first' CPU of the cluster is already initialized when the 'related'
> ones are processed.
>
I would be moving the code to the beginning of the function.
>> + return 0;
>> +}
>> +
>> +static int qcom_resources_init(struct platform_device *pdev)
>> +{
>> + struct device_node *np, *cpu_np;
>> + unsigned int cpu;
>> + int ret;
>> +
>> + for_each_possible_cpu(cpu) {
>> + cpu_np = of_cpu_device_node_get(cpu);
>> + if (!cpu_np) {
>> + dev_err(&pdev->dev, "Failed to get cpu %d device\n",
>> + cpu);
>> + continue;
>> + }
>> +
>> + np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>> + if (!np) {
>> + dev_err(&pdev->dev, "Failed to get freq-domain device\n");
>
> of_node_put(cpu_np);
>
>> + return -EINVAL;
>> + }
>> +
>> + of_node_put(cpu_np);
>> +
>> + ret = qcom_cpu_resources_init(pdev, np, cpu);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>
> Thanks
>
> Matthias
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
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-13 0:19 ` Matthias Kaehlcke
@ 2018-07-16 5:09 ` Viresh Kumar
2018-07-16 5:12 ` Viresh Kumar
2018-07-16 23:02 ` Evan Green
4 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2018-07-16 5:09 UTC (permalink / raw)
To: Taniya Das
Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd,
Rajendra Nayak, Amit Nischal, devicetree, robh, skannan,
amit.kucheria, evgreen
On 12-07-18, 23:35, Taniya Das wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
> drivers/cpufreq/Kconfig.arm | 10 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 355 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
Have you addressed my concerns that I raised for this patch in the
previous version ? You never replied to me as well.
--
viresh
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
2018-07-12 18:05 ` [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
` (2 preceding siblings ...)
2018-07-16 5:09 ` Viresh Kumar
@ 2018-07-16 5:12 ` Viresh Kumar
2018-07-17 2:59 ` Taniya Das
2018-07-16 23:02 ` Evan Green
4 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2018-07-16 5:12 UTC (permalink / raw)
To: Taniya Das
Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd,
Rajendra Nayak, Amit Nischal, devicetree, robh, skannan,
amit.kucheria, evgreen
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 ?
--
viresh
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
2018-07-16 5:12 ` Viresh Kumar
@ 2018-07-17 2:59 ` Taniya Das
0 siblings, 0 replies; 13+ messages in thread
From: Taniya Das @ 2018-07-17 2:59 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd,
Rajendra Nayak, Amit Nischal, devicetree, robh, skannan,
amit.kucheria, evgreen
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.
--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
2018-07-12 18:05 ` [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
` (3 preceding siblings ...)
2018-07-16 5:12 ` Viresh Kumar
@ 2018-07-16 23:02 ` Evan Green
2018-07-17 2:50 ` Taniya Das
4 siblings, 1 reply; 13+ messages in thread
From: Evan Green @ 2018-07-16 23:02 UTC (permalink / raw)
To: tdas
Cc: rjw, viresh.kumar, linux-kernel, linux-pm, sboyd, Rajendra Nayak,
anischal, devicetree, robh, Saravana Kannan, amit.kucheria
Hi Taniya,
On Thu, Jul 12, 2018 at 11:06 AM Taniya Das <tdas@codeaurora.org> wrote:
>
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
> drivers/cpufreq/Kconfig.arm | 10 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 355 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
...
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..fa25a95
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
...
> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_qcom *c;
> +
> + c = qcom_freq_domain_map[policy->cpu];
> + if (!c) {
> + pr_err("No scaling support for CPU%d\n", policy->cpu);
> + return -ENODEV;
> + }
> +
> + cpumask_copy(policy->cpus, &c->related_cpus);
> +
> + policy->fast_switch_possible = true;
> + policy->freq_table = c->table;
> + policy->driver_data = c;
> +
> + return 0;
I haven't looked at this driver in detail, but I have tested it.
Instead of the line above, I needed:
return cpufreq_table_validate_and_show(policy, c->table);
Without this the framework thinks that the min and max frequencies are
zero, and then you get a warning about an invalid table. I also
removed "policy->freq_table = c->table", since validate_and_show does
this.
-Evan
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v5 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
2018-07-16 23:02 ` Evan Green
@ 2018-07-17 2:50 ` Taniya Das
0 siblings, 0 replies; 13+ messages in thread
From: Taniya Das @ 2018-07-17 2:50 UTC (permalink / raw)
To: Evan Green
Cc: rjw, viresh.kumar, linux-kernel, linux-pm, sboyd, Rajendra Nayak,
anischal, devicetree, robh, Saravana Kannan, amit.kucheria
Hello Evan,
Thank you for testing the patch.
On 7/17/2018 4:32 AM, Evan Green wrote:
> Hi Taniya,
> On Thu, Jul 12, 2018 at 11:06 AM Taniya Das <tdas@codeaurora.org> wrote:
>>
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>> drivers/cpufreq/Kconfig.arm | 10 ++
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 355 insertions(+)
>> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
> ...
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..fa25a95
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> ...
>> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>> +{
>> + struct cpufreq_qcom *c;
>> +
>> + c = qcom_freq_domain_map[policy->cpu];
>> + if (!c) {
>> + pr_err("No scaling support for CPU%d\n", policy->cpu);
>> + return -ENODEV;
>> + }
>> +
>> + cpumask_copy(policy->cpus, &c->related_cpus);
>> +
>> + policy->fast_switch_possible = true;
>> + policy->freq_table = c->table;
>> + policy->driver_data = c;
>> +
>> + return 0;
> I haven't looked at this driver in detail, but I have tested it.
> Instead of the line above, I needed:
>
> return cpufreq_table_validate_and_show(policy, c->table);
>
> Without this the framework thinks that the min and max frequencies are
> zero, and then you get a warning about an invalid table. I also
> removed "policy->freq_table = c->table", since validate_and_show does
> this.
> -Evan
>
The API is no longer supported,
https://patchwork.kernel.org/patch/10320897/
If you are using the 4.14 kernel, you do need to replace
-policy->freq_table = c->table;
+cpufreq_table_validate_and_show(policy, c->table);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
^ permalink raw reply [flat|nested] 13+ messages in thread