* [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).