* [PATCH v3 0/2] Add cpufreq support for Armada 8K @ 2018-12-11 16:42 Gregory CLEMENT 2018-12-11 16:42 ` [PATCH v3 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver Gregory CLEMENT 2018-12-11 16:42 ` [PATCH v3 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K Gregory CLEMENT 0 siblings, 2 replies; 6+ messages in thread From: Gregory CLEMENT @ 2018-12-11 16:42 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar, linux-pm Cc: Andrew Lunn, Jason Cooper, Antoine Tenart, Gregory CLEMENT, Maxime Chevallier, Thomas Petazzoni, Miquèl Raynal, linux-arm-kernel, Sebastian Hesselbarth Hi, this is the third version of a the series adding the cpufreq support for the Marvell Armada 8K SoCs. Gregory Changelog: v2 -> v3 - Make the driver really modular (modprobe;modprobe -r;modprobe; is working) - Don't use anymore 100MHz as minimal frequency, but only max frequency/4 - Fix remaining warning reported by checkpatch - Rework the registering of the opp v1 -> v2: - Fix typos in MAINTAINERS (reported by Baruch Siach) and in armada-8k-cpufreq.c (reported by Thomas Petazzoni) - Fix indentation in Kconfig.arm (reported by Thomas Petazzoni) - Use const for opps_div (reported by Thomas Petazzoni) - Fix memory leak of opps_array (reported by Thomas Petazzoni) Gregory CLEMENT (2): MAINTAINERS: add new entries for Armada 8K cpufreq driver cpufreq: ap806: add cpufreq driver for Armada 8K MAINTAINERS | 1 + drivers/cpufreq/Kconfig.arm | 11 ++ drivers/cpufreq/Makefile | 1 + drivers/cpufreq/armada-8k-cpufreq.c | 186 ++++++++++++++++++++++++++++ 4 files changed, 199 insertions(+) create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c -- 2.19.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver 2018-12-11 16:42 [PATCH v3 0/2] Add cpufreq support for Armada 8K Gregory CLEMENT @ 2018-12-11 16:42 ` Gregory CLEMENT 2018-12-11 16:42 ` [PATCH v3 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K Gregory CLEMENT 1 sibling, 0 replies; 6+ messages in thread From: Gregory CLEMENT @ 2018-12-11 16:42 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar, linux-pm Cc: Andrew Lunn, Jason Cooper, Antoine Tenart, Gregory CLEMENT, Maxime Chevallier, Thomas Petazzoni, Miquèl Raynal, linux-arm-kernel, Sebastian Hesselbarth This new driver belongs to the mvebu family, update the MAINTAINER file to document it. Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index f4855974f325..c71e4f9c1173 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1717,6 +1717,7 @@ F: arch/arm/configs/mvebu_*_defconfig F: arch/arm/mach-mvebu/ F: arch/arm64/boot/dts/marvell/armada* F: drivers/cpufreq/armada-37xx-cpufreq.c +F: drivers/cpufreq/armada-8k-cpufreq.c F: drivers/cpufreq/mvebu-cpufreq.c F: drivers/irqchip/irq-armada-370-xp.c F: drivers/irqchip/irq-mvebu-* -- 2.19.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-12-11 16:42 [PATCH v3 0/2] Add cpufreq support for Armada 8K Gregory CLEMENT 2018-12-11 16:42 ` [PATCH v3 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver Gregory CLEMENT @ 2018-12-11 16:42 ` Gregory CLEMENT 2018-12-17 11:10 ` Viresh Kumar 1 sibling, 1 reply; 6+ messages in thread From: Gregory CLEMENT @ 2018-12-11 16:42 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar, linux-pm Cc: Andrew Lunn, Jason Cooper, Antoine Tenart, Gregory CLEMENT, Maxime Chevallier, Thomas Petazzoni, Miquèl Raynal, linux-arm-kernel, Sebastian Hesselbarth Add cpufreq driver for Marvell AP-806 found on Aramda 8K. The AP-806 has DFS (Dynamic Frequency Scaling) with coupled clock domain for two clusters, so this driver will directly use generic cpufreq-dt driver as backend. Based on the work of Omri Itach <omrii@marvell.com>. Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> --- drivers/cpufreq/Kconfig.arm | 11 ++ drivers/cpufreq/Makefile | 1 + drivers/cpufreq/armada-8k-cpufreq.c | 186 ++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+) create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 4e1131ef85ae..7e32a241760d 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -25,6 +25,17 @@ config ARM_ARMADA_37XX_CPUFREQ This adds the CPUFreq driver support for Marvell Armada 37xx SoCs. The Armada 37xx PMU supports 4 frequency and VDD levels. +config ARM_ARMADA_8K_CPUFREQ + tristate "Armada 8K CPUFreq driver" + depends on ARCH_MVEBU && CPUFREQ_DT + help + This enables the CPUFreq driver support for Marvell + Armada8k SOCs. + Armada8K device has the AP806 which supports scaling + to any full integer divider. + + If in doubt, say N. + # big LITTLE core layer and glue drivers config ARM_BIG_LITTLE_CPUFREQ tristate "Generic ARM big LITTLE CPUfreq driver" diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index d5ee4562ed06..db1564b610ac 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -50,6 +50,7 @@ obj-$(CONFIG_X86_SFI_CPUFREQ) += sfi-cpufreq.o obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o obj-$(CONFIG_ARM_ARMADA_37XX_CPUFREQ) += armada-37xx-cpufreq.o +obj-$(CONFIG_ARM_ARMADA_8K_CPUFREQ) += armada-8k-cpufreq.o obj-$(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ) += brcmstb-avs-cpufreq.o obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o obj-$(CONFIG_ARCH_DAVINCI) += davinci-cpufreq.o diff --git a/drivers/cpufreq/armada-8k-cpufreq.c b/drivers/cpufreq/armada-8k-cpufreq.c new file mode 100644 index 000000000000..1db1953fb43e --- /dev/null +++ b/drivers/cpufreq/armada-8k-cpufreq.c @@ -0,0 +1,186 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * CPUFreq support for Armada 8K + * + * Copyright (C) 2018 Marvell + * + * Omri Itach <omrii@marvell.com> + * Gregory Clement <gregory.clement@bootlin.com> + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/clk.h> +#include <linux/cpu.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_opp.h> +#include <linux/slab.h> + +/* + * Setup the opps list with the divider for the max frequency, that + * will be filled at runtime. + */ +static const int opps_div[] __initconst = {1, 2, 3, 4}; + +static struct platform_device *armada_8k_pdev; +static struct freq_table *freq_tables; +static int opps_index; + +struct freq_table { + struct device *cpu_dev; + struct clk *clk; + unsigned int freq[ARRAY_SIZE(opps_div)]; +}; + +/* If the CPUs share the same clock, then they are in the same cluster. */ +static void __init armada_8k_get_sharing_cpus(struct clk *cur_clk, + struct cpumask *cpumask) +{ + int cpu; + + for_each_possible_cpu(cpu) { + struct device *cpu_dev = get_cpu_device(cpu); + struct clk *clk = clk_get(cpu_dev, 0); + + if (IS_ERR(clk)) + dev_warn(cpu_dev, "Cannot get clock for CPU %d\n", cpu); + else if (clk_is_match(clk, cur_clk)) + cpumask_set_cpu(cpu, cpumask); + + clk_put(clk); + } +} + +static int armada_8k_add_opp(struct clk *clk, struct device *cpu_dev, + struct freq_table *freq_tables, int opps_index) +{ + unsigned int cur_frequency; + unsigned int freq; + int i, ret; + + /* Get nominal (current) CPU frequency. */ + cur_frequency = clk_get_rate(clk); + + if (!cur_frequency) { + dev_err(cpu_dev, + "Failed to get clock rate for this CPU\n"); + return -EINVAL; + } + + freq_tables[opps_index].cpu_dev = cpu_dev; + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { + freq = cur_frequency / opps_div[i]; + + ret = dev_pm_opp_add(cpu_dev, freq, 0); + if (ret) + return ret; + + freq_tables[opps_index].freq[i] = freq; + } + + return 0; +} + +static void armada_8k_cpufreq_free_table(void) +{ + opps_index--; + for (; opps_index >= 0; opps_index--) { + int i; + + for (i = 0; i < ARRAY_SIZE(opps_div); i++) + dev_pm_opp_remove(freq_tables[opps_index].cpu_dev, + freq_tables[opps_index].freq[i]); + } + + kfree(freq_tables); +} + +static int __init armada_8k_cpufreq_init(void) +{ + int ret = 0, cpu, nb_cpus; + struct device_node *node; + + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); + if (!node || !of_device_is_available(node)) + return -ENODEV; + + nb_cpus = num_possible_cpus(); + freq_tables = kcalloc(nb_cpus, sizeof(*freq_tables), GFP_KERNEL); + /* + * For each CPU, this loop registers the operating points + * supported (which are the nominal CPU frequency and full integer + * divisions of it). + */ + for_each_possible_cpu(cpu) { + struct device *cpu_dev; + struct cpumask cpus; + struct clk *clk; + int i, skip; + + skip = 0; + cpu_dev = get_cpu_device(cpu); + + if (!cpu_dev) { + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); + continue; + } + + clk = devm_clk_get(cpu_dev, 0); + + if (IS_ERR(clk)) { + dev_err(cpu_dev, "Cannot get clock for CPU %d\n", cpu); + ret = PTR_ERR(clk); + goto remove_opp; + } + + for (i = 0; i < nb_cpus; i++) { + if (clk_is_match(clk, freq_tables[i].clk)) { + skip = 1; + break; + } + } + if (skip) { + devm_clk_put(cpu_dev, clk); + continue; + } + + freq_tables[opps_index].clk = clk; + + ret = armada_8k_add_opp(clk, cpu_dev, freq_tables, opps_index); + if (ret) + goto remove_opp; + + opps_index++; + cpumask_clear(&cpus); + armada_8k_get_sharing_cpus(clk, &cpus); + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); + } + + armada_8k_pdev = platform_device_register_simple("cpufreq-dt", -1, + NULL, 0); + ret = PTR_ERR_OR_ZERO(armada_8k_pdev); + if (ret) + goto remove_opp; + return 0; + +remove_opp: + armada_8k_cpufreq_free_table(); + return ret; +} +module_init(armada_8k_cpufreq_init); + +static void __exit armada_8k_cpufreq_exit(void) +{ + armada_8k_cpufreq_free_table(); + platform_device_unregister(armada_8k_pdev); +} +module_exit(armada_8k_cpufreq_exit); + +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); +MODULE_LICENSE("GPL"); -- 2.19.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-12-11 16:42 ` [PATCH v3 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K Gregory CLEMENT @ 2018-12-17 11:10 ` Viresh Kumar 2018-12-18 14:32 ` Gregory CLEMENT 0 siblings, 1 reply; 6+ messages in thread From: Viresh Kumar @ 2018-12-17 11:10 UTC (permalink / raw) To: Gregory CLEMENT Cc: Andrew Lunn, Jason Cooper, linux-pm, Antoine Tenart, Rafael J. Wysocki, Maxime Chevallier, Thomas Petazzoni, Miquèl Raynal, linux-arm-kernel, Sebastian Hesselbarth On 11-12-18, 17:42, Gregory CLEMENT wrote: > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > index 4e1131ef85ae..7e32a241760d 100644 > --- a/drivers/cpufreq/Kconfig.arm > +++ b/drivers/cpufreq/Kconfig.arm > @@ -25,6 +25,17 @@ config ARM_ARMADA_37XX_CPUFREQ > This adds the CPUFreq driver support for Marvell Armada 37xx SoCs. > The Armada 37xx PMU supports 4 frequency and VDD levels. > > +config ARM_ARMADA_8K_CPUFREQ > + tristate "Armada 8K CPUFreq driver" > + depends on ARCH_MVEBU && CPUFREQ_DT > + help > + This enables the CPUFreq driver support for Marvell > + Armada8k SOCs. > + Armada8K device has the AP806 which supports scaling > + to any full integer divider. > + > + If in doubt, say N. > + > # big LITTLE core layer and glue drivers > config ARM_BIG_LITTLE_CPUFREQ > tristate "Generic ARM big LITTLE CPUfreq driver" > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile > index d5ee4562ed06..db1564b610ac 100644 > --- a/drivers/cpufreq/Makefile > +++ b/drivers/cpufreq/Makefile > @@ -50,6 +50,7 @@ obj-$(CONFIG_X86_SFI_CPUFREQ) += sfi-cpufreq.o > obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o > > obj-$(CONFIG_ARM_ARMADA_37XX_CPUFREQ) += armada-37xx-cpufreq.o > +obj-$(CONFIG_ARM_ARMADA_8K_CPUFREQ) += armada-8k-cpufreq.o > obj-$(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ) += brcmstb-avs-cpufreq.o > obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o > obj-$(CONFIG_ARCH_DAVINCI) += davinci-cpufreq.o > diff --git a/drivers/cpufreq/armada-8k-cpufreq.c b/drivers/cpufreq/armada-8k-cpufreq.c > new file mode 100644 > index 000000000000..1db1953fb43e > --- /dev/null > +++ b/drivers/cpufreq/armada-8k-cpufreq.c > @@ -0,0 +1,186 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * CPUFreq support for Armada 8K > + * > + * Copyright (C) 2018 Marvell > + * > + * Omri Itach <omrii@marvell.com> > + * Gregory Clement <gregory.clement@bootlin.com> > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/clk.h> > +#include <linux/cpu.h> > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_opp.h> > +#include <linux/slab.h> > + > +/* > + * Setup the opps list with the divider for the max frequency, that > + * will be filled at runtime. > + */ > +static const int opps_div[] __initconst = {1, 2, 3, 4}; > + > +static struct platform_device *armada_8k_pdev; > +static struct freq_table *freq_tables; > +static int opps_index; > + > +struct freq_table { > + struct device *cpu_dev; > + struct clk *clk; > + unsigned int freq[ARRAY_SIZE(opps_div)]; > +}; > + > +/* If the CPUs share the same clock, then they are in the same cluster. */ > +static void __init armada_8k_get_sharing_cpus(struct clk *cur_clk, > + struct cpumask *cpumask) > +{ > + int cpu; > + > + for_each_possible_cpu(cpu) { > + struct device *cpu_dev = get_cpu_device(cpu); What if this fails ? > + struct clk *clk = clk_get(cpu_dev, 0); > + > + if (IS_ERR(clk)) > + dev_warn(cpu_dev, "Cannot get clock for CPU %d\n", cpu); > + else if (clk_is_match(clk, cur_clk)) > + cpumask_set_cpu(cpu, cpumask); > + > + clk_put(clk); So you will do clk_put() even if clk_get() failed, that will trigger a WARN() if I am not wrong. > + } > +} > + > +static int armada_8k_add_opp(struct clk *clk, struct device *cpu_dev, > + struct freq_table *freq_tables, int opps_index) > +{ > + unsigned int cur_frequency; > + unsigned int freq; > + int i, ret; > + > + /* Get nominal (current) CPU frequency. */ > + cur_frequency = clk_get_rate(clk); > + > + if (!cur_frequency) { > + dev_err(cpu_dev, > + "Failed to get clock rate for this CPU\n"); > + return -EINVAL; > + } > + > + freq_tables[opps_index].cpu_dev = cpu_dev; > + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { > + freq = cur_frequency / opps_div[i]; > + > + ret = dev_pm_opp_add(cpu_dev, freq, 0); > + if (ret) > + return ret; > + > + freq_tables[opps_index].freq[i] = freq; > + } > + > + return 0; > +} > + > +static void armada_8k_cpufreq_free_table(void) > +{ > + opps_index--; > + for (; opps_index >= 0; opps_index--) { > + int i; > + > + for (i = 0; i < ARRAY_SIZE(opps_div); i++) > + dev_pm_opp_remove(freq_tables[opps_index].cpu_dev, > + freq_tables[opps_index].freq[i]); > + } > + > + kfree(freq_tables); > +} > + > +static int __init armada_8k_cpufreq_init(void) > +{ > + int ret = 0, cpu, nb_cpus; > + struct device_node *node; > + > + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); > + if (!node || !of_device_is_available(node)) > + return -ENODEV; > + > + nb_cpus = num_possible_cpus(); > + freq_tables = kcalloc(nb_cpus, sizeof(*freq_tables), GFP_KERNEL); Add a blank line here. > + /* > + * For each CPU, this loop registers the operating points > + * supported (which are the nominal CPU frequency and full integer > + * divisions of it). > + */ > + for_each_possible_cpu(cpu) { > + struct device *cpu_dev; > + struct cpumask cpus; > + struct clk *clk; > + int i, skip; > + > + skip = 0; > + cpu_dev = get_cpu_device(cpu); > + > + if (!cpu_dev) { > + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); > + continue; > + } > + > + clk = devm_clk_get(cpu_dev, 0); I don't think you should call devm_*() helpers on the cpu_dev pointer, this is not like the platform device structure. The problem is that no driver gets attached/detached from cpu_dev and so the resources may never get free. > + > + if (IS_ERR(clk)) { > + dev_err(cpu_dev, "Cannot get clock for CPU %d\n", cpu); > + ret = PTR_ERR(clk); > + goto remove_opp; > + } > + > + for (i = 0; i < nb_cpus; i++) { > + if (clk_is_match(clk, freq_tables[i].clk)) { > + skip = 1; Why not do devm_clk_put() from right here and "continue" after that. That way you can avoid "skip" variable as well. Also why are you doing this clk-match at all ? This looks similar to what armada_8k_get_sharing_cpus() is doing. You can optimize this stuff better I believe. I will do this: Create a cpumask variable and assign possible_cpus mask to it. And then after each iteration of this loop, I clear "cpus" (returned from armada_8k_get_sharing_cpus()) from the CPUs from that cpumask. That way the loop will never try the second CPU from the same cpumask again. > + break; > + } > + } > + if (skip) { > + devm_clk_put(cpu_dev, clk); > + continue; > + } > + > + freq_tables[opps_index].clk = clk; > + > + ret = armada_8k_add_opp(clk, cpu_dev, freq_tables, opps_index); And this opps_index thing is not very clean. Having a global variable this way makes things very messy, which is being updated from several routines. So if armada_8k_add_opp() fails after adding few OPPs, we will now call armada_8k_cpufreq_free_table() which will first do opps_index-- and so you may miss freeing the OPPs added during the last call to armada_8k_add_opp(). This is very messy and requires serious cleanup to be honest. Also it will be more preferred if armada_8k_add_opp() cleans up all the allocations it has done on a call if it fails in the middle of it, rather than a bigger routine freeing both successful and failed ones. > + if (ret) > + goto remove_opp; > + > + opps_index++; > + cpumask_clear(&cpus); > + armada_8k_get_sharing_cpus(clk, &cpus); > + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); > + } > + > + armada_8k_pdev = platform_device_register_simple("cpufreq-dt", -1, > + NULL, 0); > + ret = PTR_ERR_OR_ZERO(armada_8k_pdev); > + if (ret) > + goto remove_opp; > + return 0; > + > +remove_opp: > + armada_8k_cpufreq_free_table(); > + return ret; > +} > +module_init(armada_8k_cpufreq_init); > + > +static void __exit armada_8k_cpufreq_exit(void) > +{ > + armada_8k_cpufreq_free_table(); > + platform_device_unregister(armada_8k_pdev); > +} > +module_exit(armada_8k_cpufreq_exit); > + > +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); > +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); > +MODULE_LICENSE("GPL"); > -- > 2.19.2 -- viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-12-17 11:10 ` Viresh Kumar @ 2018-12-18 14:32 ` Gregory CLEMENT 2018-12-19 4:40 ` Viresh Kumar 0 siblings, 1 reply; 6+ messages in thread From: Gregory CLEMENT @ 2018-12-18 14:32 UTC (permalink / raw) To: Viresh Kumar Cc: Andrew Lunn, Jason Cooper, linux-pm, Antoine Tenart, Rafael J. Wysocki, Maxime Chevallier, Thomas Petazzoni, Miquèl Raynal, linux-arm-kernel, Sebastian Hesselbarth Hi Viresh, On lun., déc. 17 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 11-12-18, 17:42, Gregory CLEMENT wrote: >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm >> index 4e1131ef85ae..7e32a241760d 100644 >> --- a/drivers/cpufreq/Kconfig.arm >> +++ b/drivers/cpufreq/Kconfig.arm >> @@ -25,6 +25,17 @@ config ARM_ARMADA_37XX_CPUFREQ >> This adds the CPUFreq driver support for Marvell Armada 37xx SoCs. >> The Armada 37xx PMU supports 4 frequency and VDD levels. >> >> +config ARM_ARMADA_8K_CPUFREQ >> + tristate "Armada 8K CPUFreq driver" >> + depends on ARCH_MVEBU && CPUFREQ_DT >> + help >> + This enables the CPUFreq driver support for Marvell >> + Armada8k SOCs. >> + Armada8K device has the AP806 which supports scaling >> + to any full integer divider. >> + >> + If in doubt, say N. >> + >> # big LITTLE core layer and glue drivers >> config ARM_BIG_LITTLE_CPUFREQ >> tristate "Generic ARM big LITTLE CPUfreq driver" >> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile >> index d5ee4562ed06..db1564b610ac 100644 >> --- a/drivers/cpufreq/Makefile >> +++ b/drivers/cpufreq/Makefile >> @@ -50,6 +50,7 @@ obj-$(CONFIG_X86_SFI_CPUFREQ) += sfi-cpufreq.o >> obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o >> >> obj-$(CONFIG_ARM_ARMADA_37XX_CPUFREQ) += armada-37xx-cpufreq.o >> +obj-$(CONFIG_ARM_ARMADA_8K_CPUFREQ) += armada-8k-cpufreq.o >> obj-$(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ) += brcmstb-avs-cpufreq.o >> obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o >> obj-$(CONFIG_ARCH_DAVINCI) += davinci-cpufreq.o >> diff --git a/drivers/cpufreq/armada-8k-cpufreq.c b/drivers/cpufreq/armada-8k-cpufreq.c >> new file mode 100644 >> index 000000000000..1db1953fb43e >> --- /dev/null >> +++ b/drivers/cpufreq/armada-8k-cpufreq.c >> @@ -0,0 +1,186 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * CPUFreq support for Armada 8K >> + * >> + * Copyright (C) 2018 Marvell >> + * >> + * Omri Itach <omrii@marvell.com> >> + * Gregory Clement <gregory.clement@bootlin.com> >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include <linux/clk.h> >> +#include <linux/cpu.h> >> +#include <linux/err.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_opp.h> >> +#include <linux/slab.h> >> + >> +/* >> + * Setup the opps list with the divider for the max frequency, that >> + * will be filled at runtime. >> + */ >> +static const int opps_div[] __initconst = {1, 2, 3, 4}; >> + >> +static struct platform_device *armada_8k_pdev; >> +static struct freq_table *freq_tables; >> +static int opps_index; >> + >> +struct freq_table { >> + struct device *cpu_dev; >> + struct clk *clk; >> + unsigned int freq[ARRAY_SIZE(opps_div)]; >> +}; >> + >> +/* If the CPUs share the same clock, then they are in the same cluster. */ >> +static void __init armada_8k_get_sharing_cpus(struct clk *cur_clk, >> + struct cpumask *cpumask) >> +{ >> + int cpu; >> + >> + for_each_possible_cpu(cpu) { >> + struct device *cpu_dev = get_cpu_device(cpu); > > What if this fails ? cpu_dev will be NULL so the clk_get() will fail to find the clock which is handled just after. But if needed we can add this extra test and differed the clk_get. > >> + struct clk *clk = clk_get(cpu_dev, 0); >> + >> + if (IS_ERR(clk)) >> + dev_warn(cpu_dev, "Cannot get clock for CPU %d\n", cpu); >> + else if (clk_is_match(clk, cur_clk)) >> + cpumask_set_cpu(cpu, cpumask); >> + >> + clk_put(clk); > > So you will do clk_put() even if clk_get() failed, that will trigger a WARN() if > I am not wrong. Yes there is a WARN_ONCE in this case, I can move the clk_put in the else part. > >> + } >> +} >> + >> +static int armada_8k_add_opp(struct clk *clk, struct device *cpu_dev, >> + struct freq_table *freq_tables, int opps_index) >> +{ >> + unsigned int cur_frequency; >> + unsigned int freq; >> + int i, ret; >> + >> + /* Get nominal (current) CPU frequency. */ >> + cur_frequency = clk_get_rate(clk); >> + >> + if (!cur_frequency) { >> + dev_err(cpu_dev, >> + "Failed to get clock rate for this CPU\n"); >> + return -EINVAL; >> + } >> + >> + freq_tables[opps_index].cpu_dev = cpu_dev; >> + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { >> + freq = cur_frequency / opps_div[i]; >> + >> + ret = dev_pm_opp_add(cpu_dev, freq, 0); >> + if (ret) >> + return ret; >> + >> + freq_tables[opps_index].freq[i] = freq; >> + } >> + >> + return 0; >> +} >> + >> +static void armada_8k_cpufreq_free_table(void) >> +{ >> + opps_index--; >> + for (; opps_index >= 0; opps_index--) { >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(opps_div); i++) >> + dev_pm_opp_remove(freq_tables[opps_index].cpu_dev, >> + freq_tables[opps_index].freq[i]); >> + } >> + >> + kfree(freq_tables); >> +} >> + >> +static int __init armada_8k_cpufreq_init(void) >> +{ >> + int ret = 0, cpu, nb_cpus; >> + struct device_node *node; >> + >> + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); >> + if (!node || !of_device_is_available(node)) >> + return -ENODEV; >> + >> + nb_cpus = num_possible_cpus(); >> + freq_tables = kcalloc(nb_cpus, sizeof(*freq_tables), GFP_KERNEL); > > Add a blank line here. OK > >> + /* >> + * For each CPU, this loop registers the operating points >> + * supported (which are the nominal CPU frequency and full integer >> + * divisions of it). >> + */ >> + for_each_possible_cpu(cpu) { >> + struct device *cpu_dev; >> + struct cpumask cpus; >> + struct clk *clk; >> + int i, skip; >> + >> + skip = 0; >> + cpu_dev = get_cpu_device(cpu); >> + >> + if (!cpu_dev) { >> + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); >> + continue; >> + } >> + >> + clk = devm_clk_get(cpu_dev, 0); > > I don't think you should call devm_*() helpers on the cpu_dev pointer, this is > not like the platform device structure. The problem is that no driver gets > attached/detached from cpu_dev and so the resources may never get > free. OK > >> + >> + if (IS_ERR(clk)) { >> + dev_err(cpu_dev, "Cannot get clock for CPU %d\n", cpu); >> + ret = PTR_ERR(clk); >> + goto remove_opp; >> + } >> + >> + for (i = 0; i < nb_cpus; i++) { >> + if (clk_is_match(clk, freq_tables[i].clk)) { >> + skip = 1; > > Why not do devm_clk_put() from right here and "continue" after that. That way > you can avoid "skip" variable as well. We need to keep a reference to the clk when it used later in the loop. > Also why are you doing this clk-match at all ? This looks similar to what > armada_8k_get_sharing_cpus() is doing. You can optimize this stuff better I > believe. > > I will do this: > > Create a cpumask variable and assign possible_cpus mask to it. And then after > each iteration of this loop, I clear "cpus" (returned from > armada_8k_get_sharing_cpus()) from the CPUs from that cpumask. That way the loop > will never try the second CPU from the same cpumask again. I will try it > >> + break; >> + } >> + } >> + if (skip) { >> + devm_clk_put(cpu_dev, clk); >> + continue; >> + } >> + >> + freq_tables[opps_index].clk = clk; >> + >> + ret = armada_8k_add_opp(clk, cpu_dev, freq_tables, opps_index); > > And this opps_index thing is not very clean. Having a global variable this way > makes things very messy, which is being updated from several routines. I think you missed the fact that no variable are passed to the exit function but we still need to have a reference on the OPP to free, so we cant do without this global variable. However I could reduce the amount by hiding them in a platform data associated to the platform_device. > > So if armada_8k_add_opp() fails after adding few OPPs, we will now call > armada_8k_cpufreq_free_table() which will first do opps_index-- and so you may > miss freeing the OPPs added during the last call to armada_8k_add_opp(). > Actually opps_index is only modify after adding succefully an opp, so an OPP can't be missed. > This is very messy and requires serious cleanup to be honest. Also it will be > more preferred if armada_8k_add_opp() cleans up all the allocations it has done > on a call if it fails in the middle of it, rather than a bigger routine freeing > both successful and failed ones. OK we can duplicate code for this if you prefer. Thanks, Gregory > >> + if (ret) >> + goto remove_opp; >> + >> + opps_index++; >> + cpumask_clear(&cpus); >> + armada_8k_get_sharing_cpus(clk, &cpus); >> + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); >> + } >> + >> + armada_8k_pdev = platform_device_register_simple("cpufreq-dt", -1, >> + NULL, 0); >> + ret = PTR_ERR_OR_ZERO(armada_8k_pdev); >> + if (ret) >> + goto remove_opp; >> + return 0; >> + >> +remove_opp: >> + armada_8k_cpufreq_free_table(); >> + return ret; >> +} >> +module_init(armada_8k_cpufreq_init); >> + >> +static void __exit armada_8k_cpufreq_exit(void) >> +{ >> + armada_8k_cpufreq_free_table(); >> + platform_device_unregister(armada_8k_pdev); >> +} >> +module_exit(armada_8k_cpufreq_exit); >> + >> +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); >> +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.19.2 > > -- > viresh -- Gregory Clement, Bootlin Embedded Linux and Kernel engineering http://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-12-18 14:32 ` Gregory CLEMENT @ 2018-12-19 4:40 ` Viresh Kumar 0 siblings, 0 replies; 6+ messages in thread From: Viresh Kumar @ 2018-12-19 4:40 UTC (permalink / raw) To: Gregory CLEMENT Cc: Andrew Lunn, Jason Cooper, linux-pm, Antoine Tenart, Rafael J. Wysocki, Maxime Chevallier, Thomas Petazzoni, Miquèl Raynal, linux-arm-kernel, Sebastian Hesselbarth On 18-12-18, 15:32, Gregory CLEMENT wrote: > On lun., déc. 17 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 11-12-18, 17:42, Gregory CLEMENT wrote: > >> +++ b/drivers/cpufreq/armada-8k-cpufreq.c > >> + for (i = 0; i < nb_cpus; i++) { > >> + if (clk_is_match(clk, freq_tables[i].clk)) { > >> + skip = 1; > > > > Why not do devm_clk_put() from right here and "continue" after that. That way > > you can avoid "skip" variable as well. > > We need to keep a reference to the clk when it used later in the loop. Maybe I wasn't able to clarify what I was suggesting. This code can be written as: for (i = 0; i < nb_cpus; i++) { if (clk_is_match(clk, freq_tables[i].clk)) { devm_clk_put(cpu_dev, clk); continue; } } I don't see how the current code is different than this. > I think you missed the fact that no variable are passed to the exit > function but we still need to have a reference on the OPP to free, so we > cant do without this global variable. However I could reduce the amount > by hiding them in a platform data associated to the platform_device. Maybe, lets see how the simplified form of the driver looks like. And then see if something is still left to be done. -- viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-19 4:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-11 16:42 [PATCH v3 0/2] Add cpufreq support for Armada 8K Gregory CLEMENT 2018-12-11 16:42 ` [PATCH v3 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver Gregory CLEMENT 2018-12-11 16:42 ` [PATCH v3 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K Gregory CLEMENT 2018-12-17 11:10 ` Viresh Kumar 2018-12-18 14:32 ` Gregory CLEMENT 2018-12-19 4:40 ` Viresh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).