Re: [PATCHv2 4/8] ARM: mvebu: extend PMSU code to support dynamic frequency scaling

On 07 Jul 04:51 PM, Thomas Petazzoni wrote:

Maybe NOTIFY_DONE should be used here?

+
+	/* Clear any previous DFS DONE event */
+	reg = readl(pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(cpu));
+	reg &= ~PMSU_EVENT_STATUS_AND_MASK_DFS_DONE;
+	writel(reg, pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(cpu));
+
+	/* Mask the DFS done interrupt, since we are going to poll */
+	reg = readl(pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(cpu));
+	reg |= PMSU_EVENT_STATUS_AND_MASK_DFS_DONE_MASK;
+	writel(reg, pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(cpu));
+
+	/* Trigger the DFS on the appropriate CPU */
+	smp_call_function_single(get_logical_index(cpu),
+				 armada_xp_cpufreq_clk_set_local, NULL, false);
+
+	/* Poll until the DFS done event is generated */
+	timeout = jiffies + HZ;
+	while (time_before(jiffies, timeout)) {
+		reg = readl(pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(cpu));
+		if (reg & PMSU_EVENT_STATUS_AND_MASK_DFS_DONE)
+			break;
+		udelay(10);
+	}
+
+	/* Restore the DFS mask to its original state */
+	reg = readl(pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(cpu));
+	reg &= ~BIT(17);
+	writel(reg, pmsu_mp_base + PMSU_EVENT_STATUS_AND_MASK(cpu));
+
+	return NOTIFY_DONE;

I think NOTIFY_OK should be used here, not sure.

+}
+
+static int __init armada_xp_pmsu_cpufreq_init(void)
+{
+	struct device_node *np;
+	struct resource res;
+	int ret, cpu;
+
+	if (!of_machine_is_compatible("marvell,armadaxp"))
+		return 0;
+
+	/*
+	 * In order to have proper cpufreq handling, we need to ensure
+	 * that the Device Tree description of the CPU clock includes
+	 * the definition of the PMU DFS registers. If not, we do not
+	 * register the clock notifier and the cpufreq driver. This
+	 * piece of code is only for compatibility with old Device
+	 * Trees.
+	 */
+	np = of_find_compatible_node(NULL, NULL, "marvell,armada-xp-cpu-clock");
+	if (!np)
+		return 0;
+
+	ret = of_address_to_resource(np, 1, &res);
+	if (ret) {
+		pr_warn(FW_WARN "not enabling cpufreq, deprecated armada-xp-cpu-clock binding\n");
+		of_node_put(np);
+		return 0;
+	}
+
+	of_node_put(np);
+
+	/*
+	 * For each CPU, this loop registers the operating points
+	 * supported (which are the nominal CPU frequency and half of
+	 * it), and registers the clock notifier that will take care
+	 * of doing the PMSU part of a frequency transition.
+	 */
+	for_each_possible_cpu(cpu) {
+		struct clk *clk;
+		struct device *cpu_dev;
+		struct armada_xp_cpufreq_notifier_block *nbs;
+		int ret;
+
+		cpu_dev = get_cpu_device(cpu);
+		if (!cpu_dev) {
+			pr_err("Cannot get CPU %d\n", cpu);
+			continue;
+		}
+
+		clk = clk_get(cpu_dev, 0);
+		if (!clk) {
+			pr_err("Cannot get clock for CPU %d\n", cpu);
+			return -ENODEV;

You are leaking the notifier block here.

+		}
+
+		ret = dev_pm_opp_add(cpu_dev, clk_get_rate(clk), 0);
+		if (ret) {
+			clk_put(clk);
+			return ret;

Ditto.

+		}
+
+		ret = dev_pm_opp_add(cpu_dev, clk_get_rate(clk) / 2, 0);
+		if (ret) {
+			clk_put(clk);
+			return ret;

Ditto.

+		}
+
+		nbs = kzalloc(sizeof(struct armada_xp_cpufreq_notifier_block),
+			      GFP_KERNEL);
+		if (!nbs) {
+			pr_err("Cannot allocate memory for cpufreq notifier\n");

You don't need out of memory messages, to error will be really verbose by
itself.

+			clk_put(clk);
+			return -ENOMEM;

And here you leak as well.

+		}
+
+		nbs->nb.notifier_call = armada_xp_cpufreq_clk_notify;

I'd say this is the notifier and use "armada_xp_cpufreq_clk_notifier",
but it's just a nitpick.

+		nbs->cpu = cpu;
+
+		ret = clk_notifier_register(clk, &nbs->nb);
+		if (ret) {
+			pr_err("Cannot register clock notifier\n");
+			kfree(nbs);
+			clk_put(clk);
+			return ret;

Ditto.

+		}
+	}
+
+	platform_device_register_simple("cpufreq-generic", -1, NULL, 0);
+	return 0;
+}
+
+device_initcall(armada_xp_pmsu_cpufreq_init);
-- 
2.0.0

Reviewed-by: Ezequiel Garcia <redacted>

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com