[PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

[PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

[Zhao Chenhui]

Do hardware timebase sync. Firstly, stop all timebases, and transfer
the timebase value of the boot core to the other core. Finally,
start all timebases.

Only apply to dual-core chips, such as MPC8572, P2020, etc.

Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/include/asm/fsl_guts.h |    2 +
 arch/powerpc/platforms/85xx/smp.c   |   93 +++++++++++++++++++++++++++++++++--
 2 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/fsl_guts.h b/arch/powerpc/include/asm/fsl_guts.h
index aa4c488..dd5ba2c 100644
--- a/arch/powerpc/include/asm/fsl_guts.h
+++ b/arch/powerpc/include/asm/fsl_guts.h
@@ -48,6 +48,8 @@ struct ccsr_guts {
         __be32  dmuxcr;		/* 0x.0068 - DMA Mux Control Register */
         u8	res06c[0x70 - 0x6c];
 	__be32	devdisr;	/* 0x.0070 - Device Disable Control */
+#define CCSR_GUTS_DEVDISR_TB1	0x00001000
+#define CCSR_GUTS_DEVDISR_TB0	0x00004000
 	__be32	devdisr2;	/* 0x.0074 - Device Disable Control 2 */
 	u8	res078[0x7c - 0x78];
 	__be32  pmjcr;		/* 0x.007c - 4 Power Management Jog Control Register */
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index ff42490..6862dda 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -24,6 +24,7 @@
 #include <asm/mpic.h>
 #include <asm/cacheflush.h>
 #include <asm/dbell.h>
+#include <asm/fsl_guts.h>
 
 #include <sysdev/fsl_soc.h>
 #include <sysdev/mpic.h>
@@ -115,13 +116,70 @@ smp_85xx_kick_cpu(int nr)
 
 struct smp_ops_t smp_85xx_ops = {
 	.kick_cpu = smp_85xx_kick_cpu,
-#ifdef CONFIG_KEXEC
-	.give_timebase	= smp_generic_give_timebase,
-	.take_timebase	= smp_generic_take_timebase,
-#endif
 };
 
 #ifdef CONFIG_KEXEC
+static struct ccsr_guts __iomem *guts;
+static u64 timebase;
+static int tb_req;
+static int tb_valid;
+
+static void mpc85xx_timebase_freeze(int freeze)
+{
+	unsigned int mask;
+
+	if (!guts)
+		return;
+
+	mask = CCSR_GUTS_DEVDISR_TB0 | CCSR_GUTS_DEVDISR_TB1;
+	if (freeze)
+		setbits32(&guts->devdisr, mask);
+	else
+		clrbits32(&guts->devdisr, mask);
+
+	in_be32(&guts->devdisr);
+}
+
+static void mpc85xx_give_timebase(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+
+	while (!tb_req)
+		barrier();
+	tb_req = 0;
+
+	mpc85xx_timebase_freeze(1);
+	timebase = get_tb();
+	mb();
+	tb_valid = 1;
+
+	while (tb_valid)
+		barrier();
+
+	mpc85xx_timebase_freeze(0);
+
+	local_irq_restore(flags);
+}
+
+static void mpc85xx_take_timebase(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+
+	tb_req = 1;
+	while (!tb_valid)
+		barrier();
+
+	set_tb(timebase >> 32, timebase & 0xffffffff);
+	mb();
+	tb_valid = 0;
+
+	local_irq_restore(flags);
+}
+
 atomic_t kexec_down_cpus = ATOMIC_INIT(0);
 
 void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary)
@@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr)
 		doorbell_setup_this_cpu();
 }
 
+#ifdef CONFIG_KEXEC
+static const struct of_device_id guts_ids[] = {
+	{ .compatible = "fsl,mpc8572-guts", },
+	{ .compatible = "fsl,mpc8560-guts", },
+	{ .compatible = "fsl,mpc8536-guts", },
+	{ .compatible = "fsl,p1020-guts", },
+	{ .compatible = "fsl,p1021-guts", },
+	{ .compatible = "fsl,p1022-guts", },
+	{ .compatible = "fsl,p1023-guts", },
+	{ .compatible = "fsl,p2020-guts", },
+	{},
+};
+#endif
+
 void __init mpc85xx_smp_init(void)
 {
 	struct device_node *np;
@@ -249,6 +321,19 @@ void __init mpc85xx_smp_init(void)
 		smp_85xx_ops.cause_ipi = doorbell_cause_ipi;
 	}
 
+#ifdef CONFIG_KEXEC
+	np = of_find_matching_node(NULL, guts_ids);
+	if (np) {
+		guts = of_iomap(np, 0);
+		smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
+		smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
+		of_node_put(np);
+	} else {
+		smp_85xx_ops.give_timebase = smp_generic_give_timebase;
+		smp_85xx_ops.take_timebase = smp_generic_take_timebase;
+	}
+#endif
+
 	smp_ops = &smp_85xx_ops;
 
 #ifdef CONFIG_KEXEC
-- 
1.6.4.1

[PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support

[Zhao Chenhui]

From: Li Yang <leoli@freescale.com>

Add support to disable and re-enable individual cores at runtime
on MPC85xx/QorIQ SMP machines. Currently support e500v1/e500v2 core.

MPC85xx machines use ePAPR spin-table in boot page for CPU kick-off.
This patch uses the boot page from bootloader to boot core at runtime.
It supports 32-bit and 36-bit physical address.

Add generic_set_cpu_up() to set cpu_state as CPU_UP_PREPARE in kick_cpu().

Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Jin Qing <b24347@freescale.com>
Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
---
Changes for v5:
 * Used hardware timebase sync.

 arch/powerpc/Kconfig                  |    6 +-
 arch/powerpc/include/asm/cacheflush.h |    4 +
 arch/powerpc/include/asm/smp.h        |    2 +
 arch/powerpc/kernel/head_fsl_booke.S  |   28 +++++++
 arch/powerpc/kernel/smp.c             |   10 +++
 arch/powerpc/platforms/85xx/smp.c     |  145 ++++++++++++++++++++++++---------
 6 files changed, 153 insertions(+), 42 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index feab3ba..d65ae35 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -220,7 +220,8 @@ config ARCH_HIBERNATION_POSSIBLE
 config ARCH_SUSPEND_POSSIBLE
 	def_bool y
 	depends on ADB_PMU || PPC_EFIKA || PPC_LITE5200 || PPC_83xx || \
-		   (PPC_85xx && !SMP) || PPC_86xx || PPC_PSERIES || 44x || 40x
+		   (PPC_85xx && !PPC_E500MC) || PPC_86xx || PPC_PSERIES \
+		   || 44x || 40x
 
 config PPC_DCR_NATIVE
 	bool
@@ -331,7 +332,8 @@ config SWIOTLB
 
 config HOTPLUG_CPU
 	bool "Support for enabling/disabling CPUs"
-	depends on SMP && HOTPLUG && EXPERIMENTAL && (PPC_PSERIES || PPC_PMAC || PPC_POWERNV)
+	depends on SMP && HOTPLUG && EXPERIMENTAL && (PPC_PSERIES || \
+	PPC_PMAC || PPC_POWERNV || (PPC_85xx && !PPC_E500MC))
 	---help---
 	  Say Y here to be able to disable and re-enable individual
 	  CPUs at runtime on SMP machines.
diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
index ab9e402..94ec20a 100644
--- a/arch/powerpc/include/asm/cacheflush.h
+++ b/arch/powerpc/include/asm/cacheflush.h
@@ -30,6 +30,10 @@ extern void flush_dcache_page(struct page *page);
 #define flush_dcache_mmap_lock(mapping)		do { } while (0)
 #define flush_dcache_mmap_unlock(mapping)	do { } while (0)
 
+#if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx)
+extern void __flush_disable_L1(void);
+#endif
+
 extern void __flush_icache_range(unsigned long, unsigned long);
 static inline void flush_icache_range(unsigned long start, unsigned long stop)
 {
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index ebc24dc..e807e9d 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -65,6 +65,7 @@ int generic_cpu_disable(void);
 void generic_cpu_die(unsigned int cpu);
 void generic_mach_cpu_die(void);
 void generic_set_cpu_dead(unsigned int cpu);
+void generic_set_cpu_up(unsigned int cpu);
 int generic_check_cpu_restart(unsigned int cpu);
 #endif
 
@@ -190,6 +191,7 @@ extern unsigned long __secondary_hold_spinloop;
 extern unsigned long __secondary_hold_acknowledge;
 extern char __secondary_hold;
 
+extern void __early_start(void);
 #endif /* __ASSEMBLY__ */
 
 #endif /* __KERNEL__ */
diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
index 28e6259..0cc6f02 100644
--- a/arch/powerpc/kernel/head_fsl_booke.S
+++ b/arch/powerpc/kernel/head_fsl_booke.S
@@ -1004,6 +1004,34 @@ _GLOBAL(flush_dcache_L1)
 
 	blr
 
+/* Flush L1 d-cache, invalidate and disable d-cache and i-cache */
+_GLOBAL(__flush_disable_L1)
+	mflr	r10
+	bl	flush_dcache_L1	/* Flush L1 d-cache */
+	mtlr	r10
+
+	mfspr	r4, SPRN_L1CSR0	/* Invalidate and disable d-cache */
+	li	r5, 2
+	rlwimi	r4, r5, 0, 3
+
+	msync
+	isync
+	mtspr	SPRN_L1CSR0, r4
+	isync
+
+1:	mfspr	r4, SPRN_L1CSR0	/* Wait for the invalidate to finish */
+	andi.	r4, r4, 2
+	bne	1b
+
+	mfspr	r4, SPRN_L1CSR1	/* Invalidate and disable i-cache */
+	li	r5, 2
+	rlwimi	r4, r5, 0, 3
+
+	mtspr	SPRN_L1CSR1, r4
+	isync
+
+	blr
+
 #ifdef CONFIG_SMP
 /* When we get here, r24 needs to hold the CPU # */
 	.globl __secondary_start
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index d9f9441..e0ffe03 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -423,6 +423,16 @@ void generic_set_cpu_dead(unsigned int cpu)
 	per_cpu(cpu_state, cpu) = CPU_DEAD;
 }
 
+/*
+ * The cpu_state should be set to CPU_UP_PREPARE in kick_cpu(), otherwise
+ * the cpu_state is always CPU_DEAD after calling generic_set_cpu_dead(),
+ * which makes the delay in generic_cpu_die() not happen.
+ */
+void generic_set_cpu_up(unsigned int cpu)
+{
+	per_cpu(cpu_state, cpu) = CPU_UP_PREPARE;
+}
+
 int generic_check_cpu_restart(unsigned int cpu)
 {
 	return per_cpu(cpu_state, cpu) == CPU_UP_PREPARE;
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index 6862dda..6b95d4a 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -2,7 +2,7 @@
  * Author: Andy Fleming <afleming@freescale.com>
  * 	   Kumar Gala <galak@kernel.crashing.org>
  *
- * Copyright 2006-2008, 2011 Freescale Semiconductor Inc.
+ * Copyright 2006-2008, 2011-2012 Freescale Semiconductor Inc.
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/kexec.h>
 #include <linux/highmem.h>
+#include <linux/cpu.h>
 
 #include <asm/machdep.h>
 #include <asm/pgtable.h>
@@ -30,28 +31,55 @@
 #include <sysdev/mpic.h>
 #include "smp.h"
 
-extern void __early_start(void);
-
-#define BOOT_ENTRY_ADDR_UPPER	0
-#define BOOT_ENTRY_ADDR_LOWER	1
-#define BOOT_ENTRY_R3_UPPER	2
-#define BOOT_ENTRY_R3_LOWER	3
-#define BOOT_ENTRY_RESV		4
-#define BOOT_ENTRY_PIR		5
-#define BOOT_ENTRY_R6_UPPER	6
-#define BOOT_ENTRY_R6_LOWER	7
-#define NUM_BOOT_ENTRY		8
-#define SIZE_BOOT_ENTRY		(NUM_BOOT_ENTRY * sizeof(u32))
-
-static int __init
-smp_85xx_kick_cpu(int nr)
+struct epapr_spin_table {
+	u32	addr_h;
+	u32	addr_l;
+	u32	r3_h;
+	u32	r3_l;
+	u32	reserved;
+	u32	pir;
+};
+
+static void __cpuinit smp_85xx_setup_cpu(int cpu_nr);
+
+#ifdef CONFIG_HOTPLUG_CPU
+static void __cpuinit smp_85xx_mach_cpu_die(void)
+{
+	unsigned int cpu = smp_processor_id();
+	u32 tmp;
+
+	local_irq_disable();
+	idle_task_exit();
+	generic_set_cpu_dead(cpu);
+	mb();
+
+	mtspr(SPRN_TCR, 0);
+
+	__flush_disable_L1();
+	tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP;
+	mtspr(SPRN_HID0, tmp);
+
+	/* Enter NAP mode. */
+	tmp = mfmsr();
+	tmp |= MSR_WE;
+	mb();
+	mtmsr(tmp);
+	isync();
+
+	while (1)
+		;
+}
+#endif
+
+static int __cpuinit smp_85xx_kick_cpu(int nr)
 {
 	unsigned long flags;
 	const u64 *cpu_rel_addr;
-	__iomem u32 *bptr_vaddr;
+	__iomem struct epapr_spin_table *spin_table;
 	struct device_node *np;
-	int n = 0, hw_cpu = get_hard_smp_processor_id(nr);
+	int hw_cpu = get_hard_smp_processor_id(nr);
 	int ioremappable;
+	int ret = 0;
 
 	WARN_ON(nr < 0 || nr >= NR_CPUS);
 	WARN_ON(hw_cpu < 0 || hw_cpu >= NR_CPUS);
@@ -76,49 +104,84 @@ smp_85xx_kick_cpu(int nr)
 
 	/* Map the spin table */
 	if (ioremappable)
-		bptr_vaddr = ioremap(*cpu_rel_addr, SIZE_BOOT_ENTRY);
+		spin_table = ioremap(*cpu_rel_addr,
+				sizeof(struct epapr_spin_table));
 	else
-		bptr_vaddr = phys_to_virt(*cpu_rel_addr);
+		spin_table = phys_to_virt(*cpu_rel_addr);
 
 	local_irq_save(flags);
-
-	out_be32(bptr_vaddr + BOOT_ENTRY_PIR, hw_cpu);
 #ifdef CONFIG_PPC32
-	out_be32(bptr_vaddr + BOOT_ENTRY_ADDR_LOWER, __pa(__early_start));
+#ifdef CONFIG_HOTPLUG_CPU
+	/* Corresponding to generic_set_cpu_dead() */
+	generic_set_cpu_up(nr);
+
+	if (system_state == SYSTEM_RUNNING) {
+		out_be32(&spin_table->addr_l, 0);
+
+		/*
+		 * We don't set the BPTR register here upon it points
+		 * to the boot page properly.
+		 */
+		mpic_reset_core(hw_cpu);
+
+		/* wait until core is ready... */
+		if (!spin_event_timeout(in_be32(&spin_table->addr_l) == 1,
+						10000, 100)) {
+			pr_err("%s: timeout waiting for core %d to reset\n",
+							__func__, hw_cpu);
+			ret = -ENOENT;
+			goto out;
+		}
+
+		/*  clear the acknowledge status */
+		__secondary_hold_acknowledge = -1;
+	}
+#endif
+	out_be32(&spin_table->pir, hw_cpu);
+	out_be32(&spin_table->addr_l, __pa(__early_start));
 
 	if (!ioremappable)
-		flush_dcache_range((ulong)bptr_vaddr,
-				(ulong)(bptr_vaddr + SIZE_BOOT_ENTRY));
+		flush_dcache_range((ulong)spin_table,
+			(ulong)spin_table + sizeof(struct epapr_spin_table));
 
 	/* Wait a bit for the CPU to ack. */
-	while ((__secondary_hold_acknowledge != hw_cpu) && (++n < 1000))
-		mdelay(1);
+	if (!spin_event_timeout(__secondary_hold_acknowledge == hw_cpu,
+					10000, 100)) {
+		pr_err("%s: timeout waiting for core %d to ack\n",
+						__func__, hw_cpu);
+		ret = -ENOENT;
+		goto out;
+	}
+out:
 #else
 	smp_generic_kick_cpu(nr);
 
-	out_be64((u64 *)(bptr_vaddr + BOOT_ENTRY_ADDR_UPPER),
-		__pa((u64)*((unsigned long long *) generic_secondary_smp_init)));
+	out_be32(&spin_table->pir, hw_cpu);
+	out_be64((u64 *)(&spin_table->addr_h),
+	  __pa((u64)*((unsigned long long *)generic_secondary_smp_init)));
 
 	if (!ioremappable)
-		flush_dcache_range((ulong)bptr_vaddr,
-				(ulong)(bptr_vaddr + SIZE_BOOT_ENTRY));
+		flush_dcache_range((ulong)spin_table,
+			(ulong)spin_table + sizeof(struct epapr_spin_table));
 #endif
 
 	local_irq_restore(flags);
 
 	if (ioremappable)
-		iounmap(bptr_vaddr);
-
-	pr_debug("waited %d msecs for CPU #%d.\n", n, nr);
+		iounmap(spin_table);
 
-	return 0;
+	return ret;
 }
 
 struct smp_ops_t smp_85xx_ops = {
 	.kick_cpu = smp_85xx_kick_cpu,
+#ifdef CONFIG_HOTPLUG_CPU
+	.cpu_disable	= generic_cpu_disable,
+	.cpu_die	= generic_cpu_die,
+#endif
 };
 
-#ifdef CONFIG_KEXEC
+#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
 static struct ccsr_guts __iomem *guts;
 static u64 timebase;
 static int tb_req;
@@ -179,7 +242,9 @@ static void mpc85xx_take_timebase(void)
 
 	local_irq_restore(flags);
 }
+#endif
 
+#ifdef CONFIG_KEXEC
 atomic_t kexec_down_cpus = ATOMIC_INIT(0);
 
 void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary)
@@ -276,8 +341,7 @@ static void mpc85xx_smp_machine_kexec(struct kimage *image)
 }
 #endif /* CONFIG_KEXEC */
 
-static void __init
-smp_85xx_setup_cpu(int cpu_nr)
+static void __cpuinit smp_85xx_setup_cpu(int cpu_nr)
 {
 	if (smp_85xx_ops.probe == smp_mpic_probe)
 		mpic_setup_this_cpu();
@@ -286,7 +350,7 @@ smp_85xx_setup_cpu(int cpu_nr)
 		doorbell_setup_this_cpu();
 }
 
-#ifdef CONFIG_KEXEC
+#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
 static const struct of_device_id guts_ids[] = {
 	{ .compatible = "fsl,mpc8572-guts", },
 	{ .compatible = "fsl,mpc8560-guts", },
@@ -321,10 +385,11 @@ void __init mpc85xx_smp_init(void)
 		smp_85xx_ops.cause_ipi = doorbell_cause_ipi;
 	}
 
