Hi Jason,

On 26/03/2014 01:30, Jason Cooper wrote:

On Tue, Mar 25, 2014 at 11:48:18PM +0100, Gregory CLEMENT wrote:

quoted

The initial binding for PMSU were wrong. It didn't take into account
all the registers from the PMSU and moreover it referred to registers
which are not part of PMSU.

The Power Management Unit Service block also controls the Coherency
Fabric subsystem. These registers are needed for the CPU idle
implementation for the Armada 370/XP, it allows to enter a deep CPU
idle state where the Coherency Fabric and the L2 cache are powered
down.

This commit add support for a new compatible for the PMSU node
including the block related to the coherency fabric. It also keeps
compatibility with the old binding

This patch also adds warnings if one of the base registers set can't
be ioremapped.

Signed-off-by: Gregory CLEMENT <redacted>
---
 arch/arm/mach-mvebu/pmsu.c | 47 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index d71ef53107c4..865bcb651e01 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c

@@ -27,11 +27,21 @@
 static void __iomem *pmsu_mp_base;
 static void __iomem *pmsu_reset_base;
 
-#define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu)	((cpu * 0x100) + 0x24)
+#define PMSU_BASE_OFFSET    0x100
+#define PMSU_REG_SIZE	    0x1000
+
+#define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu)	((cpu * 0x100) + 0x124)
 #define PMSU_RESET_CTL_OFFSET(cpu)		(cpu * 0x8)
 
 static struct of_device_id of_pmsu_table[] = {
-	{.compatible = "marvell,armada-370-xp-pmsu"},
+	{
+		.compatible = "marvell,armada-370-pmsu",
+		.data = (void *) false,

This looks sketchy to me.

Could you elaborate it?

For a boolean I didn't saw the point to use a pointer.

quoted

+	},
+	{
+		.compatible = "marvell,armada-370-xp-pmsu",
+		.data = (void *) true, /* legacy */

Same.

quoted

+	},
 	{ /* end of list */ },
 };
 

@@ -59,15 +69,42 @@ int armada_xp_boot_cpu(unsigned int cpu_id, void *boot_addr)
 }
 #endif
 
+static void __init armada_370_xp_pmsu_legacy_init(struct device_node *np)
+{
+	u32 addr;
+	pr_warn("*** Warning ***  Using an old binding which will be deprecated\n");

This should be noted in the binding docs...

quoted

+	/* We just need the adress, we already know the size */

nit. s/adress/address/

Thanks

quoted

+	addr = be32_to_cpu(*of_get_address(np, 0, NULL, NULL));
+	addr -= PMSU_BASE_OFFSET;
+	pmsu_mp_base = ioremap(addr, PMSU_REG_SIZE);
+	of_node_put(np);
+}
+
 static int __init armada_370_xp_pmsu_init(void)
 {
 	struct device_node *np;
-

Please leave this empty line.

OK

quoted

 	np = of_find_matching_node(NULL, of_pmsu_table);
 	if (np) {
+		const struct of_device_id *match =
+			of_match_node(of_pmsu_table, np);
+		BUG_ON(!match);
+
 		pr_info("Initializing Power Management Service Unit\n");
-		pmsu_mp_base = of_iomap(np, 0);
-		pmsu_reset_base = of_iomap(np, 1);
+
+		if (match->data) /* legacy */
+			armada_370_xp_pmsu_legacy_init(np);

And if a new compatible string actually needs data passed?

in this case we would have to update the of_pmsu_table, so this
code could be also updated if needed in the same time. So I don't
see the problem, maybe I miss something.

But the plan is really to remove this legacy part later (after a
few kernel release)

quoted

+		else
+			pmsu_mp_base = of_iomap(np, 0);
+		WARN_ON(!pmsu_mp_base);
+		of_node_put(np);
+
+		/*
+		 * This temporaty hack will be removed as soon as we

nit. s/temporaty/temporary/

Thanks

thx,

Jason.

quoted

+		 * get the proper reset controler support
+		 */
+		np = of_find_compatible_node(NULL, NULL, "marvell,armada-xp-cpu-reset");
+		pmsu_reset_base = of_iomap(np, 0);
+		WARN_ON(!pmsu_reset_base);
 		of_node_put(np);
 	}
 
-- 
1.8.1.2

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