* [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
@ 2013-03-28 9:55 ` Yuantian.Tang
0 siblings, 0 replies; 13+ messages in thread
From: Yuantian.Tang @ 2013-03-28 9:55 UTC (permalink / raw)
To: rjw
Cc: cpufreq, linux-pm, linuxppc-dev, viresh.kumar, Tang Yuantian,
Tang Yuantian, Li Yang
From: Tang Yuantian <yuantian.tang@freescale.com>
Add cpufreq driver for Freescale e500mc, e5500 and e6500 SoCs
which are capable of changing the frequency of CPU dynamically
Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
v2:
- change the per_cpu variable to point type
- fixed other issues
drivers/cpufreq/Kconfig.powerpc | 10 ++
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/ppc-corenet-cpufreq.c | 255 ++++++++++++++++++++++++++++++++++
3 files changed, 266 insertions(+)
create mode 100644 drivers/cpufreq/ppc-corenet-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.powerpc b/drivers/cpufreq/Kconfig.powerpc
index e76992f..3a0d8d0 100644
--- a/drivers/cpufreq/Kconfig.powerpc
+++ b/drivers/cpufreq/Kconfig.powerpc
@@ -5,3 +5,13 @@ config CPU_FREQ_MAPLE
help
This adds support for frequency switching on Maple 970FX
Evaluation Board and compatible boards (IBM JS2x blades).
+
+config PPC_CORENET_CPUFREQ
+ tristate "CPU frequency scaling driver for Freescale E500MC SoCs"
+ depends on PPC_E500MC && OF && COMMON_CLK
+ select CPU_FREQ_TABLE
+ select CLK_PPC_CORENET
+ help
+ This adds the CPUFreq driver support for Freescale e500mc,
+ e5500 and e6500 series SoCs which are capable of changing
+ the CPU's frequency dynamically.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 863fd18..2416559 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -61,3 +61,4 @@ obj-$(CONFIG_ARM_IMX6Q_CPUFREQ) += imx6q-cpufreq.o
##################################################################################
# PowerPC platform drivers
obj-$(CONFIG_CPU_FREQ_MAPLE) += maple-cpufreq.o
+obj-$(CONFIG_PPC_CORENET_CPUFREQ) += ppc-corenet-cpufreq.o
diff --git a/drivers/cpufreq/ppc-corenet-cpufreq.c b/drivers/cpufreq/ppc-corenet-cpufreq.c
new file mode 100644
index 0000000..a50ab5a
--- /dev/null
+++ b/drivers/cpufreq/ppc-corenet-cpufreq.c
@@ -0,0 +1,255 @@
+/*
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * CPU Frequency Scaling driver for Freescale PowerPC corenet SoCs.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+/**
+ * struct cpufreq_data - cpufreq driver data
+ * @cpus_per_cluster: CPU numbers per cluster
+ * @cpufreq_lock: the mutex lock
+ */
+struct cpufreq_data {
+ int cpus_per_cluster;
+ struct mutex cpufreq_lock;
+};
+
+/**
+ * struct cpu_data - per CPU data struct
+ * @clk: the clk data of CPU
+ * @parent: the parent node of clock of cpu
+ * @table: frequency table point
+ */
+struct cpu_data {
+ struct clk *clk;
+ struct device_node *parent;
+ struct cpufreq_frequency_table *table;
+};
+
+static DEFINE_PER_CPU(struct cpu_data *, cpu_data);
+static struct cpufreq_data freq_data;
+
+static unsigned int corenet_cpufreq_get_speed(unsigned int cpu)
+{
+ struct cpu_data *data = per_cpu(cpu_data, cpu);
+
+ return clk_get_rate(data->clk) / 1000;
+}
+
+/* reduce the duplicated frequency in frequency table */
+static void freq_table_redup(struct cpufreq_frequency_table *freq_table,
+ int count)
+{
+ int i, j;
+
+ for (i = 1; i < count; i++) {
+ for (j = 0; j < i; j++) {
+ if (freq_table[j].frequency == CPUFREQ_ENTRY_INVALID ||
+ freq_table[j].frequency !=
+ freq_table[i].frequency)
+ continue;
+
+ freq_table[i].frequency = CPUFREQ_ENTRY_INVALID;
+ break;
+ }
+ }
+}
+
+static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+ unsigned int cpu = policy->cpu;
+ struct device_node *np;
+ int i, count;
+ struct clk *clk;
+ struct cpufreq_frequency_table *table;
+ struct cpu_data *data;
+
+ np = of_get_cpu_node(cpu, NULL);
+ if (!np)
+ return -ENODEV;
+
+ data = kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->clk = of_clk_get(np, 0);
+ data->parent = of_parse_phandle(np, "clocks", 0);
+ if (!data->parent) {
+ pr_err("%s: could not get clock information\n", __func__);
+ goto err_nomem2;
+ }
+
+ count = of_property_count_strings(data->parent, "clock-names");
+
+ table = kcalloc(count + 1,
+ sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
+ if (!table) {
+ pr_err("%s: no memory\n", __func__);
+ goto err_nomem2;
+ }
+
+ for (i = cpu; i < freq_data.cpus_per_cluster + cpu; i++)
+ cpumask_set_cpu(i, policy->cpus);
+
+ for (i = 0; i < count; i++) {
+ table[i].index = i;
+ clk = of_clk_get(data->parent, i);
+ table[i].frequency = clk_get_rate(clk) / 1000;
+ }
+ freq_table_redup(table, count);
+ table[i].frequency = CPUFREQ_TABLE_END;
+
+ data->table = table;
+ per_cpu(cpu_data, cpu) = data;
+
+ policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
+ policy->cur = corenet_cpufreq_get_speed(policy->cpu);
+
+ /* set the min and max frequency properly */
+ i = cpufreq_frequency_table_cpuinfo(policy, table);
+ if (i) {
+ pr_err("invalid frequency table: %d\n", i);
+ goto err_nomem1;
+ }
+ cpufreq_frequency_table_get_attr(table, cpu);
+
+ return 0;
+
+err_nomem1:
+ kfree(table);
+err_nomem2:
+ per_cpu(cpu_data, cpu) = NULL;
+ kfree(data);
+
+ return -ENODEV;
+}
+
+static int __exit corenet_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+ struct cpu_data *data = per_cpu(cpu_data, policy->cpu);
+
+ cpufreq_frequency_table_put_attr(policy->cpu);
+ kfree(data->table);
+ kfree(data);
+
+ return 0;
+}
+
+static int corenet_cpufreq_verify(struct cpufreq_policy *policy)
+{
+ struct cpufreq_frequency_table *table;
+
+ table = per_cpu(cpu_data, policy->cpu)->table;
+
+ return cpufreq_frequency_table_verify(policy, table);
+}
+
+static int corenet_cpufreq_target(struct cpufreq_policy *policy,
+ unsigned int target_freq, unsigned int relation)
+{
+ struct cpufreq_freqs freqs;
+ unsigned int new;
+ struct clk *parent;
+ int ret;
+ struct cpu_data *data = per_cpu(cpu_data, policy->cpu);
+
+ cpufreq_frequency_table_target(policy, data->table,
+ target_freq, relation, &new);
+
+ if (policy->cur == data->table[new].frequency)
+ return 0;
+
+ freqs.old = policy->cur;
+ freqs.new = data->table[new].frequency;
+ freqs.cpu = policy->cpu;
+
+ mutex_lock(&freq_data.cpufreq_lock);
+ cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+
+ parent = of_clk_get(data->parent, new);
+ ret = clk_set_parent(data->clk, parent);
+ if (ret) {
+ mutex_unlock(&freq_data.cpufreq_lock);
+ return ret;
+ }
+
+ cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+ mutex_unlock(&freq_data.cpufreq_lock);
+
+ return 0;
+}
+
+static struct freq_attr *corenet_cpu_clks_attr[] = {
+ &cpufreq_freq_attr_scaling_available_freqs,
+ NULL,
+};
+
+static struct cpufreq_driver ppc_corenet_cpufreq_driver = {
+ .name = "ppc_cpufreq",
+ .owner = THIS_MODULE,
+ .flags = CPUFREQ_CONST_LOOPS,
+ .init = corenet_cpufreq_cpu_init,
+ .exit = __exit_p(corenet_cpufreq_cpu_exit),
+ .verify = corenet_cpufreq_verify,
+ .target = corenet_cpufreq_target,
+ .get = corenet_cpufreq_get_speed,
+ .attr = corenet_cpu_clks_attr,
+};
+
+static const struct of_device_id node_matches[] __initconst = {
+ { .compatible = "fsl,qoriq-clockgen-1.0", .data = (void *)1, },
+ { .compatible = "fsl,qoriq-clockgen-2", .data = (void *)8, },
+ {}
+};
+
+static int __init ppc_corenet_cpufreq_init(void)
+{
+ int ret = 0;
+ struct device_node *np;
+ const struct of_device_id *match;
+
+ np = of_find_matching_node(NULL, node_matches);
+ if (!np)
+ return -ENODEV;
+
+ match = of_match_node(node_matches, np);
+ freq_data.cpus_per_cluster = (unsigned long)match->data;
+ mutex_init(&freq_data.cpufreq_lock);
+ of_node_put(np);
+
+ ret = cpufreq_register_driver(&ppc_corenet_cpufreq_driver);
+ if (ret)
+ return ret;
+
+ pr_info("Freescale PowerPC corenet CPU frequency scaling driver\n");
+
+ return ret;
+}
+module_init(ppc_corenet_cpufreq_init);
+
+static void __exit ppc_corenet_cpufreq_exit(void)
+{
+ cpufreq_unregister_driver(&ppc_corenet_cpufreq_driver);
+}
+module_exit(ppc_corenet_cpufreq_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tang Yuantian <Yuantian.Tang@freescale.com>");
+MODULE_DESCRIPTION("cpufreq driver for Freescale e500mc series SoCs");
--
1.8.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
@ 2013-03-28 9:55 ` Yuantian.Tang
0 siblings, 0 replies; 13+ messages in thread
From: Yuantian.Tang @ 2013-03-28 9:55 UTC (permalink / raw)
To: rjw; +Cc: linux-pm, viresh.kumar, cpufreq, Tang Yuantian, linuxppc-dev
From: Tang Yuantian <yuantian.tang@freescale.com>
Add cpufreq driver for Freescale e500mc, e5500 and e6500 SoCs
which are capable of changing the frequency of CPU dynamically
Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
v2:
- change the per_cpu variable to point type
- fixed other issues
drivers/cpufreq/Kconfig.powerpc | 10 ++
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/ppc-corenet-cpufreq.c | 255 ++++++++++++++++++++++++++++++++++
3 files changed, 266 insertions(+)
create mode 100644 drivers/cpufreq/ppc-corenet-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.powerpc b/drivers/cpufreq/Kconfig.powerpc
index e76992f..3a0d8d0 100644
--- a/drivers/cpufreq/Kconfig.powerpc
+++ b/drivers/cpufreq/Kconfig.powerpc
@@ -5,3 +5,13 @@ config CPU_FREQ_MAPLE
help
This adds support for frequency switching on Maple 970FX
Evaluation Board and compatible boards (IBM JS2x blades).
+
+config PPC_CORENET_CPUFREQ
+ tristate "CPU frequency scaling driver for Freescale E500MC SoCs"
+ depends on PPC_E500MC && OF && COMMON_CLK
+ select CPU_FREQ_TABLE
+ select CLK_PPC_CORENET
+ help
+ This adds the CPUFreq driver support for Freescale e500mc,
+ e5500 and e6500 series SoCs which are capable of changing
+ the CPU's frequency dynamically.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 863fd18..2416559 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -61,3 +61,4 @@ obj-$(CONFIG_ARM_IMX6Q_CPUFREQ) += imx6q-cpufreq.o
##################################################################################
# PowerPC platform drivers
obj-$(CONFIG_CPU_FREQ_MAPLE) += maple-cpufreq.o
+obj-$(CONFIG_PPC_CORENET_CPUFREQ) += ppc-corenet-cpufreq.o
diff --git a/drivers/cpufreq/ppc-corenet-cpufreq.c b/drivers/cpufreq/ppc-corenet-cpufreq.c
new file mode 100644
index 0000000..a50ab5a
--- /dev/null
+++ b/drivers/cpufreq/ppc-corenet-cpufreq.c
@@ -0,0 +1,255 @@
+/*
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * CPU Frequency Scaling driver for Freescale PowerPC corenet SoCs.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+/**
+ * struct cpufreq_data - cpufreq driver data
+ * @cpus_per_cluster: CPU numbers per cluster
+ * @cpufreq_lock: the mutex lock
+ */
+struct cpufreq_data {
+ int cpus_per_cluster;
+ struct mutex cpufreq_lock;
+};
+
+/**
+ * struct cpu_data - per CPU data struct
+ * @clk: the clk data of CPU
+ * @parent: the parent node of clock of cpu
+ * @table: frequency table point
+ */
+struct cpu_data {
+ struct clk *clk;
+ struct device_node *parent;
+ struct cpufreq_frequency_table *table;
+};
+
+static DEFINE_PER_CPU(struct cpu_data *, cpu_data);
+static struct cpufreq_data freq_data;
+
+static unsigned int corenet_cpufreq_get_speed(unsigned int cpu)
+{
+ struct cpu_data *data = per_cpu(cpu_data, cpu);
+
+ return clk_get_rate(data->clk) / 1000;
+}
+
+/* reduce the duplicated frequency in frequency table */
+static void freq_table_redup(struct cpufreq_frequency_table *freq_table,
+ int count)
+{
+ int i, j;
+
+ for (i = 1; i < count; i++) {
+ for (j = 0; j < i; j++) {
+ if (freq_table[j].frequency == CPUFREQ_ENTRY_INVALID ||
+ freq_table[j].frequency !=
+ freq_table[i].frequency)
+ continue;
+
+ freq_table[i].frequency = CPUFREQ_ENTRY_INVALID;
+ break;
+ }
+ }
+}
+
+static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+ unsigned int cpu = policy->cpu;
+ struct device_node *np;
+ int i, count;
+ struct clk *clk;
+ struct cpufreq_frequency_table *table;
+ struct cpu_data *data;
+
+ np = of_get_cpu_node(cpu, NULL);
+ if (!np)
+ return -ENODEV;
+
+ data = kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->clk = of_clk_get(np, 0);
+ data->parent = of_parse_phandle(np, "clocks", 0);
+ if (!data->parent) {
+ pr_err("%s: could not get clock information\n", __func__);
+ goto err_nomem2;
+ }
+
+ count = of_property_count_strings(data->parent, "clock-names");
+
+ table = kcalloc(count + 1,
+ sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
+ if (!table) {
+ pr_err("%s: no memory\n", __func__);
+ goto err_nomem2;
+ }
+
+ for (i = cpu; i < freq_data.cpus_per_cluster + cpu; i++)
+ cpumask_set_cpu(i, policy->cpus);
+
+ for (i = 0; i < count; i++) {
+ table[i].index = i;
+ clk = of_clk_get(data->parent, i);
+ table[i].frequency = clk_get_rate(clk) / 1000;
+ }
+ freq_table_redup(table, count);
+ table[i].frequency = CPUFREQ_TABLE_END;
+
+ data->table = table;
+ per_cpu(cpu_data, cpu) = data;
+
+ policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
+ policy->cur = corenet_cpufreq_get_speed(policy->cpu);
+
+ /* set the min and max frequency properly */
+ i = cpufreq_frequency_table_cpuinfo(policy, table);
+ if (i) {
+ pr_err("invalid frequency table: %d\n", i);
+ goto err_nomem1;
+ }
+ cpufreq_frequency_table_get_attr(table, cpu);
+
+ return 0;
+
+err_nomem1:
+ kfree(table);
+err_nomem2:
+ per_cpu(cpu_data, cpu) = NULL;
+ kfree(data);
+
+ return -ENODEV;
+}
+
+static int __exit corenet_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+ struct cpu_data *data = per_cpu(cpu_data, policy->cpu);
+
+ cpufreq_frequency_table_put_attr(policy->cpu);
+ kfree(data->table);
+ kfree(data);
+
+ return 0;
+}
+
+static int corenet_cpufreq_verify(struct cpufreq_policy *policy)
+{
+ struct cpufreq_frequency_table *table;
+
+ table = per_cpu(cpu_data, policy->cpu)->table;
+
+ return cpufreq_frequency_table_verify(policy, table);
+}
+
+static int corenet_cpufreq_target(struct cpufreq_policy *policy,
+ unsigned int target_freq, unsigned int relation)
+{
+ struct cpufreq_freqs freqs;
+ unsigned int new;
+ struct clk *parent;
+ int ret;
+ struct cpu_data *data = per_cpu(cpu_data, policy->cpu);
+
+ cpufreq_frequency_table_target(policy, data->table,
+ target_freq, relation, &new);
+
+ if (policy->cur == data->table[new].frequency)
+ return 0;
+
+ freqs.old = policy->cur;
+ freqs.new = data->table[new].frequency;
+ freqs.cpu = policy->cpu;
+
+ mutex_lock(&freq_data.cpufreq_lock);
+ cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+
+ parent = of_clk_get(data->parent, new);
+ ret = clk_set_parent(data->clk, parent);
+ if (ret) {
+ mutex_unlock(&freq_data.cpufreq_lock);
+ return ret;
+ }
+
+ cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+ mutex_unlock(&freq_data.cpufreq_lock);
+
+ return 0;
+}
+
+static struct freq_attr *corenet_cpu_clks_attr[] = {
+ &cpufreq_freq_attr_scaling_available_freqs,
+ NULL,
+};
+
+static struct cpufreq_driver ppc_corenet_cpufreq_driver = {
+ .name = "ppc_cpufreq",
+ .owner = THIS_MODULE,
+ .flags = CPUFREQ_CONST_LOOPS,
+ .init = corenet_cpufreq_cpu_init,
+ .exit = __exit_p(corenet_cpufreq_cpu_exit),
+ .verify = corenet_cpufreq_verify,
+ .target = corenet_cpufreq_target,
+ .get = corenet_cpufreq_get_speed,
+ .attr = corenet_cpu_clks_attr,
+};
+
+static const struct of_device_id node_matches[] __initconst = {
+ { .compatible = "fsl,qoriq-clockgen-1.0", .data = (void *)1, },
+ { .compatible = "fsl,qoriq-clockgen-2", .data = (void *)8, },
+ {}
+};
+
+static int __init ppc_corenet_cpufreq_init(void)
+{
+ int ret = 0;
+ struct device_node *np;
+ const struct of_device_id *match;
+
+ np = of_find_matching_node(NULL, node_matches);
+ if (!np)
+ return -ENODEV;
+
+ match = of_match_node(node_matches, np);
+ freq_data.cpus_per_cluster = (unsigned long)match->data;
+ mutex_init(&freq_data.cpufreq_lock);
+ of_node_put(np);
+
+ ret = cpufreq_register_driver(&ppc_corenet_cpufreq_driver);
+ if (ret)
+ return ret;
+
+ pr_info("Freescale PowerPC corenet CPU frequency scaling driver\n");
+
+ return ret;
+}
+module_init(ppc_corenet_cpufreq_init);
+
+static void __exit ppc_corenet_cpufreq_exit(void)
+{
+ cpufreq_unregister_driver(&ppc_corenet_cpufreq_driver);
+}
+module_exit(ppc_corenet_cpufreq_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tang Yuantian <Yuantian.Tang@freescale.com>");
+MODULE_DESCRIPTION("cpufreq driver for Freescale e500mc series SoCs");
--
1.8.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
2013-03-28 9:55 ` Yuantian.Tang
@ 2013-03-28 14:54 ` Viresh Kumar
-1 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2013-03-28 14:54 UTC (permalink / raw)
To: Yuantian.Tang; +Cc: rjw, cpufreq, linux-pm, linuxppc-dev, Li Yang
On 28 March 2013 15:25, <Yuantian.Tang@freescale.com> wrote:
> From: Tang Yuantian <yuantian.tang@freescale.com>
>
> Add cpufreq driver for Freescale e500mc, e5500 and e6500 SoCs
> which are capable of changing the frequency of CPU dynamically
>
> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> v2:
> - change the per_cpu variable to point type
> - fixed other issues
This changelog is pretty important and useful. People forget what comments
they gave on earlier version.
> diff --git a/drivers/cpufreq/ppc-corenet-cpufreq.c b/drivers/cpufreq/ppc-corenet-cpufreq.c
> +/**
> + * struct cpufreq_data - cpufreq driver data
> + * @cpus_per_cluster: CPU numbers per cluster
> + * @cpufreq_lock: the mutex lock
rather add what's its purpose and what it protects.
> + */
> +struct cpufreq_data {
> + int cpus_per_cluster;
> + struct mutex cpufreq_lock;
> +};
You actually need a struct for this? Simply create globals ?
> +/**
> + * struct cpu_data - per CPU data struct
> + * @clk: the clk data of CPU
remove "data"
> + * @parent: the parent node of clock of cpu
s/clock of cpu/cpu clock
> + * @table: frequency table point
point? you mean pointer? Just remove it.
> +struct cpu_data {
> + struct clk *clk;
> + struct device_node *parent;
> + struct cpufreq_frequency_table *table;
> +};
> +
> +static DEFINE_PER_CPU(struct cpu_data *, cpu_data);
> +static struct cpufreq_data freq_data;
> +
> +static unsigned int corenet_cpufreq_get_speed(unsigned int cpu)
> +{
> + struct cpu_data *data = per_cpu(cpu_data, cpu);
> +
> + return clk_get_rate(data->clk) / 1000;
> +}
> +
> +/* reduce the duplicated frequency in frequency table */
> +static void freq_table_redup(struct cpufreq_frequency_table *freq_table,
> + int count)
> +{
> + int i, j;
> +
> + for (i = 1; i < count; i++) {
> + for (j = 0; j < i; j++) {
> + if (freq_table[j].frequency == CPUFREQ_ENTRY_INVALID ||
> + freq_table[j].frequency !=
> + freq_table[i].frequency)
> + continue;
> +
> + freq_table[i].frequency = CPUFREQ_ENTRY_INVALID;
> + break;
> + }
> + }
> +}
Looks better now :)
> +static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> + unsigned int cpu = policy->cpu;
> + struct device_node *np;
> + int i, count;
> + struct clk *clk;
> + struct cpufreq_frequency_table *table;
> + struct cpu_data *data;
> +
> + np = of_get_cpu_node(cpu, NULL);
> + if (!np)
> + return -ENODEV;
> +
> + data = kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
I told you, you missed my comment earlier.
You need to write: sizeof(*data) :(
> + if (!data)
> + return -ENOMEM;
> +
> + data->clk = of_clk_get(np, 0);
> + data->parent = of_parse_phandle(np, "clocks", 0);
> + if (!data->parent) {
> + pr_err("%s: could not get clock information\n", __func__);
> + goto err_nomem2;
> + }
> +
> + count = of_property_count_strings(data->parent, "clock-names");
> +
> + table = kcalloc(count + 1,
> + sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
sizeof(*table)
> + if (!table) {
> + pr_err("%s: no memory\n", __func__);
> + goto err_nomem2;
> + }
> +
> + for (i = cpu; i < freq_data.cpus_per_cluster + cpu; i++)
> + cpumask_set_cpu(i, policy->cpus);
I can see some regression here. Suppose you have two clusters of 4
cpus each: (0123) and (4567).. Now at boot time above code will work
perfectly fine. Now you hot unplug 0,1,2,3 and then hotplug 3 in.
Here, init would be called for cpu 3 and so you will end up saving 3456
in your policy->cpus
:)
> + for (i = 0; i < count; i++) {
> + table[i].index = i;
> + clk = of_clk_get(data->parent, i);
> + table[i].frequency = clk_get_rate(clk) / 1000;
> + }
> + freq_table_redup(table, count);
> + table[i].frequency = CPUFREQ_TABLE_END;
> +
> + data->table = table;
> + per_cpu(cpu_data, cpu) = data;
> +
> + policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> + policy->cur = corenet_cpufreq_get_speed(policy->cpu);
> +
> + /* set the min and max frequency properly */
> + i = cpufreq_frequency_table_cpuinfo(policy, table);
i doesn't suit here, use variable like ret or err..
And move this just after the line where you set freq as TABLE_END.
So that you don't execute unnecessary code for error case.
> +static int corenet_cpufreq_verify(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_frequency_table *table;
> +
> + table = per_cpu(cpu_data, policy->cpu)->table;
merge above two lines.
> + return cpufreq_frequency_table_verify(policy, table);
> +}
> +
> +static int corenet_cpufreq_target(struct cpufreq_policy *policy,
> + unsigned int target_freq, unsigned int relation)
> +{
> + struct cpufreq_freqs freqs;
> + unsigned int new;
> + struct clk *parent;
> + int ret;
> + struct cpu_data *data = per_cpu(cpu_data, policy->cpu);
> +
> + cpufreq_frequency_table_target(policy, data->table,
> + target_freq, relation, &new);
> +
> + if (policy->cur == data->table[new].frequency)
> + return 0;
> +
> + freqs.old = policy->cur;
> + freqs.new = data->table[new].frequency;
> + freqs.cpu = policy->cpu;
> +
> + mutex_lock(&freq_data.cpufreq_lock);
> + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
You need to notify freq change for all cpus in this group :)
BUT you don't need to worry about it due to this:
http://patches.linaro.org/15569/
> + parent = of_clk_get(data->parent, new);
> + ret = clk_set_parent(data->clk, parent);
> + if (ret) {
> + mutex_unlock(&freq_data.cpufreq_lock);
Probably for error case too you need to notify others about completion
of freq change, but with old freq.
> + return ret;
> + }
> +
> + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> + mutex_unlock(&freq_data.cpufreq_lock);
> +
> + return 0;
> +}
> +
> +static struct freq_attr *corenet_cpu_clks_attr[] = {
name it better, maybe corenet_cpufreq_attr ?
> + &cpufreq_freq_attr_scaling_available_freqs,
> + NULL,
> +};
> +
> +static struct cpufreq_driver ppc_corenet_cpufreq_driver = {
> + .name = "ppc_cpufreq",
> + .owner = THIS_MODULE,
> + .flags = CPUFREQ_CONST_LOOPS,
> + .init = corenet_cpufreq_cpu_init,
> + .exit = __exit_p(corenet_cpufreq_cpu_exit),
> + .verify = corenet_cpufreq_verify,
> + .target = corenet_cpufreq_target,
> + .get = corenet_cpufreq_get_speed,
> + .attr = corenet_cpu_clks_attr,
> +};
> +
> +static const struct of_device_id node_matches[] __initconst = {
> + { .compatible = "fsl,qoriq-clockgen-1.0", .data = (void *)1, },
> + { .compatible = "fsl,qoriq-clockgen-2", .data = (void *)8, },
> + {}
> +};
> +
> +static int __init ppc_corenet_cpufreq_init(void)
> +{
> + int ret = 0;
> + struct device_node *np;
> + const struct of_device_id *match;
> +
> + np = of_find_matching_node(NULL, node_matches);
> + if (!np)
> + return -ENODEV;
> +
> + match = of_match_node(node_matches, np);
> + freq_data.cpus_per_cluster = (unsigned long)match->data;
> + mutex_init(&freq_data.cpufreq_lock);
> + of_node_put(np);
> +
> + ret = cpufreq_register_driver(&ppc_corenet_cpufreq_driver);
> + if (ret)
> + return ret;
> +
> + pr_info("Freescale PowerPC corenet CPU frequency scaling driver\n");
Or write it as:
ret = cpufreq_register_driver();
if (!ret)
pr_info();
return ret;
> +
> + return ret;
> +}
> +module_init(ppc_corenet_cpufreq_init);
> +
> +static void __exit ppc_corenet_cpufreq_exit(void)
> +{
> + cpufreq_unregister_driver(&ppc_corenet_cpufreq_driver);
> +}
> +module_exit(ppc_corenet_cpufreq_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Tang Yuantian <Yuantian.Tang@freescale.com>");
> +MODULE_DESCRIPTION("cpufreq driver for Freescale e500mc series SoCs");
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
@ 2013-03-28 14:54 ` Viresh Kumar
0 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2013-03-28 14:54 UTC (permalink / raw)
To: Yuantian.Tang; +Cc: rjw, linuxppc-dev, cpufreq, linux-pm
On 28 March 2013 15:25, <Yuantian.Tang@freescale.com> wrote:
> From: Tang Yuantian <yuantian.tang@freescale.com>
>
> Add cpufreq driver for Freescale e500mc, e5500 and e6500 SoCs
> which are capable of changing the frequency of CPU dynamically
>
> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> v2:
> - change the per_cpu variable to point type
> - fixed other issues
This changelog is pretty important and useful. People forget what comments
they gave on earlier version.
> diff --git a/drivers/cpufreq/ppc-corenet-cpufreq.c b/drivers/cpufreq/ppc-corenet-cpufreq.c
> +/**
> + * struct cpufreq_data - cpufreq driver data
> + * @cpus_per_cluster: CPU numbers per cluster
> + * @cpufreq_lock: the mutex lock
rather add what's its purpose and what it protects.
> + */
> +struct cpufreq_data {
> + int cpus_per_cluster;
> + struct mutex cpufreq_lock;
> +};
You actually need a struct for this? Simply create globals ?
> +/**
> + * struct cpu_data - per CPU data struct
> + * @clk: the clk data of CPU
remove "data"
> + * @parent: the parent node of clock of cpu
s/clock of cpu/cpu clock
> + * @table: frequency table point
point? you mean pointer? Just remove it.
> +struct cpu_data {
> + struct clk *clk;
> + struct device_node *parent;
> + struct cpufreq_frequency_table *table;
> +};
> +
> +static DEFINE_PER_CPU(struct cpu_data *, cpu_data);
> +static struct cpufreq_data freq_data;
> +
> +static unsigned int corenet_cpufreq_get_speed(unsigned int cpu)
> +{
> + struct cpu_data *data = per_cpu(cpu_data, cpu);
> +
> + return clk_get_rate(data->clk) / 1000;
> +}
> +
> +/* reduce the duplicated frequency in frequency table */
> +static void freq_table_redup(struct cpufreq_frequency_table *freq_table,
> + int count)
> +{
> + int i, j;
> +
> + for (i = 1; i < count; i++) {
> + for (j = 0; j < i; j++) {
> + if (freq_table[j].frequency == CPUFREQ_ENTRY_INVALID ||
> + freq_table[j].frequency !=
> + freq_table[i].frequency)
> + continue;
> +
> + freq_table[i].frequency = CPUFREQ_ENTRY_INVALID;
> + break;
> + }
> + }
> +}
Looks better now :)
> +static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> + unsigned int cpu = policy->cpu;
> + struct device_node *np;
> + int i, count;
> + struct clk *clk;
> + struct cpufreq_frequency_table *table;
> + struct cpu_data *data;
> +
> + np = of_get_cpu_node(cpu, NULL);
> + if (!np)
> + return -ENODEV;
> +
> + data = kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
I told you, you missed my comment earlier.
You need to write: sizeof(*data) :(
> + if (!data)
> + return -ENOMEM;
> +
> + data->clk = of_clk_get(np, 0);
> + data->parent = of_parse_phandle(np, "clocks", 0);
> + if (!data->parent) {
> + pr_err("%s: could not get clock information\n", __func__);
> + goto err_nomem2;
> + }
> +
> + count = of_property_count_strings(data->parent, "clock-names");
> +
> + table = kcalloc(count + 1,
> + sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
sizeof(*table)
> + if (!table) {
> + pr_err("%s: no memory\n", __func__);
> + goto err_nomem2;
> + }
> +
> + for (i = cpu; i < freq_data.cpus_per_cluster + cpu; i++)
> + cpumask_set_cpu(i, policy->cpus);
I can see some regression here. Suppose you have two clusters of 4
cpus each: (0123) and (4567).. Now at boot time above code will work
perfectly fine. Now you hot unplug 0,1,2,3 and then hotplug 3 in.
Here, init would be called for cpu 3 and so you will end up saving 3456
in your policy->cpus
:)
> + for (i = 0; i < count; i++) {
> + table[i].index = i;
> + clk = of_clk_get(data->parent, i);
> + table[i].frequency = clk_get_rate(clk) / 1000;
> + }
> + freq_table_redup(table, count);
> + table[i].frequency = CPUFREQ_TABLE_END;
> +
> + data->table = table;
> + per_cpu(cpu_data, cpu) = data;
> +
> + policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> + policy->cur = corenet_cpufreq_get_speed(policy->cpu);
> +
> + /* set the min and max frequency properly */
> + i = cpufreq_frequency_table_cpuinfo(policy, table);
i doesn't suit here, use variable like ret or err..
And move this just after the line where you set freq as TABLE_END.
So that you don't execute unnecessary code for error case.
> +static int corenet_cpufreq_verify(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_frequency_table *table;
> +
> + table = per_cpu(cpu_data, policy->cpu)->table;
merge above two lines.
> + return cpufreq_frequency_table_verify(policy, table);
> +}
> +
> +static int corenet_cpufreq_target(struct cpufreq_policy *policy,
> + unsigned int target_freq, unsigned int relation)
> +{
> + struct cpufreq_freqs freqs;
> + unsigned int new;
> + struct clk *parent;
> + int ret;
> + struct cpu_data *data = per_cpu(cpu_data, policy->cpu);
> +
> + cpufreq_frequency_table_target(policy, data->table,
> + target_freq, relation, &new);
> +
> + if (policy->cur == data->table[new].frequency)
> + return 0;
> +
> + freqs.old = policy->cur;
> + freqs.new = data->table[new].frequency;
> + freqs.cpu = policy->cpu;
> +
> + mutex_lock(&freq_data.cpufreq_lock);
> + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
You need to notify freq change for all cpus in this group :)
BUT you don't need to worry about it due to this:
http://patches.linaro.org/15569/
> + parent = of_clk_get(data->parent, new);
> + ret = clk_set_parent(data->clk, parent);
> + if (ret) {
> + mutex_unlock(&freq_data.cpufreq_lock);
Probably for error case too you need to notify others about completion
of freq change, but with old freq.
> + return ret;
> + }
> +
> + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> + mutex_unlock(&freq_data.cpufreq_lock);
> +
> + return 0;
> +}
> +
> +static struct freq_attr *corenet_cpu_clks_attr[] = {
name it better, maybe corenet_cpufreq_attr ?
> + &cpufreq_freq_attr_scaling_available_freqs,
> + NULL,
> +};
> +
> +static struct cpufreq_driver ppc_corenet_cpufreq_driver = {
> + .name = "ppc_cpufreq",
> + .owner = THIS_MODULE,
> + .flags = CPUFREQ_CONST_LOOPS,
> + .init = corenet_cpufreq_cpu_init,
> + .exit = __exit_p(corenet_cpufreq_cpu_exit),
> + .verify = corenet_cpufreq_verify,
> + .target = corenet_cpufreq_target,
> + .get = corenet_cpufreq_get_speed,
> + .attr = corenet_cpu_clks_attr,
> +};
> +
> +static const struct of_device_id node_matches[] __initconst = {
> + { .compatible = "fsl,qoriq-clockgen-1.0", .data = (void *)1, },
> + { .compatible = "fsl,qoriq-clockgen-2", .data = (void *)8, },
> + {}
> +};
> +
> +static int __init ppc_corenet_cpufreq_init(void)
> +{
> + int ret = 0;
> + struct device_node *np;
> + const struct of_device_id *match;
> +
> + np = of_find_matching_node(NULL, node_matches);
> + if (!np)
> + return -ENODEV;
> +
> + match = of_match_node(node_matches, np);
> + freq_data.cpus_per_cluster = (unsigned long)match->data;
> + mutex_init(&freq_data.cpufreq_lock);
> + of_node_put(np);
> +
> + ret = cpufreq_register_driver(&ppc_corenet_cpufreq_driver);
> + if (ret)
> + return ret;
> +
> + pr_info("Freescale PowerPC corenet CPU frequency scaling driver\n");
Or write it as:
ret = cpufreq_register_driver();
if (!ret)
pr_info();
return ret;
> +
> + return ret;
> +}
> +module_init(ppc_corenet_cpufreq_init);
> +
> +static void __exit ppc_corenet_cpufreq_exit(void)
> +{
> + cpufreq_unregister_driver(&ppc_corenet_cpufreq_driver);
> +}
> +module_exit(ppc_corenet_cpufreq_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Tang Yuantian <Yuantian.Tang@freescale.com>");
> +MODULE_DESCRIPTION("cpufreq driver for Freescale e500mc series SoCs");
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
2013-03-28 14:54 ` Viresh Kumar
@ 2013-03-29 2:51 ` Tang Yuantian-B29983
-1 siblings, 0 replies; 13+ messages in thread
From: Tang Yuantian-B29983 @ 2013-03-29 2:51 UTC (permalink / raw)
To: Viresh Kumar; +Cc: rjw, cpufreq, linux-pm, linuxppc-dev, Li Yang-R58472
> > +static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy) {
> > + unsigned int cpu = policy->cpu;
> > + struct device_node *np;
> > + int i, count;
> > + struct clk *clk;
> > + struct cpufreq_frequency_table *table;
> > + struct cpu_data *data;
> > +
> > + np = of_get_cpu_node(cpu, NULL);
> > + if (!np)
> > + return -ENODEV;
> > +
> > + data = kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
>
> I told you, you missed my comment earlier.
>
> You need to write: sizeof(*data) :(
>
This is new added statement, what you told last time is about the next kcalloc()...
Are there some reasons that we can't use sizeof(struct cpu_data)
instead of sizeof(*data)?
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->clk = of_clk_get(np, 0);
> > + data->parent = of_parse_phandle(np, "clocks", 0);
> > + if (!data->parent) {
> > + pr_err("%s: could not get clock information\n",
> __func__);
> > + goto err_nomem2;
> > + }
> > +
> > + count = of_property_count_strings(data->parent,
> > + "clock-names");
> > +
> > + table = kcalloc(count + 1,
> > + sizeof(struct cpufreq_frequency_table),
> > + GFP_KERNEL);
>
> sizeof(*table)
>
Ditto.
> > + if (!table) {
> > + pr_err("%s: no memory\n", __func__);
> > + goto err_nomem2;
> > + }
> > +
> > + for (i = cpu; i < freq_data.cpus_per_cluster + cpu; i++)
> > + cpumask_set_cpu(i, policy->cpus);
>
> I can see some regression here. Suppose you have two clusters of 4 cpus
> each: (0123) and (4567).. Now at boot time above code will work perfectly
> fine. Now you hot unplug 0,1,2,3 and then hotplug 3 in.
>
> Here, init would be called for cpu 3 and so you will end up saving 3456
> in your policy->cpus
>
> :)
Good catch.. will fix.
Regards,
Yuantian
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
@ 2013-03-29 2:51 ` Tang Yuantian-B29983
0 siblings, 0 replies; 13+ messages in thread
From: Tang Yuantian-B29983 @ 2013-03-29 2:51 UTC (permalink / raw)
To: Viresh Kumar; +Cc: rjw, linuxppc-dev, Li Yang-R58472, cpufreq, linux-pm
> > +static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy) {
> > + unsigned int cpu =3D policy->cpu;
> > + struct device_node *np;
> > + int i, count;
> > + struct clk *clk;
> > + struct cpufreq_frequency_table *table;
> > + struct cpu_data *data;
> > +
> > + np =3D of_get_cpu_node(cpu, NULL);
> > + if (!np)
> > + return -ENODEV;
> > +
> > + data =3D kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
>=20
> I told you, you missed my comment earlier.
>=20
> You need to write: sizeof(*data) :(
>=20
This is new added statement, what you told last time is about the next kcal=
loc()...
Are there some reasons that we can't use sizeof(struct cpu_data)
instead of sizeof(*data)?
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->clk =3D of_clk_get(np, 0);
> > + data->parent =3D of_parse_phandle(np, "clocks", 0);
> > + if (!data->parent) {
> > + pr_err("%s: could not get clock information\n",
> __func__);
> > + goto err_nomem2;
> > + }
> > +
> > + count =3D of_property_count_strings(data->parent,
> > + "clock-names");
> > +
> > + table =3D kcalloc(count + 1,
> > + sizeof(struct cpufreq_frequency_table),
> > + GFP_KERNEL);
>=20
> sizeof(*table)
>=20
Ditto.
> > + if (!table) {
> > + pr_err("%s: no memory\n", __func__);
> > + goto err_nomem2;
> > + }
> > +
> > + for (i =3D cpu; i < freq_data.cpus_per_cluster + cpu; i++)
> > + cpumask_set_cpu(i, policy->cpus);
>=20
> I can see some regression here. Suppose you have two clusters of 4 cpus
> each: (0123) and (4567).. Now at boot time above code will work perfectly
> fine. Now you hot unplug 0,1,2,3 and then hotplug 3 in.
>=20
> Here, init would be called for cpu 3 and so you will end up saving 3456
> in your policy->cpus
>=20
> :)
Good catch.. will fix.
Regards,
Yuantian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
2013-03-29 2:51 ` Tang Yuantian-B29983
@ 2013-03-29 3:16 ` Viresh Kumar
-1 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2013-03-29 3:16 UTC (permalink / raw)
To: Tang Yuantian-B29983; +Cc: rjw, cpufreq, linux-pm, linuxppc-dev, Li Yang-R58472
On 29 March 2013 08:21, Tang Yuantian-B29983 <B29983@freescale.com> wrote:
>> > +static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy) {
>> > + unsigned int cpu = policy->cpu;
>> > + struct device_node *np;
>> > + int i, count;
>> > + struct clk *clk;
>> > + struct cpufreq_frequency_table *table;
>> > + struct cpu_data *data;
>> > +
>> > + np = of_get_cpu_node(cpu, NULL);
>> > + if (!np)
>> > + return -ENODEV;
>> > +
>> > + data = kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
>>
>> I told you, you missed my comment earlier.
>>
>> You need to write: sizeof(*data) :(
>>
> This is new added statement, what you told last time is about the next kcalloc()...
I said about using sizeof() in generic, I copied below from my first
mail on this topic
> + table = kcalloc(count + 1,
kzalloc??
> + sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
sizeof(*table)
And you missed this one as you never replied to it. :)
> Are there some reasons that we can't use sizeof(struct cpu_data)
> instead of sizeof(*data)?
Documentation/CodiingStyle: Chapter 14: Allocating memory
>> > + if (!table) {
>> > + pr_err("%s: no memory\n", __func__);
>> > + goto err_nomem2;
>> > + }
>> > +
>> > + for (i = cpu; i < freq_data.cpus_per_cluster + cpu; i++)
>> > + cpumask_set_cpu(i, policy->cpus);
>>
>> I can see some regression here. Suppose you have two clusters of 4 cpus
>> each: (0123) and (4567).. Now at boot time above code will work perfectly
>> fine. Now you hot unplug 0,1,2,3 and then hotplug 3 in.
>>
>> Here, init would be called for cpu 3 and so you will end up saving 3456
>> in your policy->cpus
>>
>> :)
> Good catch.. will fix.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
@ 2013-03-29 3:16 ` Viresh Kumar
0 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2013-03-29 3:16 UTC (permalink / raw)
To: Tang Yuantian-B29983; +Cc: rjw, linuxppc-dev, Li Yang-R58472, cpufreq, linux-pm
On 29 March 2013 08:21, Tang Yuantian-B29983 <B29983@freescale.com> wrote:
>> > +static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy) {
>> > + unsigned int cpu = policy->cpu;
>> > + struct device_node *np;
>> > + int i, count;
>> > + struct clk *clk;
>> > + struct cpufreq_frequency_table *table;
>> > + struct cpu_data *data;
>> > +
>> > + np = of_get_cpu_node(cpu, NULL);
>> > + if (!np)
>> > + return -ENODEV;
>> > +
>> > + data = kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
>>
>> I told you, you missed my comment earlier.
>>
>> You need to write: sizeof(*data) :(
>>
> This is new added statement, what you told last time is about the next kcalloc()...
I said about using sizeof() in generic, I copied below from my first
mail on this topic
> + table = kcalloc(count + 1,
kzalloc??
> + sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
sizeof(*table)
And you missed this one as you never replied to it. :)
> Are there some reasons that we can't use sizeof(struct cpu_data)
> instead of sizeof(*data)?
Documentation/CodiingStyle: Chapter 14: Allocating memory
>> > + if (!table) {
>> > + pr_err("%s: no memory\n", __func__);
>> > + goto err_nomem2;
>> > + }
>> > +
>> > + for (i = cpu; i < freq_data.cpus_per_cluster + cpu; i++)
>> > + cpumask_set_cpu(i, policy->cpus);
>>
>> I can see some regression here. Suppose you have two clusters of 4 cpus
>> each: (0123) and (4567).. Now at boot time above code will work perfectly
>> fine. Now you hot unplug 0,1,2,3 and then hotplug 3 in.
>>
>> Here, init would be called for cpu 3 and so you will end up saving 3456
>> in your policy->cpus
>>
>> :)
> Good catch.. will fix.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
2013-03-29 3:16 ` Viresh Kumar
(?)
@ 2013-03-29 4:47 ` Tang Yuantian-B29983
-1 siblings, 0 replies; 13+ messages in thread
From: Tang Yuantian-B29983 @ 2013-03-29 4:47 UTC (permalink / raw)
To: Viresh Kumar; +Cc: rjw, cpufreq, linux-pm, linuxppc-dev, Li Yang-R58472
> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: 2013年3月29日 11:17
> To: Tang Yuantian-B29983
> Cc: rjw@sisk.pl; cpufreq@vger.kernel.org; linux-pm@vger.kernel.org;
> linuxppc-dev@lists.ozlabs.org; Li Yang-R58472
> Subject: Re: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale
> e500mc SoCs
>
> On 29 March 2013 08:21, Tang Yuantian-B29983 <B29983@freescale.com> wrote:
> >> > +static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy)
> {
> >> > + unsigned int cpu = policy->cpu;
> >> > + struct device_node *np;
> >> > + int i, count;
> >> > + struct clk *clk;
> >> > + struct cpufreq_frequency_table *table;
> >> > + struct cpu_data *data;
> >> > +
> >> > + np = of_get_cpu_node(cpu, NULL);
> >> > + if (!np)
> >> > + return -ENODEV;
> >> > +
> >> > + data = kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
> >>
> >> I told you, you missed my comment earlier.
> >>
> >> You need to write: sizeof(*data) :(
> >>
> > This is new added statement, what you told last time is about the next
> kcalloc()...
>
> I said about using sizeof() in generic, I copied below from my first mail
> on this topic
>
> > + table = kcalloc(count + 1,
>
> kzalloc??
>
> > + sizeof(struct cpufreq_frequency_table),
> > + GFP_KERNEL);
>
> sizeof(*table)
>
> And you missed this one as you never replied to it. :)
I thought it was OK here. Apparently, sizeof(*table) is better.
But kcalloc is OK.
>
> > Are there some reasons that we can't use sizeof(struct cpu_data)
> > instead of sizeof(*data)?
>
> Documentation/CodiingStyle: Chapter 14: Allocating memory
>
Thanks
Regards,
Yuantian
> >> > + if (!table) {
> >> > + pr_err("%s: no memory\n", __func__);
> >> > + goto err_nomem2;
> >> > + }
> >> > +
> >> > + for (i = cpu; i < freq_data.cpus_per_cluster + cpu; i++)
> >> > + cpumask_set_cpu(i, policy->cpus);
> >>
> >> I can see some regression here. Suppose you have two clusters of 4
> >> cpus
> >> each: (0123) and (4567).. Now at boot time above code will work
> >> perfectly fine. Now you hot unplug 0,1,2,3 and then hotplug 3 in.
> >>
> >> Here, init would be called for cpu 3 and so you will end up saving
> >> 3456 in your policy->cpus
> >>
> >> :)
> > Good catch.. will fix.
>
> Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
@ 2013-03-29 4:47 ` Tang Yuantian-B29983
0 siblings, 0 replies; 13+ messages in thread
From: Tang Yuantian-B29983 @ 2013-03-29 4:47 UTC (permalink / raw)
To: Viresh Kumar; +Cc: rjw, linuxppc-dev, Li Yang-R58472, cpufreq, linux-pm
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogVmlyZXNoIEt1bWFyIFtt
YWlsdG86dmlyZXNoLmt1bWFyQGxpbmFyby5vcmddDQo+IFNlbnQ6IDIwMTPE6jPUwjI5yNUgMTE6
MTcNCj4gVG86IFRhbmcgWXVhbnRpYW4tQjI5OTgzDQo+IENjOiByandAc2lzay5wbDsgY3B1ZnJl
cUB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LXBtQHZnZXIua2VybmVsLm9yZzsNCj4gbGludXhwcGMt
ZGV2QGxpc3RzLm96bGFicy5vcmc7IExpIFlhbmctUjU4NDcyDQo+IFN1YmplY3Q6IFJlOiBbUEFU
Q0ggMi8yIHYyXSBjcHVmcmVxOiBBZGQgY3B1ZnJlcSBkcml2ZXIgZm9yIEZyZWVzY2FsZQ0KPiBl
NTAwbWMgU29Dcw0KPiANCj4gT24gMjkgTWFyY2ggMjAxMyAwODoyMSwgVGFuZyBZdWFudGlhbi1C
Mjk5ODMgPEIyOTk4M0BmcmVlc2NhbGUuY29tPiB3cm90ZToNCj4gPj4gPiArc3RhdGljIGludCBj
b3JlbmV0X2NwdWZyZXFfY3B1X2luaXQoc3RydWN0IGNwdWZyZXFfcG9saWN5ICpwb2xpY3kpDQo+
IHsNCj4gPj4gPiArICAgICAgIHVuc2lnbmVkIGludCBjcHUgPSBwb2xpY3ktPmNwdTsNCj4gPj4g
PiArICAgICAgIHN0cnVjdCBkZXZpY2Vfbm9kZSAqbnA7DQo+ID4+ID4gKyAgICAgICBpbnQgaSwg
Y291bnQ7DQo+ID4+ID4gKyAgICAgICBzdHJ1Y3QgY2xrICpjbGs7DQo+ID4+ID4gKyAgICAgICBz
dHJ1Y3QgY3B1ZnJlcV9mcmVxdWVuY3lfdGFibGUgKnRhYmxlOw0KPiA+PiA+ICsgICAgICAgc3Ry
dWN0IGNwdV9kYXRhICpkYXRhOw0KPiA+PiA+ICsNCj4gPj4gPiArICAgICAgIG5wID0gb2ZfZ2V0
X2NwdV9ub2RlKGNwdSwgTlVMTCk7DQo+ID4+ID4gKyAgICAgICBpZiAoIW5wKQ0KPiA+PiA+ICsg
ICAgICAgICAgICAgICByZXR1cm4gLUVOT0RFVjsNCj4gPj4gPiArDQo+ID4+ID4gKyAgICAgICBk
YXRhID0ga3phbGxvYyhzaXplb2Yoc3RydWN0IGNwdV9kYXRhKSwgR0ZQX0tFUk5FTCk7DQo+ID4+
DQo+ID4+IEkgdG9sZCB5b3UsIHlvdSBtaXNzZWQgbXkgY29tbWVudCBlYXJsaWVyLg0KPiA+Pg0K
PiA+PiBZb3UgbmVlZCB0byB3cml0ZTogc2l6ZW9mKCpkYXRhKSA6KA0KPiA+Pg0KPiA+IFRoaXMg
aXMgbmV3IGFkZGVkIHN0YXRlbWVudCwgd2hhdCB5b3UgdG9sZCBsYXN0IHRpbWUgaXMgYWJvdXQg
dGhlIG5leHQNCj4ga2NhbGxvYygpLi4uDQo+IA0KPiBJIHNhaWQgYWJvdXQgdXNpbmcgc2l6ZW9m
KCkgaW4gZ2VuZXJpYywgSSBjb3BpZWQgYmVsb3cgZnJvbSBteSBmaXJzdCBtYWlsDQo+IG9uIHRo
aXMgdG9waWMNCj4gDQo+ID4gKyAgICAgICB0YWJsZSA9IGtjYWxsb2MoY291bnQgKyAxLA0KPiAN
Cj4ga3phbGxvYz8/DQo+IA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgIHNpemVvZihzdHJ1
Y3QgY3B1ZnJlcV9mcmVxdWVuY3lfdGFibGUpLA0KPiA+ICsgR0ZQX0tFUk5FTCk7DQo+IA0KPiBz
aXplb2YoKnRhYmxlKQ0KPiANCj4gQW5kIHlvdSBtaXNzZWQgdGhpcyBvbmUgYXMgeW91IG5ldmVy
IHJlcGxpZWQgdG8gaXQuIDopDQpJIHRob3VnaHQgaXQgd2FzIE9LIGhlcmUuIEFwcGFyZW50bHks
IHNpemVvZigqdGFibGUpIGlzIGJldHRlci4NCkJ1dCBrY2FsbG9jIGlzIE9LLg0KIA0KPiANCj4g
PiBBcmUgdGhlcmUgc29tZSByZWFzb25zIHRoYXQgd2UgY2FuJ3QgdXNlIHNpemVvZihzdHJ1Y3Qg
Y3B1X2RhdGEpDQo+ID4gaW5zdGVhZCBvZiBzaXplb2YoKmRhdGEpPw0KPiANCj4gRG9jdW1lbnRh
dGlvbi9Db2RpaW5nU3R5bGU6IENoYXB0ZXIgMTQ6IEFsbG9jYXRpbmcgbWVtb3J5DQo+IA0KVGhh
bmtzDQoNClJlZ2FyZHMsDQpZdWFudGlhbg0KDQo+ID4+ID4gKyAgICAgICBpZiAoIXRhYmxlKSB7
DQo+ID4+ID4gKyAgICAgICAgICAgICAgIHByX2VycigiJXM6IG5vIG1lbW9yeVxuIiwgX19mdW5j
X18pOw0KPiA+PiA+ICsgICAgICAgICAgICAgICBnb3RvIGVycl9ub21lbTI7DQo+ID4+ID4gKyAg
ICAgICB9DQo+ID4+ID4gKw0KPiA+PiA+ICsgICAgICAgZm9yIChpID0gY3B1OyBpIDwgZnJlcV9k
YXRhLmNwdXNfcGVyX2NsdXN0ZXIgKyBjcHU7IGkrKykNCj4gPj4gPiArICAgICAgICAgICAgICAg
Y3B1bWFza19zZXRfY3B1KGksIHBvbGljeS0+Y3B1cyk7DQo+ID4+DQo+ID4+IEkgY2FuIHNlZSBz
b21lIHJlZ3Jlc3Npb24gaGVyZS4gU3VwcG9zZSB5b3UgaGF2ZSB0d28gY2x1c3RlcnMgb2YgNA0K
PiA+PiBjcHVzDQo+ID4+IGVhY2g6ICgwMTIzKSBhbmQgKDQ1NjcpLi4gTm93IGF0IGJvb3QgdGlt
ZSBhYm92ZSBjb2RlIHdpbGwgd29yaw0KPiA+PiBwZXJmZWN0bHkgZmluZS4gTm93IHlvdSBob3Qg
dW5wbHVnIDAsMSwyLDMgYW5kIHRoZW4gaG90cGx1ZyAzIGluLg0KPiA+Pg0KPiA+PiBIZXJlLCBp
bml0IHdvdWxkIGJlIGNhbGxlZCBmb3IgY3B1IDMgYW5kIHNvIHlvdSB3aWxsIGVuZCB1cCBzYXZp
bmcNCj4gPj4gMzQ1NiBpbiB5b3VyIHBvbGljeS0+Y3B1cw0KPiA+Pg0KPiA+PiA6KQ0KPiA+IEdv
b2QgY2F0Y2guLiB3aWxsIGZpeC4NCj4gDQo+IFRoYW5rcy4NCg0K
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
@ 2013-03-29 4:47 ` Tang Yuantian-B29983
0 siblings, 0 replies; 13+ messages in thread
From: Tang Yuantian-B29983 @ 2013-03-29 4:47 UTC (permalink / raw)
To: Viresh Kumar; +Cc: rjw, cpufreq, linux-pm, linuxppc-dev, Li Yang-R58472
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="us-ascii", Size: 2490 bytes --]
> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: 2013Äê3ÔÂ29ÈÕ 11:17
> To: Tang Yuantian-B29983
> Cc: rjw@sisk.pl; cpufreq@vger.kernel.org; linux-pm@vger.kernel.org;
> linuxppc-dev@lists.ozlabs.org; Li Yang-R58472
> Subject: Re: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale
> e500mc SoCs
>
> On 29 March 2013 08:21, Tang Yuantian-B29983 <B29983@freescale.com> wrote:
> >> > +static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy)
> {
> >> > + unsigned int cpu = policy->cpu;
> >> > + struct device_node *np;
> >> > + int i, count;
> >> > + struct clk *clk;
> >> > + struct cpufreq_frequency_table *table;
> >> > + struct cpu_data *data;
> >> > +
> >> > + np = of_get_cpu_node(cpu, NULL);
> >> > + if (!np)
> >> > + return -ENODEV;
> >> > +
> >> > + data = kzalloc(sizeof(struct cpu_data), GFP_KERNEL);
> >>
> >> I told you, you missed my comment earlier.
> >>
> >> You need to write: sizeof(*data) :(
> >>
> > This is new added statement, what you told last time is about the next
> kcalloc()...
>
> I said about using sizeof() in generic, I copied below from my first mail
> on this topic
>
> > + table = kcalloc(count + 1,
>
> kzalloc??
>
> > + sizeof(struct cpufreq_frequency_table),
> > + GFP_KERNEL);
>
> sizeof(*table)
>
> And you missed this one as you never replied to it. :)
I thought it was OK here. Apparently, sizeof(*table) is better.
But kcalloc is OK.
>
> > Are there some reasons that we can't use sizeof(struct cpu_data)
> > instead of sizeof(*data)?
>
> Documentation/CodiingStyle: Chapter 14: Allocating memory
>
Thanks
Regards,
Yuantian
> >> > + if (!table) {
> >> > + pr_err("%s: no memory\n", __func__);
> >> > + goto err_nomem2;
> >> > + }
> >> > +
> >> > + for (i = cpu; i < freq_data.cpus_per_cluster + cpu; i++)
> >> > + cpumask_set_cpu(i, policy->cpus);
> >>
> >> I can see some regression here. Suppose you have two clusters of 4
> >> cpus
> >> each: (0123) and (4567).. Now at boot time above code will work
> >> perfectly fine. Now you hot unplug 0,1,2,3 and then hotplug 3 in.
> >>
> >> Here, init would be called for cpu 3 and so you will end up saving
> >> 3456 in your policy->cpus
> >>
> >> :)
> > Good catch.. will fix.
>
> Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
2013-03-29 4:47 ` Tang Yuantian-B29983
@ 2013-03-29 4:50 ` Viresh Kumar
-1 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2013-03-29 4:50 UTC (permalink / raw)
To: Tang Yuantian-B29983; +Cc: rjw, cpufreq, linux-pm, linuxppc-dev, Li Yang-R58472
On 29 March 2013 10:17, Tang Yuantian-B29983 <B29983@freescale.com> wrote:
> I thought it was OK here. Apparently, sizeof(*table) is better.
> But kcalloc is OK.
Yes yes, Kcalloc is okay... I have misread that part earlier when i
suggested kzalloc.
In last mail i was referring to sizeof() only.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs
@ 2013-03-29 4:50 ` Viresh Kumar
0 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2013-03-29 4:50 UTC (permalink / raw)
To: Tang Yuantian-B29983; +Cc: rjw, linuxppc-dev, Li Yang-R58472, cpufreq, linux-pm
On 29 March 2013 10:17, Tang Yuantian-B29983 <B29983@freescale.com> wrote:
> I thought it was OK here. Apparently, sizeof(*table) is better.
> But kcalloc is OK.
Yes yes, Kcalloc is okay... I have misread that part earlier when i
suggested kzalloc.
In last mail i was referring to sizeof() only.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-03-29 4:50 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-28 9:55 [PATCH 2/2 v2] cpufreq: Add cpufreq driver for Freescale e500mc SoCs Yuantian.Tang
2013-03-28 9:55 ` Yuantian.Tang
2013-03-28 14:54 ` Viresh Kumar
2013-03-28 14:54 ` Viresh Kumar
2013-03-29 2:51 ` Tang Yuantian-B29983
2013-03-29 2:51 ` Tang Yuantian-B29983
2013-03-29 3:16 ` Viresh Kumar
2013-03-29 3:16 ` Viresh Kumar
2013-03-29 4:47 ` Tang Yuantian-B29983
2013-03-29 4:47 ` Tang Yuantian-B29983
2013-03-29 4:47 ` Tang Yuantian-B29983
2013-03-29 4:50 ` Viresh Kumar
2013-03-29 4:50 ` Viresh Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.