diff mbox series

[2/2] cpufreq: ap806: add cpufreq driver for Armada 8K

Message ID 20180921154326.10776-2-gregory.clement@bootlin.com (mailing list archive)
State Superseded, archived
Headers show
Series [1/2] MAINTAINERS: add new entries for Armada 8K cpufreq driver | expand

Commit Message

Gregory CLEMENT Sept. 21, 2018, 3:43 p.m. UTC 
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

Comments

Thomas Petazzoni Sept. 22, 2018, 12:28 p.m. UTC | #1 
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
Gregory CLEMENT Sept. 24, 2018, 2:45 p.m. UTC | #2 
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
diff mbox series

Patch

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");