-#ifdef CONFIG_KEXEC
+#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
 	np = of_find_matching_node(NULL, guts_ids);
 	if (np) {
 		guts = of_iomap(np, 0);
+		ppc_md.cpu_die = smp_85xx_mach_cpu_die;
 		smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
 		smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
 		of_node_put(np);
-- 
1.6.4.1

Re: [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support

[Scott Wood]

On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
\> +#if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx)
> +extern void __flush_disable_L1(void);
> +#endif

Prototypes aren't normally guarded by ifdefs.

> +static void __cpuinit smp_85xx_mach_cpu_die(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	u32 tmp;
> +
> +	local_irq_disable();
> +	idle_task_exit();
> +	generic_set_cpu_dead(cpu);
> +	mb();
> +
> +	mtspr(SPRN_TCR, 0);
> +
> +	__flush_disable_L1();
> +	tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP;
> +	mtspr(SPRN_HID0, tmp);
> +
> +	/* Enter NAP mode. */
> +	tmp = mfmsr();
> +	tmp |= MSR_WE;
> +	mb();
> +	mtmsr(tmp);
> +	isync();

Need isync after writing to HID0.

> +		/*
> +		 * We don't set the BPTR register here upon it points
> +		 * to the boot page properly.
> +		 */
> +		mpic_reset_core(hw_cpu);

That comment's wording is hard to follow -- maybe s/upon it points/since
it already points/

> +		/* wait until core is ready... */
> +		if (!spin_event_timeout(in_be32(&spin_table->addr_l) == 1,
> +						10000, 100)) {
> +			pr_err("%s: timeout waiting for core %d to reset\n",
> +							__func__, hw_cpu);
> +			ret = -ENOENT;
> +			goto out;
> +		}

We need to fix U-Boot to write addr_l last (with an msync beforehand).

> -#ifdef CONFIG_KEXEC
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)

Let's not grow lists like this.  Is there any harm in building it
unconditionally?

-Scott

Re: [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support

[Zhao Chenhui]

On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote:
> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> \> +#if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx)
> > +extern void __flush_disable_L1(void);
> > +#endif
> 
> Prototypes aren't normally guarded by ifdefs.

OK. Thanks.

> 
> > +static void __cpuinit smp_85xx_mach_cpu_die(void)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +	u32 tmp;
> > +
> > +	local_irq_disable();
> > +	idle_task_exit();
> > +	generic_set_cpu_dead(cpu);
> > +	mb();
> > +
> > +	mtspr(SPRN_TCR, 0);
> > +
> > +	__flush_disable_L1();
> > +	tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP;
> > +	mtspr(SPRN_HID0, tmp);
> > +
> > +	/* Enter NAP mode. */
> > +	tmp = mfmsr();
> > +	tmp |= MSR_WE;
> > +	mb();
> > +	mtmsr(tmp);
> > +	isync();
> 
> Need isync after writing to HID0.
> 
> > +		/*
> > +		 * We don't set the BPTR register here upon it points
> > +		 * to the boot page properly.
> > +		 */
> > +		mpic_reset_core(hw_cpu);
> 
> That comment's wording is hard to follow -- maybe s/upon it points/since
> it already points/
> 
> > +		/* wait until core is ready... */
> > +		if (!spin_event_timeout(in_be32(&spin_table->addr_l) == 1,
> > +						10000, 100)) {
> > +			pr_err("%s: timeout waiting for core %d to reset\n",
> > +							__func__, hw_cpu);
> > +			ret = -ENOENT;
> > +			goto out;
> > +		}
> 
> We need to fix U-Boot to write addr_l last (with an msync beforehand).

I agree.

> 
> > -#ifdef CONFIG_KEXEC
> > +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
> 
> Let's not grow lists like this.  Is there any harm in building it
> unconditionally?
> 
> -Scott

We need this ifdef. We only set give_timebase/take_timebase
when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined.

-Chenhui

Re: [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support

[Scott Wood]

On 06/04/2012 06:04 AM, Zhao Chenhui wrote:
> On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote:
>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>>> -#ifdef CONFIG_KEXEC
>>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
>>
>> Let's not grow lists like this.  Is there any harm in building it
>> unconditionally?
>>
>> -Scott
> 
> We need this ifdef. We only set give_timebase/take_timebase
> when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined.

If we really need this to be a compile-time decision, make a new symbol
for it, but I really think this should be decided at runtime.  Just
because we have kexec or hotplug support enabled doesn't mean that's
actually what we're doing at the moment.

-Scott

Re: [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support

[Zhao Chenhui]

On Mon, Jun 04, 2012 at 11:32:47AM -0500, Scott Wood wrote:
> On 06/04/2012 06:04 AM, Zhao Chenhui wrote:
> > On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote:
> >> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> >>> -#ifdef CONFIG_KEXEC
> >>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
> >>
> >> Let's not grow lists like this.  Is there any harm in building it
> >> unconditionally?
> >>
> >> -Scott
> > 
> > We need this ifdef. We only set give_timebase/take_timebase
> > when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined.
> 
> If we really need this to be a compile-time decision, make a new symbol
> for it, but I really think this should be decided at runtime.  Just
> because we have kexec or hotplug support enabled doesn't mean that's
> actually what we're doing at the moment.
> 
> -Scott

If user does not enable kexec or hotplug, these codes are redundant.
So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them.

-Chenhui

Re: [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support

[Scott Wood]

On 06/05/2012 06:18 AM, Zhao Chenhui wrote:
> On Mon, Jun 04, 2012 at 11:32:47AM -0500, Scott Wood wrote:
>> On 06/04/2012 06:04 AM, Zhao Chenhui wrote:
>>> On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote:
>>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>>>>> -#ifdef CONFIG_KEXEC
>>>>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
>>>>
>>>> Let's not grow lists like this.  Is there any harm in building it
>>>> unconditionally?
>>>>
>>>> -Scott
>>>
>>> We need this ifdef. We only set give_timebase/take_timebase
>>> when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined.
>>
>> If we really need this to be a compile-time decision, make a new symbol
>> for it, but I really think this should be decided at runtime.  Just
>> because we have kexec or hotplug support enabled doesn't mean that's
>> actually what we're doing at the moment.
>>
>> -Scott
> 
> If user does not enable kexec or hotplug, these codes are redundant.
> So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them.

My point is that these lists tend to grow and be a maintenance pain.
For small things it's often better to not worry about saving a few
bytes.  For larger things that need to be conditional, define a new
symbol rather than growing ORed lists like this.

-Scott

Re: [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support

[Zhao Chenhui]

On Tue, Jun 05, 2012 at 11:15:52AM -0500, Scott Wood wrote:
> On 06/05/2012 06:18 AM, Zhao Chenhui wrote:
> > On Mon, Jun 04, 2012 at 11:32:47AM -0500, Scott Wood wrote:
> >> On 06/04/2012 06:04 AM, Zhao Chenhui wrote:
> >>> On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote:
> >>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> >>>>> -#ifdef CONFIG_KEXEC
> >>>>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
> >>>>
> >>>> Let's not grow lists like this.  Is there any harm in building it
> >>>> unconditionally?
> >>>>
> >>>> -Scott
> >>>
> >>> We need this ifdef. We only set give_timebase/take_timebase
> >>> when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined.
> >>
> >> If we really need this to be a compile-time decision, make a new symbol
> >> for it, but I really think this should be decided at runtime.  Just
> >> because we have kexec or hotplug support enabled doesn't mean that's
> >> actually what we're doing at the moment.
> >>
> >> -Scott
> > 
> > If user does not enable kexec or hotplug, these codes are redundant.
> > So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them.
> 
> My point is that these lists tend to grow and be a maintenance pain.
> For small things it's often better to not worry about saving a few
> bytes.  For larger things that need to be conditional, define a new
> symbol rather than growing ORed lists like this.
> 
> -Scott

I agree with you in principle. But there are only two config options
in this patch, and it is unlikely to grow. 

-Chenhui

Re: [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support

[Scott Wood]

On 06/06/2012 04:59 AM, Zhao Chenhui wrote:
> On Tue, Jun 05, 2012 at 11:15:52AM -0500, Scott Wood wrote:
>> On 06/05/2012 06:18 AM, Zhao Chenhui wrote:
>>> If user does not enable kexec or hotplug, these codes are redundant.
>>> So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them.
>>
>> My point is that these lists tend to grow and be a maintenance pain.
>> For small things it's often better to not worry about saving a few
>> bytes.  For larger things that need to be conditional, define a new
>> symbol rather than growing ORed lists like this.
>>
>> -Scott
> 
> I agree with you in principle. But there are only two config options
> in this patch, and it is unlikely to grow. 

That's what everybody says when these things start. :-)

-Scott

[PATCH v5 3/5] powerpc/85xx: add sleep and deep sleep support

[Zhao Chenhui]

From: Li Yang <leoli@freescale.com>

In sleep PM mode, the clocks of e500 core and unused IP blocks is
turned off. IP blocks which are allowed to wake up the processor
are still running.

Some Freescale chips like MPC8536 and P1022 has deep sleep PM mode
in addtion to the sleep PM mode.

While in deep sleep PM mode, additionally, the power supply is
removed from e500 core and most IP blocks. Only the blocks needed
to wake up the chip out of deep sleep are ON.

This patch supports 32-bit and 36-bit address space.

The sleep mode is equal to the Standby state in Linux. The deep sleep
mode is equal to the Suspend-to-RAM state of Linux Power Management.

Command to enter sleep mode.
  echo standby > /sys/power/state
Command to enter deep sleep mode.
  echo mem > /sys/power/state

Signed-off-by: Dave Liu <daveliu@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Jin Qing <b24347@freescale.com>
Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
Cc: Scott Wood <scottwood@freescale.com>
Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
---
Changes for v5:
 * Rename flush_disable_L1 to __flush_disable_L1.

 arch/powerpc/Kconfig                  |    2 +-
 arch/powerpc/include/asm/cacheflush.h |    5 +
 arch/powerpc/kernel/Makefile          |    3 +
 arch/powerpc/kernel/l2cache_85xx.S    |   53 +++
 arch/powerpc/platforms/85xx/Makefile  |    3 +
 arch/powerpc/platforms/85xx/sleep.S   |  609 +++++++++++++++++++++++++++++++++
 arch/powerpc/sysdev/fsl_pmc.c         |   91 ++++-
 arch/powerpc/sysdev/fsl_soc.h         |    5 +
 8 files changed, 752 insertions(+), 19 deletions(-)
 create mode 100644 arch/powerpc/kernel/l2cache_85xx.S
 create mode 100644 arch/powerpc/platforms/85xx/sleep.S

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index d65ae35..039f0a6 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -673,7 +673,7 @@ config FSL_PCI
 config FSL_PMC
 	bool
 	default y
-	depends on SUSPEND && (PPC_85xx || PPC_86xx)
+	depends on SUSPEND && (PPC_85xx || PPC_86xx) && !PPC_E500MC
 	help
 	  Freescale MPC85xx/MPC86xx power management controller support
 	  (suspend/resume). For MPC83xx see platforms/83xx/suspend.c
diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
index 94ec20a..baa000c 100644
--- a/arch/powerpc/include/asm/cacheflush.h
+++ b/arch/powerpc/include/asm/cacheflush.h
@@ -33,6 +33,11 @@ extern void flush_dcache_page(struct page *page);
 #if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx)
 extern void __flush_disable_L1(void);
 #endif
+#if defined(CONFIG_FSL_BOOKE)
+extern void flush_dcache_L1(void);
+#else
+#define flush_dcache_L1()	do { } while (0)
+#endif
 
 extern void __flush_icache_range(unsigned long, unsigned long);
 static inline void flush_icache_range(unsigned long start, unsigned long stop)
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index f5808a3..cb70dba 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -64,6 +64,9 @@ obj-$(CONFIG_FA_DUMP)		+= fadump.o
 ifeq ($(CONFIG_PPC32),y)
 obj-$(CONFIG_E500)		+= idle_e500.o
 endif
+ifneq ($(CONFIG_PPC_E500MC),y)
+obj-$(CONFIG_PPC_85xx)		+= l2cache_85xx.o
+endif
 obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6xx.o cpu_setup_6xx.o
 obj-$(CONFIG_TAU)		+= tau_6xx.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o
diff --git a/arch/powerpc/kernel/l2cache_85xx.S b/arch/powerpc/kernel/l2cache_85xx.S
new file mode 100644
index 0000000..b0b7d1c
--- /dev/null
+++ b/arch/powerpc/kernel/l2cache_85xx.S
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2009-2012 Freescale Semiconductor, Inc. All rights reserved.
+ *	Scott Wood <scottwood@freescale.com>
+ *	Dave Liu <daveliu@freescale.com>
+ * implement the L2 cache operations of e500 based L2 controller
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <asm/reg.h>
+#include <asm/cputable.h>
+#include <asm/ppc_asm.h>
+#include <asm/asm-offsets.h>
+
+	.section .text
+
+	/* r3 = virtual address of L2 controller, WIMG = 01xx */
+_GLOBAL(flush_disable_L2)
+	/* It's a write-through cache, so only invalidation is needed. */
+	mbar
+	isync
+	lwz	r4, 0(r3)
+	li	r5, 1
+	rlwimi	r4, r5, 30, 0xc0000000
+	stw	r4, 0(r3)
+
+	/* Wait for the invalidate to finish */
+1:	lwz	r4, 0(r3)
+	andis.	r4, r4, 0x4000
+	bne	1b
+	mbar
+
+	blr
+
+	/* r3 = virtual address of L2 controller, WIMG = 01xx */
+_GLOBAL(invalidate_enable_L2)
+	mbar
+	isync
+	lwz	r4, 0(r3)
+	li	r5, 3
+	rlwimi	r4, r5, 30, 0xc0000000
+	stw	r4, 0(r3)
+
+	/* Wait for the invalidate to finish */
+1:	lwz	r4, 0(r3)
+	andis.	r4, r4, 0x4000
+	bne	1b
+	mbar
+
+	blr
diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index 2125d4c..12b526a 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -2,6 +2,9 @@
 # Makefile for the PowerPC 85xx linux kernel.
 #
 obj-$(CONFIG_SMP) += smp.o
+ifneq ($(CONFIG_PPC_E500MC),y)
+obj-$(CONFIG_SUSPEND)	+= sleep.o
+endif
 
 obj-y += common.o
 
diff --git a/arch/powerpc/platforms/85xx/sleep.S b/arch/powerpc/platforms/85xx/sleep.S
new file mode 100644
index 0000000..b272f0c
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/sleep.S
@@ -0,0 +1,609 @@
+/*
+ * Enter and leave deep sleep/sleep state on MPC85xx
+ *
+ * Author: Scott Wood <scottwood@freescale.com>
+ *
+ * Copyright (C) 2006-2012 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <asm/page.h>
+#include <asm/ppc_asm.h>
+#include <asm/reg.h>
+#include <asm/asm-offsets.h>
+
+#define SS_TB		0x00
+#define SS_HID		0x08 /* 2 HIDs */
+#define SS_IAC		0x10 /* 2 IACs */
+#define SS_DAC		0x18 /* 2 DACs */
+#define SS_DBCR		0x20 /* 3 DBCRs */
+#define SS_PID		0x2c /* 3 PIDs */
+#define SS_SPRG		0x38 /* 8 SPRGs */
+#define SS_IVOR		0x58 /* 20 interrupt vectors */
+#define SS_TCR		0xa8
+#define SS_BUCSR	0xac
+#define SS_L1CSR	0xb0 /* 2 L1CSRs */
+#define SS_MSR		0xb8
+#define SS_USPRG	0xbc
+#define SS_GPREG	0xc0 /* r12-r31 */
+#define SS_LR		0x110
+#define SS_CR		0x114
+#define SS_SP		0x118
+#define SS_CURRENT	0x11c
+#define SS_IVPR		0x120
+#define SS_BPTR		0x124
+
+
+#define STATE_SAVE_SIZE 0x128
+
+	.section .data
+	.align	5
+mpc85xx_sleep_save_area:
+	.space	STATE_SAVE_SIZE
+ccsrbase_low:
+	.long	0
+ccsrbase_high:
+	.long	0
+powmgtreq:
+	.long	0
+
+	.section .text
+	.align	12
+
+	/*
+	 * r3 = high word of physical address of CCSR
+	 * r4 = low word of physical address of CCSR
+	 * r5 = JOG or deep sleep request
+	 *      JOG-0x00200000, deep sleep-0x00100000
+	 */
+_GLOBAL(mpc85xx_enter_deep_sleep)
+	lis	r6, ccsrbase_low@ha
+	stw	r4, ccsrbase_low@l(r6)
+	lis	r6, ccsrbase_high@ha
+	stw	r3, ccsrbase_high@l(r6)
+
+	lis	r6, powmgtreq@ha
+	stw	r5, powmgtreq@l(r6)
+
+	lis	r10, mpc85xx_sleep_save_area@h
+	ori	r10, r10, mpc85xx_sleep_save_area@l
+
+	mfspr	r5, SPRN_HID0
+	mfspr	r6, SPRN_HID1
+
+	stw	r5, SS_HID+0(r10)
+	stw	r6, SS_HID+4(r10)
+
+	mfspr	r4, SPRN_IAC1
+	mfspr	r5, SPRN_IAC2
+	mfspr	r6, SPRN_DAC1
+	mfspr	r7, SPRN_DAC2
+
+	stw	r4, SS_IAC+0(r10)
+	stw	r5, SS_IAC+4(r10)
+	stw	r6, SS_DAC+0(r10)
+	stw	r7, SS_DAC+4(r10)
+
+	mfspr	r4, SPRN_DBCR0
+	mfspr	r5, SPRN_DBCR1
+	mfspr	r6, SPRN_DBCR2
+
+	stw	r4, SS_DBCR+0(r10)
+	stw	r5, SS_DBCR+4(r10)
+	stw	r6, SS_DBCR+8(r10)
+
+	mfspr	r4, SPRN_PID0
+	mfspr	r5, SPRN_PID1
+	mfspr	r6, SPRN_PID2
+
+	stw	r4, SS_PID+0(r10)
+	stw	r5, SS_PID+4(r10)
+	stw	r6, SS_PID+8(r10)
+
+	mfspr	r4, SPRN_SPRG0
+	mfspr	r5, SPRN_SPRG1
+	mfspr	r6, SPRN_SPRG2
+	mfspr	r7, SPRN_SPRG3
+
+	stw	r4, SS_SPRG+0x00(r10)
+	stw	r5, SS_SPRG+0x04(r10)
+	stw	r6, SS_SPRG+0x08(r10)
+	stw	r7, SS_SPRG+0x0c(r10)
+
+	mfspr	r4, SPRN_SPRG4
+	mfspr	r5, SPRN_SPRG5
+	mfspr	r6, SPRN_SPRG6
+	mfspr	r7, SPRN_SPRG7
+
+	stw	r4, SS_SPRG+0x10(r10)
+	stw	r5, SS_SPRG+0x14(r10)
+	stw	r6, SS_SPRG+0x18(r10)
+	stw	r7, SS_SPRG+0x1c(r10)
+
+	mfspr	r4, SPRN_IVPR
+	stw	r4, SS_IVPR(r10)
+
+	mfspr	r4, SPRN_IVOR0
+	mfspr	r5, SPRN_IVOR1
+	mfspr	r6, SPRN_IVOR2
+	mfspr	r7, SPRN_IVOR3
+
+	stw	r4, SS_IVOR+0x00(r10)
+	stw	r5, SS_IVOR+0x04(r10)
+	stw	r6, SS_IVOR+0x08(r10)
+	stw	r7, SS_IVOR+0x0c(r10)
+
+	mfspr	r4, SPRN_IVOR4
+	mfspr	r5, SPRN_IVOR5
+	mfspr	r6, SPRN_IVOR6
+	mfspr	r7, SPRN_IVOR7
+
+	stw	r4, SS_IVOR+0x10(r10)
+	stw	r5, SS_IVOR+0x14(r10)
+	stw	r6, SS_IVOR+0x18(r10)
+	stw	r7, SS_IVOR+0x1c(r10)
+
+	mfspr	r4, SPRN_IVOR8
+	mfspr	r5, SPRN_IVOR9
+	mfspr	r6, SPRN_IVOR10
+	mfspr	r7, SPRN_IVOR11
+
+	stw	r4, SS_IVOR+0x20(r10)
+	stw	r5, SS_IVOR+0x24(r10)
+	stw	r6, SS_IVOR+0x28(r10)
+	stw	r7, SS_IVOR+0x2c(r10)
+
+	mfspr	r4, SPRN_IVOR12
+	mfspr	r5, SPRN_IVOR13
+	mfspr	r6, SPRN_IVOR14
+	mfspr	r7, SPRN_IVOR15
+
+	stw	r4, SS_IVOR+0x30(r10)
+	stw	r5, SS_IVOR+0x34(r10)
+	stw	r6, SS_IVOR+0x38(r10)
+	stw	r7, SS_IVOR+0x3c(r10)
+
+	mfspr	r4, SPRN_IVOR32
+	mfspr	r5, SPRN_IVOR33
+	mfspr	r6, SPRN_IVOR34
+	mfspr	r7, SPRN_IVOR35
+
+	stw	r4, SS_IVOR+0x40(r10)
+	stw	r5, SS_IVOR+0x44(r10)
+	stw	r6, SS_IVOR+0x48(r10)
+	stw	r7, SS_IVOR+0x4c(r10)
+
+	mfspr	r4, SPRN_TCR
+	mfspr	r5, SPRN_BUCSR
+	mfspr	r6, SPRN_L1CSR0
+	mfspr	r7, SPRN_L1CSR1
+	mfspr	r8, SPRN_USPRG0
+
+	stw	r4, SS_TCR(r10)
+	stw	r5, SS_BUCSR(r10)
+	stw	r6, SS_L1CSR+0(r10)
+	stw	r7, SS_L1CSR+4(r10)
+	stw	r8, SS_USPRG+0(r10)
+
+	stmw	r12, SS_GPREG(r10)
+
+	mfmsr	r4
+	mflr	r5
+	mfcr	r6
+
+	stw	r4, SS_MSR(r10)
+	stw	r5, SS_LR(r10)
+	stw	r6, SS_CR(r10)
+	stw	r1, SS_SP(r10)
+	stw	r2, SS_CURRENT(r10)
+
+1:	mftbu	r4
+	mftb	r5
+	mftbu	r6
+	cmpw	r4, r6
+	bne	1b
+
+	stw	r4, SS_TB+0(r10)
+	stw	r5, SS_TB+4(r10)
+
+	lis	r5, ccsrbase_low@ha
+	lwz	r4, ccsrbase_low@l(r5)
+	lis	r5, ccsrbase_high@ha
+	lwz	r3, ccsrbase_high@l(r5)
+
+	/* Disable machine checks and critical exceptions */
+	mfmsr	r5
+	rlwinm	r5, r5, 0, ~MSR_CE
+	rlwinm	r5, r5, 0, ~MSR_ME
+	mtmsr	r5
+	isync
+
+	/* Use TLB1[15] to map the CCSR at 0xf0000000 */
+	lis	r5, 0x100f
+	mtspr	SPRN_MAS0, r5
+	lis	r5, 0xc000
+	ori	r5, r5, 0x0500
+	mtspr	SPRN_MAS1, r5
+	lis	r5, 0xf000
+	ori	r5, r5, 0x000a
+	mtspr	SPRN_MAS2, r5
+	rlwinm	r5, r4, 0, 0xfffff000
+	ori	r5, r5, 0x0005
+	mtspr	SPRN_MAS3, r5
+	mtspr	SPRN_MAS7, r3
+	isync
+	tlbwe
+	isync
+
+	lis	r3, 0xf000
+	lwz	r4, 0x20(r3)
+	stw	r4, SS_BPTR(r10)
+
+	lis	r3, 0xf002	/* L2 cache controller at CCSR+0x20000 */
+	bl	flush_disable_L2
+	bl	__flush_disable_L1
+
+	/* Enable I-cache, so as not to upset the bus
+	 * with our loop.
+	 */
+
+	mfspr	r4, SPRN_L1CSR1
+	ori	r4, r4, 1
+	mtspr	SPRN_L1CSR1, r4
+	isync
+
+	/* Set boot page translation */
+	lis	r3, 0xf000
+	lis	r4, (mpc85xx_deep_resume - PAGE_OFFSET)@h
+	ori	r4, r4, (mpc85xx_deep_resume - PAGE_OFFSET)@l
+	rlwinm	r4, r4, 20, 0x000fffff
+	oris	r4, r4, 0x8000
+	stw	r4, 0x20(r3)
+	lwz	r4, 0x20(r3)		/* read-back to flush write */
+	twi	0, r4, 0
+	isync
+
+	/* Disable the decrementer */
+	mfspr	r4, SPRN_TCR
+	rlwinm	r4, r4, 0, ~TCR_DIE
+	mtspr	SPRN_TCR, r4
+
+	mfspr	r4, SPRN_TSR
+	oris	r4, r4, TSR_DIS@h
+	mtspr	SPRN_TSR, r4
+
+	/* set PMRCCR[VRCNT] to wait power stable for 40ms */
+	lis	r3, 0xf00e
+	lwz	r4, 0x84(r3)
+	clrlwi	r4, r4, 16
+	oris	r4, r4, 0x12a3
+	stw	r4, 0x84(r3)
+	lwz	r4, 0x84(r3)
+
+	/* set deep sleep bit in POWMGTSCR */
+	lis	r3, powmgtreq@ha
+	lwz	r8, powmgtreq@l(r3)
+
+	lis	r3, 0xf00e
+	lwz	r4, 0x80(r3)
+	or	r4, r4, r8
+	stw	r4, 0x80(r3)
+	lwz	r4, 0x80(r3)		/* read-back to flush write */
+	twi	0, r4, 0
+	isync
+
+	mftb	r5
+1:	/* spin until either we enter deep sleep, or the sleep process is
+	 * aborted due to a pending wakeup event.  Wait some time between
+	 * accesses, so we don't flood the bus and prevent the pmc from
+	 * detecting an idle system.
+	 */
+
+	mftb	r4
+	subf	r7, r5, r4
+	cmpwi	r7, 1000
+	blt	1b
+	mr	r5, r4
+
+	lwz	r6, 0x80(r3)
+	andis.	r6, r6, 0x0010
+	bne	1b
+	b	2f
+
+2:	mfspr	r4, SPRN_PIR
+	andi.	r4, r4, 1
+99:	bne	99b
+
+	/* Establish a temporary 64MB 0->0 mapping in TLB1[1]. */
+	lis	r4, 0x1001
+	mtspr	SPRN_MAS0, r4
+	lis	r4, 0xc000
+	ori	r4, r4, 0x0800
+	mtspr	SPRN_MAS1, r4
+	li	r4, 0
+	mtspr	SPRN_MAS2, r4
+	li	r4, 0x0015
+	mtspr	SPRN_MAS3, r4
+	li	r4, 0
+	mtspr	SPRN_MAS7, r4
+	isync
+	tlbwe
+	isync
+
+	lis	r3, (3f - PAGE_OFFSET)@h
+	ori	r3, r3, (3f - PAGE_OFFSET)@l
+	mtctr	r3
+	bctr
+
+	/* Locate the resume vector in the last word of the current page. */
+	. = mpc85xx_enter_deep_sleep + 0xffc
+mpc85xx_deep_resume:
+	b	2b
+
+3:
+	/* Restore the contents of TLB1[0].  It is assumed that it covers
+	 * the currently executing code and the sleep save area, and that
+	 * it does not alias our temporary mapping (which is at virtual zero).
+	 */
+	lis	r3, (TLBCAM - PAGE_OFFSET)@h
+	ori	r3, r3, (TLBCAM - PAGE_OFFSET)@l
+
+	lwz	r4, 0(r3)
+	lwz	r5, 4(r3)
+	lwz	r6, 8(r3)
+	lwz	r7, 12(r3)
+	lwz	r8, 16(r3)
+
+	mtspr	SPRN_MAS0, r4
+	mtspr	SPRN_MAS1, r5
+	mtspr	SPRN_MAS2, r6
+	mtspr	SPRN_MAS3, r7
+	mtspr	SPRN_MAS7, r8
+
+	isync
+	tlbwe
+	isync
+
+	/* Access the ccsrbase address with TLB1[0] */
+	lis	r5, ccsrbase_low@ha
+	lwz	r4, ccsrbase_low@l(r5)
+	lis	r5, ccsrbase_high@ha
+	lwz	r3, ccsrbase_high@l(r5)
+
+	/* Use TLB1[15] to map the CCSR at 0xf0000000 */
+	lis	r5, 0x100f
+	mtspr	SPRN_MAS0, r5
+	lis	r5, 0xc000
+	ori	r5, r5, 0x0500
+	mtspr	SPRN_MAS1, r5
+	lis	r5, 0xf000
+	ori	r5, r5, 0x000a
+	mtspr	SPRN_MAS2, r5
+	rlwinm	r5, r4, 0, 0xfffff000
+	ori	r5, r5, 0x0005
+	mtspr	SPRN_MAS3, r5
+	mtspr	SPRN_MAS7, r3
+	isync
+	tlbwe
+	isync
+
+	lis	r3, 0xf002	/* L2 cache controller at CCSR+0x20000 */
+	bl	invalidate_enable_L2
+
+	/* Access the MEM(r10) with TLB1[0] */
+	lis	r10, mpc85xx_sleep_save_area@h
+	ori	r10, r10, mpc85xx_sleep_save_area@l
+
+	lis	r3, 0xf000
+	lwz	r4, SS_BPTR(r10)
+	stw	r4, 0x20(r3)		/* restore BPTR */
+
+	/* Program shift running space to PAGE_OFFSET */
+	mfmsr	r3
+	lis	r4, 1f@h
+	ori	r4, r4, 1f@l
+
+	mtsrr1	r3
+	mtsrr0	r4
+	rfi
+
+1:	/* Restore the rest of TLB1, in ascending order so that
+	 * the TLB1[1] gets invalidated first.
+	 *
+	 * XXX: It's better to invalidate the temporary mapping
+	 * TLB1[15] for CCSR before restore any TLB1 entry include 0.
+	 */
+	lis	r4, 0x100f
+	mtspr	SPRN_MAS0, r4
+	lis	r4, 0
+	mtspr	SPRN_MAS1, r4
+	isync
+	tlbwe
+	isync
+
+	lis	r3, (TLBCAM + 5*4 - 4)@h
+	ori	r3, r3, (TLBCAM + 5*4 - 4)@l
+	li	r4, 15
+	mtctr	r4
+
+2:
+	lwz	r5, 4(r3)
+	lwz	r6, 8(r3)
+	lwz	r7, 12(r3)
+	lwz	r8, 16(r3)
+	lwzu	r9, 20(r3)
+
+	mtspr	SPRN_MAS0, r5
+	mtspr	SPRN_MAS1, r6
+	mtspr	SPRN_MAS2, r7
+	mtspr	SPRN_MAS3, r8
+	mtspr	SPRN_MAS7, r9
+
+	isync
+	tlbwe
+	isync
+	bdnz	2b
+
+	lis	r10, mpc85xx_sleep_save_area@h
+	ori	r10, r10, mpc85xx_sleep_save_area@l
+
+	lwz	r5, SS_HID+0(r10)
+	lwz	r6, SS_HID+4(r10)
+
+	isync
+	mtspr	SPRN_HID0, r5
+	isync
+
+	msync
+	mtspr	SPRN_HID1, r6
+	isync
+
+	lwz	r4, SS_IAC+0(r10)
+	lwz	r5, SS_IAC+4(r10)
+	lwz	r6, SS_DAC+0(r10)
+	lwz	r7, SS_DAC+4(r10)
+
+	mtspr	SPRN_IAC1, r4
+	mtspr	SPRN_IAC2, r5
+	mtspr	SPRN_DAC1, r6
+	mtspr	SPRN_DAC2, r7
+
+	lwz	r4, SS_DBCR+0(r10)
+	lwz	r5, SS_DBCR+4(r10)
+	lwz	r6, SS_DBCR+8(r10)
+
+	mtspr	SPRN_DBCR0, r4
+	mtspr	SPRN_DBCR1, r5
+	mtspr	SPRN_DBCR2, r6
+
+	lwz	r4, SS_PID+0(r10)
+	lwz	r5, SS_PID+4(r10)
+	lwz	r6, SS_PID+8(r10)
+
+	mtspr	SPRN_PID0, r4
+	mtspr	SPRN_PID1, r5
+	mtspr	SPRN_PID2, r6
+
+	lwz	r4, SS_SPRG+0x00(r10)
+	lwz	r5, SS_SPRG+0x04(r10)
+	lwz	r6, SS_SPRG+0x08(r10)
+	lwz	r7, SS_SPRG+0x0c(r10)
+
+	mtspr	SPRN_SPRG0, r4
+	mtspr	SPRN_SPRG1, r5
+	mtspr	SPRN_SPRG2, r6
+	mtspr	SPRN_SPRG3, r7
+
+	lwz	r4, SS_SPRG+0x10(r10)
+	lwz	r5, SS_SPRG+0x14(r10)
+	lwz	r6, SS_SPRG+0x18(r10)
+	lwz	r7, SS_SPRG+0x1c(r10)
+
+	mtspr	SPRN_SPRG4, r4
+	mtspr	SPRN_SPRG5, r5
+	mtspr	SPRN_SPRG6, r6
+	mtspr	SPRN_SPRG7, r7
+
+	lwz	r4, SS_IVPR(r10)
+	mtspr	SPRN_IVPR, r4
+
+	lwz	r4, SS_IVOR+0x00(r10)
+	lwz	r5, SS_IVOR+0x04(r10)
+	lwz	r6, SS_IVOR+0x08(r10)
+	lwz	r7, SS_IVOR+0x0c(r10)
+
+	mtspr	SPRN_IVOR0, r4
+	mtspr	SPRN_IVOR1, r5
+	mtspr	SPRN_IVOR2, r6
+	mtspr	SPRN_IVOR3, r7
+
+	lwz	r4, SS_IVOR+0x10(r10)
+	lwz	r5, SS_IVOR+0x14(r10)
+	lwz	r6, SS_IVOR+0x18(r10)
+	lwz	r7, SS_IVOR+0x1c(r10)
+
+	mtspr	SPRN_IVOR4, r4
+	mtspr	SPRN_IVOR5, r5
+	mtspr	SPRN_IVOR6, r6
+	mtspr	SPRN_IVOR7, r7
+
+	lwz	r4, SS_IVOR+0x20(r10)
+	lwz	r5, SS_IVOR+0x24(r10)
+	lwz	r6, SS_IVOR+0x28(r10)
+	lwz	r7, SS_IVOR+0x2c(r10)
+
+	mtspr	SPRN_IVOR8, r4
+	mtspr	SPRN_IVOR9, r5
+	mtspr	SPRN_IVOR10, r6
+	mtspr	SPRN_IVOR11, r7
+
+	lwz	r4, SS_IVOR+0x30(r10)
+	lwz	r5, SS_IVOR+0x34(r10)
+	lwz	r6, SS_IVOR+0x38(r10)
+	lwz	r7, SS_IVOR+0x3c(r10)
+
+	mtspr	SPRN_IVOR12, r4
+	mtspr	SPRN_IVOR13, r5
+	mtspr	SPRN_IVOR14, r6
+	mtspr	SPRN_IVOR15, r7
+
+	lwz	r4, SS_IVOR+0x40(r10)
+	lwz	r5, SS_IVOR+0x44(r10)
+	lwz	r6, SS_IVOR+0x48(r10)
+	lwz	r7, SS_IVOR+0x4c(r10)
+
+	mtspr	SPRN_IVOR32, r4
+	mtspr	SPRN_IVOR33, r5
+	mtspr	SPRN_IVOR34, r6
+	mtspr	SPRN_IVOR35, r7
+
+	lwz	r4, SS_TCR(r10)
+	lwz	r5, SS_BUCSR(r10)
+	lwz	r6, SS_L1CSR+0(r10)
+	lwz	r7, SS_L1CSR+4(r10)
+	lwz	r8, SS_USPRG+0(r10)
+
+	mtspr	SPRN_TCR, r4
+	mtspr	SPRN_BUCSR, r5
+
+	msync
+	isync
+	mtspr	SPRN_L1CSR0, r6
+	isync
+
+	mtspr	SPRN_L1CSR1, r7
+	isync
+
+	mtspr	SPRN_USPRG0, r8
+
+	lmw	r12, SS_GPREG(r10)
+
+	lwz	r1, SS_SP(r10)
+	lwz	r2, SS_CURRENT(r10)
+	lwz	r4, SS_MSR(r10)
+	lwz	r5, SS_LR(r10)
+	lwz	r6, SS_CR(r10)
+
+	msync
+	mtmsr	r4
+	isync
+
+	mtlr	r5
+	mtcr	r6
+
+	li	r4, 0
+	mtspr	SPRN_TBWL, r4
+
+	lwz	r4, SS_TB+0(r10)
+	lwz	r5, SS_TB+4(r10)
+
+	mtspr	SPRN_TBWU, r4
+	mtspr	SPRN_TBWL, r5
+
+	lis	r3, 1
+	mtdec	r3
+
+	blr
diff --git a/arch/powerpc/sysdev/fsl_pmc.c b/arch/powerpc/sysdev/fsl_pmc.c
index 592a0f8..1dc6e9e 100644
--- a/arch/powerpc/sysdev/fsl_pmc.c
+++ b/arch/powerpc/sysdev/fsl_pmc.c
@@ -2,6 +2,7 @@
  * Suspend/resume support
  *
  * Copyright 2009  MontaVista Software, Inc.
+ * Copyright 2010-2012 Freescale Semiconductor Inc.
  *
  * Author: Anton Vorontsov <avorontsov@ru.mvista.com>
  *
@@ -19,39 +20,83 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/of_platform.h>
+#include <linux/pm.h>
+#include <asm/cacheflush.h>
+#include <asm/switch_to.h>
+
+#include <sysdev/fsl_soc.h>
 
 struct pmc_regs {
 	__be32 devdisr;
 	__be32 devdisr2;
-	__be32 :32;
-	__be32 :32;
-	__be32 pmcsr;
-#define PMCSR_SLP	(1 << 17)
+	__be32 res1;
+	__be32 res2;
+	__be32 powmgtcsr;
+#define POWMGTCSR_SLP		0x00020000
+#define POWMGTCSR_DPSLP		0x00100000
+	__be32 res3[2];
+	__be32 pmcdr;
 };
 
-static struct device *pmc_dev;
 static struct pmc_regs __iomem *pmc_regs;
+static unsigned int pmc_flag;
+
+#define PMC_SLEEP	0x1
+#define PMC_DEEP_SLEEP	0x2
 
 static int pmc_suspend_enter(suspend_state_t state)
 {
-	int ret;
+	int ret = 0;
+
+	switch (state) {
+#ifdef CONFIG_PPC_85xx
+	case PM_SUSPEND_MEM:
+#ifdef CONFIG_SPE
+		enable_kernel_spe();
+#endif
+		enable_kernel_fp();
+
+		pr_debug("%s: Entering deep sleep\n", __func__);
+
+		local_irq_disable();
+		mpc85xx_enter_deep_sleep(get_immrbase(), POWMGTCSR_DPSLP);
+
+		pr_debug("%s: Resumed from deep sleep\n", __func__);
+		break;
+#endif
+
+	case PM_SUSPEND_STANDBY:
+		local_irq_disable();
+		flush_dcache_L1();
 
-	setbits32(&pmc_regs->pmcsr, PMCSR_SLP);
-	/* At this point, the CPU is asleep. */
+		setbits32(&pmc_regs->powmgtcsr, POWMGTCSR_SLP);
+		/* At this point, the CPU is asleep. */
 
-	/* Upon resume, wait for SLP bit to be clear. */
-	ret = spin_event_timeout((in_be32(&pmc_regs->pmcsr) & PMCSR_SLP) == 0,
-				 10000, 10) ? 0 : -ETIMEDOUT;
-	if (ret)
-		dev_err(pmc_dev, "tired waiting for SLP bit to clear\n");
+		/* Upon resume, wait for SLP bit to be clear. */
+		ret = spin_event_timeout(
+			(in_be32(&pmc_regs->powmgtcsr) & POWMGTCSR_SLP) == 0,
+			10000, 10);
+		if (!ret) {
+			pr_err("%s: timeout waiting for SLP bit "
+				"to be cleared\n", __func__);
+			ret = -EINVAL;
+		}
+		break;
+
+	default:
+		ret = -EINVAL;
+
+	}
 	return ret;
 }
 
 static int pmc_suspend_valid(suspend_state_t state)
 {
-	if (state != PM_SUSPEND_STANDBY)
+	if (((pmc_flag & PMC_SLEEP) && (state == PM_SUSPEND_STANDBY)) ||
+	    ((pmc_flag & PMC_DEEP_SLEEP) && (state == PM_SUSPEND_MEM)))
+		return 1;
+	else
 		return 0;
-	return 1;
 }
 
 static const struct platform_suspend_ops pmc_suspend_ops = {
@@ -59,14 +104,24 @@ static const struct platform_suspend_ops pmc_suspend_ops = {
 	.enter = pmc_suspend_enter,
 };
 
-static int pmc_probe(struct platform_device *ofdev)
+static int pmc_probe(struct platform_device *pdev)
 {
-	pmc_regs = of_iomap(ofdev->dev.of_node, 0);
+	struct device_node *np = pdev->dev.of_node;
+
+	pmc_regs = of_iomap(np, 0);
 	if (!pmc_regs)
 		return -ENOMEM;
 
-	pmc_dev = &ofdev->dev;
+	pmc_flag = PMC_SLEEP;
+	if (of_device_is_compatible(np, "fsl,mpc8536-pmc"))
+		pmc_flag |= PMC_DEEP_SLEEP;
+
+	if (of_device_is_compatible(np, "fsl,p1022-pmc"))
+		pmc_flag |= PMC_DEEP_SLEEP;
+
 	suspend_set_ops(&pmc_suspend_ops);
+
+	pr_info("Freescale PMC driver\n");
 	return 0;
 }
 
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index c6d0073..949377d 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -48,5 +48,10 @@ extern struct platform_diu_data_ops diu_ops;
 void fsl_hv_restart(char *cmd);
 void fsl_hv_halt(void);
 
+/*
+ * Cast the ccsrbar to 64-bit parameter so that the assembly
+ * code can be compatible with both 32-bit & 36-bit.
+ */
+extern void mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq);
 #endif
 #endif
-- 
1.6.4.1

Re: [PATCH v5 3/5] powerpc/85xx: add sleep and deep sleep support

[Scott Wood]

On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> From: Li Yang <leoli@freescale.com>
> 
> In sleep PM mode, the clocks of e500 core and unused IP blocks is
> turned off. IP blocks which are allowed to wake up the processor
> are still running.
> 
> Some Freescale chips like MPC8536 and P1022 has deep sleep PM mode
> in addtion to the sleep PM mode.
> 
> While in deep sleep PM mode, additionally, the power supply is
> removed from e500 core and most IP blocks. Only the blocks needed
> to wake up the chip out of deep sleep are ON.
> 
> This patch supports 32-bit and 36-bit address space.
> 
> The sleep mode is equal to the Standby state in Linux. The deep sleep
> mode is equal to the Suspend-to-RAM state of Linux Power Management.
> 
> Command to enter sleep mode.
>   echo standby > /sys/power/state
> Command to enter deep sleep mode.
>   echo mem > /sys/power/state
> 
> Signed-off-by: Dave Liu <daveliu@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Jin Qing <b24347@freescale.com>
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> Cc: Scott Wood <scottwood@freescale.com>
> Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> ---
> Changes for v5:
>  * Rename flush_disable_L1 to __flush_disable_L1.
> 
>  arch/powerpc/Kconfig                  |    2 +-
>  arch/powerpc/include/asm/cacheflush.h |    5 +
>  arch/powerpc/kernel/Makefile          |    3 +
>  arch/powerpc/kernel/l2cache_85xx.S    |   53 +++
>  arch/powerpc/platforms/85xx/Makefile  |    3 +
>  arch/powerpc/platforms/85xx/sleep.S   |  609 +++++++++++++++++++++++++++++++++
>  arch/powerpc/sysdev/fsl_pmc.c         |   91 ++++-
>  arch/powerpc/sysdev/fsl_soc.h         |    5 +
>  8 files changed, 752 insertions(+), 19 deletions(-)
>  create mode 100644 arch/powerpc/kernel/l2cache_85xx.S
>  create mode 100644 arch/powerpc/platforms/85xx/sleep.S
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index d65ae35..039f0a6 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -673,7 +673,7 @@ config FSL_PCI
>  config FSL_PMC
>  	bool
>  	default y
> -	depends on SUSPEND && (PPC_85xx || PPC_86xx)
> +	depends on SUSPEND && (PPC_85xx || PPC_86xx) && !PPC_E500MC
>  	help
>  	  Freescale MPC85xx/MPC86xx power management controller support
>  	  (suspend/resume). For MPC83xx see platforms/83xx/suspend.c
> diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
> index 94ec20a..baa000c 100644
> --- a/arch/powerpc/include/asm/cacheflush.h
> +++ b/arch/powerpc/include/asm/cacheflush.h
> @@ -33,6 +33,11 @@ extern void flush_dcache_page(struct page *page);
>  #if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx)
>  extern void __flush_disable_L1(void);
>  #endif
> +#if defined(CONFIG_FSL_BOOKE)
> +extern void flush_dcache_L1(void);
> +#else
> +#define flush_dcache_L1()	do { } while (0)
> +#endif

It doesn't seem right to no-op this on other platforms.

>  extern void __flush_icache_range(unsigned long, unsigned long);
>  static inline void flush_icache_range(unsigned long start, unsigned long stop)
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index f5808a3..cb70dba 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -64,6 +64,9 @@ obj-$(CONFIG_FA_DUMP)		+= fadump.o
>  ifeq ($(CONFIG_PPC32),y)
>  obj-$(CONFIG_E500)		+= idle_e500.o
>  endif
> +ifneq ($(CONFIG_PPC_E500MC),y)
> +obj-$(CONFIG_PPC_85xx)		+= l2cache_85xx.o
> +endif

Can we introduce a symbol that specifically means pre-e500mc e500,
rather than using negative logic?

I think something like CONFIG_PPC_E500_V1_V2 has been proposed before.

> -static int pmc_probe(struct platform_device *ofdev)
> +static int pmc_probe(struct platform_device *pdev)
>  {
> -	pmc_regs = of_iomap(ofdev->dev.of_node, 0);
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	pmc_regs = of_iomap(np, 0);
>  	if (!pmc_regs)
>  		return -ENOMEM;
>  
> -	pmc_dev = &ofdev->dev;
> +	pmc_flag = PMC_SLEEP;
> +	if (of_device_is_compatible(np, "fsl,mpc8536-pmc"))
> +		pmc_flag |= PMC_DEEP_SLEEP;
> +
> +	if (of_device_is_compatible(np, "fsl,p1022-pmc"))
> +		pmc_flag |= PMC_DEEP_SLEEP;
> +
>  	suspend_set_ops(&pmc_suspend_ops);
> +
> +	pr_info("Freescale PMC driver\n");

If you're going to be noisy on probe, at least provide some useful info
like whether deep sleep or jog are supported.

>  	return 0;
>  }
>  
> diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
> index c6d0073..949377d 100644
> --- a/arch/powerpc/sysdev/fsl_soc.h
> +++ b/arch/powerpc/sysdev/fsl_soc.h
> @@ -48,5 +48,10 @@ extern struct platform_diu_data_ops diu_ops;
>  void fsl_hv_restart(char *cmd);
>  void fsl_hv_halt(void);
>  
> +/*
> + * Cast the ccsrbar to 64-bit parameter so that the assembly
> + * code can be compatible with both 32-bit & 36-bit.
> + */
> +extern void mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq);

s/Cast the ccsrbar to 64-bit parameter/ccsrbar is u64 rather than
phys_addr_t/

-Scott

Re: [PATCH v5 3/5] powerpc/85xx: add sleep and deep sleep support

[Zhao Chenhui]

On Fri, Jun 01, 2012 at 04:54:35PM -0500, Scott Wood wrote:
> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> > From: Li Yang <leoli@freescale.com>
> > 
> > In sleep PM mode, the clocks of e500 core and unused IP blocks is
> > turned off. IP blocks which are allowed to wake up the processor
> > are still running.
> > 
> > Some Freescale chips like MPC8536 and P1022 has deep sleep PM mode
> > in addtion to the sleep PM mode.
> > 
> > While in deep sleep PM mode, additionally, the power supply is
> > removed from e500 core and most IP blocks. Only the blocks needed
> > to wake up the chip out of deep sleep are ON.
> > 
> > This patch supports 32-bit and 36-bit address space.
> > 
> > The sleep mode is equal to the Standby state in Linux. The deep sleep
> > mode is equal to the Suspend-to-RAM state of Linux Power Management.
> > 
> > Command to enter sleep mode.
> >   echo standby > /sys/power/state
> > Command to enter deep sleep mode.
> >   echo mem > /sys/power/state
> > 
> > Signed-off-by: Dave Liu <daveliu@freescale.com>
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Jin Qing <b24347@freescale.com>
> > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > Cc: Scott Wood <scottwood@freescale.com>
> > Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> > ---
> > Changes for v5:
> >  * Rename flush_disable_L1 to __flush_disable_L1.
> > 
> >  arch/powerpc/Kconfig                  |    2 +-
> >  arch/powerpc/include/asm/cacheflush.h |    5 +
> >  arch/powerpc/kernel/Makefile          |    3 +
> >  arch/powerpc/kernel/l2cache_85xx.S    |   53 +++
> >  arch/powerpc/platforms/85xx/Makefile  |    3 +
> >  arch/powerpc/platforms/85xx/sleep.S   |  609 +++++++++++++++++++++++++++++++++
> >  arch/powerpc/sysdev/fsl_pmc.c         |   91 ++++-
> >  arch/powerpc/sysdev/fsl_soc.h         |    5 +
> >  8 files changed, 752 insertions(+), 19 deletions(-)
> >  create mode 100644 arch/powerpc/kernel/l2cache_85xx.S
> >  create mode 100644 arch/powerpc/platforms/85xx/sleep.S
> > 
> > diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
> > index 94ec20a..baa000c 100644
> > --- a/arch/powerpc/include/asm/cacheflush.h
> > +++ b/arch/powerpc/include/asm/cacheflush.h
> > @@ -33,6 +33,11 @@ extern void flush_dcache_page(struct page *page);
> >  #if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx)
> >  extern void __flush_disable_L1(void);
> >  #endif
> > +#if defined(CONFIG_FSL_BOOKE)
> > +extern void flush_dcache_L1(void);
> > +#else
> > +#define flush_dcache_L1()	do { } while (0)
> > +#endif
> 
> It doesn't seem right to no-op this on other platforms.

The pmc_suspend_enter() in fsl_pmc.c used by mpc85xx and mpc86xx,
but flush_dcache_L1() have no definition in mpc86xx platform.
I will write flush_dcache_L1() for mpc86xx platform.

> 
> >  extern void __flush_icache_range(unsigned long, unsigned long);
> >  static inline void flush_icache_range(unsigned long start, unsigned long stop)
> > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> > index f5808a3..cb70dba 100644
> > --- a/arch/powerpc/kernel/Makefile
> > +++ b/arch/powerpc/kernel/Makefile
> > @@ -64,6 +64,9 @@ obj-$(CONFIG_FA_DUMP)		+= fadump.o
> >  ifeq ($(CONFIG_PPC32),y)
> >  obj-$(CONFIG_E500)		+= idle_e500.o
> >  endif
> > +ifneq ($(CONFIG_PPC_E500MC),y)
> > +obj-$(CONFIG_PPC_85xx)		+= l2cache_85xx.o
> > +endif
> 
> Can we introduce a symbol that specifically means pre-e500mc e500,
> rather than using negative logic?
> 
> I think something like CONFIG_PPC_E500_V1_V2 has been proposed before.

Agree. But CONFIG_PPC_E500_V1_V2 haven't been merged.

> 
> > -static int pmc_probe(struct platform_device *ofdev)
> > +static int pmc_probe(struct platform_device *pdev)
> >  {
> > -	pmc_regs = of_iomap(ofdev->dev.of_node, 0);
> > +	struct device_node *np = pdev->dev.of_node;
> > +
> > +	pmc_regs = of_iomap(np, 0);
> >  	if (!pmc_regs)
> >  		return -ENOMEM;
> >  
> > -	pmc_dev = &ofdev->dev;
> > +	pmc_flag = PMC_SLEEP;
> > +	if (of_device_is_compatible(np, "fsl,mpc8536-pmc"))
> > +		pmc_flag |= PMC_DEEP_SLEEP;
> > +
> > +	if (of_device_is_compatible(np, "fsl,p1022-pmc"))
> > +		pmc_flag |= PMC_DEEP_SLEEP;
> > +
> >  	suspend_set_ops(&pmc_suspend_ops);
> > +
> > +	pr_info("Freescale PMC driver\n");
> 
> If you're going to be noisy on probe, at least provide some useful info
> like whether deep sleep or jog are supported.
> 
> >  	return 0;
> >  }
> >  
> > diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
> > index c6d0073..949377d 100644
> > --- a/arch/powerpc/sysdev/fsl_soc.h
> > +++ b/arch/powerpc/sysdev/fsl_soc.h
> > @@ -48,5 +48,10 @@ extern struct platform_diu_data_ops diu_ops;
> >  void fsl_hv_restart(char *cmd);
> >  void fsl_hv_halt(void);
> >  
> > +/*
> > + * Cast the ccsrbar to 64-bit parameter so that the assembly
> > + * code can be compatible with both 32-bit & 36-bit.
> > + */
> > +extern void mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq);
> 
> s/Cast the ccsrbar to 64-bit parameter/ccsrbar is u64 rather than
> phys_addr_t/
> 
> -Scott

OK. Thanks.

-Chenhui

Re: [PATCH v5 3/5] powerpc/85xx: add sleep and deep sleep support

[Scott Wood]

On 06/04/2012 06:12 AM, Zhao Chenhui wrote:
> On Fri, Jun 01, 2012 at 04:54:35PM -0500, Scott Wood wrote:
>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>>> diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
>>> index 94ec20a..baa000c 100644
>>> --- a/arch/powerpc/include/asm/cacheflush.h
>>> +++ b/arch/powerpc/include/asm/cacheflush.h
>>> @@ -33,6 +33,11 @@ extern void flush_dcache_page(struct page *page);
>>>  #if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx)
>>>  extern void __flush_disable_L1(void);
>>>  #endif
>>> +#if defined(CONFIG_FSL_BOOKE)
>>> +extern void flush_dcache_L1(void);
>>> +#else
>>> +#define flush_dcache_L1()	do { } while (0)
>>> +#endif
>>
>> It doesn't seem right to no-op this on other platforms.
> 
> The pmc_suspend_enter() in fsl_pmc.c used by mpc85xx and mpc86xx,
> but flush_dcache_L1() have no definition in mpc86xx platform.
> I will write flush_dcache_L1() for mpc86xx platform.

How about only calling the function when it's needed?  If we didn't need
an L1 flush here on 86xx before, why do we need it now?

>>>  extern void __flush_icache_range(unsigned long, unsigned long);
>>>  static inline void flush_icache_range(unsigned long start, unsigned long stop)
>>> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
>>> index f5808a3..cb70dba 100644
>>> --- a/arch/powerpc/kernel/Makefile
>>> +++ b/arch/powerpc/kernel/Makefile
>>> @@ -64,6 +64,9 @@ obj-$(CONFIG_FA_DUMP)		+= fadump.o
>>>  ifeq ($(CONFIG_PPC32),y)
>>>  obj-$(CONFIG_E500)		+= idle_e500.o
>>>  endif
>>> +ifneq ($(CONFIG_PPC_E500MC),y)
>>> +obj-$(CONFIG_PPC_85xx)		+= l2cache_85xx.o
>>> +endif
>>
>> Can we introduce a symbol that specifically means pre-e500mc e500,
>> rather than using negative logic?
>>
>> I think something like CONFIG_PPC_E500_V1_V2 has been proposed before.
> 
> Agree. But CONFIG_PPC_E500_V1_V2 haven't been merged.

Has the concept been NACKed, or just forgotten?  If the latter, you
could include it in this patchset.

-Scott

Re: [PATCH v5 3/5] powerpc/85xx: add sleep and deep sleep support

[Zhao Chenhui]

On Mon, Jun 04, 2012 at 05:58:38PM -0500, Scott Wood wrote:
> On 06/04/2012 06:12 AM, Zhao Chenhui wrote:
> > On Fri, Jun 01, 2012 at 04:54:35PM -0500, Scott Wood wrote:
> >> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> >>> diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
> >>> index 94ec20a..baa000c 100644
> >>> --- a/arch/powerpc/include/asm/cacheflush.h
> >>> +++ b/arch/powerpc/include/asm/cacheflush.h
> >>> @@ -33,6 +33,11 @@ extern void flush_dcache_page(struct page *page);
> >>>  #if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx)
> >>>  extern void __flush_disable_L1(void);
> >>>  #endif
> >>> +#if defined(CONFIG_FSL_BOOKE)
> >>> +extern void flush_dcache_L1(void);
> >>> +#else
> >>> +#define flush_dcache_L1()	do { } while (0)
> >>> +#endif
> >>
> >> It doesn't seem right to no-op this on other platforms.
> > 
> > The pmc_suspend_enter() in fsl_pmc.c used by mpc85xx and mpc86xx,
> > but flush_dcache_L1() have no definition in mpc86xx platform.
> > I will write flush_dcache_L1() for mpc86xx platform.
> 
> How about only calling the function when it's needed?  If we didn't need
> an L1 flush here on 86xx before, why do we need it now?

How about using CONFIG_PPC_85xx to gard it, like this.

	case PM_SUSPEND_STANDBY:
		local_irq_disable();
#ifdef CONFIG_PPC_85xx
		flush_dcache_L1();
#endif
		setbits32(&pmc_regs->powmgtcsr, POWMGTCSR_SLP);

> 
> >>>  extern void __flush_icache_range(unsigned long, unsigned long);
> >>>  static inline void flush_icache_range(unsigned long start, unsigned long stop)
> >>> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> >>> index f5808a3..cb70dba 100644
> >>> --- a/arch/powerpc/kernel/Makefile
> >>> +++ b/arch/powerpc/kernel/Makefile
> >>> @@ -64,6 +64,9 @@ obj-$(CONFIG_FA_DUMP)		+= fadump.o
> >>>  ifeq ($(CONFIG_PPC32),y)
> >>>  obj-$(CONFIG_E500)		+= idle_e500.o
> >>>  endif
> >>> +ifneq ($(CONFIG_PPC_E500MC),y)
> >>> +obj-$(CONFIG_PPC_85xx)		+= l2cache_85xx.o
> >>> +endif
> >>
> >> Can we introduce a symbol that specifically means pre-e500mc e500,
> >> rather than using negative logic?
> >>
> >> I think something like CONFIG_PPC_E500_V1_V2 has been proposed before.
> > 
> > Agree. But CONFIG_PPC_E500_V1_V2 haven't been merged.
> 
> Has the concept been NACKed, or just forgotten?  If the latter, you
> could include it in this patchset.
> 
> -Scott

In patchwork, it's state is "Superseded".
http://patchwork.ozlabs.org/patch/124284/

-Chenhui

Re: [PATCH v5 3/5] powerpc/85xx: add sleep and deep sleep support

[Scott Wood]

On 06/05/2012 06:35 AM, Zhao Chenhui wrote:
> On Mon, Jun 04, 2012 at 05:58:38PM -0500, Scott Wood wrote:
>> On 06/04/2012 06:12 AM, Zhao Chenhui wrote:
>>> On Fri, Jun 01, 2012 at 04:54:35PM -0500, Scott Wood wrote:
>>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>>>>> diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
>>>>> index 94ec20a..baa000c 100644
>>>>> --- a/arch/powerpc/include/asm/cacheflush.h
>>>>> +++ b/arch/powerpc/include/asm/cacheflush.h
>>>>> @@ -33,6 +33,11 @@ extern void flush_dcache_page(struct page *page);
>>>>>  #if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx)
>>>>>  extern void __flush_disable_L1(void);
>>>>>  #endif
>>>>> +#if defined(CONFIG_FSL_BOOKE)
>>>>> +extern void flush_dcache_L1(void);
>>>>> +#else
>>>>> +#define flush_dcache_L1()	do { } while (0)
>>>>> +#endif
>>>>
>>>> It doesn't seem right to no-op this on other platforms.
>>>
>>> The pmc_suspend_enter() in fsl_pmc.c used by mpc85xx and mpc86xx,
>>> but flush_dcache_L1() have no definition in mpc86xx platform.
>>> I will write flush_dcache_L1() for mpc86xx platform.
>>
>> How about only calling the function when it's needed?  If we didn't need
>> an L1 flush here on 86xx before, why do we need it now?
> 
> How about using CONFIG_PPC_85xx to gard it, like this.
> 
> 	case PM_SUSPEND_STANDBY:
> 		local_irq_disable();
> #ifdef CONFIG_PPC_85xx
> 		flush_dcache_L1();
> #endif
> 		setbits32(&pmc_regs->powmgtcsr, POWMGTCSR_SLP);

We don't support building 85xx/86xx in the same kernel and likely never
will, so this is OK.

>>>> Can we introduce a symbol that specifically means pre-e500mc e500,
>>>> rather than using negative logic?
>>>>
>>>> I think something like CONFIG_PPC_E500_V1_V2 has been proposed before.
>>>
>>> Agree. But CONFIG_PPC_E500_V1_V2 haven't been merged.
>>
>> Has the concept been NACKed, or just forgotten?  If the latter, you
>> could include it in this patchset.
>>
>> -Scott
> 
> In patchwork, it's state is "Superseded".
> http://patchwork.ozlabs.org/patch/124284/

I still think there's value in adding such a symbol.

-Scott

[PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Zhao Chenhui]

Add APIs for setting wakeup source and lossless Ethernet in low power modes.
These APIs can be used by wake-on-packet feature.

Signed-off-by: Dave Liu <daveliu@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Jin Qing <b24347@freescale.com>
Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
---
 arch/powerpc/sysdev/fsl_pmc.c |   71 ++++++++++++++++++++++++++++++++++++++++-
 arch/powerpc/sysdev/fsl_soc.h |    9 +++++
 2 files changed, 79 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_pmc.c b/arch/powerpc/sysdev/fsl_pmc.c
index 1dc6e9e..c1170f7 100644
--- a/arch/powerpc/sysdev/fsl_pmc.c
+++ b/arch/powerpc/sysdev/fsl_pmc.c
@@ -34,6 +34,7 @@ struct pmc_regs {
 	__be32 powmgtcsr;
 #define POWMGTCSR_SLP		0x00020000
 #define POWMGTCSR_DPSLP		0x00100000
+#define POWMGTCSR_LOSSLESS	0x00400000
 	__be32 res3[2];
 	__be32 pmcdr;
 };
@@ -43,6 +44,74 @@ static unsigned int pmc_flag;
 
 #define PMC_SLEEP	0x1
 #define PMC_DEEP_SLEEP	0x2
+#define PMC_LOSSLESS	0x4
+
+/**
+ * mpc85xx_pmc_set_wake - enable devices as wakeup event source
+ * @pdev: platform device affected
+ * @enable: True to enable event generation; false to disable
+ *
+ * This enables the device as a wakeup event source, or disables it.
+ *
+ * RETURN VALUE:
+ * 0 is returned on success
+ * -EINVAL is returned if device is not supposed to wake up the system
+ * Error code depending on the platform is returned if both the platform and
+ * the native mechanism fail to enable the generation of wake-up events
+ */
+int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)
+{
+	int ret = 0;
+	struct device_node *clk_np;
+	u32 *prop;
+	u32 pmcdr_mask;
+
+	if (!pmc_regs) {
+		pr_err("%s: PMC is unavailable\n", __func__);
+		return -ENODEV;
+	}
+
+	if (enable && !device_may_wakeup(&pdev->dev))
+		return -EINVAL;
+
+	clk_np = of_parse_phandle(pdev->dev.of_node, "fsl,pmc-handle", 0);
+	if (!clk_np)
+		return -EINVAL;
+
+	prop = (u32 *)of_get_property(clk_np, "fsl,pmcdr-mask", NULL);
+	if (!prop) {
+		ret = -EINVAL;
+		goto out;
+	}
+	pmcdr_mask = be32_to_cpup(prop);
+
+	if (enable)
+		/* clear to enable clock in low power mode */
+		clrbits32(&pmc_regs->pmcdr, pmcdr_mask);
+	else
+		setbits32(&pmc_regs->pmcdr, pmcdr_mask);
+
+out:
+	of_node_put(clk_np);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(mpc85xx_pmc_set_wake);
+
+/**
+ * mpc85xx_pmc_set_lossless_ethernet - enable lossless ethernet
+ * in (deep) sleep mode
+ * @enable: True to enable event generation; false to disable
+ */
+void mpc85xx_pmc_set_lossless_ethernet(int enable)
+{
+	if (pmc_flag & PMC_LOSSLESS) {
+		if (enable)
+			setbits32(&pmc_regs->powmgtcsr,	POWMGTCSR_LOSSLESS);
+		else
+			clrbits32(&pmc_regs->powmgtcsr, POWMGTCSR_LOSSLESS);
+	}
+}
+EXPORT_SYMBOL_GPL(mpc85xx_pmc_set_lossless_ethernet);
 
 static int pmc_suspend_enter(suspend_state_t state)
 {
@@ -117,7 +186,7 @@ static int pmc_probe(struct platform_device *pdev)
 		pmc_flag |= PMC_DEEP_SLEEP;
 
 	if (of_device_is_compatible(np, "fsl,p1022-pmc"))
-		pmc_flag |= PMC_DEEP_SLEEP;
+		pmc_flag |= PMC_DEEP_SLEEP | PMC_LOSSLESS;
 
 	suspend_set_ops(&pmc_suspend_ops);
 
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index 949377d..8976534 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -3,6 +3,7 @@
 #ifdef __KERNEL__
 
 #include <asm/mmu.h>
+#include <linux/platform_device.h>
 
 struct spi_device;
 
@@ -21,6 +22,14 @@ struct device_node;
 
 extern void fsl_rstcr_restart(char *cmd);
 
+#ifdef CONFIG_FSL_PMC
+extern int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable);
+extern void mpc85xx_pmc_set_lossless_ethernet(int enable);
+#else
+#define mpc85xx_pmc_set_wake(pdev, enable)		do { } while (0)
+#define mpc85xx_pmc_set_lossless_ethernet(enable)	do { } while (0)
+#endif
+
 #if defined(CONFIG_FB_FSL_DIU) || defined(CONFIG_FB_FSL_DIU_MODULE)
 
 /* The different ports that the DIU can be connected to */
-- 
1.6.4.1

Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Scott Wood]

On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> Add APIs for setting wakeup source and lossless Ethernet in low power modes.
> These APIs can be used by wake-on-packet feature.
> 
> Signed-off-by: Dave Liu <daveliu@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Jin Qing <b24347@freescale.com>
> Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> ---
>  arch/powerpc/sysdev/fsl_pmc.c |   71 ++++++++++++++++++++++++++++++++++++++++-
>  arch/powerpc/sysdev/fsl_soc.h |    9 +++++
>  2 files changed, 79 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/fsl_pmc.c b/arch/powerpc/sysdev/fsl_pmc.c
> index 1dc6e9e..c1170f7 100644
> --- a/arch/powerpc/sysdev/fsl_pmc.c
> +++ b/arch/powerpc/sysdev/fsl_pmc.c
> @@ -34,6 +34,7 @@ struct pmc_regs {
>  	__be32 powmgtcsr;
>  #define POWMGTCSR_SLP		0x00020000
>  #define POWMGTCSR_DPSLP		0x00100000
> +#define POWMGTCSR_LOSSLESS	0x00400000
>  	__be32 res3[2];
>  	__be32 pmcdr;
>  };
> @@ -43,6 +44,74 @@ static unsigned int pmc_flag;
>  
>  #define PMC_SLEEP	0x1
>  #define PMC_DEEP_SLEEP	0x2
> +#define PMC_LOSSLESS	0x4
> +
> +/**
> + * mpc85xx_pmc_set_wake - enable devices as wakeup event source
> + * @pdev: platform device affected
> + * @enable: True to enable event generation; false to disable
> + *
> + * This enables the device as a wakeup event source, or disables it.
> + *
> + * RETURN VALUE:
> + * 0 is returned on success
> + * -EINVAL is returned if device is not supposed to wake up the system
> + * Error code depending on the platform is returned if both the platform and
> + * the native mechanism fail to enable the generation of wake-up events
> + */
> +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)

Why does it have to be a platform_device?  Would a bare device_node work
here?  If it's for stuff like device_may_wakeup() that could be in a
platform_device wrapper function.

Where does this get called from?  I don't see an example user in this
patchset.

> +{
> +	int ret = 0;
> +	struct device_node *clk_np;
> +	u32 *prop;
> +	u32 pmcdr_mask;
> +
> +	if (!pmc_regs) {
> +		pr_err("%s: PMC is unavailable\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	if (enable && !device_may_wakeup(&pdev->dev))
> +		return -EINVAL;

Who is setting can_wakeup for these devices?

> +	clk_np = of_parse_phandle(pdev->dev.of_node, "fsl,pmc-handle", 0);
> +	if (!clk_np)
> +		return -EINVAL;
> +
> +	prop = (u32 *)of_get_property(clk_np, "fsl,pmcdr-mask", NULL);

Don't cast the const away.

> +	if (!prop) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	pmcdr_mask = be32_to_cpup(prop);
> +
> +	if (enable)
> +		/* clear to enable clock in low power mode */
> +		clrbits32(&pmc_regs->pmcdr, pmcdr_mask);
> +	else
> +		setbits32(&pmc_regs->pmcdr, pmcdr_mask);

What is the default PMCDR if this function is never called?  Should init
to all bits set on PM driver probe (or maybe limit it to defined bits
only, though that's a little harder to do generically).

-Scot

Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Zhao Chenhui]

On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> > Add APIs for setting wakeup source and lossless Ethernet in low power modes.
> > These APIs can be used by wake-on-packet feature.
> > 
> > Signed-off-by: Dave Liu <daveliu@freescale.com>
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Jin Qing <b24347@freescale.com>
> > Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> > ---
> >  arch/powerpc/sysdev/fsl_pmc.c |   71 ++++++++++++++++++++++++++++++++++++++++-
> >  arch/powerpc/sysdev/fsl_soc.h |    9 +++++
> >  2 files changed, 79 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/powerpc/sysdev/fsl_pmc.c b/arch/powerpc/sysdev/fsl_pmc.c
> > index 1dc6e9e..c1170f7 100644
> > --- a/arch/powerpc/sysdev/fsl_pmc.c
> > +++ b/arch/powerpc/sysdev/fsl_pmc.c
> > @@ -34,6 +34,7 @@ struct pmc_regs {
> >  	__be32 powmgtcsr;
> >  #define POWMGTCSR_SLP		0x00020000
> >  #define POWMGTCSR_DPSLP		0x00100000
> > +#define POWMGTCSR_LOSSLESS	0x00400000
> >  	__be32 res3[2];
> >  	__be32 pmcdr;
> >  };
> > @@ -43,6 +44,74 @@ static unsigned int pmc_flag;
> >  
> >  #define PMC_SLEEP	0x1
> >  #define PMC_DEEP_SLEEP	0x2
> > +#define PMC_LOSSLESS	0x4
> > +
> > +/**
> > + * mpc85xx_pmc_set_wake - enable devices as wakeup event source
> > + * @pdev: platform device affected
> > + * @enable: True to enable event generation; false to disable
> > + *
> > + * This enables the device as a wakeup event source, or disables it.
> > + *
> > + * RETURN VALUE:
> > + * 0 is returned on success
> > + * -EINVAL is returned if device is not supposed to wake up the system
> > + * Error code depending on the platform is returned if both the platform and
> > + * the native mechanism fail to enable the generation of wake-up events
> > + */
> > +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)
> 
> Why does it have to be a platform_device?  Would a bare device_node work
> here?  If it's for stuff like device_may_wakeup() that could be in a
> platform_device wrapper function.

It does not have to be a platform_device. I think it can be a struct device.

> 
> Where does this get called from?  I don't see an example user in this
> patchset.

It will be used by a gianfar related patch. I plan to submit that patch
after these patches accepted.

> 
> > +{
> > +	int ret = 0;
> > +	struct device_node *clk_np;
> > +	u32 *prop;
> > +	u32 pmcdr_mask;
> > +
> > +	if (!pmc_regs) {
> > +		pr_err("%s: PMC is unavailable\n", __func__);
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (enable && !device_may_wakeup(&pdev->dev))
> > +		return -EINVAL;
> 
> Who is setting can_wakeup for these devices?

The device driver is responsible to set can_wakeup.

> 
> > +	clk_np = of_parse_phandle(pdev->dev.of_node, "fsl,pmc-handle", 0);
> > +	if (!clk_np)
> > +		return -EINVAL;
> > +
> > +	prop = (u32 *)of_get_property(clk_np, "fsl,pmcdr-mask", NULL);
> 
> Don't cast the const away.

OK.

> 
> > +	if (!prop) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +	pmcdr_mask = be32_to_cpup(prop);
> > +
> > +	if (enable)
> > +		/* clear to enable clock in low power mode */
> > +		clrbits32(&pmc_regs->pmcdr, pmcdr_mask);
> > +	else
> > +		setbits32(&pmc_regs->pmcdr, pmcdr_mask);
> 
> What is the default PMCDR if this function is never called?  Should init
> to all bits set on PM driver probe (or maybe limit it to defined bits
> only, though that's a little harder to do generically).
> 
> -Scot

The default PMCDR is defined separately by individual chip.
I agree with you. I will have a try.

-Chenhui

Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Scott Wood]

On 06/04/2012 06:36 AM, Zhao Chenhui wrote:
> On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>>> Add APIs for setting wakeup source and lossless Ethernet in low power modes.
>>> These APIs can be used by wake-on-packet feature.
>>>
>>> Signed-off-by: Dave Liu <daveliu@freescale.com>
>>> Signed-off-by: Li Yang <leoli@freescale.com>
>>> Signed-off-by: Jin Qing <b24347@freescale.com>
>>> Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
>>> ---
>>>  arch/powerpc/sysdev/fsl_pmc.c |   71 ++++++++++++++++++++++++++++++++++++++++-
>>>  arch/powerpc/sysdev/fsl_soc.h |    9 +++++
>>>  2 files changed, 79 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/powerpc/sysdev/fsl_pmc.c b/arch/powerpc/sysdev/fsl_pmc.c
>>> index 1dc6e9e..c1170f7 100644
>>> --- a/arch/powerpc/sysdev/fsl_pmc.c
>>> +++ b/arch/powerpc/sysdev/fsl_pmc.c
>>> @@ -34,6 +34,7 @@ struct pmc_regs {
>>>  	__be32 powmgtcsr;
>>>  #define POWMGTCSR_SLP		0x00020000
>>>  #define POWMGTCSR_DPSLP		0x00100000
>>> +#define POWMGTCSR_LOSSLESS	0x00400000
>>>  	__be32 res3[2];
>>>  	__be32 pmcdr;
>>>  };
>>> @@ -43,6 +44,74 @@ static unsigned int pmc_flag;
>>>  
>>>  #define PMC_SLEEP	0x1
>>>  #define PMC_DEEP_SLEEP	0x2
>>> +#define PMC_LOSSLESS	0x4
>>> +
>>> +/**
>>> + * mpc85xx_pmc_set_wake - enable devices as wakeup event source
>>> + * @pdev: platform device affected
>>> + * @enable: True to enable event generation; false to disable
>>> + *
>>> + * This enables the device as a wakeup event source, or disables it.
>>> + *
>>> + * RETURN VALUE:
>>> + * 0 is returned on success
>>> + * -EINVAL is returned if device is not supposed to wake up the system
>>> + * Error code depending on the platform is returned if both the platform and
>>> + * the native mechanism fail to enable the generation of wake-up events
>>> + */
>>> +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)
>>
>> Why does it have to be a platform_device?  Would a bare device_node work
>> here?  If it's for stuff like device_may_wakeup() that could be in a
>> platform_device wrapper function.
> 
> It does not have to be a platform_device. I think it can be a struct device.

Why does it even need that?  The low level mechanism for influencing
PMCDR should only need a device node, not a Linux device struct.

>> Where does this get called from?  I don't see an example user in this
>> patchset.
> 
> It will be used by a gianfar related patch. I plan to submit that patch
> after these patches accepted.

It would be nice to see how this is used when reviewing this.

>>> +{
>>> +	int ret = 0;
>>> +	struct device_node *clk_np;
>>> +	u32 *prop;
>>> +	u32 pmcdr_mask;
>>> +
>>> +	if (!pmc_regs) {
>>> +		pr_err("%s: PMC is unavailable\n", __func__);
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	if (enable && !device_may_wakeup(&pdev->dev))
>>> +		return -EINVAL;
>>
>> Who is setting can_wakeup for these devices?
> 
> The device driver is responsible to set can_wakeup.

How would the device driver know how to set it?  Wouldn't this depend on
the particular SoC and low power mode?

-Scott

RE: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Li Yang-R58472]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, June 05, 2012 7:03 AM
> To: Zhao Chenhui-B35336
> Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> galak@kernel.crashing.org; Li Yang-R58472
> Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup
> event source
> 
> On 06/04/2012 06:36 AM, Zhao Chenhui wrote:
> > On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
> >> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> >>> Add APIs for setting wakeup source and lossless Ethernet in low power
> modes.
> >>> These APIs can be used by wake-on-packet feature.
> >>>
> >>> Signed-off-by: Dave Liu <daveliu@freescale.com>
> >>> Signed-off-by: Li Yang <leoli@freescale.com>
> >>> Signed-off-by: Jin Qing <b24347@freescale.com>
> >>> Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> >>> ---
> >>>  arch/powerpc/sysdev/fsl_pmc.c |   71
> ++++++++++++++++++++++++++++++++++++++++-
> >>>  arch/powerpc/sysdev/fsl_soc.h |    9 +++++
> >>>  2 files changed, 79 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/sysdev/fsl_pmc.c
> b/arch/powerpc/sysdev/fsl_pmc.c
> >>> index 1dc6e9e..c1170f7 100644
> >>> --- a/arch/powerpc/sysdev/fsl_pmc.c
> >>> +++ b/arch/powerpc/sysdev/fsl_pmc.c
> >>> @@ -34,6 +34,7 @@ struct pmc_regs {
> >>>  	__be32 powmgtcsr;
> >>>  #define POWMGTCSR_SLP		0x00020000
> >>>  #define POWMGTCSR_DPSLP		0x00100000
> >>> +#define POWMGTCSR_LOSSLESS	0x00400000
> >>>  	__be32 res3[2];
> >>>  	__be32 pmcdr;
> >>>  };
> >>> @@ -43,6 +44,74 @@ static unsigned int pmc_flag;
> >>>
> >>>  #define PMC_SLEEP	0x1
> >>>  #define PMC_DEEP_SLEEP	0x2
> >>> +#define PMC_LOSSLESS	0x4
> >>> +
> >>> +/**
> >>> + * mpc85xx_pmc_set_wake - enable devices as wakeup event source
> >>> + * @pdev: platform device affected
> >>> + * @enable: True to enable event generation; false to disable
> >>> + *
> >>> + * This enables the device as a wakeup event source, or disables it.
> >>> + *
> >>> + * RETURN VALUE:
> >>> + * 0 is returned on success
> >>> + * -EINVAL is returned if device is not supposed to wake up the
> system
> >>> + * Error code depending on the platform is returned if both the
> platform and
> >>> + * the native mechanism fail to enable the generation of wake-up
> events
> >>> + */
> >>> +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)
> >>
> >> Why does it have to be a platform_device?  Would a bare device_node
> work
> >> here?  If it's for stuff like device_may_wakeup() that could be in a
> >> platform_device wrapper function.
> >
> > It does not have to be a platform_device. I think it can be a struct
> device.
> 
> Why does it even need that?  The low level mechanism for influencing
> PMCDR should only need a device node, not a Linux device struct.

It does no harm to pass the device structure and makes more sense for object oriented interface design. 

> 
> >> Where does this get called from?  I don't see an example user in this
> >> patchset.
> >
> > It will be used by a gianfar related patch. I plan to submit that patch
> > after these patches accepted.
> 
> It would be nice to see how this is used when reviewing this.
> 
> >>> +{
> >>> +	int ret = 0;
> >>> +	struct device_node *clk_np;
> >>> +	u32 *prop;
> >>> +	u32 pmcdr_mask;
> >>> +
> >>> +	if (!pmc_regs) {
> >>> +		pr_err("%s: PMC is unavailable\n", __func__);
> >>> +		return -ENODEV;
> >>> +	}
> >>> +
> >>> +	if (enable && !device_may_wakeup(&pdev->dev))
> >>> +		return -EINVAL;
> >>
> >> Who is setting can_wakeup for these devices?
> >
> > The device driver is responsible to set can_wakeup.
> 
> How would the device driver know how to set it?  Wouldn't this depend on
> the particular SoC and low power mode?

It is based on the "fsl,magic-packet" and "fsl,wake-on-filer" device tree properties.

Leo

Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Scott Wood]

On 06/04/2012 11:08 PM, Li Yang-R58472 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Tuesday, June 05, 2012 7:03 AM
>> To: Zhao Chenhui-B35336
>> Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>> galak@kernel.crashing.org; Li Yang-R58472
>> Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup
>> event source
>>
>> On 06/04/2012 06:36 AM, Zhao Chenhui wrote:
>>> On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
>>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>>>>> +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)
>>>>
>>>> Why does it have to be a platform_device?  Would a bare device_node
>> work
>>>> here?  If it's for stuff like device_may_wakeup() that could be in a
>>>> platform_device wrapper function.
>>>
>>> It does not have to be a platform_device. I think it can be a struct
>> device.
>>
>> Why does it even need that?  The low level mechanism for influencing
>> PMCDR should only need a device node, not a Linux device struct.
> 
> It does no harm to pass the device structure and makes more sense for object oriented interface design. 

It does do harm if you don't have a device structure to pass, if for
some reason you found the device by directly looking for it rather than
going through the device model.

>>>> Who is setting can_wakeup for these devices?
>>>
>>> The device driver is responsible to set can_wakeup.
>>
>> How would the device driver know how to set it?  Wouldn't this depend on
>> the particular SoC and low power mode?
> 
> It is based on the "fsl,magic-packet" and "fsl,wake-on-filer" device tree properties.

fsl,magic-packet was a mistake.  It is equivalent to checking the
compatible for etsec.  It does not convey any information about whether
the eTSEC is still active in a given low power mode.

How is fsl,wake-os-filer relevant to this decision?  When will it be set
but not fsl,magic-packet?

What about devices other than ethernet?  What about differences between
ordinary sleep and deep sleep?

-Scott

RE: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Li Yang-R58472]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, June 06, 2012 12:12 AM
> To: Li Yang-R58472
> Cc: Wood Scott-B07421; Zhao Chenhui-B35336; linuxppc-dev@lists.ozlabs.org;
> linux-kernel@vger.kernel.org; galak@kernel.crashing.org
> Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup
> event source
> 
> On 06/04/2012 11:08 PM, Li Yang-R58472 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Wood Scott-B07421
> >> Sent: Tuesday, June 05, 2012 7:03 AM
> >> To: Zhao Chenhui-B35336
> >> Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> >> galak@kernel.crashing.org; Li Yang-R58472
> >> Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as
> >> wakeup event source
> >>
> >> On 06/04/2012 06:36 AM, Zhao Chenhui wrote:
> >>> On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
> >>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> >>>>> +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool
> >>>>> +enable)
> >>>>
> >>>> Why does it have to be a platform_device?  Would a bare device_node
> >> work
> >>>> here?  If it's for stuff like device_may_wakeup() that could be in
> >>>> a platform_device wrapper function.
> >>>
> >>> It does not have to be a platform_device. I think it can be a struct
> >> device.
> >>
> >> Why does it even need that?  The low level mechanism for influencing
> >> PMCDR should only need a device node, not a Linux device struct.
> >
> > It does no harm to pass the device structure and makes more sense for
> object oriented interface design.
> 
> It does do harm if you don't have a device structure to pass, if for some
> reason you found the device by directly looking for it rather than going
> through the device model.

Whether or not a device is a wakeup source not only depends on the SoC specification but also the configuration and current state for the device.  I only expect the device driver to have this knowledge and call this function rather than some standalone platform code.  Therefore I don't think your concern matters.
 
> 
> >>>> Who is setting can_wakeup for these devices?
> >>>
> >>> The device driver is responsible to set can_wakeup.
> >>
> >> How would the device driver know how to set it?  Wouldn't this depend
> >> on the particular SoC and low power mode?
> >
> > It is based on the "fsl,magic-packet" and "fsl,wake-on-filer" device
> tree properties.
> 
> fsl,magic-packet was a mistake.  It is equivalent to checking the
> compatible for etsec.  It does not convey any information about whether

It can be described either by explicit feature property or by the compatible.  I don't think it is a problem that we choose one against another.

> the eTSEC is still active in a given low power mode.

Whether or not the eTSEC is still active in both sleep and deep sleep is only depending on if we set it to be a wakeup source.  If it behaves differently for low power modes in the future, we could address that by adding additional property.

> 
> How is fsl,wake-os-filer relevant to this decision?  When will it be set
> but not fsl,magic-packet?

I mean either fsl,magic-packet or fsl,wake-on-filer shows it can be a wakeup source.  Currently we don't have an SoC to have wake-on-filer while not wake-on-magic.  But I think it's better to consider both as they are independent features.

> 
> What about devices other than ethernet?  What about differences between
> ordinary sleep and deep sleep?

There is no difference for sleep and deep sleep for all wakeup sources currently.  We can address the problem if it is not the case in the future.

Leo

Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Scott Wood]

On 06/05/2012 11:49 AM, Li Yang-R58472 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Wednesday, June 06, 2012 12:12 AM
>> To: Li Yang-R58472
>> Cc: Wood Scott-B07421; Zhao Chenhui-B35336; linuxppc-dev@lists.ozlabs.org;
>> linux-kernel@vger.kernel.org; galak@kernel.crashing.org
>> Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup
>> event source
>>
>> On 06/04/2012 11:08 PM, Li Yang-R58472 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Wood Scott-B07421
>>>> Sent: Tuesday, June 05, 2012 7:03 AM
>>>> To: Zhao Chenhui-B35336
>>>> Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>>>> galak@kernel.crashing.org; Li Yang-R58472
>>>> Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as
>>>> wakeup event source
>>>>
>>>> On 06/04/2012 06:36 AM, Zhao Chenhui wrote:
>>>>> On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
>>>>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>>>>>>> +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool
>>>>>>> +enable)
>>>>>>
>>>>>> Why does it have to be a platform_device?  Would a bare device_node
>>>> work
>>>>>> here?  If it's for stuff like device_may_wakeup() that could be in
>>>>>> a platform_device wrapper function.
>>>>>
>>>>> It does not have to be a platform_device. I think it can be a struct
>>>> device.
>>>>
>>>> Why does it even need that?  The low level mechanism for influencing
>>>> PMCDR should only need a device node, not a Linux device struct.
>>>
>>> It does no harm to pass the device structure and makes more sense for
>> object oriented interface design.
>>
>> It does do harm if you don't have a device structure to pass, if for some
>> reason you found the device by directly looking for it rather than going
>> through the device model.
> 
> Whether or not a device is a wakeup source not only depends on the
> SoC specification but also the configuration and current state for
> the device.  I only expect the device driver to have this knowledge
> and call this function rather than some standalone platform code.
> Therefore I don't think your concern matters.

First, I think it's bad API to force the passing of a higher level
object when a lower level object would suffice (and there are no
legitimate future-proofing or abstraction reasons for hiding the lower
level object).

But regardless, the entity you call a "device driver" may or may not use
the standard driver model (e.g. look at PCI root complexes), and your
assumption about what platform code knows may or may not be correct.  I
could just as well say that only platform code knows about the SoC
clock/power routing during a given low power state.

>>>>>> Who is setting can_wakeup for these devices?
>>>>>
>>>>> The device driver is responsible to set can_wakeup.
>>>>
>>>> How would the device driver know how to set it?  Wouldn't this depend
>>>> on the particular SoC and low power mode?
>>>
>>> It is based on the "fsl,magic-packet" and "fsl,wake-on-filer" device
>> tree properties.
>>
>> fsl,magic-packet was a mistake.  It is equivalent to checking the
>> compatible for etsec.  It does not convey any information about whether
> 
> It can be described either by explicit feature property or by the
> compatible.  I don't think it is a problem that we choose one against
> another.

I do think it's a problem, because it's unnecessarily complicated, and
more error prone (we probably didn't have fsl,magic-packet on all the
SoCs that support it before the .dtsi refactoring).  But my point was
that it says nothing about whether the eTSEC will still be functioning
during a low power state.

>> the eTSEC is still active in a given low power mode.
> 
> Whether or not the eTSEC is still active in both sleep and deep sleep
> is only depending on if we set it to be a wakeup source.

Only because we happen to support eTSEC as a wakeup source on all SoCs.

> If it behaves differently for low power modes in the future, we could
> address that by adding additional property.
> 
>>
>> How is fsl,wake-os-filer relevant to this decision?  When will it be set
>> but not fsl,magic-packet?
> 
> I mean either fsl,magic-packet or fsl,wake-on-filer shows it can be a
> wakeup source.  Currently we don't have an SoC to have wake-on-filer
> while not wake-on-magic.  But I think it's better to consider both as
> they are independent features.

You're not willing to consider an SoC where waking on eTSEC is
unsupported, but you're willing to consider an eTSEC that has
wake-on-filer but magic packet support has for some reason been dropped?

>> What about devices other than ethernet?  What about differences between
>> ordinary sleep and deep sleep?
> 
> There is no difference for sleep and deep sleep for all wakeup sources currently.

I recall being able to wake an mpc85xx chip (maybe mpc8544?) from
ordinary sleep using the DUART (even though the manual suggests that
only external interrupts can be used -- not even eTSEC).  You can't do
that in deep sleep.

You ignored "what about devices other than ethernet".

-Scott

Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Li Yang]

On Wed, Jun 6, 2012 at 2:05 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 06/05/2012 11:49 AM, Li Yang-R58472 wrote:
>>
>>

>>>>> On 06/04/2012 06:36 AM, Zhao Chenhui wrote:
>>>>>> On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
>>>>>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>>>>>>>> +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool
>>>>>>>> +enable)
>>>>>>>
>>>>>>> Why does it have to be a platform_device?  Would a bare device_node
>>>>> work
>>>>>>> here?  If it's for stuff like device_may_wakeup() that could be in
>>>>>>> a platform_device wrapper function.
>>>>>>
>>>>>> It does not have to be a platform_device. I think it can be a struct
>>>>> device.
>>>>>
>>>>> Why does it even need that?  The low level mechanism for influencing
>>>>> PMCDR should only need a device node, not a Linux device struct.
>>>>
>>>> It does no harm to pass the device structure and makes more sense for
>>> object oriented interface design.
>>>
>>> It does do harm if you don't have a device structure to pass, if for some
>>> reason you found the device by directly looking for it rather than going
>>> through the device model.
>>
>> Whether or not a device is a wakeup source not only depends on the
>> SoC specification but also the configuration and current state for
>> the device.  I only expect the device driver to have this knowledge
>> and call this function rather than some standalone platform code.
>> Therefore I don't think your concern matters.
>
> First, I think it's bad API to force the passing of a higher level
> object when a lower level object would suffice (and there are no
> legitimate future-proofing or abstraction reasons for hiding the lower
> level object).
>
> But regardless, the entity you call a "device driver" may or may not use
> the standard driver model (e.g. look at PCI root complexes), and your
> assumption about what platform code knows may or may not be correct.  I
> could just as well say that only platform code knows about the SoC
> clock/power routing during a given low power state.

