Thanks for your comments. Your comments will be addressed in next version. Some explanations inline.
Thanks, Yuantian > -----Original Message----- > From: Viresh Kumar [mailto:viresh.ku...@linaro.org] > Sent: Friday, October 17, 2014 4:09 PM > To: Tang Yuantian-B29983 > Cc: Rafael J. Wysocki; Linux Kernel Mailing List; linux...@vger.kernel.org; > linuxppc-...@ozlabs.org; Tang Yuantian-B29983 > Subject: Re: [PATCH] cpufreq: qoriq: Make the driver usable on all QorIQ > platforms > > On 17 October 2014 08:43, <b29...@freescale.com> wrote: > > Hi B29983 :) > > > From: Tang Yuantian <yuantian.t...@freescale.com> > > > > Freescale introduced new ARM core-based SoCs which support dynamic > > frequency switch feature. DFS on new SoCs are compatible with current > > PowerPC CoreNet platforms. In order to support those new platforms, > > this driver needs to be slightly adjusted. The main changes include: > > > > 1. Changed the names of driver and functions in driver. > > 2. Added two new functions get_cpu_physical_id() and get_bus_freq(). > > 3. Used a new way to get all the CPUs which sharing clock wire. > > > > Signed-off-by: Tang Yuantian <yuantian.t...@freescale.com> > > --- > > drivers/cpufreq/Kconfig.arm | 8 ++ > > drivers/cpufreq/Kconfig.powerpc | 11 +- > > drivers/cpufreq/Makefile | 2 +- > > .../{ppc-corenet-cpufreq.c => qoriq-cpufreq.c} | 150 > ++++++++++++++------- > > 4 files changed, 114 insertions(+), 57 deletions(-) rename > > drivers/cpufreq/{ppc-corenet-cpufreq.c => qoriq-cpufreq.c} (72%) > > > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > > index 83a75dc..1925ae94 100644 > > --- a/drivers/cpufreq/Kconfig.arm > > +++ b/drivers/cpufreq/Kconfig.arm > > @@ -247,3 +247,11 @@ config ARM_TEGRA_CPUFREQ > > default y > > help > > This adds the CPUFreq driver support for TEGRA SOCs. > > + > > +config QORIQ_CPUFREQ > > + tristate "CPU frequency scaling driver for Freescale QorIQ SoCs" > > + depends on OF && COMMON_CLK > > + select CLK_PPC_CORENET > > + help > > + This adds the CPUFreq driver support for Freescale QorIQ SoCs > > + which are capable of changing the CPU's frequency dynamically. > > diff --git a/drivers/cpufreq/Kconfig.powerpc > > b/drivers/cpufreq/Kconfig.powerpc index 72564b7..3a34248 100644 > > --- a/drivers/cpufreq/Kconfig.powerpc > > +++ b/drivers/cpufreq/Kconfig.powerpc > > @@ -23,14 +23,13 @@ config CPU_FREQ_MAPLE > > 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 > > +config QORIQ_CPUFREQ > > + tristate "CPU frequency scaling driver for Freescale QorIQ SoCs" > > + depends on OF && COMMON_CLK > > 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. > > + This adds the CPUFreq driver support for Freescale QorIQ SoCs > > + which are capable of changing the CPU's frequency dynamically. > > > > config CPU_FREQ_PMAC > > bool "Support for Apple PowerBooks" > > Don't need this duplication at all. Just move this to Kconfig instead of .arm > and > ppc. > > > diff --git a/drivers/cpufreq/ppc-corenet-cpufreq.c > > b/drivers/cpufreq/qoriq-cpufreq.c > > > /** > > * struct cpu_data - per CPU data struct @@ -69,9 +68,6 @@ static > > const u32 *fmask; > > > > static DEFINE_PER_CPU(struct cpu_data *, cpu_data); > > > > -/* cpumask in a cluster */ > > -static DEFINE_PER_CPU(cpumask_var_t, cpu_mask); > > - > > #ifndef CONFIG_SMP > > static inline const struct cpumask *cpu_core_mask(int cpu) { @@ > > -79,6 +75,79 @@ static inline const struct cpumask *cpu_core_mask(int > > cpu) } #endif > > > > +#if defined(CONFIG_PPC_E500MC) > > +static int get_cpu_physical_id(int cpu) { > > + return get_hard_smp_processor_id(cpu); } #elif > > +defined(CONFIG_ARM) > > Wouldn't a #else work here as there are just two platforms we are talking > about ? > > > +static int get_cpu_physical_id(int cpu) { > > + return topology_core_id(cpu); > > +} > > +#endif > > + > > +static u32 get_bus_freq(void) > > +{ > > + struct device_node *soc; > > + u32 sysfreq; > > + > > + soc = of_find_node_by_type(NULL, "soc"); > > + if (!soc) > > + return 0; > > + > > + if (of_property_read_u32(soc, "bus-frequency", &sysfreq)) > > + sysfreq = 0; > > + > > + of_node_put(soc); > > + > > + return sysfreq; > > +} > > + > > +static struct device_node *cpu_to_clk_node(int cpu) { > > + struct device_node *np, *clk_np; > > + > > + if (!cpu_present(cpu)) > > + return NULL; > > + > > + np = of_get_cpu_node(cpu, NULL); > > + if (!np) > > + return NULL; > > + > > + clk_np = of_parse_phandle(np, "clocks", 0); > > + if (!clk_np) > > + return NULL; > > + > > + of_node_put(np); > > + > > + return clk_np; > > +} > > + > > +/* traverse cpu nodes to get cpu mask of sharing clock wire */ static > > +void set_affected_cpus(struct cpufreq_policy *policy) { > > + struct device_node *np, *clk_np; > > + struct cpumask *dstp = policy->cpus; > > + int i; > > + > > + np = cpu_to_clk_node(policy->cpu); > > + if (!np) > > + return; > > + > > + for_each_present_cpu(i) { > > + clk_np = cpu_to_clk_node(i); > > + if (!clk_np) > > + continue; > > + > > + if (clk_np == np) > > + cpumask_set_cpu(i, dstp); > > So you are depending on matching the clock-nodes from DT for getting this > information, right ? There is nothing that the architecture gives? > Yes. I used an architecture dependent way. > > + > > + of_node_put(clk_np); > > + } > > + of_node_put(np); > > +} > > + > > /* reduce the duplicated frequencies in frequency table */ static > > void freq_table_redup(struct cpufreq_frequency_table *freq_table, > > int count) > > @@ -105,6 +174,7 @@ static void freq_table_sort(struct > cpufreq_frequency_table *freq_table, > > int i, j, ind; > > unsigned int freq, max_freq; > > struct cpufreq_frequency_table table; > > + > > for (i = 0; i < count - 1; i++) { > > max_freq = freq_table[i].frequency; > > ind = i; > > @@ -129,7 +199,7 @@ static void freq_table_sort(struct > cpufreq_frequency_table *freq_table, > > } > > } > > > > -static int corenet_cpufreq_cpu_init(struct cpufreq_policy *policy) > > +static int qoriq_cpufreq_cpu_init(struct cpufreq_policy *policy) > > { > > struct device_node *np; > > int i, count, ret; > > @@ -145,10 +215,8 @@ static int corenet_cpufreq_cpu_init(struct > cpufreq_policy *policy) > > return -ENODEV; > > > > data = kzalloc(sizeof(*data), GFP_KERNEL); > > - if (!data) { > > - pr_err("%s: no memory\n", __func__); > > Wasn't this useful ? > To get rid of checkpatch warning. Currently, checkpatch script doesn't allow to output memory allocation error information. We will get memory information eventually if there are memory related error. Regards, Yuantian > > + if (!data) > > goto err_np; > > - } > > > > policy->clk = of_clk_get(np, 0); > > if (IS_ERR(policy->clk)) { > > @@ -170,7 +238,7 @@ static int corenet_cpufreq_cpu_init(struct > cpufreq_policy *policy) > > } > > > > if (fmask) > > - mask = fmask[get_hard_smp_processor_id(cpu)]; > > + mask = fmask[get_cpu_physical_id(cpu)]; > > else > > mask = 0x0; > > > > @@ -201,13 +269,13 @@ static int corenet_cpufreq_cpu_init(struct > cpufreq_policy *policy) > > data->table = table; > > > > /* update ->cpus if we have cluster, no harm if not */ > > - cpumask_copy(policy->cpus, per_cpu(cpu_mask, cpu)); > > - for_each_cpu(i, per_cpu(cpu_mask, cpu)) > > + set_affected_cpus(policy); > > + for_each_cpu(i, policy->cpus) > > per_cpu(cpu_data, i) = data; > > Get rid of this per-cpu data and use policy->driver_data instead. > > > > > /* Minimum transition latency is 12 platform clocks */ > > u64temp = 12ULL * NSEC_PER_SEC; > > - do_div(u64temp, fsl_get_sys_freq()); > > + do_div(u64temp, get_bus_freq()); > > policy->cpuinfo.transition_latency = u64temp + 1; > > > > of_node_put(np); > > @@ -227,7 +295,7 @@ err_np: > > return -ENODEV; > > } > > > > -static int __exit corenet_cpufreq_cpu_exit(struct cpufreq_policy > > *policy) > > +static int __exit qoriq_cpufreq_cpu_exit(struct cpufreq_policy > > +*policy) > > { > > struct cpu_data *data = per_cpu(cpu_data, policy->cpu); > > unsigned int cpu; > > @@ -236,13 +304,13 @@ static int __exit corenet_cpufreq_cpu_exit(struct > cpufreq_policy *policy) > > kfree(data->table); > > kfree(data); > > > > - for_each_cpu(cpu, per_cpu(cpu_mask, policy->cpu)) > > + for_each_cpu(cpu, policy->cpus) > > per_cpu(cpu_data, cpu) = NULL; > > > > return 0; > > } > > > > -static int corenet_cpufreq_target(struct cpufreq_policy *policy, > > +static int qoriq_cpufreq_target(struct cpufreq_policy *policy, > > unsigned int index) > > { > > struct clk *parent; > > @@ -252,18 +320,18 @@ static int corenet_cpufreq_target(struct > cpufreq_policy *policy, > > return clk_set_parent(policy->clk, parent); } > > > > -static struct cpufreq_driver ppc_corenet_cpufreq_driver = { > > - .name = "ppc_cpufreq", > > +static struct cpufreq_driver qoriq_cpufreq_driver = { > > + .name = "qoriq_cpufreq", > > .flags = CPUFREQ_CONST_LOOPS, > > - .init = corenet_cpufreq_cpu_init, > > - .exit = __exit_p(corenet_cpufreq_cpu_exit), > > + .init = qoriq_cpufreq_cpu_init, > > + .exit = __exit_p(qoriq_cpufreq_cpu_exit), > > .verify = cpufreq_generic_frequency_table_verify, > > - .target_index = corenet_cpufreq_target, > > + .target_index = qoriq_cpufreq_target, > > .get = cpufreq_generic_get, > > .attr = cpufreq_generic_attr, > > }; > > > > -static const struct of_device_id node_matches[] __initdata = { > > +static const struct of_device_id node_matches[] __initconst = { > > { .compatible = "fsl,p2041-clockgen", .data = &sdata[0], }, > > { .compatible = "fsl,p3041-clockgen", .data = &sdata[0], }, > > { .compatible = "fsl,p5020-clockgen", .data = &sdata[1], }, @@ > > -273,61 +341,43 @@ static const struct of_device_id node_matches[] > __initdata = { > > {} > > }; > > > > -static int __init ppc_corenet_cpufreq_init(void) > > +static int __init qoriq_cpufreq_init(void) > > { > > int ret; > > struct device_node *np; > > const struct of_device_id *match; > > const struct soc_data *data; > > - unsigned int cpu; > > > > np = of_find_matching_node(NULL, node_matches); > > if (!np) > > return -ENODEV; > > > > - for_each_possible_cpu(cpu) { > > - if (!alloc_cpumask_var(&per_cpu(cpu_mask, cpu), > GFP_KERNEL)) > > - goto err_mask; > > - cpumask_copy(per_cpu(cpu_mask, cpu), > cpu_core_mask(cpu)); > > - } > > - > > match = of_match_node(node_matches, np); > > data = match->data; > > if (data) { > > if (data->flag) > > fmask = data->freq_mask; > > - min_cpufreq = fsl_get_sys_freq(); > > + min_cpufreq = get_bus_freq(); > > } else { > > - min_cpufreq = fsl_get_sys_freq() / 2; > > + min_cpufreq = get_bus_freq() / 2; > > } > > > > of_node_put(np); > > > > - ret = cpufreq_register_driver(&ppc_corenet_cpufreq_driver); > > + ret = cpufreq_register_driver(&qoriq_cpufreq_driver); > > if (!ret) > > - pr_info("Freescale PowerPC corenet CPU frequency scaling > driver\n"); > > + pr_info("Freescale QorIQ CPU frequency scaling > > + driver\n"); > > > > return ret; > > - > > -err_mask: > > - for_each_possible_cpu(cpu) > > - free_cpumask_var(per_cpu(cpu_mask, cpu)); > > - > > - return -ENOMEM; > > } > > -module_init(ppc_corenet_cpufreq_init); > > +module_init(qoriq_cpufreq_init); > > > > -static void __exit ppc_corenet_cpufreq_exit(void) > > +static void __exit qoriq_cpufreq_exit(void) > > { > > - unsigned int cpu; > > - > > - for_each_possible_cpu(cpu) > > - free_cpumask_var(per_cpu(cpu_mask, cpu)); > > - > > - cpufreq_unregister_driver(&ppc_corenet_cpufreq_driver); > > + cpufreq_unregister_driver(&qoriq_cpufreq_driver); > > } > > -module_exit(ppc_corenet_cpufreq_exit); > > +module_exit(qoriq_cpufreq_exit); > > > > MODULE_LICENSE("GPL"); > > MODULE_AUTHOR("Tang Yuantian <yuantian.t...@freescale.com>"); > > -MODULE_DESCRIPTION("cpufreq driver for Freescale e500mc series > > SoCs"); > > +MODULE_DESCRIPTION("cpufreq driver for Freescale QorIQ series SoCs"); > > + comments from Kumar Gala :) N�����r��y����b�X��ǧv�^�){.n�+����{����zX����ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z� �w���?�����&�)ߢf��^jǫy�m��@A�a��� 0��h���i
