Gregory,

On Fri, 27 Jun 2014 15:22:56 +0200, Gregory CLEMENT wrote:

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?".

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

Beside -> Besides.

of SCU -> of the SCU

+static void __iomem *scu_base;

It's really a shame that the SCU driver isn't a driver on its own,
capable of storing the base address of the SCU instead of having to
pass it around everywhere. But for sure it's not the point of this
patch series to improve this as well.

This change.

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

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

+
+	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?

Spurious change.

Should go to the cpuidle driver, IMO.

As mentioned earlier, this function should return an 'int'.

+{
+	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;

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

		return -ENODEV;

+	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.

+
+	/* Set up reset mask when powering down the cpus */
+	reg = readl(mpsoc_base + MPCORE_RESET_CTL);
+	reg |= MPCORE_RESET_CTL_L2;
+	reg |= MPCORE_RESET_CTL_DEBUG;
+	writel(reg, mpsoc_base + MPCORE_RESET_CTL);
+	iounmap(mpsoc_base);
+	of_node_put(np);
+
+	/* Set up delay */
+	reg = readl(pmsu_mp_base + PMSU_POWERDOWN_DELAY);
+	reg &= ~PMSU_POWERDOWN_DELAY_MASK;
+	reg |= PMSU_DFLT_ARMADA38X_DELAY;
+	reg |= PMSU_POWERDOWN_DELAY_PMU;
+	writel(reg, pmsu_mp_base + PMSU_POWERDOWN_DELAY);
+
+	scu_base = mvebu_get_scu_base();
+	mvebu_cpu_resume = armada_38x_cpu_resume;
+	mvebu_v7_cpuidle_device.dev.platform_data = &armada_38x_cpuidle;
+	return true;

	return 0;

As mentioned in the comments to a previous commit, I'm not sure using a
match table is the best idea here.

Yes, I believe we will need it. I'll try to do some testing and report.

+	bl	v7_invalidate_l1
+	mrc     p15, 4, r1, c15, c0	@ get SCU base address
+	orr	r1, r1, #0x8		@ SCU CPU Power Status Register
+	mrc	15, 0, r0, cr0, cr0, 5	@ get the CPU ID
+	and	r0, r0, #15
+	add	r1, r1, r0
+	mov	r0, #0x0
+	strb	r0, [r1]		@ switch SCU power state to Normal mode
+	b	cpu_resume
+ENDPROC(armada_38x_cpu_resume)
+
 .global mvebu_boot_wa_start
 .global mvebu_boot_wa_end
 

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com