Good point.  We need to fix such non-standard drivers.  The new PM
framework depends a lot on the standard Linux Driver Model.  We need
to change our drivers to make them work better with PM.  Also we have
already submitted a patch series to change the PCI root complex driver
in that regard.

>
>>>>>>> Who is setting can_wakeup for these devices?
>>>>>>
>>>>>> The device driver is responsible to set can_wakeup.
>>>>>
>>>>> How would the device driver know how to set it?  Wouldn't this depend
>>>>> on the particular SoC and low power mode?
>>>>
>>>> It is based on the "fsl,magic-packet" and "fsl,wake-on-filer" device
>>> tree properties.
>>>
>>> fsl,magic-packet was a mistake.  It is equivalent to checking the
>>> compatible for etsec.  It does not convey any information about whether
>>
>> It can be described either by explicit feature property or by the
>> compatible.  I don't think it is a problem that we choose one against
>> another.
>
> I do think it's a problem, because it's unnecessarily complicated, and
> more error prone (we probably didn't have fsl,magic-packet on all the
> SoCs that support it before the .dtsi refactoring).  But my point was
> that it says nothing about whether the eTSEC will still be functioning
> during a low power state.
>
>>> the eTSEC is still active in a given low power mode.
>>
>> Whether or not the eTSEC is still active in both sleep and deep sleep
>> is only depending on if we set it to be a wakeup source.
>
> Only because we happen to support eTSEC as a wakeup source on all SoCs.
>
>> If it behaves differently for low power modes in the future, we could
>> address that by adding additional property.
>>
>>>
>>> How is fsl,wake-os-filer relevant to this decision?  When will it be set
>>> but not fsl,magic-packet?
>>
>> I mean either fsl,magic-packet or fsl,wake-on-filer shows it can be a
>> wakeup source.  Currently we don't have an SoC to have wake-on-filer
>> while not wake-on-magic.  But I think it's better to consider both as
>> they are independent features.
>
> You're not willing to consider an SoC where waking on eTSEC is
> unsupported, but you're willing to consider an eTSEC that has
> wake-on-filer but magic packet support has for some reason been dropped?

