* [PATCH 0/2] Add cpufreq support for Armada 8K @ 2018-09-24 15:12 ` Gregory CLEMENT 0 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-24 15:12 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar, linux-pm Cc: Andrew Lunn, Baruch Siach, Jason Cooper, Antoine Tenart, Gregory CLEMENT, Omri Itach, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Thomas Petazzoni, Miquèl Raynal, Marcin Wojtas, Hanna Hawa, linux-arm-kernel, Sebastian Hesselbarth Hi, this is the second version of a the series adding the cpufreq support for the Marvell Armada 8K SoCs. Gregory Changelog: 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 | 148 ++++++++++++++++++++++++++++ 4 files changed, 161 insertions(+) create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c -- 2.19.0 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 0/2] Add cpufreq support for Armada 8K @ 2018-09-24 15:12 ` Gregory CLEMENT 0 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-24 15:12 UTC (permalink / raw) To: linux-arm-kernel Hi, this is the second version of a the series adding the cpufreq support for the Marvell Armada 8K SoCs. Gregory Changelog: 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 | 148 ++++++++++++++++++++++++++++ 4 files changed, 161 insertions(+) create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c -- 2.19.0 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver 2018-09-24 15:12 ` Gregory CLEMENT @ 2018-09-24 15:12 ` Gregory CLEMENT -1 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-24 15:12 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar, linux-pm Cc: Andrew Lunn, Baruch Siach, Jason Cooper, Antoine Tenart, Gregory CLEMENT, Omri Itach, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Thomas Petazzoni, Miquèl Raynal, Marcin Wojtas, Hanna Hawa, 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 a5b256b25905..a48cf7b36156 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1657,6 +1657,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.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver @ 2018-09-24 15:12 ` Gregory CLEMENT 0 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-24 15:12 UTC (permalink / raw) To: linux-arm-kernel 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 a5b256b25905..a48cf7b36156 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1657,6 +1657,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.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-09-24 15:12 ` Gregory CLEMENT @ 2018-09-24 15:12 ` Gregory CLEMENT -1 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-24 15:12 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar, linux-pm Cc: Andrew Lunn, Baruch Siach, Jason Cooper, Antoine Tenart, Gregory CLEMENT, Omri Itach, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Thomas Petazzoni, Miquèl Raynal, Marcin Wojtas, Hanna Hawa, 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 | 148 ++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+) create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 0cd8eb76ad59..fd34095743a7 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 c1ffeabe4ecf..21f4f56229db 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -53,6 +53,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.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..1c604f1d27d2 --- /dev/null +++ b/drivers/cpufreq/armada-8k-cpufreq.c @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * CPUFreq support for Armada 7K/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> + +#define MIN_FREQ 100000000 + +/* + * Setup the opps list with the divider for the max frequency, that + * will be filled at runtime, 0 meaning 100Mhz + */ +static const int opps_div[] __initconst = {1, 2, 3, 0}; + +struct opps_array { + struct device *cpu_dev; + 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); + + if (clk_is_match(clk, cur_clk)) + cpumask_set_cpu(cpu, cpumask); + } + +} + +static int __init armada_8k_cpufreq_init(void) +{ + struct opps_array *opps_arrays; + struct platform_device *pdev; + int ret, cpu, opps_index = 0; + unsigned int cur_frequency; + struct device_node *node; + + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); + if (!node || !of_device_is_available(node)) + return -ENODEV; + + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), + 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; + unsigned int freq; + struct clk *clk; + int i; + + cpu_dev = get_cpu_device(cpu); + if (!cpu_dev) { + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); + continue; + } + + clk = 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 free_opp_array; + } + + /* Get nominal (current) CPU frequency */ + cur_frequency = clk_get_rate(clk); + if (!cur_frequency) { + dev_err(cpu_dev, + "Failed to get clock rate for CPU %d\n", cpu); + ret = -EINVAL; + goto free_opp_array; + } + + opps_arrays[opps_index].cpu_dev = cpu_dev; + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { + if (opps_div[i]) + freq = cur_frequency / opps_div[i]; + else + freq = MIN_FREQ; + + ret = dev_pm_opp_add(cpu_dev, freq, 0); + if (ret) + goto remove_opp; + + opps_arrays[opps_index].freq[i] = freq; + } + + cpumask_clear(&cpus); + armada_8k_get_sharing_cpus(clk, &cpus); + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); + } + + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); + ret = PTR_ERR_OR_ZERO(pdev); + if (ret) + goto remove_opp; + kfree(opps_arrays); + return 0; + +remove_opp: + for (; opps_index >= 0; opps_index--) { + int i = 0; + + while (opps_arrays[opps_index].freq[i]) { + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, + opps_arrays[opps_index].freq[i]); + i++; + } + } +free_opp_array: + kfree(opps_arrays); + return ret; +} +module_init(armada_8k_cpufreq_init); + +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); +MODULE_LICENSE("GPL"); -- 2.19.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K @ 2018-09-24 15:12 ` Gregory CLEMENT 0 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-24 15:12 UTC (permalink / raw) To: linux-arm-kernel 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 | 148 ++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+) create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 0cd8eb76ad59..fd34095743a7 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 c1ffeabe4ecf..21f4f56229db 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -53,6 +53,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.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..1c604f1d27d2 --- /dev/null +++ b/drivers/cpufreq/armada-8k-cpufreq.c @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * CPUFreq support for Armada 7K/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> + +#define MIN_FREQ 100000000 + +/* + * Setup the opps list with the divider for the max frequency, that + * will be filled at runtime, 0 meaning 100Mhz + */ +static const int opps_div[] __initconst = {1, 2, 3, 0}; + +struct opps_array { + struct device *cpu_dev; + 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); + + if (clk_is_match(clk, cur_clk)) + cpumask_set_cpu(cpu, cpumask); + } + +} + +static int __init armada_8k_cpufreq_init(void) +{ + struct opps_array *opps_arrays; + struct platform_device *pdev; + int ret, cpu, opps_index = 0; + unsigned int cur_frequency; + struct device_node *node; + + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); + if (!node || !of_device_is_available(node)) + return -ENODEV; + + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), + 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; + unsigned int freq; + struct clk *clk; + int i; + + cpu_dev = get_cpu_device(cpu); + if (!cpu_dev) { + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); + continue; + } + + clk = 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 free_opp_array; + } + + /* Get nominal (current) CPU frequency */ + cur_frequency = clk_get_rate(clk); + if (!cur_frequency) { + dev_err(cpu_dev, + "Failed to get clock rate for CPU %d\n", cpu); + ret = -EINVAL; + goto free_opp_array; + } + + opps_arrays[opps_index].cpu_dev = cpu_dev; + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { + if (opps_div[i]) + freq = cur_frequency / opps_div[i]; + else + freq = MIN_FREQ; + + ret = dev_pm_opp_add(cpu_dev, freq, 0); + if (ret) + goto remove_opp; + + opps_arrays[opps_index].freq[i] = freq; + } + + cpumask_clear(&cpus); + armada_8k_get_sharing_cpus(clk, &cpus); + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); + } + + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); + ret = PTR_ERR_OR_ZERO(pdev); + if (ret) + goto remove_opp; + kfree(opps_arrays); + return 0; + +remove_opp: + for (; opps_index >= 0; opps_index--) { + int i = 0; + + while (opps_arrays[opps_index].freq[i]) { + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, + opps_arrays[opps_index].freq[i]); + i++; + } + } +free_opp_array: + kfree(opps_arrays); + return ret; +} +module_init(armada_8k_cpufreq_init); + +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); +MODULE_LICENSE("GPL"); -- 2.19.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-09-24 15:12 ` Gregory CLEMENT @ 2018-10-04 5:06 ` Viresh Kumar -1 siblings, 0 replies; 18+ messages in thread From: Viresh Kumar @ 2018-10-04 5:06 UTC (permalink / raw) To: Gregory CLEMENT Cc: Andrew Lunn, Baruch Siach, Jason Cooper, linux-pm, Antoine Tenart, Hanna Hawa, Omri Itach, Rafael J. Wysocki, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Thomas Petazzoni, Miquèl Raynal, Marcin Wojtas, linux-arm-kernel, Sebastian Hesselbarth On 24-09-18, 17:12, Gregory CLEMENT wrote: > 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 | 148 ++++++++++++++++++++++++++++ > 3 files changed, 160 insertions(+) > create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > index 0cd8eb76ad59..fd34095743a7 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" Since you wanted this to be tristate, have you tried insmod armada-8k-cpufreq.ko rmmod armada-8k-cpufreq.ko insmod armada-8k-cpufreq.ko ? That will show some failures I believe. > + 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 c1ffeabe4ecf..21f4f56229db 100644 > --- a/drivers/cpufreq/Makefile > +++ b/drivers/cpufreq/Makefile > @@ -53,6 +53,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o > obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.o > > obj-$(CONFIG_ARM_ARMADA_37XX_CPUFREQ) += armada-37xx-cpufreq.o > +obj-$(CONFIG_ARM_ARMADA_8K_CPUFREQ) += armada-8k-cpufreq.o Maybe add another tab before + to align with other lines ? > 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..1c604f1d27d2 > --- /dev/null > +++ b/drivers/cpufreq/armada-8k-cpufreq.c > @@ -0,0 +1,148 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * CPUFreq support for Armada 7K/8K 7K ? > + * > + * 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> > + > +#define MIN_FREQ 100000000 > + > +/* > + * Setup the opps list with the divider for the max frequency, that > + * will be filled at runtime, 0 meaning 100Mhz Add a full-stop at the end please. I am not sure why will you want to support 100 MHz as the CPUs may end up running on that a lot :) > + */ > +static const int opps_div[] __initconst = {1, 2, 3, 0}; > + > +struct opps_array { Maybe name this freq_table. > + struct device *cpu_dev; > + 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) Maybe align better to calm down checkpatch --strict :) > +{ > + int cpu; > + > + for_each_possible_cpu(cpu) { > + struct device *cpu_dev = get_cpu_device(cpu); > + struct clk *clk = clk_get(cpu_dev, 0); Don't you need to do a corresponding clk_put() ? > + > + if (IS_ERR(clk)) > + dev_warn(cpu_dev, "Cannot get clock for CPU %d\n", cpu); Why continue after this point and check the next if ? Maybe write this as if/else. > + > + if (clk_is_match(clk, cur_clk)) > + cpumask_set_cpu(cpu, cpumask); > + } > + Remove blank line ? > +} > + > +static int __init armada_8k_cpufreq_init(void) > +{ > + struct opps_array *opps_arrays; > + struct platform_device *pdev; > + int ret, cpu, opps_index = 0; > + unsigned int cur_frequency; > + struct device_node *node; > + > + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); > + if (!node || !of_device_is_available(node)) > + return -ENODEV; > + > + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), > + 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; > + unsigned int freq; > + struct clk *clk; > + int i; > + > + cpu_dev = get_cpu_device(cpu); > + if (!cpu_dev) { > + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); > + continue; > + } > + > + clk = 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 free_opp_array; What if this is the second iteration of this loop. Who will remove the OPPs added during earlier iteration ? > + } > + > + /* Get nominal (current) CPU frequency */ > + cur_frequency = clk_get_rate(clk); > + if (!cur_frequency) { > + dev_err(cpu_dev, > + "Failed to get clock rate for CPU %d\n", cpu); > + ret = -EINVAL; clk_put() ? > + goto free_opp_array; > + } > + > + opps_arrays[opps_index].cpu_dev = cpu_dev; I will add a blank line here. > + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { > + if (opps_div[i]) > + freq = cur_frequency / opps_div[i]; > + else > + freq = MIN_FREQ; > + > + ret = dev_pm_opp_add(cpu_dev, freq, 0); > + if (ret) > + goto remove_opp; > + > + opps_arrays[opps_index].freq[i] = freq; > + } > + > + cpumask_clear(&cpus); > + armada_8k_get_sharing_cpus(clk, &cpus); > + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); Don't you get any error or print messages like duplicate-OPPs ? Maybe they are dev_dbg() :( You have already done above for a group of CPUs and you shouldn't be adding the OPPs again for the same group for an iteration that belongs to the next CPU. So if you have CPU 0-3 sharing the clock line, then you will add 4 OPPs in the first iteration and then try the same for 3 more times for the CPU 1,2,3. You should really avoid it. And generally the error handling should improve here. > + } > + > + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); > + ret = PTR_ERR_OR_ZERO(pdev); > + if (ret) > + goto remove_opp; > + kfree(opps_arrays); > + return 0; > + > +remove_opp: > + for (; opps_index >= 0; opps_index--) { > + int i = 0; > + > + while (opps_arrays[opps_index].freq[i]) { > + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, > + opps_arrays[opps_index].freq[i]); > + i++; > + } > + } > +free_opp_array: > + kfree(opps_arrays); > + return ret; > +} > +module_init(armada_8k_cpufreq_init); > + > +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); > +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); > +MODULE_LICENSE("GPL"); > -- > 2.19.0 -- viresh ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K @ 2018-10-04 5:06 ` Viresh Kumar 0 siblings, 0 replies; 18+ messages in thread From: Viresh Kumar @ 2018-10-04 5:06 UTC (permalink / raw) To: linux-arm-kernel On 24-09-18, 17:12, Gregory CLEMENT wrote: > 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 | 148 ++++++++++++++++++++++++++++ > 3 files changed, 160 insertions(+) > create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > index 0cd8eb76ad59..fd34095743a7 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" Since you wanted this to be tristate, have you tried insmod armada-8k-cpufreq.ko rmmod armada-8k-cpufreq.ko insmod armada-8k-cpufreq.ko ? That will show some failures I believe. > + 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 c1ffeabe4ecf..21f4f56229db 100644 > --- a/drivers/cpufreq/Makefile > +++ b/drivers/cpufreq/Makefile > @@ -53,6 +53,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o > obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.o > > obj-$(CONFIG_ARM_ARMADA_37XX_CPUFREQ) += armada-37xx-cpufreq.o > +obj-$(CONFIG_ARM_ARMADA_8K_CPUFREQ) += armada-8k-cpufreq.o Maybe add another tab before + to align with other lines ? > 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..1c604f1d27d2 > --- /dev/null > +++ b/drivers/cpufreq/armada-8k-cpufreq.c > @@ -0,0 +1,148 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * CPUFreq support for Armada 7K/8K 7K ? > + * > + * 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> > + > +#define MIN_FREQ 100000000 > + > +/* > + * Setup the opps list with the divider for the max frequency, that > + * will be filled at runtime, 0 meaning 100Mhz Add a full-stop at the end please. I am not sure why will you want to support 100 MHz as the CPUs may end up running on that a lot :) > + */ > +static const int opps_div[] __initconst = {1, 2, 3, 0}; > + > +struct opps_array { Maybe name this freq_table. > + struct device *cpu_dev; > + 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) Maybe align better to calm down checkpatch --strict :) > +{ > + int cpu; > + > + for_each_possible_cpu(cpu) { > + struct device *cpu_dev = get_cpu_device(cpu); > + struct clk *clk = clk_get(cpu_dev, 0); Don't you need to do a corresponding clk_put() ? > + > + if (IS_ERR(clk)) > + dev_warn(cpu_dev, "Cannot get clock for CPU %d\n", cpu); Why continue after this point and check the next if ? Maybe write this as if/else. > + > + if (clk_is_match(clk, cur_clk)) > + cpumask_set_cpu(cpu, cpumask); > + } > + Remove blank line ? > +} > + > +static int __init armada_8k_cpufreq_init(void) > +{ > + struct opps_array *opps_arrays; > + struct platform_device *pdev; > + int ret, cpu, opps_index = 0; > + unsigned int cur_frequency; > + struct device_node *node; > + > + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); > + if (!node || !of_device_is_available(node)) > + return -ENODEV; > + > + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), > + 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; > + unsigned int freq; > + struct clk *clk; > + int i; > + > + cpu_dev = get_cpu_device(cpu); > + if (!cpu_dev) { > + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); > + continue; > + } > + > + clk = 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 free_opp_array; What if this is the second iteration of this loop. Who will remove the OPPs added during earlier iteration ? > + } > + > + /* Get nominal (current) CPU frequency */ > + cur_frequency = clk_get_rate(clk); > + if (!cur_frequency) { > + dev_err(cpu_dev, > + "Failed to get clock rate for CPU %d\n", cpu); > + ret = -EINVAL; clk_put() ? > + goto free_opp_array; > + } > + > + opps_arrays[opps_index].cpu_dev = cpu_dev; I will add a blank line here. > + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { > + if (opps_div[i]) > + freq = cur_frequency / opps_div[i]; > + else > + freq = MIN_FREQ; > + > + ret = dev_pm_opp_add(cpu_dev, freq, 0); > + if (ret) > + goto remove_opp; > + > + opps_arrays[opps_index].freq[i] = freq; > + } > + > + cpumask_clear(&cpus); > + armada_8k_get_sharing_cpus(clk, &cpus); > + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); Don't you get any error or print messages like duplicate-OPPs ? Maybe they are dev_dbg() :( You have already done above for a group of CPUs and you shouldn't be adding the OPPs again for the same group for an iteration that belongs to the next CPU. So if you have CPU 0-3 sharing the clock line, then you will add 4 OPPs in the first iteration and then try the same for 3 more times for the CPU 1,2,3. You should really avoid it. And generally the error handling should improve here. > + } > + > + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); > + ret = PTR_ERR_OR_ZERO(pdev); > + if (ret) > + goto remove_opp; > + kfree(opps_arrays); > + return 0; > + > +remove_opp: > + for (; opps_index >= 0; opps_index--) { > + int i = 0; > + > + while (opps_arrays[opps_index].freq[i]) { > + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, > + opps_arrays[opps_index].freq[i]); > + i++; > + } > + } > +free_opp_array: > + kfree(opps_arrays); > + return ret; > +} > +module_init(armada_8k_cpufreq_init); > + > +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); > +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); > +MODULE_LICENSE("GPL"); > -- > 2.19.0 -- viresh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-10-04 5:06 ` Viresh Kumar @ 2018-11-15 10:36 ` Gregory CLEMENT -1 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-11-15 10:36 UTC (permalink / raw) To: Viresh Kumar Cc: Andrew Lunn, Baruch Siach, Jason Cooper, linux-pm, Antoine Tenart, Hanna Hawa, Omri Itach, Rafael J. Wysocki, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Thomas Petazzoni, Miquèl Raynal, Marcin Wojtas, linux-arm-kernel, Sebastian Hesselbarth Hi Viresh, Sorry for the delay. On jeu., oct. 04 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 24-09-18, 17:12, Gregory CLEMENT wrote: >> 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 | 148 ++++++++++++++++++++++++++++ >> 3 files changed, 160 insertions(+) >> create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c >> >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm >> index 0cd8eb76ad59..fd34095743a7 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" > > Since you wanted this to be tristate, have you tried > > insmod armada-8k-cpufreq.ko > rmmod armada-8k-cpufreq.ko > insmod armada-8k-cpufreq.ko > ? Actually the purpose of it is to be able to have the driver available as a module. Thanks to this the size kernel image is not increased. Once the system is running then we can load the module and have the feature but it is true that is no more possible to unload it. However, except for developing or debugging, I don't see the real interest to unload the driver. > > That will show some failures I believe. > >> + 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 c1ffeabe4ecf..21f4f56229db 100644 >> --- a/drivers/cpufreq/Makefile >> +++ b/drivers/cpufreq/Makefile >> @@ -53,6 +53,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o >> obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.o >> >> obj-$(CONFIG_ARM_ARMADA_37XX_CPUFREQ) += armada-37xx-cpufreq.o >> +obj-$(CONFIG_ARM_ARMADA_8K_CPUFREQ) += armada-8k-cpufreq.o > > Maybe add another tab before + to align with other lines ? I think the issue is introduced by the way the diff patch show it, because in my file they are aligned and there is onl on tab before +. It make sense because there is less than 8 character between ')' and '+' on the first line. > >> 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..1c604f1d27d2 >> --- /dev/null >> +++ b/drivers/cpufreq/armada-8k-cpufreq.c >> @@ -0,0 +1,148 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * CPUFreq support for Armada 7K/8K > > 7K ? Right, for now only 8K is supported so I will fix it. > >> + * >> + * 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> >> + >> +#define MIN_FREQ 100000000 >> + >> +/* >> + * Setup the opps list with the divider for the max frequency, that >> + * will be filled at runtime, 0 meaning 100Mhz > > Add a full-stop at the end please. I am not sure to understand what you means by a "full-stop": a 0Hz frequency? But this case should be handle by the CPU idle driver. > I am not sure why will you want to support 100 MHz as the CPUs may end > up running on that a lot :) It should depend of the policy, or am I missing something. > >> + */ >> +static const int opps_div[] __initconst = {1, 2, 3, 0}; >> + >> +struct opps_array { > > Maybe name this freq_table. OK > >> + struct device *cpu_dev; >> + 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) > > Maybe align better to calm down checkpatch --strict :) I use checkpatch --strict so I wonder how I missed it, but I will fix it. > >> +{ >> + int cpu; >> + >> + for_each_possible_cpu(cpu) { >> + struct device *cpu_dev = get_cpu_device(cpu); >> + struct clk *clk = clk_get(cpu_dev, 0); > > Don't you need to do a corresponding clk_put() ? Indeed > >> + >> + if (IS_ERR(clk)) >> + dev_warn(cpu_dev, "Cannot get clock for CPU %d\n", cpu); > > Why continue after this point and check the next if ? Maybe write this > as if/else. Yes I can do this > >> + >> + if (clk_is_match(clk, cur_clk)) >> + cpumask_set_cpu(cpu, cpumask); >> + } >> + > > Remove blank line ? > OK (and it was also reported by checkpacth!) >> +} >> + >> +static int __init armada_8k_cpufreq_init(void) >> +{ >> + struct opps_array *opps_arrays; >> + struct platform_device *pdev; >> + int ret, cpu, opps_index = 0; >> + unsigned int cur_frequency; >> + struct device_node *node; >> + >> + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); >> + if (!node || !of_device_is_available(node)) >> + return -ENODEV; >> + >> + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), >> + 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; >> + unsigned int freq; >> + struct clk *clk; >> + int i; >> + >> + cpu_dev = get_cpu_device(cpu); >> + if (!cpu_dev) { >> + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); >> + continue; >> + } >> + >> + clk = 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 free_opp_array; > > What if this is the second iteration of this loop. Who will remove the > OPPs added during earlier iteration ? > >> + } >> + >> + /* Get nominal (current) CPU frequency */ >> + cur_frequency = clk_get_rate(clk); >> + if (!cur_frequency) { >> + dev_err(cpu_dev, >> + "Failed to get clock rate for CPU %d\n", cpu); >> + ret = -EINVAL; > > clk_put() ? > >> + goto free_opp_array; >> + } >> + >> + opps_arrays[opps_index].cpu_dev = cpu_dev; > > I will add a blank line here. > >> + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { >> + if (opps_div[i]) >> + freq = cur_frequency / opps_div[i]; >> + else >> + freq = MIN_FREQ; >> + >> + ret = dev_pm_opp_add(cpu_dev, freq, 0); >> + if (ret) >> + goto remove_opp; >> + >> + opps_arrays[opps_index].freq[i] = freq; >> + } >> + >> + cpumask_clear(&cpus); >> + armada_8k_get_sharing_cpus(clk, &cpus); >> + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); > > Don't you get any error or print messages like duplicate-OPPs ? Maybe > they are dev_dbg() :( > > You have already done above for a group of CPUs and you shouldn't be > adding the OPPs again for the same group for an iteration that belongs > to the next CPU. > > So if you have CPU 0-3 sharing the clock line, then you will add 4 > OPPs in the first iteration and then try the same for 3 more times for > the CPU 1,2,3. You should really avoid it. > > And generally the error handling should improve here. I am going to see how to improve this function. Thanks, Gregory > >> + } >> + >> + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); >> + ret = PTR_ERR_OR_ZERO(pdev); >> + if (ret) >> + goto remove_opp; >> + kfree(opps_arrays); >> + return 0; >> + >> +remove_opp: >> + for (; opps_index >= 0; opps_index--) { >> + int i = 0; >> + >> + while (opps_arrays[opps_index].freq[i]) { >> + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, >> + opps_arrays[opps_index].freq[i]); >> + i++; >> + } >> + } >> +free_opp_array: >> + kfree(opps_arrays); >> + return ret; >> +} >> +module_init(armada_8k_cpufreq_init); >> + >> +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); >> +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.19.0 > > -- > viresh -- Gregory Clement, Bootlin Embedded Linux and Kernel engineering http://bootlin.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K @ 2018-11-15 10:36 ` Gregory CLEMENT 0 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-11-15 10:36 UTC (permalink / raw) To: linux-arm-kernel Hi Viresh, Sorry for the delay. On jeu., oct. 04 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 24-09-18, 17:12, Gregory CLEMENT wrote: >> 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 | 148 ++++++++++++++++++++++++++++ >> 3 files changed, 160 insertions(+) >> create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c >> >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm >> index 0cd8eb76ad59..fd34095743a7 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" > > Since you wanted this to be tristate, have you tried > > insmod armada-8k-cpufreq.ko > rmmod armada-8k-cpufreq.ko > insmod armada-8k-cpufreq.ko > ? Actually the purpose of it is to be able to have the driver available as a module. Thanks to this the size kernel image is not increased. Once the system is running then we can load the module and have the feature but it is true that is no more possible to unload it. However, except for developing or debugging, I don't see the real interest to unload the driver. > > That will show some failures I believe. > >> + 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 c1ffeabe4ecf..21f4f56229db 100644 >> --- a/drivers/cpufreq/Makefile >> +++ b/drivers/cpufreq/Makefile >> @@ -53,6 +53,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o >> obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.o >> >> obj-$(CONFIG_ARM_ARMADA_37XX_CPUFREQ) += armada-37xx-cpufreq.o >> +obj-$(CONFIG_ARM_ARMADA_8K_CPUFREQ) += armada-8k-cpufreq.o > > Maybe add another tab before + to align with other lines ? I think the issue is introduced by the way the diff patch show it, because in my file they are aligned and there is onl on tab before +. It make sense because there is less than 8 character between ')' and '+' on the first line. > >> 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..1c604f1d27d2 >> --- /dev/null >> +++ b/drivers/cpufreq/armada-8k-cpufreq.c >> @@ -0,0 +1,148 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * CPUFreq support for Armada 7K/8K > > 7K ? Right, for now only 8K is supported so I will fix it. > >> + * >> + * 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> >> + >> +#define MIN_FREQ 100000000 >> + >> +/* >> + * Setup the opps list with the divider for the max frequency, that >> + * will be filled at runtime, 0 meaning 100Mhz > > Add a full-stop at the end please. I am not sure to understand what you means by a "full-stop": a 0Hz frequency? But this case should be handle by the CPU idle driver. > I am not sure why will you want to support 100 MHz as the CPUs may end > up running on that a lot :) It should depend of the policy, or am I missing something. > >> + */ >> +static const int opps_div[] __initconst = {1, 2, 3, 0}; >> + >> +struct opps_array { > > Maybe name this freq_table. OK > >> + struct device *cpu_dev; >> + 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) > > Maybe align better to calm down checkpatch --strict :) I use checkpatch --strict so I wonder how I missed it, but I will fix it. > >> +{ >> + int cpu; >> + >> + for_each_possible_cpu(cpu) { >> + struct device *cpu_dev = get_cpu_device(cpu); >> + struct clk *clk = clk_get(cpu_dev, 0); > > Don't you need to do a corresponding clk_put() ? Indeed > >> + >> + if (IS_ERR(clk)) >> + dev_warn(cpu_dev, "Cannot get clock for CPU %d\n", cpu); > > Why continue after this point and check the next if ? Maybe write this > as if/else. Yes I can do this > >> + >> + if (clk_is_match(clk, cur_clk)) >> + cpumask_set_cpu(cpu, cpumask); >> + } >> + > > Remove blank line ? > OK (and it was also reported by checkpacth!) >> +} >> + >> +static int __init armada_8k_cpufreq_init(void) >> +{ >> + struct opps_array *opps_arrays; >> + struct platform_device *pdev; >> + int ret, cpu, opps_index = 0; >> + unsigned int cur_frequency; >> + struct device_node *node; >> + >> + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); >> + if (!node || !of_device_is_available(node)) >> + return -ENODEV; >> + >> + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), >> + 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; >> + unsigned int freq; >> + struct clk *clk; >> + int i; >> + >> + cpu_dev = get_cpu_device(cpu); >> + if (!cpu_dev) { >> + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); >> + continue; >> + } >> + >> + clk = 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 free_opp_array; > > What if this is the second iteration of this loop. Who will remove the > OPPs added during earlier iteration ? > >> + } >> + >> + /* Get nominal (current) CPU frequency */ >> + cur_frequency = clk_get_rate(clk); >> + if (!cur_frequency) { >> + dev_err(cpu_dev, >> + "Failed to get clock rate for CPU %d\n", cpu); >> + ret = -EINVAL; > > clk_put() ? > >> + goto free_opp_array; >> + } >> + >> + opps_arrays[opps_index].cpu_dev = cpu_dev; > > I will add a blank line here. > >> + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { >> + if (opps_div[i]) >> + freq = cur_frequency / opps_div[i]; >> + else >> + freq = MIN_FREQ; >> + >> + ret = dev_pm_opp_add(cpu_dev, freq, 0); >> + if (ret) >> + goto remove_opp; >> + >> + opps_arrays[opps_index].freq[i] = freq; >> + } >> + >> + cpumask_clear(&cpus); >> + armada_8k_get_sharing_cpus(clk, &cpus); >> + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); > > Don't you get any error or print messages like duplicate-OPPs ? Maybe > they are dev_dbg() :( > > You have already done above for a group of CPUs and you shouldn't be > adding the OPPs again for the same group for an iteration that belongs > to the next CPU. > > So if you have CPU 0-3 sharing the clock line, then you will add 4 > OPPs in the first iteration and then try the same for 3 more times for > the CPU 1,2,3. You should really avoid it. > > And generally the error handling should improve here. I am going to see how to improve this function. Thanks, Gregory > >> + } >> + >> + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); >> + ret = PTR_ERR_OR_ZERO(pdev); >> + if (ret) >> + goto remove_opp; >> + kfree(opps_arrays); >> + return 0; >> + >> +remove_opp: >> + for (; opps_index >= 0; opps_index--) { >> + int i = 0; >> + >> + while (opps_arrays[opps_index].freq[i]) { >> + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, >> + opps_arrays[opps_index].freq[i]); >> + i++; >> + } >> + } >> +free_opp_array: >> + kfree(opps_arrays); >> + return ret; >> +} >> +module_init(armada_8k_cpufreq_init); >> + >> +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); >> +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.19.0 > > -- > viresh -- Gregory Clement, Bootlin Embedded Linux and Kernel engineering http://bootlin.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-11-15 10:36 ` Gregory CLEMENT @ 2018-11-16 4:30 ` Viresh Kumar -1 siblings, 0 replies; 18+ messages in thread From: Viresh Kumar @ 2018-11-16 4:30 UTC (permalink / raw) To: Gregory CLEMENT Cc: Andrew Lunn, Baruch Siach, Jason Cooper, linux-pm, Antoine Tenart, Hanna Hawa, Omri Itach, Rafael J. Wysocki, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Thomas Petazzoni, Miquèl Raynal, Marcin Wojtas, linux-arm-kernel, Sebastian Hesselbarth On 15-11-18, 11:36, Gregory CLEMENT wrote: > On jeu., oct. 04 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > On 24-09-18, 17:12, Gregory CLEMENT wrote: > >> 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 | 148 ++++++++++++++++++++++++++++ > >> 3 files changed, 160 insertions(+) > >> create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c > >> > >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > >> index 0cd8eb76ad59..fd34095743a7 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" > > > > Since you wanted this to be tristate, have you tried > > > > insmod armada-8k-cpufreq.ko > > rmmod armada-8k-cpufreq.ko > > insmod armada-8k-cpufreq.ko > > ? > > Actually the purpose of it is to be able to have the driver available as > a module. Thanks to this the size kernel image is not increased. Once > the system is running then we can load the module and have the feature > but it is true that is no more possible to unload it. However, except > for developing or debugging, I don't see the real interest to unload > the driver. Sure, probably it will never get used that way but I see that as broken. If you want to have the driver as module, better support unloading of it as well. > >> +++ b/drivers/cpufreq/armada-8k-cpufreq.c > >> +/* > >> + * Setup the opps list with the divider for the max frequency, that > >> + * will be filled at runtime, 0 meaning 100Mhz > > > > Add a full-stop at the end please. > > I am not sure to understand what you means by a "full-stop": a 0Hz > frequency? > > But this case should be handle by the CPU idle driver. I meant '.' by full-stop. I wanted you to add . at the end of the line, like this: Setup the opps list with the divider for the max frequency, that will be filled at runtime, 0 meaning 100Mhz. > > I am not sure why will you want to support 100 MHz as the CPUs may end > > up running on that a lot :) > > It should depend of the policy, or am I missing something. Sure, but with any of the general governors like ondemand/conservative/schedutil, you will end up running at 100 MHz when the system doesn't have high load. And 100MHz may be way too slow. -- viresh ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K @ 2018-11-16 4:30 ` Viresh Kumar 0 siblings, 0 replies; 18+ messages in thread From: Viresh Kumar @ 2018-11-16 4:30 UTC (permalink / raw) To: linux-arm-kernel On 15-11-18, 11:36, Gregory CLEMENT wrote: > On jeu., oct. 04 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > On 24-09-18, 17:12, Gregory CLEMENT wrote: > >> 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 | 148 ++++++++++++++++++++++++++++ > >> 3 files changed, 160 insertions(+) > >> create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c > >> > >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > >> index 0cd8eb76ad59..fd34095743a7 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" > > > > Since you wanted this to be tristate, have you tried > > > > insmod armada-8k-cpufreq.ko > > rmmod armada-8k-cpufreq.ko > > insmod armada-8k-cpufreq.ko > > ? > > Actually the purpose of it is to be able to have the driver available as > a module. Thanks to this the size kernel image is not increased. Once > the system is running then we can load the module and have the feature > but it is true that is no more possible to unload it. However, except > for developing or debugging, I don't see the real interest to unload > the driver. Sure, probably it will never get used that way but I see that as broken. If you want to have the driver as module, better support unloading of it as well. > >> +++ b/drivers/cpufreq/armada-8k-cpufreq.c > >> +/* > >> + * Setup the opps list with the divider for the max frequency, that > >> + * will be filled at runtime, 0 meaning 100Mhz > > > > Add a full-stop at the end please. > > I am not sure to understand what you means by a "full-stop": a 0Hz > frequency? > > But this case should be handle by the CPU idle driver. I meant '.' by full-stop. I wanted you to add . at the end of the line, like this: Setup the opps list with the divider for the max frequency, that will be filled at runtime, 0 meaning 100Mhz. > > I am not sure why will you want to support 100 MHz as the CPUs may end > > up running on that a lot :) > > It should depend of the policy, or am I missing something. Sure, but with any of the general governors like ondemand/conservative/schedutil, you will end up running at 100 MHz when the system doesn't have high load. And 100MHz may be way too slow. -- viresh ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver @ 2018-09-21 15:43 Gregory CLEMENT 2018-09-21 15:43 ` Gregory CLEMENT 0 siblings, 1 reply; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-21 15:43 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar, linux-pm Cc: Andrew Lunn, Jason Cooper, Antoine Tenart, Gregory CLEMENT, Omri Itach, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Thomas Petazzoni, Miquèl Raynal, Marcin Wojtas, Hanna Hawa, 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 a5b256b25905..7e969fbb3626 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1657,6 +1657,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.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-09-21 15:43 [PATCH 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver Gregory CLEMENT @ 2018-09-21 15:43 ` Gregory CLEMENT 0 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-21 15:43 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar, linux-pm Cc: Andrew Lunn, Jason Cooper, Antoine Tenart, Gregory CLEMENT, Omri Itach, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Thomas Petazzoni, Miquèl Raynal, Marcin Wojtas, Hanna Hawa, 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 | 145 ++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 0cd8eb76ad59..26326918f179 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 c1ffeabe4ecf..21f4f56229db 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -53,6 +53,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.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..bde78887d7f7 --- /dev/null +++ b/drivers/cpufreq/armada-8k-cpufreq.c @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * CPUFreq support for Armada 7K/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> + +#define MIN_FREQ 100000000 + +/* + * Setup the opps list with the divider for the max frequency, that + * will be filled at runtime, 0 meaning 100Mhz + */ +static int opps_div[] __initdata = {1, 2, 3, 0}; + +struct opps_array { + struct device *cpu_dev; + unsigned int freq[ARRAY_SIZE(opps_div)]; +}; + +/* If the CPUs share the same clock, then they are in the same cluster */ +static void __init aramda_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); + + if (clk_is_match(clk, cur_clk)) + cpumask_set_cpu(cpu, cpumask); + } + +} + +static int __init armada_8k_cpufreq_init(void) +{ + struct opps_array *opps_arrays; + struct platform_device *pdev; + int ret, cpu, opps_index = 0; + unsigned int cur_frequency; + struct device_node *node; + + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); + if (!node || !of_device_is_available(node)) + return -ENODEV; + + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), + 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; + unsigned int freq; + struct clk *clk; + int i; + + cpu_dev = get_cpu_device(cpu); + if (!cpu_dev) { + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); + continue; + } + + clk = clk_get(cpu_dev, 0); + if (IS_ERR(clk)) { + dev_err(cpu_dev, "Cannot get clock for CPU %d\n", cpu); + return PTR_ERR(clk); + } + + /* Get nominal (current) CPU frequency */ + cur_frequency = clk_get_rate(clk); + if (!cur_frequency) { + dev_err(cpu_dev, + "Failed to get clock rate for CPU %d\n", cpu); + return -EINVAL; + } + + opps_arrays[opps_index].cpu_dev = cpu_dev; + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { + if (opps_div[i]) + freq = cur_frequency / opps_div[i]; + else + freq = MIN_FREQ; + + ret = dev_pm_opp_add(cpu_dev, freq, 0); + + if (ret) + goto remove_opp; + opps_arrays[opps_index].freq[i] = freq; + } + + cpumask_clear(&cpus); + aramda_8k_get_sharing_cpus(clk, &cpus); + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); + } + + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); + ret = PTR_ERR_OR_ZERO(pdev); + if (ret) + goto remove_opp; + kfree(opps_arrays); + return 0; +remove_opp: + + for (; opps_index >= 0; opps_index--) { + int i = 0; + + while (opps_arrays[opps_index].freq[i]) { + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, + opps_arrays[opps_index].freq[i]); + i++; + } + } + kfree(opps_arrays); + return ret; +} +module_init(armada_8k_cpufreq_init); + +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); +MODULE_LICENSE("GPL"); -- 2.19.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K @ 2018-09-21 15:43 ` Gregory CLEMENT 0 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-21 15:43 UTC (permalink / raw) To: linux-arm-kernel 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 | 145 ++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 drivers/cpufreq/armada-8k-cpufreq.c diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 0cd8eb76ad59..26326918f179 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 c1ffeabe4ecf..21f4f56229db 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -53,6 +53,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.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..bde78887d7f7 --- /dev/null +++ b/drivers/cpufreq/armada-8k-cpufreq.c @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * CPUFreq support for Armada 7K/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> + +#define MIN_FREQ 100000000 + +/* + * Setup the opps list with the divider for the max frequency, that + * will be filled at runtime, 0 meaning 100Mhz + */ +static int opps_div[] __initdata = {1, 2, 3, 0}; + +struct opps_array { + struct device *cpu_dev; + unsigned int freq[ARRAY_SIZE(opps_div)]; +}; + +/* If the CPUs share the same clock, then they are in the same cluster */ +static void __init aramda_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); + + if (clk_is_match(clk, cur_clk)) + cpumask_set_cpu(cpu, cpumask); + } + +} + +static int __init armada_8k_cpufreq_init(void) +{ + struct opps_array *opps_arrays; + struct platform_device *pdev; + int ret, cpu, opps_index = 0; + unsigned int cur_frequency; + struct device_node *node; + + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); + if (!node || !of_device_is_available(node)) + return -ENODEV; + + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), + 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; + unsigned int freq; + struct clk *clk; + int i; + + cpu_dev = get_cpu_device(cpu); + if (!cpu_dev) { + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); + continue; + } + + clk = clk_get(cpu_dev, 0); + if (IS_ERR(clk)) { + dev_err(cpu_dev, "Cannot get clock for CPU %d\n", cpu); + return PTR_ERR(clk); + } + + /* Get nominal (current) CPU frequency */ + cur_frequency = clk_get_rate(clk); + if (!cur_frequency) { + dev_err(cpu_dev, + "Failed to get clock rate for CPU %d\n", cpu); + return -EINVAL; + } + + opps_arrays[opps_index].cpu_dev = cpu_dev; + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { + if (opps_div[i]) + freq = cur_frequency / opps_div[i]; + else + freq = MIN_FREQ; + + ret = dev_pm_opp_add(cpu_dev, freq, 0); + + if (ret) + goto remove_opp; + opps_arrays[opps_index].freq[i] = freq; + } + + cpumask_clear(&cpus); + aramda_8k_get_sharing_cpus(clk, &cpus); + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); + } + + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); + ret = PTR_ERR_OR_ZERO(pdev); + if (ret) + goto remove_opp; + kfree(opps_arrays); + return 0; +remove_opp: + + for (; opps_index >= 0; opps_index--) { + int i = 0; + + while (opps_arrays[opps_index].freq[i]) { + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, + opps_arrays[opps_index].freq[i]); + i++; + } + } + kfree(opps_arrays); + return ret; +} +module_init(armada_8k_cpufreq_init); + +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); +MODULE_LICENSE("GPL"); -- 2.19.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-09-21 15:43 ` Gregory CLEMENT @ 2018-09-22 12:28 ` Thomas Petazzoni -1 siblings, 0 replies; 18+ messages in thread From: Thomas Petazzoni @ 2018-09-22 12:28 UTC (permalink / raw) To: Gregory CLEMENT Cc: Andrew Lunn, Jason Cooper, linux-pm, Antoine Tenart, Viresh Kumar, Hanna Hawa, Omri Itach, Rafael J. Wysocki, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Miquèl Raynal, Marcin Wojtas, linux-arm-kernel, Sebastian Hesselbarth Hello, This is not a full review, just a few things I noticed. On Fri, 21 Sep 2018 17:43:26 +0200, Gregory CLEMENT wrote: > Add cpufreq driver for Marvell AP-806 found on Aramda 8K. Armada, not Aramda. > +config ARM_ARMADA_8K_CPUFREQ > + tristate "Armada 8K CPUFreq driver" > + depends on ARCH_MVEBU && CPUFREQ_DT > + help Indentation of those items should use one tab, not spaces. > + This enables the CPUFreq driver support for Marvell > + Armada8k SOCs. > + Armada8K device has the AP806 which supports scaling > + to any full integer divider. And for the help text, it should be one tab + two spaces. > +/* > + * Setup the opps list with the divider for the max frequency, that > + * will be filled at runtime, 0 meaning 100Mhz > + */ > +static int opps_div[] __initdata = {1, 2, 3, 0}; const ? > + > +struct opps_array { > + struct device *cpu_dev; > + unsigned int freq[ARRAY_SIZE(opps_div)]; > +}; > + > +/* If the CPUs share the same clock, then they are in the same cluster */ > +static void __init aramda_8k_get_sharing_cpus(struct clk *cur_clk, Typo in function name, it should use armada, not aramda. > + 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); > + > + if (clk_is_match(clk, cur_clk)) Is it OK to call clk_is_match() is clk being an error ? > + cpumask_set_cpu(cpu, cpumask); > + } > + > +} > + > +static int __init armada_8k_cpufreq_init(void) > +{ > + struct opps_array *opps_arrays; > + struct platform_device *pdev; > + int ret, cpu, opps_index = 0; > + unsigned int cur_frequency; > + struct device_node *node; > + > + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); > + if (!node || !of_device_is_available(node)) > + return -ENODEV; > + > + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), > + 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; > + unsigned int freq; > + struct clk *clk; > + int i; > + > + cpu_dev = get_cpu_device(cpu); > + if (!cpu_dev) { > + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); > + continue; > + } > + > + clk = clk_get(cpu_dev, 0); > + if (IS_ERR(clk)) { > + dev_err(cpu_dev, "Cannot get clock for CPU %d\n", cpu); > + return PTR_ERR(clk); So here you're leaking the memory allocation of opps_array[] and everything that was done during all previous iterations of the loop. > + } > + > + /* Get nominal (current) CPU frequency */ > + cur_frequency = clk_get_rate(clk); > + if (!cur_frequency) { > + dev_err(cpu_dev, > + "Failed to get clock rate for CPU %d\n", cpu); > + return -EINVAL; Ditto. > + } > + > + opps_arrays[opps_index].cpu_dev = cpu_dev; > + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { > + if (opps_div[i]) > + freq = cur_frequency / opps_div[i]; > + else > + freq = MIN_FREQ; > + > + ret = dev_pm_opp_add(cpu_dev, freq, 0); > + Remove this blank line, to have the if (ret) just below the function call. > + if (ret) > + goto remove_opp; And add a blank line here instead. > + opps_arrays[opps_index].freq[i] = freq; > + } > + > + cpumask_clear(&cpus); > + aramda_8k_get_sharing_cpus(clk, &cpus); > + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); > + } > + > + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); > + ret = PTR_ERR_OR_ZERO(pdev); > + if (ret) > + goto remove_opp; > + kfree(opps_arrays); > + return 0; > +remove_opp: > + Please exchange those two lines: one blank line between the return and the goto label, and no blank line between the goto label and the core of the code. > + for (; opps_index >= 0; opps_index--) { > + int i = 0; > + > + while (opps_arrays[opps_index].freq[i]) { > + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, > + opps_arrays[opps_index].freq[i]); > + i++; > + } > + } > + kfree(opps_arrays); > + return ret; > +} > +module_init(armada_8k_cpufreq_init); > + > +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); > +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); > +MODULE_LICENSE("GPL"); Once again, this is not a full review, I haven't reviewed the logic of the driver itself, just a few obvious things I noticed. Best regards, Thomas Petazzoni -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K @ 2018-09-22 12:28 ` Thomas Petazzoni 0 siblings, 0 replies; 18+ messages in thread From: Thomas Petazzoni @ 2018-09-22 12:28 UTC (permalink / raw) To: linux-arm-kernel Hello, This is not a full review, just a few things I noticed. On Fri, 21 Sep 2018 17:43:26 +0200, Gregory CLEMENT wrote: > Add cpufreq driver for Marvell AP-806 found on Aramda 8K. Armada, not Aramda. > +config ARM_ARMADA_8K_CPUFREQ > + tristate "Armada 8K CPUFreq driver" > + depends on ARCH_MVEBU && CPUFREQ_DT > + help Indentation of those items should use one tab, not spaces. > + This enables the CPUFreq driver support for Marvell > + Armada8k SOCs. > + Armada8K device has the AP806 which supports scaling > + to any full integer divider. And for the help text, it should be one tab + two spaces. > +/* > + * Setup the opps list with the divider for the max frequency, that > + * will be filled at runtime, 0 meaning 100Mhz > + */ > +static int opps_div[] __initdata = {1, 2, 3, 0}; const ? > + > +struct opps_array { > + struct device *cpu_dev; > + unsigned int freq[ARRAY_SIZE(opps_div)]; > +}; > + > +/* If the CPUs share the same clock, then they are in the same cluster */ > +static void __init aramda_8k_get_sharing_cpus(struct clk *cur_clk, Typo in function name, it should use armada, not aramda. > + 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); > + > + if (clk_is_match(clk, cur_clk)) Is it OK to call clk_is_match() is clk being an error ? > + cpumask_set_cpu(cpu, cpumask); > + } > + > +} > + > +static int __init armada_8k_cpufreq_init(void) > +{ > + struct opps_array *opps_arrays; > + struct platform_device *pdev; > + int ret, cpu, opps_index = 0; > + unsigned int cur_frequency; > + struct device_node *node; > + > + node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock"); > + if (!node || !of_device_is_available(node)) > + return -ENODEV; > + > + opps_arrays = kcalloc(num_possible_cpus(), sizeof(*opps_arrays), > + 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; > + unsigned int freq; > + struct clk *clk; > + int i; > + > + cpu_dev = get_cpu_device(cpu); > + if (!cpu_dev) { > + dev_err(cpu_dev, "Cannot get CPU %d\n", cpu); > + continue; > + } > + > + clk = clk_get(cpu_dev, 0); > + if (IS_ERR(clk)) { > + dev_err(cpu_dev, "Cannot get clock for CPU %d\n", cpu); > + return PTR_ERR(clk); So here you're leaking the memory allocation of opps_array[] and everything that was done during all previous iterations of the loop. > + } > + > + /* Get nominal (current) CPU frequency */ > + cur_frequency = clk_get_rate(clk); > + if (!cur_frequency) { > + dev_err(cpu_dev, > + "Failed to get clock rate for CPU %d\n", cpu); > + return -EINVAL; Ditto. > + } > + > + opps_arrays[opps_index].cpu_dev = cpu_dev; > + for (i = 0; i < ARRAY_SIZE(opps_div); i++) { > + if (opps_div[i]) > + freq = cur_frequency / opps_div[i]; > + else > + freq = MIN_FREQ; > + > + ret = dev_pm_opp_add(cpu_dev, freq, 0); > + Remove this blank line, to have the if (ret) just below the function call. > + if (ret) > + goto remove_opp; And add a blank line here instead. > + opps_arrays[opps_index].freq[i] = freq; > + } > + > + cpumask_clear(&cpus); > + aramda_8k_get_sharing_cpus(clk, &cpus); > + dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus); > + } > + > + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); > + ret = PTR_ERR_OR_ZERO(pdev); > + if (ret) > + goto remove_opp; > + kfree(opps_arrays); > + return 0; > +remove_opp: > + Please exchange those two lines: one blank line between the return and the goto label, and no blank line between the goto label and the core of the code. > + for (; opps_index >= 0; opps_index--) { > + int i = 0; > + > + while (opps_arrays[opps_index].freq[i]) { > + dev_pm_opp_remove(opps_arrays[opps_index].cpu_dev, > + opps_arrays[opps_index].freq[i]); > + i++; > + } > + } > + kfree(opps_arrays); > + return ret; > +} > +module_init(armada_8k_cpufreq_init); > + > +MODULE_AUTHOR("Gregory Clement <gregory.clement@bootlin.com>"); > +MODULE_DESCRIPTION("Armada 8K cpufreq driver"); > +MODULE_LICENSE("GPL"); Once again, this is not a full review, I haven't reviewed the logic of the driver itself, just a few obvious things I noticed. Best regards, Thomas Petazzoni -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K 2018-09-22 12:28 ` Thomas Petazzoni @ 2018-09-24 14:45 ` Gregory CLEMENT -1 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-24 14:45 UTC (permalink / raw) To: Thomas Petazzoni Cc: Andrew Lunn, Jason Cooper, linux-pm, Antoine Tenart, Viresh Kumar, Hanna Hawa, Omri Itach, Rafael J. Wysocki, Maxime Chevallier, Nadav Haklai, Shadi Ammouri, Igal Liberman, Miquèl Raynal, Marcin Wojtas, linux-arm-kernel, Sebastian Hesselbarth Hi Thomas, On sam., sept. 22 2018, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: [...] >> +{ >> + 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); >> + >> + if (clk_is_match(clk, cur_clk)) > > Is it OK to call clk_is_match() is clk being an error ? > yes the function check the validity of the pointers. [...] > Once again, this is not a full review, I haven't reviewed the logic of > the driver itself, just a few obvious things I noticed. Thanks for this first level review, I took into account all your remarks. Gregory > > Best regards, > > Thomas Petazzoni > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com -- Gregory Clement, Bootlin Embedded Linux and Kernel engineering http://bootlin.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K @ 2018-09-24 14:45 ` Gregory CLEMENT 0 siblings, 0 replies; 18+ messages in thread From: Gregory CLEMENT @ 2018-09-24 14:45 UTC (permalink / raw) To: linux-arm-kernel Hi Thomas, On sam., sept. 22 2018, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: [...] >> +{ >> + 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); >> + >> + if (clk_is_match(clk, cur_clk)) > > Is it OK to call clk_is_match() is clk being an error ? > yes the function check the validity of the pointers. [...] > Once again, this is not a full review, I haven't reviewed the logic of > the driver itself, just a few obvious things I noticed. Thanks for this first level review, I took into account all your remarks. Gregory > > Best regards, > > Thomas Petazzoni > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com -- Gregory Clement, Bootlin Embedded Linux and Kernel engineering http://bootlin.com ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-11-16 4:30 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-24 15:12 [PATCH 0/2] Add cpufreq support for Armada 8K Gregory CLEMENT 2018-09-24 15:12 ` Gregory CLEMENT 2018-09-24 15:12 ` [PATCH 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver Gregory CLEMENT 2018-09-24 15:12 ` Gregory CLEMENT 2018-09-24 15:12 ` [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K Gregory CLEMENT 2018-09-24 15:12 ` Gregory CLEMENT 2018-10-04 5:06 ` Viresh Kumar 2018-10-04 5:06 ` Viresh Kumar 2018-11-15 10:36 ` Gregory CLEMENT 2018-11-15 10:36 ` Gregory CLEMENT 2018-11-16 4:30 ` Viresh Kumar 2018-11-16 4:30 ` Viresh Kumar -- strict thread matches above, loose matches on Subject: below -- 2018-09-21 15:43 [PATCH 1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver Gregory CLEMENT 2018-09-21 15:43 ` [PATCH 2/2] cpufreq: ap806: add cpufreq driver for Armada 8K Gregory CLEMENT 2018-09-21 15:43 ` Gregory CLEMENT 2018-09-22 12:28 ` Thomas Petazzoni 2018-09-22 12:28 ` Thomas Petazzoni 2018-09-24 14:45 ` Gregory CLEMENT 2018-09-24 14:45 ` Gregory CLEMENT
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.