Hi Thomas,

quoted

Unlike the Armada XP and the Armada 370, this SoC uses a Cortex A9
core.

Isn't there some more details missing in this sentence? When I read it,
at the end of the sentence, I'm wondering "and so what? what does this
implies in terms of cpuidle support?".

Humm, actually, it the next sentence was wrongly formulate. Using a
Cortex A9 implies(or at least encourage) the use of ARM L2 cache and of
the SCU.

quoted

Beside this, the main difference for the cpu idle is the way to
handle the L2 cache and the use of SCU.

[...]

quoted

 		pr_err("unable to request region\n");
 		ret = -EBUSY;
+

This change.

quoted

 		goto out;
 	}
 

@@ -163,7 +181,6 @@ static int __init mvebu_v7_pmsu_init(void)
 		ret = -ENOMEM;
 		goto out;
 	}
-

And this one look like spurious changes not related to the patch.

yes of course.

quoted

  out:
 	of_node_put(np);
 	return ret;

@@ -260,6 +277,27 @@ static int armada_xp_370_cpu_suspend(unsigned long deepidle)
 	return cpu_suspend(deepidle, do_armada_xp_370_cpu_suspend);
 }
 
+static noinline int do_armada_38x_cpu_suspend(unsigned long deepidle)
+{
+	mvebu_v7_pmsu_idle_prepare(deepidle, false);
+	/*
+	 * Already flushed cache, but do it again as the outer cache
+	 * functions dirty the cache with spinlocks
+	 */
+	v7_exit_coherency_flush(louis);
+
+	scu_power_mode(scu_base, SCU_PM_POWEROFF);
+
+	cpu_do_idle();

I see cpu_do_idle() does dsb() and wfi(), so why don't we use in the
do_armada_370_xp_cpu_suspend() function ?

We can use cpu_do_idle() in do_armada_370_xp_cpu_suspend() too.

quoted

+
+	return 1;

You return 1 here, but in the do_armada_370_xp_cpu_suspend() function
you return zero. Is the return value being used? Why use 0 in one case
and 1 in the other?

return 1 means error. But I think it was a nasty trick to not enter in the
CPU_PM_EXIT case. I forgot to go back on this, once the cpu idle was working
on Armada 38x. I will take care of it in the next series.

[...]

quoted

+{
+	struct device_node *np;
+	void __iomem *mpsoc_base;
+	u32 reg;
+
+	np = of_find_compatible_node(NULL, NULL,
+				"marvell,armada-380-coherency-fabric");
+	if (!np)
+		return false;

		return -ENODEV;

quoted

+	of_node_put(np);
+
+	np = of_find_compatible_node(NULL, NULL,
+				"marvell,armada-380-mpcore-soc-ctrl");
+	if (!np)
+		return false;

		return -ENODEV;

quoted

+	mpsoc_base = of_iomap(np, 0);
+	WARN_ON(!mpsoc_base);

WARN_ON() seems a bit weak for something that will make the next line
crash the kernel. What about:

	if (!mpsoc_base)
		return -ENOMEM;

I think the of_node_put(np) should be here.

OK


Thanks,

Gregory



-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com