Good findings.  :)  I think it's fine to keep the extra that have
already been done as long as it does no harm.  But we should stop
adding more extras that are not necessary for now.

>
>>> What about devices other than ethernet?  What about differences between
>>> ordinary sleep and deep sleep?
>>
>> There is no difference for sleep and deep sleep for all wakeup sources currently.
>
> I recall being able to wake an mpc85xx chip (maybe mpc8544?) from
> ordinary sleep using the DUART (even though the manual suggests that
> only external interrupts can be used -- not even eTSEC).  You can't do
> that in deep sleep.

I doubt that as the blocks are clock gated in sleep.  We only have
MPC8536 and P1022 in the e500 family to support deep sleep now.  I
agree with you the sleep and deep sleep can imply different wakeup
source in the future.  But can we worry about it later?

>
> You ignored "what about devices other than ethernet".

No, I haven't.  Other devices are so at least for now.

- Leo

Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Scott Wood]

On 06/05/2012 11:06 PM, Li Yang wrote:
> On Wed, Jun 6, 2012 at 2:05 AM, Scott Wood <scottwood@freescale.com> wrote:
>> You ignored "what about devices other than ethernet".
> 
> No, I haven't.  Other devices are so at least for now.

I don't understand that last sentence.  Other devices are what?

-Scott

Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

