On 24-09-18, 17:12, Gregory CLEMENT wrote:

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.

Maybe add another tab before + to align with other lines ?

7K ?

+ *
+ * Copyright (C) 2018 Marvell
+ *
+ * Omri Itach [off-list ref]
+ * Gregory Clement [off-list ref]
+ */
+
+#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 [off-list ref]");
+MODULE_DESCRIPTION("Armada 8K cpufreq driver");
+MODULE_LICENSE("GPL");
-- 
2.19.0

-- 
viresh