[Li Yang]

On Thu, Jun 7, 2012 at 2:29 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 06/05/2012 11:06 PM, Li Yang wrote:
>> On Wed, Jun 6, 2012 at 2:05 AM, Scott Wood <scottwood@freescale.com> wrote:
>>> You ignored "what about devices other than ethernet".
>>
>> No, I haven't.  Other devices are so at least for now.
>
> I don't understand that last sentence.  Other devices are what?

Probably I misunderstood your question "what about devices other than
ethernet".  Did you mean how would other devices other than ethernet
know how to set it?

Other wakeup capable devices can call the API when it is up and
running.  It will be the pmc driver's responsibility to find out if
that specific device can be configured as a wakeup source for the SoC.

Leo

[PATCH v5 5/5] powerpc/85xx: add support to JOG feature using cpufreq interface

[Zhao Chenhui]

Some 85xx silicons like MPC8536 and P1022 have a JOG feature, which provides
a dynamic mechanism to lower or raise the CPU core clock at runtime.

This patch adds the support to change CPU frequency using the standard
cpufreq interface. The ratio CORE to CCB can be 1:1(except MPC8536), 3:2,
2:1, 5:2, 3:1, 7:2 and 4:1.

Two CPU cores on P1022 must not in the low power state during the frequency
transition. The driver uses a atomic counter to meet the requirement.

The jog mode frequency transition process on the MPC8536 is similar to
the deep sleep process. The driver need save the CPU state and restore
it after CPU warm reset.

Note:
 * The I/O peripherals such as PCIe and eTSEC may lose packets during
   the jog mode frequency transition.
 * The driver doesn't support MPC8536 Rev 1.0 due to a JOG erratum.
   Subsequent revisions of MPC8536 have corrected the erratum.

Signed-off-by: Dave Liu <daveliu@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
CC: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/platforms/85xx/Makefile      |    1 +
 arch/powerpc/platforms/85xx/cpufreq-jog.c |  416 +++++++++++++++++++++++++++++
 arch/powerpc/platforms/Kconfig            |   11 +
 arch/powerpc/sysdev/fsl_soc.h             |    5 +
 include/linux/cpu.h                       |    4 +
 kernel/cpu.c                              |   60 ++--
 6 files changed, 467 insertions(+), 30 deletions(-)
 create mode 100644 arch/powerpc/platforms/85xx/cpufreq-jog.c

diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index 12b526a..b49a0b6 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_SMP) += smp.o
 ifneq ($(CONFIG_PPC_E500MC),y)
 obj-$(CONFIG_SUSPEND)	+= sleep.o
 endif
+obj-$(CONFIG_MPC85xx_CPUFREQ) += cpufreq-jog.o
 
 obj-y += common.o
 
diff --git a/arch/powerpc/platforms/85xx/cpufreq-jog.c b/arch/powerpc/platforms/85xx/cpufreq-jog.c
new file mode 100644
index 0000000..5d427ab
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/cpufreq-jog.c
@@ -0,0 +1,416 @@
+/*
+ * Copyright (C) 2008-2012 Freescale Semiconductor, Inc.
+ * Author: Dave Liu <daveliu@freescale.com>
+ * Modifier: Chenhui Zhao <chenhui.zhao@freescale.com>
+ *
+ * The cpufreq driver is for Freescale 85xx processor,
+ * based on arch/powerpc/platforms/cell/cbe_cpufreq.c
+ * (C) Copyright IBM Deutschland Entwicklung GmbH 2005-2007
+ *	Christian Krafft <krafft@de.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/cpufreq.h>
+#include <linux/of_platform.h>
+#include <linux/suspend.h>
+#include <linux/cpu.h>
+
+#include <asm/prom.h>
+#include <asm/time.h>
+#include <asm/reg.h>
+#include <asm/io.h>
+#include <asm/machdep.h>
+#include <asm/smp.h>
+
+#include <sysdev/fsl_soc.h>
+
+static DEFINE_MUTEX(mpc85xx_switch_mutex);
+static void __iomem *guts;
+
+static u32 sysfreq;
+static unsigned int max_pll[2];
+static atomic_t in_jog_process;
+static struct cpufreq_frequency_table *mpc85xx_freqs;
+static int (*set_pll)(unsigned int cpu, unsigned int pll);
+
+static struct cpufreq_frequency_table mpc8536_freqs_table[] = {
+	{3,	0},
+	{4,	0},
+	{5,	0},
+	{6,	0},
+	{7,	0},
+	{8,	0},
+	{0,	CPUFREQ_TABLE_END},
+};
+
+static struct cpufreq_frequency_table p1022_freqs_table[] = {
+	{2,	0},
+	{3,	0},
+	{4,	0},
+	{5,	0},
+	{6,	0},
+	{7,	0},
+	{8,	0},
+	{0,	CPUFREQ_TABLE_END},
+};
+
+#define FREQ_500MHz	500000000
+#define FREQ_800MHz	800000000
+
+#define CORE_RATIO_STRIDE	8
+#define CORE_RATIO_MASK		0x3f
+#define CORE_RATIO_SHIFT	16
+
+#define PORPLLSR	0x0	/* Power-On Reset PLL ratio status register */
+
+#define PMJCR		0x7c	/* Power Management Jog Control Register */
+#define PMJCR_CORE0_SPD	0x00001000
+#define PMJCR_CORE_SPD	0x00002000
+
+#define POWMGTCSR	0x80 /* Power management control and status register */
+#define POWMGTCSR_JOG		0x00200000
+#define POWMGTCSR_INT_MASK	0x00000f00
+
+static void spin_while_jogging(void *dummy)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+
+	atomic_inc(&in_jog_process);
+
+	while (atomic_read(&in_jog_process) != 0)
+		barrier();
+
+	local_irq_restore(flags);
+}
+
+static int get_pll(int hw_cpu)
+{
+	int shift;
+	u32 val = in_be32(guts + PORPLLSR);
+
+	shift = hw_cpu * CORE_RATIO_STRIDE + CORE_RATIO_SHIFT;
+
+	return (val >> shift) & CORE_RATIO_MASK;
+}
+
+static int mpc8536_set_pll(unsigned int cpu, unsigned int pll)
+{
+	u32 corefreq, val, mask;
+	unsigned int cur_pll = get_pll(0);
+	unsigned long flags;
+
+	if (pll == cur_pll)
+		return 0;
+
+	val = (pll & CORE_RATIO_MASK) << CORE_RATIO_SHIFT;
+
+	corefreq = sysfreq * pll / 2;
+	/*
+	 * Set the COREx_SPD bit if the requested core frequency
+	 * is larger than the threshold frequency.
+	 */
+	if (corefreq > FREQ_800MHz)
+			val |= PMJCR_CORE_SPD;
+
+	mask = (CORE_RATIO_MASK << CORE_RATIO_SHIFT) | PMJCR_CORE_SPD;
+	clrsetbits_be32(guts + PMJCR, mask, val);
+
+	/* readback to sync write */
+	in_be32(guts + PMJCR);
+
+	local_irq_save(flags);
+	mpc85xx_enter_jog(get_immrbase(), POWMGTCSR_JOG);
+	local_irq_restore(flags);
+
+	/* verify */
+	cur_pll =  get_pll(0);
+	if (cur_pll != pll) {
+		pr_err("%s: error. The current PLL of core 0 is %d instead of %d.\n",
+				__func__, cur_pll, pll);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int p1022_set_pll(unsigned int cpu, unsigned int pll)
+{
+	int index, hw_cpu = get_hard_smp_processor_id(cpu);
+	int shift;
+	u32 corefreq, val, mask = 0;
+	unsigned int cur_pll = get_pll(hw_cpu);
+	unsigned long flags;
+	int ret = 0;
+
+	if (pll == cur_pll)
+		return 0;
+
+	shift = hw_cpu * CORE_RATIO_STRIDE + CORE_RATIO_SHIFT;
+	val = (pll & CORE_RATIO_MASK) << shift;
+
+	corefreq = sysfreq * pll / 2;
+	/*
+	 * Set the COREx_SPD bit if the requested core frequency
+	 * is larger than the threshold frequency.
+	 */
+	if (corefreq > FREQ_500MHz)
+		val |= PMJCR_CORE0_SPD << hw_cpu;
+
+	mask = (CORE_RATIO_MASK << shift) | (PMJCR_CORE0_SPD << hw_cpu);
+	clrsetbits_be32(guts + PMJCR, mask, val);
+
+	/* readback to sync write */
+	in_be32(guts + PMJCR);
+
+	cpu_hotplug_disable_before_freeze();
+	/*
+	 * A Jog request can not be asserted when any core is in a low
+	 * power state on P1022. Before executing a jog request, any
+	 * core which is in a low power state must be waked by a
+	 * interrupt, and keep waking up until the sequence is
+	 * finished.
+	 */
+	for_each_present_cpu(index) {
+		if (!cpu_online(index)) {
+			cpu_hotplug_enable_after_thaw();
+			pr_err("%s: error, core%d is down.\n", __func__, index);
+			return -1;
+		}
+	}
+
+	atomic_set(&in_jog_process, 0);
+	smp_call_function(spin_while_jogging, NULL, 0);
+
+	local_irq_save(flags);
+
+	/* Wait for the other core to wake. */
+	if (!spin_event_timeout(atomic_read(&in_jog_process) == 1, 1000, 100)) {
+		pr_err("%s: timeout, the other core is not at running state.\n",
+					__func__);
+		ret = -1;
+		goto err;
+	}
+
+	out_be32(guts + POWMGTCSR, POWMGTCSR_JOG | POWMGTCSR_INT_MASK);
+
+	if (!spin_event_timeout(
+		(in_be32(guts + POWMGTCSR) & POWMGTCSR_JOG) == 0, 1000, 100)) {
+		pr_err("%s: timeout, fail to switch the core frequency.\n",
+				__func__);
+		ret = -1;
+		goto err;
+	}
+
+	clrbits32(guts + POWMGTCSR, POWMGTCSR_INT_MASK);
+	in_be32(guts + POWMGTCSR);
+
+	atomic_set(&in_jog_process, 0);
+err:
+	local_irq_restore(flags);
+	cpu_hotplug_enable_after_thaw();
+
+	/* verify */
+	cur_pll =  get_pll(hw_cpu);
+	if (cur_pll != pll) {
+		pr_err("%s: error, the current PLL of core %d is %d instead of %d.\n",
+				__func__, hw_cpu, cur_pll, pll);
+		return -1;
+	}
+
+	return ret;
+}
+
+/*
+ * cpufreq functions
+ */
+static int mpc85xx_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+	unsigned int i, cur_pll;
+	int hw_cpu = get_hard_smp_processor_id(policy->cpu);
+
+	if (!cpu_present(policy->cpu))
+		return -ENODEV;
+
+	/* the latency of a transition, the unit is ns */
+	policy->cpuinfo.transition_latency = 2000;
+
+	cur_pll = get_pll(hw_cpu);
+
+	/* initialize frequency table */
+	pr_debug("core%d frequency table:\n", hw_cpu);
+	for (i = 0; mpc85xx_freqs[i].frequency != CPUFREQ_TABLE_END; i++) {
+		if (mpc85xx_freqs[i].index <= max_pll[hw_cpu]) {
+			/* The frequency unit is kHz. */
+			mpc85xx_freqs[i].frequency =
+				(sysfreq * mpc85xx_freqs[i].index / 2) / 1000;
+		} else {
+			mpc85xx_freqs[i].frequency = CPUFREQ_ENTRY_INVALID;
+		}
+
+		pr_debug("%d: %dkHz\n", i, mpc85xx_freqs[i].frequency);
+
+		if (mpc85xx_freqs[i].index == cur_pll)
+			policy->cur = mpc85xx_freqs[i].frequency;
+	}
+	pr_debug("current pll is at %d, and core freq is%d\n",
+			cur_pll, policy->cur);
+
+	cpufreq_frequency_table_get_attr(mpc85xx_freqs, policy->cpu);
+
+	/*
+	 * This ensures that policy->cpuinfo_min
+	 * and policy->cpuinfo_max are set correctly.
+	 */
+	return cpufreq_frequency_table_cpuinfo(policy, mpc85xx_freqs);
+}
+
+static int mpc85xx_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+	cpufreq_frequency_table_put_attr(policy->cpu);
+
+	return 0;
+}
+
+static int mpc85xx_cpufreq_verify(struct cpufreq_policy *policy)
+{
+	return cpufreq_frequency_table_verify(policy, mpc85xx_freqs);
+}
+
+static int mpc85xx_cpufreq_target(struct cpufreq_policy *policy,
+			      unsigned int target_freq,
+			      unsigned int relation)
+{
+	struct cpufreq_freqs freqs;
+	unsigned int new;
+	int ret = 0;
+
+	if (!set_pll)
+		return -ENODEV;
+
+	cpufreq_frequency_table_target(policy,
+				       mpc85xx_freqs,
+				       target_freq,
+				       relation,
+				       &new);
+
+	freqs.old = policy->cur;
+	freqs.new = mpc85xx_freqs[new].frequency;
+	freqs.cpu = policy->cpu;
+
+	mutex_lock(&mpc85xx_switch_mutex);
+	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+
+	ret = set_pll(policy->cpu, mpc85xx_freqs[new].index);
+	if (!ret) {
+		pr_info("cpufreq: Setting core%d frequency to %d kHz and PLL ratio to %d:2\n",
+			 policy->cpu, mpc85xx_freqs[new].frequency,
+			 mpc85xx_freqs[new].index);
+
+		ppc_proc_freq = freqs.new * 1000ul;
+	}
+	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	mutex_unlock(&mpc85xx_switch_mutex);
+
+	return ret;
+}
+
+static struct cpufreq_driver mpc85xx_cpufreq_driver = {
+	.verify		= mpc85xx_cpufreq_verify,
+	.target		= mpc85xx_cpufreq_target,
+	.init		= mpc85xx_cpufreq_cpu_init,
+	.exit		= mpc85xx_cpufreq_cpu_exit,
+	.name		= "mpc85xx-JOG",
+	.owner		= THIS_MODULE,
+	.flags		= CPUFREQ_CONST_LOOPS,
+};
+
+static int mpc85xx_job_probe(struct platform_device *ofdev)
+{
+	struct device_node *np = ofdev->dev.of_node;
+	unsigned int svr;
+
+	if (of_device_is_compatible(np, "fsl,mpc8536-guts")) {
+		svr = mfspr(SPRN_SVR);
+		if ((svr & 0x7fff) == 0x10) {
+			pr_err("MPC8536 Rev 1.0 do not support JOG.\n");
+			return -ENODEV;
+		}
+		mpc85xx_freqs = mpc8536_freqs_table;
+		set_pll = mpc8536_set_pll;
+	} else if (of_device_is_compatible(np, "fsl,p1022-guts")) {
+		mpc85xx_freqs = p1022_freqs_table;
+		set_pll = p1022_set_pll;
+	} else {
+		return -ENODEV;
+	}
+
+	sysfreq = fsl_get_sys_freq();
+
+	guts = of_iomap(np, 0);
+	if (!guts)
+		return -ENODEV;
+
+	max_pll[0] = get_pll(0);
+	if (mpc85xx_freqs == p1022_freqs_table)
+		max_pll[1] = get_pll(1);
+
+	pr_info("Freescale MPC85xx CPU frequency switching(JOG) driver\n");
+
+	return cpufreq_register_driver(&mpc85xx_cpufreq_driver);
+}
+
+static int mpc85xx_jog_remove(struct platform_device *ofdev)
+{
+	iounmap(guts);
+	cpufreq_unregister_driver(&mpc85xx_cpufreq_driver);
+
+	return 0;
+}
+
+static struct of_device_id mpc85xx_jog_ids[] = {
+	{ .compatible = "fsl,mpc8536-guts", },
+	{ .compatible = "fsl,p1022-guts", },
+	{}
+};
+
+static struct platform_driver mpc85xx_jog_driver = {
+	.driver = {
+		.name = "mpc85xx_cpufreq_jog",
+		.owner = THIS_MODULE,
+		.of_match_table = mpc85xx_jog_ids,
+	},
+	.probe = mpc85xx_job_probe,
+	.remove = mpc85xx_jog_remove,
+};
+
+static int __init mpc85xx_jog_init(void)
+{
+	return platform_driver_register(&mpc85xx_jog_driver);
+}
+
+static void __exit mpc85xx_jog_exit(void)
+{
+	platform_driver_unregister(&mpc85xx_jog_driver);
+}
+
+module_init(mpc85xx_jog_init);
+module_exit(mpc85xx_jog_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Dave Liu <daveliu@freescale.com>");
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index a35ca44..445bedd 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -204,6 +204,17 @@ config CPU_FREQ_PMAC64
 	  This adds support for frequency switching on Apple iMac G5,
 	  and some of the more recent desktop G5 machines as well.
 
+config MPC85xx_CPUFREQ
+	bool "Support for Freescale MPC85xx CPU freq"
+	depends on PPC_85xx && PPC32 && !PPC_E500MC
+	default y
+	select CPU_FREQ_TABLE
+	help
+	  This adds support for dynamic frequency switching on
+	  Freescale MPC85xx by cpufreq interface. MPC8536 and P1022
+	  have a JOG feature, which provides a dynamic mechanism
+	  to lower or raise the CPU core clock at runtime.
+
 config PPC_PASEMI_CPUFREQ
 	bool "Support for PA Semi PWRficient"
 	depends on PPC_PASEMI
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index 8976534..401cac0 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -62,5 +62,10 @@ void fsl_hv_halt(void);
  * code can be compatible with both 32-bit & 36-bit.
  */
 extern void mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq);
+
+static inline void mpc85xx_enter_jog(u64 ccsrbar, u32 powmgtreq)
+{
+	mpc85xx_enter_deep_sleep(ccsrbar, powmgtreq);
+}
 #endif
 #endif
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index ee28844..eceb399 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -147,6 +147,8 @@ void notify_cpu_starting(unsigned int cpu);
 extern void cpu_maps_update_begin(void);
 extern void cpu_maps_update_done(void);
 
+extern void cpu_hotplug_disable_before_freeze(void);
+extern void cpu_hotplug_enable_after_thaw(void);
 #else	/* CONFIG_SMP */
 
 #define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
@@ -168,6 +170,8 @@ static inline void cpu_maps_update_done(void)
 {
 }
 
+static inline void cpu_hotplug_disable_before_freeze(void)	{}
+static inline void cpu_hotplug_enable_after_thaw(void)	{}
 #endif /* CONFIG_SMP */
 extern struct bus_type cpu_subsys;
 
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..c4cd342 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -381,6 +381,36 @@ out:
 }
 EXPORT_SYMBOL_GPL(cpu_up);
 
+/*
+ * Prevent regular CPU hotplug from racing with the freezer, by disabling CPU
+ * hotplug when tasks are about to be frozen. Also, don't allow the freezer
+ * to continue until any currently running CPU hotplug operation gets
+ * completed.
+ * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
+ * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
+ * CPU hotplug path and released only after it is complete. Thus, we
+ * (and hence the freezer) will block here until any currently running CPU
+ * hotplug operation gets completed.
+ */
+void cpu_hotplug_disable_before_freeze(void)
+{
+	cpu_maps_update_begin();
+	cpu_hotplug_disabled = 1;
+	cpu_maps_update_done();
+}
+
+
+/*
+ * When tasks have been thawed, re-enable regular CPU hotplug (which had been
+ * disabled while beginning to freeze tasks).
+ */
+void cpu_hotplug_enable_after_thaw(void)
+{
+	cpu_maps_update_begin();
+	cpu_hotplug_disabled = 0;
+	cpu_maps_update_done();
+}
+
 #ifdef CONFIG_PM_SLEEP_SMP
 static cpumask_var_t frozen_cpus;
 
@@ -479,36 +509,6 @@ static int __init alloc_frozen_cpus(void)
 core_initcall(alloc_frozen_cpus);
 
 /*
- * Prevent regular CPU hotplug from racing with the freezer, by disabling CPU
- * hotplug when tasks are about to be frozen. Also, don't allow the freezer
- * to continue until any currently running CPU hotplug operation gets
- * completed.
- * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
- * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
- * CPU hotplug path and released only after it is complete. Thus, we
- * (and hence the freezer) will block here until any currently running CPU
- * hotplug operation gets completed.
- */
-void cpu_hotplug_disable_before_freeze(void)
-{
-	cpu_maps_update_begin();
-	cpu_hotplug_disabled = 1;
-	cpu_maps_update_done();
-}
-
-
-/*
- * When tasks have been thawed, re-enable regular CPU hotplug (which had been
- * disabled while beginning to freeze tasks).
- */
-void cpu_hotplug_enable_after_thaw(void)
-{
-	cpu_maps_update_begin();
-	cpu_hotplug_disabled = 0;
-	cpu_maps_update_done();
-}
-
-/*
  * When callbacks for CPU hotplug notifications are being executed, we must
  * ensure that the state of the system with respect to the tasks being frozen
  * or not, as reported by the notification, remains unchanged *throughout the
-- 
1.6.4.1

Re: [PATCH v5 5/5] powerpc/85xx: add support to JOG feature using cpufreq interface

[Scott Wood]

On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> Some 85xx silicons like MPC8536 and P1022 have a JOG feature, which provides
> a dynamic mechanism to lower or raise the CPU core clock at runtime.

Is there a reason P1023 isn't supported?

> This patch adds the support to change CPU frequency using the standard
> cpufreq interface. The ratio CORE to CCB can be 1:1(except MPC8536), 3:2,
> 2:1, 5:2, 3:1, 7:2 and 4:1.
> 
> Two CPU cores on P1022 must not in the low power state during the frequency
> transition. The driver uses a atomic counter to meet the requirement.
> 
> The jog mode frequency transition process on the MPC8536 is similar to
> the deep sleep process. The driver need save the CPU state and restore
> it after CPU warm reset.
> 
> Note:
>  * The I/O peripherals such as PCIe and eTSEC may lose packets during
>    the jog mode frequency transition.

That might be acceptable for eTSEC, but it is not acceptable to lose
anything on PCIe.  Especially not if you're going to make this "default y".


> +static int mpc85xx_job_probe(struct platform_device *ofdev)

Job?

> +{
> +	struct device_node *np = ofdev->dev.of_node;
> +	unsigned int svr;
> +
> +	if (of_device_is_compatible(np, "fsl,mpc8536-guts")) {
> +		svr = mfspr(SPRN_SVR);
> +		if ((svr & 0x7fff) == 0x10) {
> +			pr_err("MPC8536 Rev 1.0 do not support JOG.\n");
> +			return -ENODEV;
> +		}

s/do not support JOG/does not support cpufreq/

> +		mpc85xx_freqs = mpc8536_freqs_table;
> +		set_pll = mpc8536_set_pll;
> +	} else if (of_device_is_compatible(np, "fsl,p1022-guts")) {
> +		mpc85xx_freqs = p1022_freqs_table;
> +		set_pll = p1022_set_pll;
> +	} else {
> +		return -ENODEV;
> +	}
> +
> +	sysfreq = fsl_get_sys_freq();
> +
> +	guts = of_iomap(np, 0);
> +	if (!guts)
> +		return -ENODEV;
> +
> +	max_pll[0] = get_pll(0);
> +	if (mpc85xx_freqs == p1022_freqs_table)
> +		max_pll[1] = get_pll(1);
> +
> +	pr_info("Freescale MPC85xx CPU frequency switching(JOG) driver\n");
> +
> +	return cpufreq_register_driver(&mpc85xx_cpufreq_driver);
> +}
> +
> +static int mpc85xx_jog_remove(struct platform_device *ofdev)
> +{
> +	iounmap(guts);
> +	cpufreq_unregister_driver(&mpc85xx_cpufreq_driver);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id mpc85xx_jog_ids[] = {
> +	{ .compatible = "fsl,mpc8536-guts", },
> +	{ .compatible = "fsl,p1022-guts", },
> +	{}
> +};
> +
> +static struct platform_driver mpc85xx_jog_driver = {
> +	.driver = {
> +		.name = "mpc85xx_cpufreq_jog",
> +		.owner = THIS_MODULE,
> +		.of_match_table = mpc85xx_jog_ids,
> +	},
> +	.probe = mpc85xx_job_probe,
> +	.remove = mpc85xx_jog_remove,
> +};

Why is this a separate driver from fsl_pmc.c?

Only one driver can bind to a node through normal mechanisms -- you
don't get to take the entire guts node for this.

> +static int __init mpc85xx_jog_init(void)
> +{
> +	return platform_driver_register(&mpc85xx_jog_driver);
> +}
> +
> +static void __exit mpc85xx_jog_exit(void)
> +{
> +	platform_driver_unregister(&mpc85xx_jog_driver);
> +}
> +
> +module_init(mpc85xx_jog_init);
> +module_exit(mpc85xx_jog_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Dave Liu <daveliu@freescale.com>");
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index a35ca44..445bedd 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -204,6 +204,17 @@ config CPU_FREQ_PMAC64
>  	  This adds support for frequency switching on Apple iMac G5,
>  	  and some of the more recent desktop G5 machines as well.
>  
> +config MPC85xx_CPUFREQ
> +	bool "Support for Freescale MPC85xx CPU freq"
> +	depends on PPC_85xx && PPC32 && !PPC_E500MC

PPC32 is redundant given the !PPC_E500MC.

> index 8976534..401cac0 100644
> --- a/arch/powerpc/sysdev/fsl_soc.h
> +++ b/arch/powerpc/sysdev/fsl_soc.h
> @@ -62,5 +62,10 @@ void fsl_hv_halt(void);
>   * code can be compatible with both 32-bit & 36-bit.
>   */
>  extern void mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq);
> +
> +static inline void mpc85xx_enter_jog(u64 ccsrbar, u32 powmgtreq)
> +{
> +	mpc85xx_enter_deep_sleep(ccsrbar, powmgtreq);
> +}

What value is this function adding over mpc85xx_enter_deep_sleep()?

> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 2060c6e..c4cd342 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -381,6 +381,36 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(cpu_up);
>  
> +/*
> + * Prevent regular CPU hotplug from racing with the freezer, by disabling CPU
> + * hotplug when tasks are about to be frozen. Also, don't allow the freezer
> + * to continue until any currently running CPU hotplug operation gets
> + * completed.
> + * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
> + * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
> + * CPU hotplug path and released only after it is complete. Thus, we
> + * (and hence the freezer) will block here until any currently running CPU
> + * hotplug operation gets completed.
> + */
> +void cpu_hotplug_disable_before_freeze(void)
> +{
> +	cpu_maps_update_begin();
> +	cpu_hotplug_disabled = 1;
> +	cpu_maps_update_done();
> +}
> +
> +
> +/*
> + * When tasks have been thawed, re-enable regular CPU hotplug (which had been
> + * disabled while beginning to freeze tasks).
> + */
> +void cpu_hotplug_enable_after_thaw(void)
> +{
> +	cpu_maps_update_begin();
> +	cpu_hotplug_disabled = 0;
> +	cpu_maps_update_done();
> +}
> +
>  #ifdef CONFIG_PM_SLEEP_SMP
>  static cpumask_var_t frozen_cpus;
>  
> @@ -479,36 +509,6 @@ static int __init alloc_frozen_cpus(void)
>  core_initcall(alloc_frozen_cpus);
>  
>  /*
> - * Prevent regular CPU hotplug from racing with the freezer, by disabling CPU
> - * hotplug when tasks are about to be frozen. Also, don't allow the freezer
> - * to continue until any currently running CPU hotplug operation gets
> - * completed.
> - * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
> - * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
> - * CPU hotplug path and released only after it is complete. Thus, we
> - * (and hence the freezer) will block here until any currently running CPU
> - * hotplug operation gets completed.
> - */
> -void cpu_hotplug_disable_before_freeze(void)
> -{
> -	cpu_maps_update_begin();
> -	cpu_hotplug_disabled = 1;
> -	cpu_maps_update_done();
> -}
> -
> -
> -/*
> - * When tasks have been thawed, re-enable regular CPU hotplug (which had been
> - * disabled while beginning to freeze tasks).
> - */
> -void cpu_hotplug_enable_after_thaw(void)
> -{
> -	cpu_maps_update_begin();
> -	cpu_hotplug_disabled = 0;
> -	cpu_maps_update_done();
> -}
> -
> -/*
>   * When callbacks for CPU hotplug notifications are being executed, we must
>   * ensure that the state of the system with respect to the tasks being frozen
>   * or not, as reported by the notification, remains unchanged *throughout the

Why did you need to move this?

-Scott

Re: [PATCH v5 5/5] powerpc/85xx: add support to JOG feature using cpufreq interface

[Zhao Chenhui]

On Fri, Jun 01, 2012 at 06:30:55PM -0500, Scott Wood wrote:
> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> > Some 85xx silicons like MPC8536 and P1022 have a JOG feature, which provides
> > a dynamic mechanism to lower or raise the CPU core clock at runtime.
> 
> Is there a reason P1023 isn't supported?

P1023 support is deferred.

> 
> > This patch adds the support to change CPU frequency using the standard
> > cpufreq interface. The ratio CORE to CCB can be 1:1(except MPC8536), 3:2,
> > 2:1, 5:2, 3:1, 7:2 and 4:1.
> > 
> > Two CPU cores on P1022 must not in the low power state during the frequency
> > transition. The driver uses a atomic counter to meet the requirement.
> > 
> > The jog mode frequency transition process on the MPC8536 is similar to
> > the deep sleep process. The driver need save the CPU state and restore
> > it after CPU warm reset.
> > 
> > Note:
> >  * The I/O peripherals such as PCIe and eTSEC may lose packets during
> >    the jog mode frequency transition.
> 
> That might be acceptable for eTSEC, but it is not acceptable to lose
> anything on PCIe.  Especially not if you're going to make this "default y".

It is a hardware limitation. Peripherals in the platform will not be operating
during the jog mode frequency transition process.

I can make it "default n".

> 
> 
> > +static int mpc85xx_job_probe(struct platform_device *ofdev)
> 
> Job?

Sorry, It should be "jog".

> 
> > +{
> > +	struct device_node *np = ofdev->dev.of_node;
> > +	unsigned int svr;
> > +
> > +	if (of_device_is_compatible(np, "fsl,mpc8536-guts")) {
> > +		svr = mfspr(SPRN_SVR);
> > +		if ((svr & 0x7fff) == 0x10) {
> > +			pr_err("MPC8536 Rev 1.0 do not support JOG.\n");
> > +			return -ENODEV;
> > +		}
> 
> s/do not support JOG/does not support cpufreq/
> 
> > +		mpc85xx_freqs = mpc8536_freqs_table;
> > +		set_pll = mpc8536_set_pll;
> > +	} else if (of_device_is_compatible(np, "fsl,p1022-guts")) {
> > +		mpc85xx_freqs = p1022_freqs_table;
> > +		set_pll = p1022_set_pll;
> > +	} else {
> > +		return -ENODEV;
> > +	}
> > +
> > +	sysfreq = fsl_get_sys_freq();
> > +
> > +	guts = of_iomap(np, 0);
> > +	if (!guts)
> > +		return -ENODEV;
> > +
> > +	max_pll[0] = get_pll(0);
> > +	if (mpc85xx_freqs == p1022_freqs_table)
> > +		max_pll[1] = get_pll(1);
> > +
> > +	pr_info("Freescale MPC85xx CPU frequency switching(JOG) driver\n");
> > +
> > +	return cpufreq_register_driver(&mpc85xx_cpufreq_driver);
> > +}
> > +
> > +static int mpc85xx_jog_remove(struct platform_device *ofdev)
> > +{
> > +	iounmap(guts);
> > +	cpufreq_unregister_driver(&mpc85xx_cpufreq_driver);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct of_device_id mpc85xx_jog_ids[] = {
> > +	{ .compatible = "fsl,mpc8536-guts", },
> > +	{ .compatible = "fsl,p1022-guts", },
> > +	{}
> > +};
> > +
> > +static struct platform_driver mpc85xx_jog_driver = {
> > +	.driver = {
> > +		.name = "mpc85xx_cpufreq_jog",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = mpc85xx_jog_ids,
> > +	},
> > +	.probe = mpc85xx_job_probe,
> > +	.remove = mpc85xx_jog_remove,
> > +};
> 
> Why is this a separate driver from fsl_pmc.c?
> 
> Only one driver can bind to a node through normal mechanisms -- you
> don't get to take the entire guts node for this.

You are right. I will not bind this to the guts node.

> 
> > +static int __init mpc85xx_jog_init(void)
> > +{
> > +	return platform_driver_register(&mpc85xx_jog_driver);
> > +}
> > +
> > +static void __exit mpc85xx_jog_exit(void)
> > +{
> > +	platform_driver_unregister(&mpc85xx_jog_driver);
> > +}
> > +
> > +module_init(mpc85xx_jog_init);
> > +module_exit(mpc85xx_jog_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Dave Liu <daveliu@freescale.com>");
> > diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> > index a35ca44..445bedd 100644
> > --- a/arch/powerpc/platforms/Kconfig
> > +++ b/arch/powerpc/platforms/Kconfig
> > @@ -204,6 +204,17 @@ config CPU_FREQ_PMAC64
> >  	  This adds support for frequency switching on Apple iMac G5,
> >  	  and some of the more recent desktop G5 machines as well.
> >  
> > +config MPC85xx_CPUFREQ
> > +	bool "Support for Freescale MPC85xx CPU freq"
> > +	depends on PPC_85xx && PPC32 && !PPC_E500MC
> 
> PPC32 is redundant given the !PPC_E500MC.

Yes.

> 
> > index 8976534..401cac0 100644
> > --- a/arch/powerpc/sysdev/fsl_soc.h
> > +++ b/arch/powerpc/sysdev/fsl_soc.h
> > @@ -62,5 +62,10 @@ void fsl_hv_halt(void);
> >   * code can be compatible with both 32-bit & 36-bit.
> >   */
> >  extern void mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq);
> > +
> > +static inline void mpc85xx_enter_jog(u64 ccsrbar, u32 powmgtreq)
> > +{
> > +	mpc85xx_enter_deep_sleep(ccsrbar, powmgtreq);
> > +}
> 
> What value is this function adding over mpc85xx_enter_deep_sleep()?

Just an alias name. If this is improper, I could use mpc85xx_enter_deep_sleep() directly.

-Chenhui

Re: [PATCH v5 5/5] powerpc/85xx: add support to JOG feature using cpufreq interface

[Scott Wood]

On 06/05/2012 05:59 AM, Zhao Chenhui wrote:
> On Fri, Jun 01, 2012 at 06:30:55PM -0500, Scott Wood wrote:
>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>>> The jog mode frequency transition process on the MPC8536 is similar to
>>> the deep sleep process. The driver need save the CPU state and restore
>>> it after CPU warm reset.
>>>
>>> Note:
>>>  * The I/O peripherals such as PCIe and eTSEC may lose packets during
>>>    the jog mode frequency transition.
>>
>> That might be acceptable for eTSEC, but it is not acceptable to lose
>> anything on PCIe.  Especially not if you're going to make this "default y".
> 
> It is a hardware limitation.

Then make sure jog isn't used if PCIe is used.

Maybe you could do something with the suspend infrastructure, but this
is sufficiently heavyweight that transitions should be manually
requested, not triggered by the automatic cpufreq governor.

Does this apply to p1022, or just mpc8536?

> Peripherals in the platform will not be operating
> during the jog mode frequency transition process.

What ensures this?

-Scott

Re: [PATCH v5 5/5] powerpc/85xx: add support to JOG feature using cpufreq interface

[Zhao Chenhui]

On Tue, Jun 05, 2012 at 10:58:41AM -0500, Scott Wood wrote:
> On 06/05/2012 05:59 AM, Zhao Chenhui wrote:
> > On Fri, Jun 01, 2012 at 06:30:55PM -0500, Scott Wood wrote:
> >> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> >>> The jog mode frequency transition process on the MPC8536 is similar to
> >>> the deep sleep process. The driver need save the CPU state and restore
> >>> it after CPU warm reset.
> >>>
> >>> Note:
> >>>  * The I/O peripherals such as PCIe and eTSEC may lose packets during
> >>>    the jog mode frequency transition.
> >>
> >> That might be acceptable for eTSEC, but it is not acceptable to lose
> >> anything on PCIe.  Especially not if you're going to make this "default y".
> > 
> > It is a hardware limitation.
> 
> Then make sure jog isn't used if PCIe is used.
> 
> Maybe you could do something with the suspend infrastructure, but this
> is sufficiently heavyweight that transitions should be manually
> requested, not triggered by the automatic cpufreq governor.
> 
> Does this apply to p1022, or just mpc8536?

Both of them.

> 
> > Peripherals in the platform will not be operating
> > during the jog mode frequency transition process.
> 
> What ensures this?
> 
> -Scott

Hardware ensures it without software intervention.

-Chenhui

Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

[Li Yang]

Hi Scott,

Thanks for the valuable comment raised before and we have updated the
patches accordingly.  Please review the updated patch set and ACK if
they are good to you.  We hope it can be applied in this window.

Leo

On Fri, May 11, 2012 at 7:53 PM, Zhao Chenhui
<chenhui.zhao@freescale.com> wrote:
> Do hardware timebase sync. Firstly, stop all timebases, and transfer
> the timebase value of the boot core to the other core. Finally,
> start all timebases.
>
> Only apply to dual-core chips, such as MPC8572, P2020, etc.
>
> Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
>  arch/powerpc/include/asm/fsl_guts.h |    2 +
>  arch/powerpc/platforms/85xx/smp.c   |   93 +++++++++++++++++++++++++++++++++--
>  2 files changed, 91 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/fsl_guts.h b/arch/powerpc/include/asm/fsl_guts.h
> index aa4c488..dd5ba2c 100644
> --- a/arch/powerpc/include/asm/fsl_guts.h
> +++ b/arch/powerpc/include/asm/fsl_guts.h
> @@ -48,6 +48,8 @@ struct ccsr_guts {
>         __be32  dmuxcr;                /* 0x.0068 - DMA Mux Control Register */
>         u8     res06c[0x70 - 0x6c];
>        __be32  devdisr;        /* 0x.0070 - Device Disable Control */
> +#define CCSR_GUTS_DEVDISR_TB1  0x00001000
> +#define CCSR_GUTS_DEVDISR_TB0  0x00004000
>        __be32  devdisr2;       /* 0x.0074 - Device Disable Control 2 */
>        u8      res078[0x7c - 0x78];
>        __be32  pmjcr;          /* 0x.007c - 4 Power Management Jog Control Register */
> diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
> index ff42490..6862dda 100644
> --- a/arch/powerpc/platforms/85xx/smp.c
> +++ b/arch/powerpc/platforms/85xx/smp.c
> @@ -24,6 +24,7 @@
>  #include <asm/mpic.h>
>  #include <asm/cacheflush.h>
>  #include <asm/dbell.h>
> +#include <asm/fsl_guts.h>
>
>  #include <sysdev/fsl_soc.h>
>  #include <sysdev/mpic.h>
> @@ -115,13 +116,70 @@ smp_85xx_kick_cpu(int nr)
>
>  struct smp_ops_t smp_85xx_ops = {
>        .kick_cpu = smp_85xx_kick_cpu,
> -#ifdef CONFIG_KEXEC
> -       .give_timebase  = smp_generic_give_timebase,
> -       .take_timebase  = smp_generic_take_timebase,
> -#endif
>  };
>
>  #ifdef CONFIG_KEXEC
> +static struct ccsr_guts __iomem *guts;
> +static u64 timebase;
> +static int tb_req;
> +static int tb_valid;
> +
> +static void mpc85xx_timebase_freeze(int freeze)
> +{
> +       unsigned int mask;
> +
> +       if (!guts)
> +               return;
> +
> +       mask = CCSR_GUTS_DEVDISR_TB0 | CCSR_GUTS_DEVDISR_TB1;
> +       if (freeze)
> +               setbits32(&guts->devdisr, mask);
> +       else
> +               clrbits32(&guts->devdisr, mask);
> +
> +       in_be32(&guts->devdisr);
> +}
> +
> +static void mpc85xx_give_timebase(void)
> +{
> +       unsigned long flags;
> +
> +       local_irq_save(flags);
> +
> +       while (!tb_req)
> +               barrier();
> +       tb_req = 0;
> +
> +       mpc85xx_timebase_freeze(1);
> +       timebase = get_tb();
> +       mb();
> +       tb_valid = 1;
> +
> +       while (tb_valid)
> +               barrier();
> +
> +       mpc85xx_timebase_freeze(0);
> +
> +       local_irq_restore(flags);
> +}
> +
> +static void mpc85xx_take_timebase(void)
> +{
> +       unsigned long flags;
> +
> +       local_irq_save(flags);
> +
> +       tb_req = 1;
> +       while (!tb_valid)
> +               barrier();
> +
> +       set_tb(timebase >> 32, timebase & 0xffffffff);
> +       mb();
> +       tb_valid = 0;
> +
> +       local_irq_restore(flags);
> +}
> +
>  atomic_t kexec_down_cpus = ATOMIC_INIT(0);
>
>  void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary)
> @@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr)
>                doorbell_setup_this_cpu();
>  }
>
> +#ifdef CONFIG_KEXEC
> +static const struct of_device_id guts_ids[] = {
> +       { .compatible = "fsl,mpc8572-guts", },
> +       { .compatible = "fsl,mpc8560-guts", },
> +       { .compatible = "fsl,mpc8536-guts", },
> +       { .compatible = "fsl,p1020-guts", },
> +       { .compatible = "fsl,p1021-guts", },
> +       { .compatible = "fsl,p1022-guts", },
> +       { .compatible = "fsl,p1023-guts", },
> +       { .compatible = "fsl,p2020-guts", },
> +       {},
> +};
> +#endif
> +
>  void __init mpc85xx_smp_init(void)
>  {
>        struct device_node *np;
> @@ -249,6 +321,19 @@ void __init mpc85xx_smp_init(void)
>                smp_85xx_ops.cause_ipi = doorbell_cause_ipi;
>        }
>
> +#ifdef CONFIG_KEXEC
> +       np = of_find_matching_node(NULL, guts_ids);
> +       if (np) {
> +               guts = of_iomap(np, 0);
> +               smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
> +               smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
> +               of_node_put(np);
> +       } else {
> +               smp_85xx_ops.give_timebase = smp_generic_give_timebase;
> +               smp_85xx_ops.take_timebase = smp_generic_take_timebase;
> +       }
> +#endif
> +
>        smp_ops = &smp_85xx_ops;
>
>  #ifdef CONFIG_KEXEC
> --
> 1.6.4.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
- Leo

Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

[Scott Wood]

On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
>  #ifdef CONFIG_KEXEC
> +static struct ccsr_guts __iomem *guts;
> +static u64 timebase;
> +static int tb_req;
> +static int tb_valid;
> +
> +static void mpc85xx_timebase_freeze(int freeze)

Why is this under CONFIG_KEXEC?  It'll also be needed for CPU hotplug.

> +{
> +	unsigned int mask;
> +
> +	if (!guts)
> +		return;
> +
> +	mask = CCSR_GUTS_DEVDISR_TB0 | CCSR_GUTS_DEVDISR_TB1;
> +	if (freeze)
> +		setbits32(&guts->devdisr, mask);
> +	else
> +		clrbits32(&guts->devdisr, mask);
> +
> +	in_be32(&guts->devdisr);
> +}
> +
> +static void mpc85xx_give_timebase(void)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +
> +	while (!tb_req)
> +		barrier();
> +	tb_req = 0;
> +
> +	mpc85xx_timebase_freeze(1);
> +	timebase = get_tb();
> +	mb();
> +	tb_valid = 1;
> +
> +	while (tb_valid)
> +		barrier();
> +
> +	mpc85xx_timebase_freeze(0);
> +
> +	local_irq_restore(flags);
> +}
> +
> +static void mpc85xx_take_timebase(void)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +
> +	tb_req = 1;
> +	while (!tb_valid)
> +		barrier();
> +
> +	set_tb(timebase >> 32, timebase & 0xffffffff);
> +	mb();
> +	tb_valid = 0;
> +
> +	local_irq_restore(flags);
> +}

I know you say this is for dual-core chips only, but it would be nice if
you'd write this in a way that doesn't assume that (even if the
corenet-specific timebase freezing comes later).

Do we need an isync after setting the timebase, to ensure it's happened
before we enable the timebase?  Likewise, do we need a readback after
disabling the timebase to ensure it's disabled before we read the
timebase in give_timebase?

>  atomic_t kexec_down_cpus = ATOMIC_INIT(0);
>  
>  void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary)
> @@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr)
>  		doorbell_setup_this_cpu();
>  }
>  
> +#ifdef CONFIG_KEXEC
> +static const struct of_device_id guts_ids[] = {
> +	{ .compatible = "fsl,mpc8572-guts", },
> +	{ .compatible = "fsl,mpc8560-guts", },
> +	{ .compatible = "fsl,mpc8536-guts", },
> +	{ .compatible = "fsl,p1020-guts", },
> +	{ .compatible = "fsl,p1021-guts", },
> +	{ .compatible = "fsl,p1022-guts", },
> +	{ .compatible = "fsl,p1023-guts", },
> +	{ .compatible = "fsl,p2020-guts", },
> +	{},
> +};
> +#endif

MPC8560 and MPC8536 are single-core...

Also please use a more specific name, such as e500v2_smp_guts_ids or
mpc85xx_smp_guts_ids -- when corenet support is added it will likely be
in the same file.

>  void __init mpc85xx_smp_init(void)
>  {
>  	struct device_node *np;
> @@ -249,6 +321,19 @@ void __init mpc85xx_smp_init(void)
>  		smp_85xx_ops.cause_ipi = doorbell_cause_ipi;
>  	}
>  
> +#ifdef CONFIG_KEXEC
> +	np = of_find_matching_node(NULL, guts_ids);
> +	if (np) {
> +		guts = of_iomap(np, 0);
> +		smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
> +		smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
> +		of_node_put(np);
> +	} else {
> +		smp_85xx_ops.give_timebase = smp_generic_give_timebase;
> +		smp_85xx_ops.take_timebase = smp_generic_take_timebase;
> +	}

Do not use smp_generic_give/take_timebase, ever.  If you don't have the
guts node, then just assume the timebase is already synced.

-Scott

Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

[Zhao Chenhui]

On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote:
> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> >  #ifdef CONFIG_KEXEC
> > +static struct ccsr_guts __iomem *guts;
> > +static u64 timebase;
> > +static int tb_req;
> > +static int tb_valid;
> > +
> > +static void mpc85xx_timebase_freeze(int freeze)
> 
> Why is this under CONFIG_KEXEC?  It'll also be needed for CPU hotplug.

Yes, the timebase sync is also needed for CPU hotplug, but this patch is unrelated to CPU hotplug.
I added CONFIG_HOTPLUG_CPU in the next patch.

> 
> > +{
> > +	unsigned int mask;
> > +
> > +	if (!guts)
> > +		return;
> > +
> > +	mask = CCSR_GUTS_DEVDISR_TB0 | CCSR_GUTS_DEVDISR_TB1;
> > +	if (freeze)
> > +		setbits32(&guts->devdisr, mask);
> > +	else
> > +		clrbits32(&guts->devdisr, mask);
> > +
> > +	in_be32(&guts->devdisr);
> > +}
> > +
> > +static void mpc85xx_give_timebase(void)
> > +{
> > +	unsigned long flags;
> > +
> > +	local_irq_save(flags);
> > +
> > +	while (!tb_req)
> > +		barrier();
> > +	tb_req = 0;
> > +
> > +	mpc85xx_timebase_freeze(1);
> > +	timebase = get_tb();
> > +	mb();
> > +	tb_valid = 1;
> > +
> > +	while (tb_valid)
> > +		barrier();
> > +
> > +	mpc85xx_timebase_freeze(0);
> > +
> > +	local_irq_restore(flags);
> > +}
> > +
> > +static void mpc85xx_take_timebase(void)
> > +{
> > +	unsigned long flags;
> > +
> > +	local_irq_save(flags);
> > +
> > +	tb_req = 1;
> > +	while (!tb_valid)
> > +		barrier();
> > +
> > +	set_tb(timebase >> 32, timebase & 0xffffffff);
> > +	mb();
> > +	tb_valid = 0;
> > +
> > +	local_irq_restore(flags);
> > +}
> 
> I know you say this is for dual-core chips only, but it would be nice if
> you'd write this in a way that doesn't assume that (even if the
> corenet-specific timebase freezing comes later).

At this point, I have not thought about how to implement the cornet-specific timebase freezing.

> 
> Do we need an isync after setting the timebase, to ensure it's happened
> before we enable the timebase?  Likewise, do we need a readback after
> disabling the timebase to ensure it's disabled before we read the
> timebase in give_timebase?

I checked the e500 core manual (Chapter 2.16 Synchronization Requirements for SPRs).
Only some SPR registers need an isync. The timebase registers do not.

I did a readback in mpc85xx_timebase_freeze().

> 
> >  atomic_t kexec_down_cpus = ATOMIC_INIT(0);
> >  
> >  void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary)
> > @@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr)
> >  		doorbell_setup_this_cpu();
> >  }
> >  
> > +#ifdef CONFIG_KEXEC
> > +static const struct of_device_id guts_ids[] = {
> > +	{ .compatible = "fsl,mpc8572-guts", },
> > +	{ .compatible = "fsl,mpc8560-guts", },
> > +	{ .compatible = "fsl,mpc8536-guts", },
> > +	{ .compatible = "fsl,p1020-guts", },
> > +	{ .compatible = "fsl,p1021-guts", },
> > +	{ .compatible = "fsl,p1022-guts", },
> > +	{ .compatible = "fsl,p1023-guts", },
> > +	{ .compatible = "fsl,p2020-guts", },
> > +	{},
> > +};
> > +#endif
> 
> MPC8560 and MPC8536 are single-core...

Thanks. I will remove them.

> 
> Also please use a more specific name, such as e500v2_smp_guts_ids or
> mpc85xx_smp_guts_ids -- when corenet support is added it will likely be
> in the same file.
> 
> >  void __init mpc85xx_smp_init(void)
> >  {
> >  	struct device_node *np;
> > @@ -249,6 +321,19 @@ void __init mpc85xx_smp_init(void)
> >  		smp_85xx_ops.cause_ipi = doorbell_cause_ipi;
> >  	}
> >  
> > +#ifdef CONFIG_KEXEC
> > +	np = of_find_matching_node(NULL, guts_ids);
> > +	if (np) {
> > +		guts = of_iomap(np, 0);
> > +		smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
> > +		smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
> > +		of_node_put(np);
> > +	} else {
> > +		smp_85xx_ops.give_timebase = smp_generic_give_timebase;
> > +		smp_85xx_ops.take_timebase = smp_generic_take_timebase;
> > +	}
> 
> Do not use smp_generic_give/take_timebase, ever.  If you don't have the
> guts node, then just assume the timebase is already synced.
> 
> -Scott

smp_generic_give/take_timebase is the default in KEXEC before.
If do not set them, it may make KEXEC fail on other platforms.

-Chenhui

Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

[Scott Wood]

On 06/05/2012 04:08 AM, Zhao Chenhui wrote:
> On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote:
>> I know you say this is for dual-core chips only, but it would be nice if
>> you'd write this in a way that doesn't assume that (even if the
>> corenet-specific timebase freezing comes later).
> 
> At this point, I have not thought about how to implement the cornet-specific timebase freezing.

I wasn't asking you to.  I was asking you to not have logic that breaks
with more than 2 CPUs.

>> Do we need an isync after setting the timebase, to ensure it's happened
>> before we enable the timebase?  Likewise, do we need a readback after
>> disabling the timebase to ensure it's disabled before we read the
>> timebase in give_timebase?
> 
> I checked the e500 core manual (Chapter 2.16 Synchronization Requirements for SPRs).
> Only some SPR registers need an isync. The timebase registers do not.

I don't trust that, and the consequences of having the sync be imperfect
are too unpleasant to chance it.

> I did a readback in mpc85xx_timebase_freeze().

Sorry, missed that somehow.

>>> +#ifdef CONFIG_KEXEC
>>> +	np = of_find_matching_node(NULL, guts_ids);
>>> +	if (np) {
>>> +		guts = of_iomap(np, 0);
>>> +		smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
>>> +		smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
>>> +		of_node_put(np);
>>> +	} else {
>>> +		smp_85xx_ops.give_timebase = smp_generic_give_timebase;
>>> +		smp_85xx_ops.take_timebase = smp_generic_take_timebase;
>>> +	}
>>
>> Do not use smp_generic_give/take_timebase, ever.  If you don't have the
>> guts node, then just assume the timebase is already synced.
>>
>> -Scott
> 
> smp_generic_give/take_timebase is the default in KEXEC before.

That was a mistake.

> If do not set them, it may make KEXEC fail on other platforms.

What platforms?

-Scott

Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

[Zhao Chenhui]

On Tue, Jun 05, 2012 at 11:07:41AM -0500, Scott Wood wrote:
> On 06/05/2012 04:08 AM, Zhao Chenhui wrote:
> > On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote:
> >> I know you say this is for dual-core chips only, but it would be nice if
> >> you'd write this in a way that doesn't assume that (even if the
> >> corenet-specific timebase freezing comes later).
> > 
> > At this point, I have not thought about how to implement the cornet-specific timebase freezing.
> 
> I wasn't asking you to.  I was asking you to not have logic that breaks
> with more than 2 CPUs.

These routines only called in the dual-core case. 

> 
> >> Do we need an isync after setting the timebase, to ensure it's happened
> >> before we enable the timebase?  Likewise, do we need a readback after
> >> disabling the timebase to ensure it's disabled before we read the
> >> timebase in give_timebase?
> > 
> > I checked the e500 core manual (Chapter 2.16 Synchronization Requirements for SPRs).
> > Only some SPR registers need an isync. The timebase registers do not.
> 
> I don't trust that, and the consequences of having the sync be imperfect
> are too unpleasant to chance it.
> 
> > I did a readback in mpc85xx_timebase_freeze().
> 
> Sorry, missed that somehow.
> 
> >>> +#ifdef CONFIG_KEXEC
> >>> +	np = of_find_matching_node(NULL, guts_ids);
> >>> +	if (np) {
> >>> +		guts = of_iomap(np, 0);
> >>> +		smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
> >>> +		smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
> >>> +		of_node_put(np);
> >>> +	} else {
> >>> +		smp_85xx_ops.give_timebase = smp_generic_give_timebase;
> >>> +		smp_85xx_ops.take_timebase = smp_generic_take_timebase;
> >>> +	}
> >>
> >> Do not use smp_generic_give/take_timebase, ever.  If you don't have the
> >> guts node, then just assume the timebase is already synced.
> >>
> >> -Scott
> > 
> > smp_generic_give/take_timebase is the default in KEXEC before.
> 
> That was a mistake.
> 
> > If do not set them, it may make KEXEC fail on other platforms.
> 
> What platforms?
> 
> -Scott

Such as P4080, P3041, etc.

-Chenhui

Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

[Scott Wood]

On 06/06/2012 04:31 AM, Zhao Chenhui wrote:
> On Tue, Jun 05, 2012 at 11:07:41AM -0500, Scott Wood wrote:
>> On 06/05/2012 04:08 AM, Zhao Chenhui wrote:
>>> On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote:
>>>> I know you say this is for dual-core chips only, but it would be nice if
>>>> you'd write this in a way that doesn't assume that (even if the
>>>> corenet-specific timebase freezing comes later).
>>>
>>> At this point, I have not thought about how to implement the cornet-specific timebase freezing.
>>
>> I wasn't asking you to.  I was asking you to not have logic that breaks
>> with more than 2 CPUs.
> 
> These routines only called in the dual-core case. 

Come on, you know we have chips with more than two cores.  Why design
such a limitation into it, just because you're not personally interested
in supporting anything but e500v2?

Is it so hard to make it work for an arbitrary number of cores?

>>> If do not set them, it may make KEXEC fail on other platforms.
>>
>> What platforms?
> 
> Such as P4080, P3041, etc.

So we need to wait for corenet timebase sync before we stop causing
problems in virtualization, simulators, etc. if a kernel has kexec or
cpu hotplug enabled (whether used or not)?

Can you at least make sure we're actually in a kexec/hotplug scenario at
runtime?

Or just implement corenet timebase sync -- it's not that different.

-Scott

Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

[Zhao Chenhui]

On Wed, Jun 06, 2012 at 01:26:16PM -0500, Scott Wood wrote:
> On 06/06/2012 04:31 AM, Zhao Chenhui wrote:
> > On Tue, Jun 05, 2012 at 11:07:41AM -0500, Scott Wood wrote:
> >> On 06/05/2012 04:08 AM, Zhao Chenhui wrote:
> >>> On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote:
> >>>> I know you say this is for dual-core chips only, but it would be nice if
> >>>> you'd write this in a way that doesn't assume that (even if the
> >>>> corenet-specific timebase freezing comes later).
> >>>
> >>> At this point, I have not thought about how to implement the cornet-specific timebase freezing.
> >>
> >> I wasn't asking you to.  I was asking you to not have logic that breaks
> >> with more than 2 CPUs.
> > 
> > These routines only called in the dual-core case. 
> 
> Come on, you know we have chips with more than two cores.  Why design
> such a limitation into it, just because you're not personally interested
> in supporting anything but e500v2?
> 
> Is it so hard to make it work for an arbitrary number of cores?
> 
> >>> If do not set them, it may make KEXEC fail on other platforms.
> >>
> >> What platforms?
> > 
> > Such as P4080, P3041, etc.
> 
> So we need to wait for corenet timebase sync before we stop causing
> problems in virtualization, simulators, etc. if a kernel has kexec or
> cpu hotplug enabled (whether used or not)?
> 
> Can you at least make sure we're actually in a kexec/hotplug scenario at
> runtime?
> 
> Or just implement corenet timebase sync -- it's not that different.
> 
> -Scott

We also work on the corenet timebase sync. Our plan is first the dual-core case,
then the case of more than 2 cores. We will submit the corenet timebase sync patch soon.

-Chenhui