[PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[b29983]

From: Tang Yuantian <Yuantian.Tang@freescale.com>

Freescale CoreNet-based and Non-CoreNet-based platforms require
different PM operations. This patch extracted existing PM operations
on Non-CoreNet-based platforms to a new file which can accommodate
both platforms.
In this way, PM operation codes are clearer structurally.

Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com>
Signed-off-by: Tang Yuantian <Yuantian.Tang@feescale.com>
---
 arch/powerpc/platforms/85xx/Makefile         |  1 +
 arch/powerpc/platforms/85xx/common.c         |  3 +
 arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c | 95 ++++++++++++++++++++++++++++
 arch/powerpc/platforms/85xx/smp.c            | 86 +++++++++----------------
 arch/powerpc/sysdev/fsl_rcpm.c               |  2 -
 5 files changed, 128 insertions(+), 59 deletions(-)
 create mode 100644 arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c

diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index 1fe7fb9..7bc86da 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the PowerPC 85xx linux kernel.
 #
 obj-$(CONFIG_SMP) += smp.o
+obj-$(CONFIG_FSL_PMC)		  += mpc85xx_pm_ops.o
 
 obj-y += common.o
 
diff --git a/arch/powerpc/platforms/85xx/common.c b/arch/powerpc/platforms/85xx/common.c
index 7bfb9b1..91475f5 100644
--- a/arch/powerpc/platforms/85xx/common.c
+++ b/arch/powerpc/platforms/85xx/common.c
@@ -10,10 +10,13 @@
 #include <linux/of_platform.h>
 
 #include <asm/qe.h>
+#include <asm/fsl_pm.h>
 #include <sysdev/cpm2_pic.h>
 
 #include "mpc85xx.h"
 
+const struct fsl_pm_ops *qoriq_pm_ops;
+
 static const struct of_device_id mpc85xx_common_ids[] __initconst = {
 	{ .type = "soc", },
 	{ .compatible = "soc", },
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c b/arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c
new file mode 100644
index 0000000..e59714e
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c
@@ -0,0 +1,95 @@
+/*
+ * MPC85xx PM operators
+ *
+ * Copyright 2015 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
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include <asm/io.h>
+#include <asm/fsl_guts.h>
+#include <asm/fsl_pm.h>
+
+static struct ccsr_guts __iomem *guts;
+
+static const struct of_device_id mpc85xx_smp_guts_ids[] = {
+	{ .compatible = "fsl,mpc8572-guts", },
+	{ .compatible = "fsl,p1020-guts", },
+	{ .compatible = "fsl,p1021-guts", },
+	{ .compatible = "fsl,p1022-guts", },
+	{ .compatible = "fsl,p1023-guts", },
+	{ .compatible = "fsl,p2020-guts", },
+	{ .compatible = "fsl,bsc9132-guts", },
+	{},
+};
+
+static void mpc85xx_irq_mask(int cpu)
+{
+
+}
+
+static void mpc85xx_irq_unmask(int cpu)
+{
+
+}
+
+static void mpc85xx_cpu_die(int cpu)
+{
+
+}
+
+static void mpc85xx_cpu_up(int cpu)
+{
+
+}
+
+static void mpc85xx_freeze_time_base(bool freeze)
+{
+	uint32_t mask;
+
+	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 const struct fsl_pm_ops mpc85xx_pm_ops = {
+	.freeze_time_base = mpc85xx_freeze_time_base,
+	.irq_mask = mpc85xx_irq_mask,
+	.irq_unmask = mpc85xx_irq_unmask,
+	.cpu_die = mpc85xx_cpu_die,
+	.cpu_up = mpc85xx_cpu_up,
+};
+
+static int __init mpc85xx_setup_pmc(void)
+{
+	struct device_node *np;
+
+	np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
+	if (np) {
+		guts = of_iomap(np, 0);
+		of_node_put(np);
+		if (!guts) {
+			pr_err("%s: Could not map guts node address\n",
+								__func__);
+			return -ENOMEM;
+		}
+	}
+
+	qoriq_pm_ops = &mpc85xx_pm_ops;
+
+	return 0;
+}
+
+/* need to call this before SMP init */
+early_initcall(mpc85xx_setup_pmc)
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index 1180f78..6811a5b 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-2012 Freescale Semiconductor Inc.
+ * Copyright 2006-2008, 2011-2012, 2015 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
@@ -15,7 +15,6 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/of.h>
-#include <linux/of_address.h>
 #include <linux/kexec.h>
 #include <linux/highmem.h>
 #include <linux/cpu.h>
@@ -26,9 +25,9 @@
 #include <asm/mpic.h>
 #include <asm/cacheflush.h>
 #include <asm/dbell.h>
-#include <asm/fsl_guts.h>
 #include <asm/code-patching.h>
 #include <asm/cputhreads.h>
+#include <asm/fsl_pm.h>
 
 #include <sysdev/fsl_soc.h>
 #include <sysdev/mpic.h>
@@ -43,24 +42,10 @@ struct epapr_spin_table {
 	u32	pir;
 };
 
-static struct ccsr_guts __iomem *guts;
 static u64 timebase;
 static int tb_req;
 static int tb_valid;
 
-static void mpc85xx_timebase_freeze(int freeze)
-{
-	uint32_t mask;
-
-	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;
@@ -71,7 +56,7 @@ static void mpc85xx_give_timebase(void)
 		barrier();
 	tb_req = 0;
 
-	mpc85xx_timebase_freeze(1);
+	qoriq_pm_ops->freeze_time_base(1);
 #ifdef CONFIG_PPC64
 	/*
 	 * e5500/e6500 have a workaround for erratum A-006958 in place
@@ -104,7 +89,7 @@ static void mpc85xx_give_timebase(void)
 	while (tb_valid)
 		barrier();
 
-	mpc85xx_timebase_freeze(0);
+	qoriq_pm_ops->freeze_time_base(0);
 
 	local_irq_restore(flags);
 }
@@ -127,20 +112,10 @@ static void mpc85xx_take_timebase(void)
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
-static void smp_85xx_mach_cpu_die(void)
+static void e500_cpu_idle(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);
-
-	cur_cpu_spec->cpu_down_flush();
-
 	tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP;
 	mtspr(SPRN_HID0, tmp);
 	isync();
@@ -151,6 +126,25 @@ static void smp_85xx_mach_cpu_die(void)
 	mb();
 	mtmsr(tmp);
 	isync();
+}
+
+static void qoriq_cpu_dying(void)
+{
+	unsigned int cpu = smp_processor_id();
+
+	hard_irq_disable();
+	/* mask all irqs to prevent cpu wakeup */
+	qoriq_pm_ops->irq_mask(cpu);
+	idle_task_exit();
+
+	mtspr(SPRN_TCR, 0);
+	mtspr(SPRN_TSR, mfspr(SPRN_TSR));
+
+	cur_cpu_spec->cpu_down_flush();
+
+	generic_set_cpu_dead(cpu);
+
+	e500_cpu_idle();
 
 	while (1)
 		;
@@ -332,9 +326,9 @@ struct smp_ops_t smp_85xx_ops = {
 	.cpu_disable	= generic_cpu_disable,
 	.cpu_die	= generic_cpu_die,
 #endif
-#ifdef CONFIG_KEXEC
-	.give_timebase	= smp_generic_give_timebase,
-	.take_timebase	= smp_generic_take_timebase,
+#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
+	.give_timebase	= mpc85xx_give_timebase,
+	.take_timebase	= mpc85xx_take_timebase,
 #endif
 };
 
@@ -346,7 +340,7 @@ void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary)
 	local_irq_disable();
 
 	if (secondary) {
-		__flush_disable_L1();
+		cur_cpu_spec->cpu_down_flush();
 		atomic_inc(&kexec_down_cpus);
 		/* loop forever */
 		while (1);
@@ -398,16 +392,6 @@ static void smp_85xx_setup_cpu(int cpu_nr)
 	smp_85xx_basic_setup(cpu_nr);
 }
 
-static const struct of_device_id mpc85xx_smp_guts_ids[] = {
-	{ .compatible = "fsl,mpc8572-guts", },
-	{ .compatible = "fsl,p1020-guts", },
-	{ .compatible = "fsl,p1021-guts", },
-	{ .compatible = "fsl,p1022-guts", },
-	{ .compatible = "fsl,p1023-guts", },
-	{ .compatible = "fsl,p2020-guts", },
-	{},
-};
-
 void __init mpc85xx_smp_init(void)
 {
 	struct device_node *np;
@@ -431,21 +415,9 @@ void __init mpc85xx_smp_init(void)
 		smp_85xx_ops.probe = NULL;
 	}
 
-	np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
-	if (np) {
-		guts = of_iomap(np, 0);
-		of_node_put(np);
-		if (!guts) {
-			pr_err("%s: Could not map guts node address\n",
-								__func__);
-			return;
-		}
-		smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
-		smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
 #ifdef CONFIG_HOTPLUG_CPU
-		ppc_md.cpu_die = smp_85xx_mach_cpu_die;
+	ppc_md.cpu_die = qoriq_cpu_dying;
 #endif
-	}
 
 	smp_ops = &smp_85xx_ops;
 
diff --git a/arch/powerpc/sysdev/fsl_rcpm.c b/arch/powerpc/sysdev/fsl_rcpm.c
index acf3f94..e6ae678 100644
--- a/arch/powerpc/sysdev/fsl_rcpm.c
+++ b/arch/powerpc/sysdev/fsl_rcpm.c
@@ -20,8 +20,6 @@
 #include <asm/cputhreads.h>
 #include <asm/fsl_pm.h>
 
-const struct fsl_pm_ops *qoriq_pm_ops;
-
 static struct ccsr_rcpm_v1 __iomem *rcpm_v1_regs;
 static struct ccsr_rcpm_v2 __iomem *rcpm_v2_regs;
 static unsigned int fsl_supported_pm_modes;
-- 
2.1.0.27.g96db324

[PATCH 2/3] PowerPC/mpc85xx: Add hotplug support on E5500 and E500MC cores

[b29983]

From: Tang Yuantian <Yuantian.Tang@freescale.com>

Freescale E500MC and E5500 core-based platforms, like P4080, T1040,
support disabling/enabling CPU dynamically.
This patch adds this feature on those platforms.

Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com>
Signed-off-by: Tang Yuantian <Yuantian.Tang@feescale.com>
---
 arch/powerpc/Kconfig              |  2 +-
 arch/powerpc/include/asm/smp.h    |  1 +
 arch/powerpc/kernel/smp.c         |  5 +++++
 arch/powerpc/platforms/85xx/smp.c | 39 ++++++++++++++++++++++++++++++++-------
 4 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 5ef2711..dd9e252 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -386,7 +386,7 @@ config SWIOTLB
 config HOTPLUG_CPU
 	bool "Support for enabling/disabling CPUs"
 	depends on SMP && (PPC_PSERIES || \
-	PPC_PMAC || PPC_POWERNV || (PPC_85xx && !PPC_E500MC))
+	PPC_PMAC || PPC_POWERNV || FSL_SOC_BOOKE)
 	---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/smp.h b/arch/powerpc/include/asm/smp.h
index 825663c..bf37d17 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -67,6 +67,7 @@ void generic_cpu_die(unsigned int cpu);
 void generic_set_cpu_dead(unsigned int cpu);
 void generic_set_cpu_up(unsigned int cpu);
 int generic_check_cpu_restart(unsigned int cpu);
+int generic_check_cpu_dead(unsigned int cpu);
 #endif
 
 #ifdef CONFIG_PPC64
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index ec9ec20..2cca27a 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -454,6 +454,11 @@ int generic_check_cpu_restart(unsigned int cpu)
 	return per_cpu(cpu_state, cpu) == CPU_UP_PREPARE;
 }
 
+int generic_check_cpu_dead(unsigned int cpu)
+{
+	return per_cpu(cpu_state, cpu) == CPU_DEAD;
+}
+
 static bool secondaries_inhibited(void)
 {
 	return kvm_hv_mode_active();
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index 6811a5b..7f0dadb 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -42,6 +42,7 @@ struct epapr_spin_table {
 	u32	pir;
 };
 
+#ifdef CONFIG_HOTPLUG_CPU
 static u64 timebase;
 static int tb_req;
 static int tb_valid;
@@ -111,7 +112,7 @@ static void mpc85xx_take_timebase(void)
 	local_irq_restore(flags);
 }
 
-#ifdef CONFIG_HOTPLUG_CPU
+#ifndef CONFIG_PPC_E500MC
 static void e500_cpu_idle(void)
 {
 	u32 tmp;
@@ -127,6 +128,7 @@ static void e500_cpu_idle(void)
 	mtmsr(tmp);
 	isync();
 }
+#endif
 
 static void qoriq_cpu_dying(void)
 {
@@ -144,11 +146,30 @@ static void qoriq_cpu_dying(void)
 
 	generic_set_cpu_dead(cpu);
 
+#ifndef CONFIG_PPC_E500MC
 	e500_cpu_idle();
+#endif
 
 	while (1)
 		;
 }
+
+static void qoriq_real_cpu_die(unsigned int cpu)
+{
+	int i;
+
+	for (i = 0; i < 50000; i++) {
+		if (generic_check_cpu_dead(cpu)) {
+			qoriq_pm_ops->cpu_die(cpu);
+#ifdef CONFIG_PPC64
+			paca[cpu].cpu_start = 0;
+#endif
+			return;
+		}
+		udelay(10);
+	}
+	pr_err("%s: CPU%d didn't die...\n", __func__, cpu);
+}
 #endif
 
 static inline void flush_spin_table(void *spin_table)
@@ -246,11 +267,7 @@ static int smp_85xx_kick_cpu(int nr)
 		spin_table = phys_to_virt(*cpu_rel_addr);
 
 	local_irq_save(flags);
-#ifdef CONFIG_PPC32
 #ifdef CONFIG_HOTPLUG_CPU
-	/* Corresponding to generic_set_cpu_dead() */
-	generic_set_cpu_up(nr);
-
 	if (system_state == SYSTEM_RUNNING) {
 		/*
 		 * To keep it compatible with old boot program which uses
@@ -263,6 +280,7 @@ static int smp_85xx_kick_cpu(int nr)
 		out_be32(&spin_table->addr_l, 0);
 		flush_spin_table(spin_table);
 
+		qoriq_pm_ops->cpu_up(nr);
 		/*
 		 * We don't set the BPTR register here since it already points
 		 * to the boot page properly.
@@ -286,7 +304,12 @@ static int smp_85xx_kick_cpu(int nr)
 		/*  clear the acknowledge status */
 		__secondary_hold_acknowledge = -1;
 	}
+
+	/* Corresponding to generic_set_cpu_dead() */
+	generic_set_cpu_up(nr);
 #endif
+
+#ifdef CONFIG_PPC32
 	flush_spin_table(spin_table);
 	out_be32(&spin_table->pir, hw_cpu);
 	out_be32(&spin_table->addr_l, __pa(__early_start));
@@ -300,7 +323,6 @@ static int smp_85xx_kick_cpu(int nr)
 		ret = -ENOENT;
 		goto out;
 	}
-out:
 #else
 	smp_generic_kick_cpu(nr);
 
@@ -311,6 +333,9 @@ out:
 	flush_spin_table(spin_table);
 #endif
 
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_PPC32)
+out:
+#endif
 	local_irq_restore(flags);
 
 	if (ioremappable)
@@ -324,7 +349,7 @@ struct smp_ops_t smp_85xx_ops = {
 	.cpu_bootable = smp_generic_cpu_bootable,
 #ifdef CONFIG_HOTPLUG_CPU
 	.cpu_disable	= generic_cpu_disable,
-	.cpu_die	= generic_cpu_die,
+	.cpu_die	= qoriq_real_cpu_die,
 #endif
 #if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
 	.give_timebase	= mpc85xx_give_timebase,
-- 
2.1.0.27.g96db324

Re: [PATCH 2/3] PowerPC/mpc85xx: Add hotplug support on E5500 and E500MC cores

[Scott Wood]

On Fri, 2015-07-31 at 17:20 +0800, b29983@freescale.com wrote:
> From: Tang Yuantian <Yuantian.Tang@freescale.com>
> 
> Freescale E500MC and E5500 core-based platforms, like P4080, T1040,
> support disabling/enabling CPU dynamically.
> This patch adds this feature on those platforms.
> 
> Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com>
> Signed-off-by: Tang Yuantian <Yuantian.Tang@feescale.com>
> ---
>  arch/powerpc/Kconfig              |  2 +-
>  arch/powerpc/include/asm/smp.h    |  1 +
>  arch/powerpc/kernel/smp.c         |  5 +++++
>  arch/powerpc/platforms/85xx/smp.c | 39 ++++++++++++++++++++++++++++++++----
> ---
>  4 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 5ef2711..dd9e252 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -386,7 +386,7 @@ config SWIOTLB
>  config HOTPLUG_CPU
>       bool "Support for enabling/disabling CPUs"
>       depends on SMP && (PPC_PSERIES || \
> -     PPC_PMAC || PPC_POWERNV || (PPC_85xx && !PPC_E500MC))
> +     PPC_PMAC || PPC_POWERNV || FSL_SOC_BOOKE)
>       ---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/smp.h b/arch/powerpc/include/asm/smp.h
> index 825663c..bf37d17 100644
> --- a/arch/powerpc/include/asm/smp.h
> +++ b/arch/powerpc/include/asm/smp.h
> @@ -67,6 +67,7 @@ void generic_cpu_die(unsigned int cpu);
>  void generic_set_cpu_dead(unsigned int cpu);
>  void generic_set_cpu_up(unsigned int cpu);
>  int generic_check_cpu_restart(unsigned int cpu);
> +int generic_check_cpu_dead(unsigned int cpu);
>  #endif
>  
>  #ifdef CONFIG_PPC64
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index ec9ec20..2cca27a 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -454,6 +454,11 @@ int generic_check_cpu_restart(unsigned int cpu)
>       return per_cpu(cpu_state, cpu) == CPU_UP_PREPARE;
>  }
>  
> +int generic_check_cpu_dead(unsigned int cpu)
> +{
> +     return per_cpu(cpu_state, cpu) == CPU_DEAD;
> +}

Is there a non-generic check_cpu_dead()?

It gets open-coded in generic_cpu_die()... Either open-code it elsewhere, or 
call it check_cpu_dead() and use it everywhere there's a CPU_DEAD check.


> +
>  static bool secondaries_inhibited(void)
>  {
>       return kvm_hv_mode_active();
> diff --git a/arch/powerpc/platforms/85xx/smp.c 
> b/arch/powerpc/platforms/85xx/smp.c
> index 6811a5b..7f0dadb 100644
> --- a/arch/powerpc/platforms/85xx/smp.c
> +++ b/arch/powerpc/platforms/85xx/smp.c
> @@ -42,6 +42,7 @@ struct epapr_spin_table {
>       u32     pir;
>  };
>  
> +#ifdef CONFIG_HOTPLUG_CPU
>  static u64 timebase;
>  static int tb_req;
>  static int tb_valid;
> @@ -111,7 +112,7 @@ static void mpc85xx_take_timebase(void)
>       local_irq_restore(flags);
>  }
>  
> -#ifdef CONFIG_HOTPLUG_CPU
> +#ifndef CONFIG_PPC_E500MC
>  static void e500_cpu_idle(void)

What happens if we bisect to patch 1/3 and run this on e500mc?

Please move the ifdef to that patch.

>  {
>       u32 tmp;
> @@ -127,6 +128,7 @@ static void e500_cpu_idle(void)
>       mtmsr(tmp);
>       isync();
>  }
> +#endif
>  
>  static void qoriq_cpu_dying(void)
>  {
> @@ -144,11 +146,30 @@ static void qoriq_cpu_dying(void)
>  
>       generic_set_cpu_dead(cpu);
>  
> +#ifndef CONFIG_PPC_E500MC
>       e500_cpu_idle();
> +#endif
>  
>       while (1)
>               ;
>  }
> +
> +static void qoriq_real_cpu_die(unsigned int cpu)

Real as opposed to...?

> +{
> +     int i;
> +
> +     for (i = 0; i < 50000; i++) {
> +             if (generic_check_cpu_dead(cpu)) {
> +                     qoriq_pm_ops->cpu_die(cpu);
> +#ifdef CONFIG_PPC64
> +                     paca[cpu].cpu_start = 0;
> +#endif
> +                     return;
> +             }
> +             udelay(10);
> +     }
> +     pr_err("%s: CPU%d didn't die...\n", __func__, cpu);
> +}

Only 500ms timeout, versus 10sec in generic_cpu_die()?

>  #endif
>  
>  static inline void flush_spin_table(void *spin_table)
> @@ -246,11 +267,7 @@ static int smp_85xx_kick_cpu(int nr)
>               spin_table = phys_to_virt(*cpu_rel_addr);
>  
>       local_irq_save(flags);
> -#ifdef CONFIG_PPC32
>  #ifdef CONFIG_HOTPLUG_CPU
> -     /* Corresponding to generic_set_cpu_dead() */
> -     generic_set_cpu_up(nr);
> -
>       if (system_state == SYSTEM_RUNNING) {
>               /*
>                * To keep it compatible with old boot program which uses
> @@ -263,6 +280,7 @@ static int smp_85xx_kick_cpu(int nr)
>               out_be32(&spin_table->addr_l, 0);
>               flush_spin_table(spin_table);
>  
> +             qoriq_pm_ops->cpu_up(nr);

Again, is it possible to get here without a valid qoriq_pm_ops (i.e. is there 
anything stopping the user from trying to initiate CPU hotplug)?

-Scott

Re: [PATCH 2/3] PowerPC/mpc85xx: Add hotplug support on E5500 and E500MC cores

[Chenhui Zhao]

On Sat, Aug 1, 2015 at 8:14 AM, Scott Wood <scottwood@freescale.com> 
wrote:
> On Fri, 2015-07-31 at 17:20 +0800, b29983@freescale.com wrote:
>>  From: Tang Yuantian <Yuantian.Tang@freescale.com>
>> 
>>  Freescale E500MC and E5500 core-based platforms, like P4080, T1040,
>>  support disabling/enabling CPU dynamically.
>>  This patch adds this feature on those platforms.
>> 
>>  Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com>
>>  Signed-off-by: Tang Yuantian <Yuantian.Tang@feescale.com>
>>  ---
>>   arch/powerpc/Kconfig              |  2 +-
>>   arch/powerpc/include/asm/smp.h    |  1 +
>>   arch/powerpc/kernel/smp.c         |  5 +++++
>>   arch/powerpc/platforms/85xx/smp.c | 39 
>> ++++++++++++++++++++++++++++++++----
>>  ---
>>   4 files changed, 39 insertions(+), 8 deletions(-)
>> 
>>  diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>  index 5ef2711..dd9e252 100644
>>  --- a/arch/powerpc/Kconfig
>>  +++ b/arch/powerpc/Kconfig
>>  @@ -386,7 +386,7 @@ config SWIOTLB
>>   config HOTPLUG_CPU
>>        bool "Support for enabling/disabling CPUs"
>>        depends on SMP && (PPC_PSERIES || \
>>  -     PPC_PMAC || PPC_POWERNV || (PPC_85xx && !PPC_E500MC))
>>  +     PPC_PMAC || PPC_POWERNV || FSL_SOC_BOOKE)
>>        ---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/smp.h 
>> b/arch/powerpc/include/asm/smp.h
>>  index 825663c..bf37d17 100644
>>  --- a/arch/powerpc/include/asm/smp.h
>>  +++ b/arch/powerpc/include/asm/smp.h
>>  @@ -67,6 +67,7 @@ void generic_cpu_die(unsigned int cpu);
>>   void generic_set_cpu_dead(unsigned int cpu);
>>   void generic_set_cpu_up(unsigned int cpu);
>>   int generic_check_cpu_restart(unsigned int cpu);
>>  +int generic_check_cpu_dead(unsigned int cpu);
>>   #endif
>> 
>>   #ifdef CONFIG_PPC64
>>  diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
>>  index ec9ec20..2cca27a 100644
>>  --- a/arch/powerpc/kernel/smp.c
>>  +++ b/arch/powerpc/kernel/smp.c
>>  @@ -454,6 +454,11 @@ int generic_check_cpu_restart(unsigned int cpu)
>>        return per_cpu(cpu_state, cpu) == CPU_UP_PREPARE;
>>   }
>> 
>>  +int generic_check_cpu_dead(unsigned int cpu)
>>  +{
>>  +     return per_cpu(cpu_state, cpu) == CPU_DEAD;
>>  +}
> 
> Is there a non-generic check_cpu_dead()?

NO, just follow the name "generic_check_cpu_restart()".

> 
> It gets open-coded in generic_cpu_die()... Either open-code it 
> elsewhere, or
> call it check_cpu_dead() and use it everywhere there's a CPU_DEAD 
> check.
> 
> 
>>  +
>>   static bool secondaries_inhibited(void)
>>   {
>>        return kvm_hv_mode_active();
>>  diff --git a/arch/powerpc/platforms/85xx/smp.c
>>  b/arch/powerpc/platforms/85xx/smp.c
>>  index 6811a5b..7f0dadb 100644
>>  --- a/arch/powerpc/platforms/85xx/smp.c
>>  +++ b/arch/powerpc/platforms/85xx/smp.c
>>  @@ -42,6 +42,7 @@ struct epapr_spin_table {
>>        u32     pir;
>>   };
>> 
>>  +#ifdef CONFIG_HOTPLUG_CPU
>>   static u64 timebase;
>>   static int tb_req;
>>   static int tb_valid;
>>  @@ -111,7 +112,7 @@ static void mpc85xx_take_timebase(void)
>>        local_irq_restore(flags);
>>   }
>> 
>>  -#ifdef CONFIG_HOTPLUG_CPU
>>  +#ifndef CONFIG_PPC_E500MC
>>   static void e500_cpu_idle(void)
> 
> What happens if we bisect to patch 1/3 and run this on e500mc?
> 
> Please move the ifdef to that patch.

OK.

> 
> 
>>   {
>>        u32 tmp;
>>  @@ -127,6 +128,7 @@ static void e500_cpu_idle(void)
>>        mtmsr(tmp);
>>        isync();
>>   }
>>  +#endif
>> 
>>   static void qoriq_cpu_dying(void)
>>   {
>>  @@ -144,11 +146,30 @@ static void qoriq_cpu_dying(void)
>> 
>>        generic_set_cpu_dead(cpu);
>> 
>>  +#ifndef CONFIG_PPC_E500MC
>>        e500_cpu_idle();
>>  +#endif
>> 
>>        while (1)
>>                ;
>>   }
>>  +
>>  +static void qoriq_real_cpu_die(unsigned int cpu)
> 
> Real as opposed to...?

It's hard to find a good name. :)

> 
> 
>>  +{
>>  +     int i;
>>  +
>>  +     for (i = 0; i < 50000; i++) {
>>  +             if (generic_check_cpu_dead(cpu)) {
>>  +                     qoriq_pm_ops->cpu_die(cpu);
>>  +#ifdef CONFIG_PPC64
>>  +                     paca[cpu].cpu_start = 0;
>>  +#endif
>>  +                     return;
>>  +             }
>>  +             udelay(10);
>>  +     }
>>  +     pr_err("%s: CPU%d didn't die...\n", __func__, cpu);
>>  +}
> 
> Only 500ms timeout, versus 10sec in generic_cpu_die()?

The process is fast. Maybe 10 second is too large.

> 
>>   #endif
>> 
>>   static inline void flush_spin_table(void *spin_table)
>>  @@ -246,11 +267,7 @@ static int smp_85xx_kick_cpu(int nr)
>>                spin_table = phys_to_virt(*cpu_rel_addr);
>> 
>>        local_irq_save(flags);
>>  -#ifdef CONFIG_PPC32
>>   #ifdef CONFIG_HOTPLUG_CPU
>>  -     /* Corresponding to generic_set_cpu_dead() */
>>  -     generic_set_cpu_up(nr);
>>  -
>>        if (system_state == SYSTEM_RUNNING) {
>>                /*
>>                 * To keep it compatible with old boot program which 
>> uses
>>  @@ -263,6 +280,7 @@ static int smp_85xx_kick_cpu(int nr)
>>                out_be32(&spin_table->addr_l, 0);
>>                flush_spin_table(spin_table);
>> 
>>  +             qoriq_pm_ops->cpu_up(nr);
> 
> Again, is it possible to get here without a valid qoriq_pm_ops (i.e. 
> is there
> anything stopping the user from trying to initiate CPU hotplug)?
> 
> -Scott

For every platform running this code, should has a valid qoriq_pm_ops. 
If not valid, it's a bug.

-Chenhui

Re: [PATCH 2/3] PowerPC/mpc85xx: Add hotplug support on E5500 and E500MC cores

[Scott Wood]

[Added linuxppc-dev@lists.ozlabs.org.  Besides that list being required for 
review of PPC patches, it feeds the patchwork that I use to track and apply 
patches.]

On Mon, 2015-08-03 at 19:52 +0800, Chenhui Zhao wrote:
> On Sat, Aug 1, 2015 at 8:14 AM, Scott Wood <scottwood@freescale.com> 
> wrote:
> > On Fri, 2015-07-31 at 17:20 +0800,  b29983@freescale.comwrote:
> > >  From: Tang Yuantian <Yuantian.Tang@freescale.com>
> > > 
> > >  Freescale E500MC and E5500 core-based platforms, like P4080, T1040,
> > >  support disabling/enabling CPU dynamically.
> > >  This patch adds this feature on those platforms.
> > > 
> > >  Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com>
> > >  Signed-off-by: Tang Yuantian <Yuantian.Tang@feescale.com>
> > >  ---
> > >   arch/powerpc/Kconfig              |  2 +-
> > >   arch/powerpc/include/asm/smp.h    |  1 +
> > >   arch/powerpc/kernel/smp.c         |  5 +++++
> > >   arch/powerpc/platforms/85xx/smp.c | 39 
> > > ++++++++++++++++++++++++++++++++----
> > >  ---
> > >   4 files changed, 39 insertions(+), 8 deletions(-)
> > > 
> > >  diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > >  index 5ef2711..dd9e252 100644
> > >  --- a/arch/powerpc/Kconfig
> > >  +++ b/arch/powerpc/Kconfig
> > >  @@ -386,7 +386,7 @@ config SWIOTLB
> > >   config HOTPLUG_CPU
> > >        bool "Support for enabling/disabling CPUs"
> > >        depends on SMP && (PPC_PSERIES || \
> > >  -     PPC_PMAC || PPC_POWERNV || (PPC_85xx && !PPC_E500MC))
> > >  +     PPC_PMAC || PPC_POWERNV || FSL_SOC_BOOKE)
> > >        ---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/smp.h 
> > > b/arch/powerpc/include/asm/smp.h
> > >  index 825663c..bf37d17 100644
> > >  --- a/arch/powerpc/include/asm/smp.h
> > >  +++ b/arch/powerpc/include/asm/smp.h
> > >  @@ -67,6 +67,7 @@ void generic_cpu_die(unsigned int cpu);
> > >   void generic_set_cpu_dead(unsigned int cpu);
> > >   void generic_set_cpu_up(unsigned int cpu);
> > >   int generic_check_cpu_restart(unsigned int cpu);
> > >  +int generic_check_cpu_dead(unsigned int cpu);
> > >   #endif
> > > 
> > >   #ifdef CONFIG_PPC64
> > >  diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > >  index ec9ec20..2cca27a 100644
> > >  --- a/arch/powerpc/kernel/smp.c
> > >  +++ b/arch/powerpc/kernel/smp.c
> > >  @@ -454,6 +454,11 @@ int generic_check_cpu_restart(unsigned int cpu)
> > >        return per_cpu(cpu_state, cpu) == CPU_UP_PREPARE;
> > >   }
> > > 
> > >  +int generic_check_cpu_dead(unsigned int cpu)
> > >  +{
> > >  +     return per_cpu(cpu_state, cpu) == CPU_DEAD;
> > >  +}
> > 
> > Is there a non-generic check_cpu_dead()?
> 
> NO, just follow the name "generic_check_cpu_restart()".

But it's not the same situation as generic_check_cpu_restart().

> > It gets open-coded in generic_cpu_die()... Either open-code it 
> > elsewhere, or
> > call it check_cpu_dead() and use it everywhere there's a CPU_DEAD 
> > check.
> > 
> > 
> > >  +
> > >   static bool secondaries_inhibited(void)
> > >   {
> > >        return kvm_hv_mode_active();
> > >  diff --git a/arch/powerpc/platforms/85xx/smp.c
> > >  b/arch/powerpc/platforms/85xx/smp.c
> > >  index 6811a5b..7f0dadb 100644
> > >  --- a/arch/powerpc/platforms/85xx/smp.c
> > >  +++ b/arch/powerpc/platforms/85xx/smp.c
> > >  @@ -42,6 +42,7 @@ struct epapr_spin_table {
> > >        u32     pir;
> > >   };
> > > 
> > >  +#ifdef CONFIG_HOTPLUG_CPU
> > >   static u64 timebase;
> > >   static int tb_req;
> > >   static int tb_valid;
> > >  @@ -111,7 +112,7 @@ static void mpc85xx_take_timebase(void)
> > >        local_irq_restore(flags);
> > >   }
> > > 
> > >  -#ifdef CONFIG_HOTPLUG_CPU
> > >  +#ifndef CONFIG_PPC_E500MC
> > >   static void e500_cpu_idle(void)
> > 
> > What happens if we bisect to patch 1/3 and run this on e500mc?
> > 
> > Please move the ifdef to that patch.
> 
> OK.
> 
> > 
> > 
> > >   {
> > >        u32 tmp;
> > >  @@ -127,6 +128,7 @@ static void e500_cpu_idle(void)
> > >        mtmsr(tmp);
> > >        isync();
> > >   }
> > >  +#endif
> > > 
> > >   static void qoriq_cpu_dying(void)
> > >   {
> > >  @@ -144,11 +146,30 @@ static void qoriq_cpu_dying(void)
> > > 
> > >        generic_set_cpu_dead(cpu);
> > > 
> > >  +#ifndef CONFIG_PPC_E500MC
> > >        e500_cpu_idle();
> > >  +#endif
> > > 
> > >        while (1)
> > >                ;
> > >   }
> > >  +
> > >  +static void qoriq_real_cpu_die(unsigned int cpu)
> > 
> > Real as opposed to...?
> 
> It's hard to find a good name. :)

There are too many cpu_die() functions as is, and adding cpu_dying makes it 
worse.  Even just trying to come up with suggestions I've been having a hard 
time keeping track of which one goes in which ops struct.  This problem goes 
beyond the 85xx code, to the ridiculous and undocumented distinction between 
cpu_die() and __cpu_die().

It wouldn't be so bad if each layer were self contained, rather than multiple 
layers being defined in the same file.  I suggest keeping the existing 
convention whereby ppc_md.cpu_die ends in "_mach_cpu_die".  Don't call 
anything "cpu_dying".

I'd call qoriq_pm_ops->cpu_die something else (e.g. cpu_kill) even though it 
is in a separate file, just because of how confused and overused the name is 
elsewhere.


> +{
> > >  +     int i;
> > >  +
> > >  +     for (i = 0; i < 50000; i++) {
> > >  +             if (generic_check_cpu_dead(cpu)) {
> > >  +                     qoriq_pm_ops->cpu_die(cpu);
> > >  +#ifdef CONFIG_PPC64
> > >  +                     paca[cpu].cpu_start = 0;
> > >  +#endif
> > >  +                     return;
> > >  +             }
> > >  +             udelay(10);
> > >  +     }
> > >  +     pr_err("%s: CPU%d didn't die...\n", __func__, cpu);
> > >  +}
> > 
> > Only 500ms timeout, versus 10sec in generic_cpu_die()?
> 
> The process is fast. Maybe 10 second is too large.

Is it fast 100% of the time?  What if the CPU you intend to die is in a long 
critical section?  What harm is there to having a longer timeout, similar to 
what other platforms use?

> 
> > 
> > >   #endif
> > > 
> > >   static inline void flush_spin_table(void *spin_table)
> > >  @@ -246,11 +267,7 @@ static int smp_85xx_kick_cpu(int nr)
> > >                spin_table = phys_to_virt(*cpu_rel_addr);
> > > 
> > >        local_irq_save(flags);
> > >  -#ifdef CONFIG_PPC32
> > >   #ifdef CONFIG_HOTPLUG_CPU
> > >  -     /* Corresponding to generic_set_cpu_dead() */
> > >  -     generic_set_cpu_up(nr);
> > >  -
> > >        if (system_state == SYSTEM_RUNNING) {
> > >                /*
> > >                 * To keep it compatible with old boot program which 
> > > uses
> > >  @@ -263,6 +280,7 @@ static int smp_85xx_kick_cpu(int nr)
> > >                out_be32(&spin_table->addr_l, 0);
> > >                flush_spin_table(spin_table);
> > > 
> > >  +             qoriq_pm_ops->cpu_up(nr);
> > 
> > Again, is it possible to get here without a valid qoriq_pm_ops (i.e. 
> > is there
> > anything stopping the user from trying to initiate CPU hotplug)?
> > 
> > -Scott
> 
> For every platform running this code, should has a valid qoriq_pm_ops. 
> If not valid, it's a bug.

How do you prevent this code from running when there is no valid qoriq_pm_ops?

-Scott

Re: [PATCH 2/3] PowerPC/mpc85xx: Add hotplug support on E5500 and E500MC cores

[Chenhui Zhao]

On Tue, Aug 4, 2015 at 5:18 AM, Scott Wood <scottwood@freescale.com> 
wrote:
> [Added linuxppc-dev@lists.ozlabs.org.  Besides that list being 
> required for
> review of PPC patches, it feeds the patchwork that I use to track and 
> apply
> patches.]
> 
> On Mon, 2015-08-03 at 19:52 +0800, Chenhui Zhao wrote:
>>  On Sat, Aug 1, 2015 at 8:14 AM, Scott Wood <scottwood@freescale.com>
>>  wrote:
>>  > On Fri, 2015-07-31 at 17:20 +0800,  b29983@freescale.comwrote:
>>  > >  From: Tang Yuantian <Yuantian.Tang@freescale.com>
>>  > >
>>  > >  Freescale E500MC and E5500 core-based platforms, like P4080, 
>> T1040,
>>  > >  support disabling/enabling CPU dynamically.
>>  > >  This patch adds this feature on those platforms.
>>  > >
>>  > >  Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com>
>>  > >  Signed-off-by: Tang Yuantian <Yuantian.Tang@feescale.com>


>>  +{
>>  > >  +     int i;
>>  > >  +
>>  > >  +     for (i = 0; i < 50000; i++) {
>>  > >  +             if (generic_check_cpu_dead(cpu)) {
>>  > >  +                     qoriq_pm_ops->cpu_die(cpu);
>>  > >  +#ifdef CONFIG_PPC64
>>  > >  +                     paca[cpu].cpu_start = 0;
>>  > >  +#endif
>>  > >  +                     return;
>>  > >  +             }
>>  > >  +             udelay(10);
>>  > >  +     }
>>  > >  +     pr_err("%s: CPU%d didn't die...\n", __func__, cpu);
>>  > >  +}
>>  >
>>  > Only 500ms timeout, versus 10sec in generic_cpu_die()?
>> 
>>  The process is fast. Maybe 10 second is too large.
> 
> Is it fast 100% of the time?  What if the CPU you intend to die is in 
> a long
> critical section?  What harm is there to having a longer timeout, 
> similar to
> what other platforms use?

Will change the max timeout to 10 seconds.

> 
>> 
>>  >
>>  > >   #endif
>>  > >
>>  > >   static inline void flush_spin_table(void *spin_table)
>>  > >  @@ -246,11 +267,7 @@ static int smp_85xx_kick_cpu(int nr)
>>  > >                spin_table = phys_to_virt(*cpu_rel_addr);
>>  > >
>>  > >        local_irq_save(flags);
>>  > >  -#ifdef CONFIG_PPC32
>>  > >   #ifdef CONFIG_HOTPLUG_CPU
>>  > >  -     /* Corresponding to generic_set_cpu_dead() */
>>  > >  -     generic_set_cpu_up(nr);
>>  > >  -
>>  > >        if (system_state == SYSTEM_RUNNING) {
>>  > >                /*
>>  > >                 * To keep it compatible with old boot program 
>> which
>>  > > uses
>>  > >  @@ -263,6 +280,7 @@ static int smp_85xx_kick_cpu(int nr)
>>  > >                out_be32(&spin_table->addr_l, 0);
>>  > >                flush_spin_table(spin_table);
>>  > >
>>  > >  +             qoriq_pm_ops->cpu_up(nr);
>>  >
>>  > Again, is it possible to get here without a valid qoriq_pm_ops 
>> (i.e.
>>  > is there
>>  > anything stopping the user from trying to initiate CPU hotplug)?
>>  >
>>  > -Scott
>> 
>>  For every platform running this code, should has a valid 
>> qoriq_pm_ops.
>>  If not valid, it's a bug.
> 
> How do you prevent this code from running when there is no valid 
> qoriq_pm_ops?
> 
> -Scott
> 

Will check if qoriq_pm_ops is valid.

-Chenhui

[PATCH 3/3] PowerPC/mpc85xx: Add hotplug support on E6500 cores

[b29983]

From: Tang Yuantian <Yuantian.Tang@freescale.com>

Freescale E6500 core-based platforms, like t4240, support
disabling/enabling CPU dynamically.
This patch adds this feature on those platforms.

Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com>
Signed-off-by: Tang Yuantian <Yuantian.Tang@feescale.com>
---
 arch/powerpc/include/asm/smp.h    |  1 +
 arch/powerpc/kernel/head_64.S     | 10 +++++--
 arch/powerpc/platforms/85xx/smp.c | 63 ++++++++++++++++++++++++++-------------
 3 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index bf37d17..c7bd27d 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -198,6 +198,7 @@ extern void generic_secondary_thread_init(void);
 extern unsigned long __secondary_hold_spinloop;
 extern unsigned long __secondary_hold_acknowledge;
 extern char __secondary_hold;
+extern unsigned int booting_cpu_hwid;
 
 extern void __early_start(void);
 #endif /* __ASSEMBLY__ */
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index d48125d..c47544d 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -181,6 +181,10 @@ exception_marker:
 #endif
 
 #ifdef CONFIG_PPC_BOOK3E
+	.globl	booting_cpu_hwid
+booting_cpu_hwid:
+	.long  0x0
+	.align 3
 _GLOBAL(fsl_secondary_thread_init)
 	/* Enable branch prediction */
 	lis     r3,BUCSR_INIT@h
@@ -197,8 +201,10 @@ _GLOBAL(fsl_secondary_thread_init)
 	 * but the low bit right by two bits so that the cpu numbering is
 	 * continuous.
 	 */
-	mfspr	r3, SPRN_PIR
-	rlwimi	r3, r3, 30, 2, 30
+	bl	10f
+10:	mflr	r22
+	addi	r22,r22,(booting_cpu_hwid - 10b)
+	lwz	r3,0(r22)
 	mtspr	SPRN_PIR, r3
 #endif
 
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index 7f0dadb..8652a49 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -189,15 +189,22 @@ static inline u32 read_spin_table_addr_l(void *spin_table)
 static void wake_hw_thread(void *info)
 {
 	void fsl_secondary_thread_init(void);
-	unsigned long imsr1, inia1;
+	unsigned long imsr, inia;
 	int nr = *(const int *)info;
-
-	imsr1 = MSR_KERNEL;
-	inia1 = *(unsigned long *)fsl_secondary_thread_init;
-
-	mttmr(TMRN_IMSR1, imsr1);
-	mttmr(TMRN_INIA1, inia1);
-	mtspr(SPRN_TENS, TEN_THREAD(1));
+	int hw_cpu = get_hard_smp_processor_id(nr);
+	int thread_idx = cpu_thread_in_core(hw_cpu);
+
+	booting_cpu_hwid = (u32)hw_cpu;
+	imsr = MSR_KERNEL;
+	inia = *(unsigned long *)fsl_secondary_thread_init;
+	if (thread_idx == 0) {
+		mttmr(TMRN_IMSR0, imsr);
+		mttmr(TMRN_INIA0, inia);
+	} else {
+		mttmr(TMRN_IMSR1, imsr);
+		mttmr(TMRN_INIA1, inia);
+	}
+	mtspr(SPRN_TENS, TEN_THREAD(thread_idx));
 
 	smp_generic_kick_cpu(nr);
 }
@@ -219,27 +226,43 @@ static int smp_85xx_kick_cpu(int nr)
 	pr_debug("smp_85xx_kick_cpu: kick CPU #%d\n", nr);
 
 #ifdef CONFIG_PPC64
-	/* Threads don't use the spin table */
-	if (cpu_thread_in_core(nr) != 0) {
+	if (threads_per_core > 1) {
 		int primary = cpu_first_thread_sibling(nr);
 
 		if (WARN_ON_ONCE(!cpu_has_feature(CPU_FTR_SMT)))
 			return -ENOENT;
 
-		if (cpu_thread_in_core(nr) != 1) {
-			pr_err("%s: cpu %d: invalid hw thread %d\n",
-			       __func__, nr, cpu_thread_in_core(nr));
-			return -ENOENT;
+		/*
+		 * If either one of threads in the same core is online,
+		 * use the online one to start the other.
+		 */
+		if (cpu_online(primary) || cpu_online(primary + 1)) {
+			qoriq_pm_ops->cpu_up(nr);
+			if (cpu_online(primary))
+				smp_call_function_single(primary,
+						wake_hw_thread, &nr, 1);
+			else
+				smp_call_function_single(primary + 1,
+						wake_hw_thread, &nr, 1);
+			return 0;
 		}
 
-		if (!cpu_online(primary)) {
-			pr_err("%s: cpu %d: primary %d not online\n",
-			       __func__, nr, primary);
-			return -ENOENT;
+		/*
+		 * If both threads are offline, reset core to start.
+		 * When core is up, Thread 0 always gets up first,
+		 * so bind the current logical cpu with Thread 0.
+		 */
+		if (hw_cpu != cpu_first_thread_sibling(hw_cpu)) {
+			int hw_cpu1, hw_cpu2;
+
+			hw_cpu1 = get_hard_smp_processor_id(primary);
+			hw_cpu2 = get_hard_smp_processor_id(primary + 1);
+			set_hard_smp_processor_id(primary, hw_cpu2);
+			set_hard_smp_processor_id(primary + 1, hw_cpu1);
+			/* get new physical cpu id */
+			hw_cpu = get_hard_smp_processor_id(nr);
 		}
 
-		smp_call_function_single(primary, wake_hw_thread, &nr, 0);
-		return 0;
 	}
 #endif
 
-- 
2.1.0.27.g96db324

Re: [PATCH 3/3] PowerPC/mpc85xx: Add hotplug support on E6500 cores

[Scott Wood]

On Fri, 2015-07-31 at 17:20 +0800, b29983@freescale.com wrote:
> diff --git a/arch/powerpc/platforms/85xx/smp.c 
> b/arch/powerpc/platforms/85xx/smp.c
> index 7f0dadb..8652a49 100644
> --- a/arch/powerpc/platforms/85xx/smp.c
> +++ b/arch/powerpc/platforms/85xx/smp.c
> @@ -189,15 +189,22 @@ static inline u32 read_spin_table_addr_l(void 
> *spin_table)
>  static void wake_hw_thread(void *info)
>  {
>       void fsl_secondary_thread_init(void);
> -     unsigned long imsr1, inia1;
> +     unsigned long imsr, inia;
>       int nr = *(const int *)info;
> -
> -     imsr1 = MSR_KERNEL;
> -     inia1 = *(unsigned long *)fsl_secondary_thread_init;
> -
> -     mttmr(TMRN_IMSR1, imsr1);
> -     mttmr(TMRN_INIA1, inia1);
> -     mtspr(SPRN_TENS, TEN_THREAD(1));
> +     int hw_cpu = get_hard_smp_processor_id(nr);
> +     int thread_idx = cpu_thread_in_core(hw_cpu);
> +
> +     booting_cpu_hwid = (u32)hw_cpu;

Unnecessary cast.  Please explain why you need booting_cpu_hwid.

> +     imsr = MSR_KERNEL;
> +     inia = *(unsigned long *)fsl_secondary_thread_init;
> +     if (thread_idx == 0) {
> +             mttmr(TMRN_IMSR0, imsr);
> +             mttmr(TMRN_INIA0, inia);
> +     } else {
> +             mttmr(TMRN_IMSR1, imsr);
> +             mttmr(TMRN_INIA1, inia);
> +     }
> +     mtspr(SPRN_TENS, TEN_THREAD(thread_idx));

Please rebase this on top of http://patchwork.ozlabs.org/patch/496952/

> +             /*
> +              * If both threads are offline, reset core to start.
> +              * When core is up, Thread 0 always gets up first,
> +              * so bind the current logical cpu with Thread 0.
> +              */
> +             if (hw_cpu != cpu_first_thread_sibling(hw_cpu)) {
> +                     int hw_cpu1, hw_cpu2;
> +
> +                     hw_cpu1 = get_hard_smp_processor_id(primary);
> +                     hw_cpu2 = get_hard_smp_processor_id(primary + 1);
> +                     set_hard_smp_processor_id(primary, hw_cpu2);
> +                     set_hard_smp_processor_id(primary + 1, hw_cpu1);
> +                     /* get new physical cpu id */
> +                     hw_cpu = get_hard_smp_processor_id(nr);

NACK as discussed in http://patchwork.ozlabs.org/patch/454944/

-Scott

Re: [PATCH 3/3] PowerPC/mpc85xx: Add hotplug support on E6500 cores

[Chenhui Zhao]

On Sat, Aug 1, 2015 at 8:22 AM, Scott Wood <scottwood@freescale.com> 
wrote:
> On Fri, 2015-07-31 at 17:20 +0800, b29983@freescale.com wrote:
>>  diff --git a/arch/powerpc/platforms/85xx/smp.c
>>  b/arch/powerpc/platforms/85xx/smp.c
>>  index 7f0dadb..8652a49 100644
>>  --- a/arch/powerpc/platforms/85xx/smp.c
>>  +++ b/arch/powerpc/platforms/85xx/smp.c
>>  @@ -189,15 +189,22 @@ static inline u32 read_spin_table_addr_l(void
>>  *spin_table)
>>   static void wake_hw_thread(void *info)
>>   {
>>        void fsl_secondary_thread_init(void);
>>  -     unsigned long imsr1, inia1;
>>  +     unsigned long imsr, inia;
>>        int nr = *(const int *)info;
>>  -
>>  -     imsr1 = MSR_KERNEL;
>>  -     inia1 = *(unsigned long *)fsl_secondary_thread_init;
>>  -
>>  -     mttmr(TMRN_IMSR1, imsr1);
>>  -     mttmr(TMRN_INIA1, inia1);
>>  -     mtspr(SPRN_TENS, TEN_THREAD(1));
>>  +     int hw_cpu = get_hard_smp_processor_id(nr);
>>  +     int thread_idx = cpu_thread_in_core(hw_cpu);
>>  +
>>  +     booting_cpu_hwid = (u32)hw_cpu;
> 
> Unnecessary cast.  Please explain why you need booting_cpu_hwid.
> 
> 
>>  +     imsr = MSR_KERNEL;
>>  +     inia = *(unsigned long *)fsl_secondary_thread_init;
>>  +     if (thread_idx == 0) {
>>  +             mttmr(TMRN_IMSR0, imsr);
>>  +             mttmr(TMRN_INIA0, inia);
>>  +     } else {
>>  +             mttmr(TMRN_IMSR1, imsr);
>>  +             mttmr(TMRN_INIA1, inia);
>>  +     }
>>  +     mtspr(SPRN_TENS, TEN_THREAD(thread_idx));
> 
> Please rebase this on top of http://patchwork.ozlabs.org/patch/496952/

OK.

> 
> 
>>  +             /*
>>  +              * If both threads are offline, reset core to start.
>>  +              * When core is up, Thread 0 always gets up first,
>>  +              * so bind the current logical cpu with Thread 0.
>>  +              */
>>  +             if (hw_cpu != cpu_first_thread_sibling(hw_cpu)) {
>>  +                     int hw_cpu1, hw_cpu2;
>>  +
>>  +                     hw_cpu1 = get_hard_smp_processor_id(primary);
>>  +                     hw_cpu2 = get_hard_smp_processor_id(primary + 
>> 1);
>>  +                     set_hard_smp_processor_id(primary, hw_cpu2);
>>  +                     set_hard_smp_processor_id(primary + 1, 
>> hw_cpu1);
>>  +                     /* get new physical cpu id */
>>  +                     hw_cpu = get_hard_smp_processor_id(nr);
> 
> NACK as discussed in http://patchwork.ozlabs.org/patch/454944/
> 
> -Scott

You said,

    There's no need for this. I have booting from a thread1, and having 
it
    kick its thread0, working locally without messing with the hwid/cpu
    mapping.

I still have questions here. After a core reset, how can you boot 
Thread1
of the core first. As I know, Thread0 boots up first by default.

-Chenhui

Re: [PATCH 3/3] PowerPC/mpc85xx: Add hotplug support on E6500 cores

[Scott Wood]

On Wed, 2015-08-05 at 19:08 +0800, Chenhui Zhao wrote:
> On Sat, Aug 1, 2015 at 8:22 AM, Scott Wood <scottwood@freescale.com> 
> wrote:
> > On Fri, 2015-07-31 at 17:20 +0800,  b29983@freescale.comwrote:
> > >  +             /*
> > >  +              * If both threads are offline, reset core to start.
> > >  +              * When core is up, Thread 0 always gets up first,
> > >  +              * so bind the current logical cpu with Thread 0.
> > >  +              */
> > >  +             if (hw_cpu != cpu_first_thread_sibling(hw_cpu)) {
> > >  +                     int hw_cpu1, hw_cpu2;
> > >  +
> > >  +                     hw_cpu1 = get_hard_smp_processor_id(primary);
> > >  +                     hw_cpu2 = get_hard_smp_processor_id(primary + 
> > > 1);
> > >  +                     set_hard_smp_processor_id(primary, hw_cpu2);
> > >  +                     set_hard_smp_processor_id(primary + 1, 
> > > hw_cpu1);
> > >  +                     /* get new physical cpu id */
> > >  +                     hw_cpu = get_hard_smp_processor_id(nr);
> > 
> > NACK as discussed in http://patchwork.ozlabs.org/patch/454944/
> > 
> > -Scott
> 
> You said,
> 
>     There's no need for this. I have booting from a thread1, and having 
> it
>     kick its thread0, working locally without messing with the hwid/cpu
>     mapping.
> 
> I still have questions here. After a core reset, how can you boot 
> Thread1
> of the core first. As I know, Thread0 boots up first by default.

So the issue isn't that thread1 comes up first, but that you *want* thread1 
to come up first and it won't.  I don't think this remapping is an acceptable 
answer, though.  Instead, if you need only thread1 to come up, start the 
core, have thread0 start thread1, and then send thread0 into whatever waiting 
state it would be in if thread1 had never been offlined.

-Scott

Re: [PATCH 3/3] PowerPC/mpc85xx: Add hotplug support on E6500 cores

[Chenhui Zhao]

On Thu, Aug 6, 2015 at 11:16 AM, Scott Wood <scottwood@freescale.com> 
wrote:
> On Wed, 2015-08-05 at 19:08 +0800, Chenhui Zhao wrote:
>>  On Sat, Aug 1, 2015 at 8:22 AM, Scott Wood <scottwood@freescale.com>
>>  wrote:
>>  > On Fri, 2015-07-31 at 17:20 +0800,  b29983@freescale.comwrote:
>>  > >  +             /*
>>  > >  +              * If both threads are offline, reset core to 
>> start.
>>  > >  +              * When core is up, Thread 0 always gets up 
>> first,
>>  > >  +              * so bind the current logical cpu with Thread 0.
>>  > >  +              */
>>  > >  +             if (hw_cpu != cpu_first_thread_sibling(hw_cpu)) {
>>  > >  +                     int hw_cpu1, hw_cpu2;
>>  > >  +
>>  > >  +                     hw_cpu1 = 
>> get_hard_smp_processor_id(primary);
>>  > >  +                     hw_cpu2 = 
>> get_hard_smp_processor_id(primary +
>>  > > 1);
>>  > >  +                     set_hard_smp_processor_id(primary, 
>> hw_cpu2);
>>  > >  +                     set_hard_smp_processor_id(primary + 1,
>>  > > hw_cpu1);
>>  > >  +                     /* get new physical cpu id */
>>  > >  +                     hw_cpu = get_hard_smp_processor_id(nr);
>>  >
>>  > NACK as discussed in http://patchwork.ozlabs.org/patch/454944/
>>  >
>>  > -Scott
>> 
>>  You said,
>> 
>>      There's no need for this. I have booting from a thread1, and 
>> having
>>  it
>>      kick its thread0, working locally without messing with the 
>> hwid/cpu
>>      mapping.
>> 
>>  I still have questions here. After a core reset, how can you boot
>>  Thread1
>>  of the core first. As I know, Thread0 boots up first by default.
> 
> So the issue isn't that thread1 comes up first, but that you *want* 
> thread1
> to come up first and it won't.  I don't think this remapping is an 
> acceptable
> answer, though.  Instead, if you need only thread1 to come up, start 
> the
> core, have thread0 start thread1, and then send thread0 into whatever 
> waiting
> state it would be in if thread1 had never been offlined.
> 
> -Scott

Remapping is a concise solution. what's the harm of it?
Keeping things simple is good in my opinion.

-Chenhui

Re: [PATCH 3/3] PowerPC/mpc85xx: Add hotplug support on E6500 cores

[Scott Wood]

On Thu, 2015-08-06 at 12:32 +0800, Chenhui Zhao wrote:
> On Thu, Aug 6, 2015 at 11:16 AM, Scott Wood <scottwood@freescale.com> 
> wrote:
> > On Wed, 2015-08-05 at 19:08 +0800, Chenhui Zhao wrote:
> > >  On Sat, Aug 1, 2015 at 8:22 AM, Scott Wood <scottwood@freescale.com>
> > >  wrote:
> > >  > On Fri, 2015-07-31 at 17:20 +0800,  b29983@freescale.comwrote:
> > >  > >  +             /*
> > >  > >  +              * If both threads are offline, reset core to 
> > > start.
> > >  > >  +              * When core is up, Thread 0 always gets up 
> > > first,
> > >  > >  +              * so bind the current logical cpu with Thread 0.
> > >  > >  +              */
> > >  > >  +             if (hw_cpu != cpu_first_thread_sibling(hw_cpu)) {
> > >  > >  +                     int hw_cpu1, hw_cpu2;
> > >  > >  +
> > >  > >  +                     hw_cpu1 = 
> > > get_hard_smp_processor_id(primary);
> > >  > >  +                     hw_cpu2 = 
> > > get_hard_smp_processor_id(primary +
> > >  > > 1);
> > >  > >  +                     set_hard_smp_processor_id(primary, 
> > > hw_cpu2);
> > >  > >  +                     set_hard_smp_processor_id(primary + 1,
> > >  > > hw_cpu1);
> > >  > >  +                     /* get new physical cpu id */
> > >  > >  +                     hw_cpu = get_hard_smp_processor_id(nr);
> > >  >
> > >  > NACK as discussed in http://patchwork.ozlabs.org/patch/454944/
> > >  >
> > >  > -Scott
> > > 
> > >  You said,
> > > 
> > >      There's no need for this. I have booting from a thread1, and 
> > > having
> > >  it
> > >      kick its thread0, working locally without messing with the 
> > > hwid/cpu
> > >      mapping.
> > > 
> > >  I still have questions here. After a core reset, how can you boot
> > >  Thread1
> > >  of the core first. As I know, Thread0 boots up first by default.
> > 
> > So the issue isn't that thread1 comes up first, but that you *want* 
> > thread1
> > to come up first and it won't.  I don't think this remapping is an 
> > acceptable
> > answer, though.  Instead, if you need only thread1 to come up, start 
> > the
> > core, have thread0 start thread1, and then send thread0 into whatever 
> > waiting
> > state it would be in if thread1 had never been offlined.
> > 
> > -Scott
> 
> Remapping is a concise solution. what's the harm of it?
> Keeping things simple is good in my opinion.

Remapping is not simple.  Remapping will make debugging more complicated (I 
see an oops on CPU <n>, which CPU's registers do I dump in the debugger?), 
could expose bugs where smp_processor_id() is used where 
hard_smp_processor_id() is needed, etc.

Having thread0 start thread1 and then go wherever it would have gone if 
thread1 were up the whole time is much more straightforward.

-Scott

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Scott Wood]

On Fri, 2015-07-31 at 17:20 +0800, b29983@freescale.com wrote:
> @@ -71,7 +56,7 @@ static void mpc85xx_give_timebase(void)
>               barrier();
>       tb_req = 0;
>  
> -     mpc85xx_timebase_freeze(1);
> +     qoriq_pm_ops->freeze_time_base(1);

freeze_time_base() takes a bool.  Use true/false.

>  #ifdef CONFIG_PPC64
>       /*
>        * e5500/e6500 have a workaround for erratum A-006958 in place
> @@ -104,7 +89,7 @@ static void mpc85xx_give_timebase(void)
>       while (tb_valid)
>               barrier();
>  
> -     mpc85xx_timebase_freeze(0);
> +     qoriq_pm_ops->freeze_time_base(0);
>  
>       local_irq_restore(flags);
>  }
> @@ -127,20 +112,10 @@ static void mpc85xx_take_timebase(void)
>  }
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> -static void smp_85xx_mach_cpu_die(void)
> +static void e500_cpu_idle(void)

This is not the function that gets called during normal cpu idle, and it 
shouldn't be named to look like it is.

>  {
> -     unsigned int cpu = smp_processor_id();
>       u32 tmp;
>  
> -     local_irq_disable();
> -     idle_task_exit();
> -     generic_set_cpu_dead(cpu);
> -     mb();
> -
> -     mtspr(SPRN_TCR, 0);
> -
> -     cur_cpu_spec->cpu_down_flush();
> -
>       tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP;
>       mtspr(SPRN_HID0, tmp);
>       isync();
> @@ -151,6 +126,25 @@ static void smp_85xx_mach_cpu_die(void)
>       mb();
>       mtmsr(tmp);
>       isync();
> +}
> +
> +static void qoriq_cpu_dying(void)
> +{
> +     unsigned int cpu = smp_processor_id();
> +
> +     hard_irq_disable();
> +     /* mask all irqs to prevent cpu wakeup */
> +     qoriq_pm_ops->irq_mask(cpu);
> +     idle_task_exit();
> +
> +     mtspr(SPRN_TCR, 0);
> +     mtspr(SPRN_TSR, mfspr(SPRN_TSR));
> +
> +     cur_cpu_spec->cpu_down_flush();
> +
> +     generic_set_cpu_dead(cpu);
> +
> +     e500_cpu_idle();

Why is something that claims to be applicable to all qoriq directly calling 
an e500v2-specific function?

Could you explain irq_mask()?  Why would there still be IRQs destined for 
this CPU at this point?

@@ -431,21 +415,9 @@ void __init mpc85xx_smp_init(void)
>               smp_85xx_ops.probe = NULL;
>       }
>  
> -     np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
> -     if (np) {
> -             guts = of_iomap(np, 0);
> -             of_node_put(np);
> -             if (!guts) {
> -                     pr_err("%s: Could not map guts node address\n",
> -                                                             __func__);
> -                     return;
> -             }
> -             smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
> -             smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
>  #ifdef CONFIG_HOTPLUG_CPU
> -             ppc_md.cpu_die = smp_85xx_mach_cpu_die;
> +     ppc_md.cpu_die = qoriq_cpu_dying;
>  #endif

Shouldn't you make sure there's a valid qoriq_pm_ops before setting 
cpu_die()?  Or make sure that qoriq_cpu_dying() works regardless.

-Scott

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Chenhui Zhao]

On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood <scottwood@freescale.com> 
wrote:
> On Fri, 2015-07-31 at 17:20 +0800, b29983@freescale.com wrote:
>>  @@ -71,7 +56,7 @@ static void mpc85xx_give_timebase(void)
>>                barrier();
>>        tb_req = 0;
>> 
>>  -     mpc85xx_timebase_freeze(1);
>>  +     qoriq_pm_ops->freeze_time_base(1);
> 
> freeze_time_base() takes a bool.  Use true/false.

OK.

> 
> 
>>   #ifdef CONFIG_PPC64
>>        /*
>>         * e5500/e6500 have a workaround for erratum A-006958 in place
>>  @@ -104,7 +89,7 @@ static void mpc85xx_give_timebase(void)
>>        while (tb_valid)
>>                barrier();
>> 
>>  -     mpc85xx_timebase_freeze(0);
>>  +     qoriq_pm_ops->freeze_time_base(0);
>> 
>>        local_irq_restore(flags);
>>   }
>>  @@ -127,20 +112,10 @@ static void mpc85xx_take_timebase(void)
>>   }
>> 
>>   #ifdef CONFIG_HOTPLUG_CPU
>>  -static void smp_85xx_mach_cpu_die(void)
>>  +static void e500_cpu_idle(void)
> 
> This is not the function that gets called during normal cpu idle, and 
> it
> shouldn't be named to look like it is.

Sorry, it's a typo. It shoule be "e500_cpu_die".

> 
>>   {
>>  -     unsigned int cpu = smp_processor_id();
>>        u32 tmp;
>> 
>>  -     local_irq_disable();
>>  -     idle_task_exit();
>>  -     generic_set_cpu_dead(cpu);
>>  -     mb();
>>  -
>>  -     mtspr(SPRN_TCR, 0);
>>  -
>>  -     cur_cpu_spec->cpu_down_flush();
>>  -
>>        tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP;
>>        mtspr(SPRN_HID0, tmp);
>>        isync();
>>  @@ -151,6 +126,25 @@ static void smp_85xx_mach_cpu_die(void)
>>        mb();
>>        mtmsr(tmp);
>>        isync();
>>  +}
>>  +
>>  +static void qoriq_cpu_dying(void)
>>  +{
>>  +     unsigned int cpu = smp_processor_id();
>>  +
>>  +     hard_irq_disable();
>>  +     /* mask all irqs to prevent cpu wakeup */
>>  +     qoriq_pm_ops->irq_mask(cpu);
>>  +     idle_task_exit();
>>  +
>>  +     mtspr(SPRN_TCR, 0);
>>  +     mtspr(SPRN_TSR, mfspr(SPRN_TSR));
>>  +
>>  +     cur_cpu_spec->cpu_down_flush();
>>  +
>>  +     generic_set_cpu_dead(cpu);
>>  +
>>  +     e500_cpu_idle();
> 
> Why is something that claims to be applicable to all qoriq directly 
> calling
> an e500v2-specific function?

Added "#ifndef CONFIG_PPC_E500MC" in the following patch.

> 
> Could you explain irq_mask()?  Why would there still be IRQs destined 
> for
> this CPU at this point?

This function just masks irq by setting the registers in RCPM (for 
example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all irqs to this CPU 
have been migrated to other CPUs.

> 
> @@ -431,21 +415,9 @@ void __init mpc85xx_smp_init(void)
>>                smp_85xx_ops.probe = NULL;
>>        }
>> 
>>  -     np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
>>  -     if (np) {
>>  -             guts = of_iomap(np, 0);
>>  -             of_node_put(np);
>>  -             if (!guts) {
>>  -                     pr_err("%s: Could not map guts node 
>> address\n",
>>  -                                                             
>> __func__);
>>  -                     return;
>>  -             }
>>  -             smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
>>  -             smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
>>   #ifdef CONFIG_HOTPLUG_CPU
>>  -             ppc_md.cpu_die = smp_85xx_mach_cpu_die;
>>  +     ppc_md.cpu_die = qoriq_cpu_dying;
>>   #endif
> 
> Shouldn't you make sure there's a valid qoriq_pm_ops before setting
> cpu_die()?  Or make sure that qoriq_cpu_dying() works regardless.
> 
> -Scott

This patch is just for e500v2. The following patches will handle the 
case of e500mc, e5500 and e6500.

-Chenhui

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Scott Wood]

On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> > 

> On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood <scottwood@freescale.com> 
> wrote:

> >
> > Could you explain irq_mask()?  Why would there still be IRQs destined 
> > for
> > this CPU at this point?
> 
> This function just masks irq by setting the registers in RCPM (for 
> example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all irqs to this CPU 
> have been migrated to other CPUs.

So why do we need to set those bits in RCPM?  Is it just caution?

> > @@ -431,21 +415,9 @@ void __init mpc85xx_smp_init(void)
> > >                smp_85xx_ops.probe = NULL;
> > >        }
> > > 
> > >  -     np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
> > >  -     if (np) {
> > >  -             guts = of_iomap(np, 0);
> > >  -             of_node_put(np);
> > >  -             if (!guts) {
> > >  -                     pr_err("%s: Could not map guts node 
> > > address\n",
> > >  -                                                             
> > > __func__);
> > >  -                     return;
> > >  -             }
> > >  -             smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
> > >  -             smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
> > >   #ifdef CONFIG_HOTPLUG_CPU
> > >  -             ppc_md.cpu_die = smp_85xx_mach_cpu_die;
> > >  +     ppc_md.cpu_die = qoriq_cpu_dying;
> > >   #endif
> > 
> > Shouldn't you make sure there's a valid qoriq_pm_ops before setting
> > cpu_die()?  Or make sure that qoriq_cpu_dying() works regardless.
> > 
> > -Scott
> 
> This patch is just for e500v2. The following patches will handle the 
> case of e500mc, e5500 and e6500.

What stops a user from trying to use cpu hotplug on unsupported cpus, or in a 
virtualized environment, and crashing here?

-Scott

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Chenhui Zhao]

On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood <scottwood@freescale.com> 
wrote:
> On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
>>  >
> 
>>  On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood <scottwood@freescale.com>
>>  wrote:
> 
>>  >
>>  > Could you explain irq_mask()?  Why would there still be IRQs 
>> destined
>>  > for
>>  > this CPU at this point?
>> 
>>  This function just masks irq by setting the registers in RCPM (for
>>  example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all irqs to this CPU
>>  have been migrated to other CPUs.
> 
> So why do we need to set those bits in RCPM?  Is it just caution?

Setting these bits can mask interrupts signalled to RCPM from MPIC as a 
means of
waking up from a lower power state. So, cores will not be waked up 
unexpectedly.

> 
>>  > @@ -431,21 +415,9 @@ void __init mpc85xx_smp_init(void)
>>  > >                smp_85xx_ops.probe = NULL;
>>  > >        }
>>  > >
>>  > >  -     np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
>>  > >  -     if (np) {
>>  > >  -             guts = of_iomap(np, 0);
>>  > >  -             of_node_put(np);
>>  > >  -             if (!guts) {
>>  > >  -                     pr_err("%s: Could not map guts node
>>  > > address\n",
>>  > >  -
>>  > > __func__);
>>  > >  -                     return;
>>  > >  -             }
>>  > >  -             smp_85xx_ops.give_timebase = 
>> mpc85xx_give_timebase;
>>  > >  -             smp_85xx_ops.take_timebase = 
>> mpc85xx_take_timebase;
>>  > >   #ifdef CONFIG_HOTPLUG_CPU
>>  > >  -             ppc_md.cpu_die = smp_85xx_mach_cpu_die;
>>  > >  +     ppc_md.cpu_die = qoriq_cpu_dying;
>>  > >   #endif
>>  >
>>  > Shouldn't you make sure there's a valid qoriq_pm_ops before 
>> setting
>>  > cpu_die()?  Or make sure that qoriq_cpu_dying() works regardless.
>>  >
>>  > -Scott
>> 
>>  This patch is just for e500v2. The following patches will handle the
>>  case of e500mc, e5500 and e6500.
> 
> What stops a user from trying to use cpu hotplug on unsupported cpus, 
> or in a
> virtualized environment, and crashing here?
> 
> -Scott

Will set these callback functions only if qoriq_pm_ops is valid.

-Chenhui

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Scott Wood]

On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
> On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood <scottwood@freescale.com> 
> wrote:
> > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> > >  >
> > 
> > >  On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood <scottwood@freescale.com>
> > >  wrote:
> > 
> > >  >
> > >  > Could you explain irq_mask()?  Why would there still be IRQs 
> > > destined
> > >  > for
> > >  > this CPU at this point?
> > > 
> > >  This function just masks irq by setting the registers in RCPM (for
> > >  example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all irqs to this CPU
> > >  have been migrated to other CPUs.
> > 
> > So why do we need to set those bits in RCPM?  Is it just caution?
> 
> Setting these bits can mask interrupts signalled to RCPM from MPIC as a 
> means of
> waking up from a lower power state. So, cores will not be waked up 
> unexpectedly.

Why would the MPIC be signalling those interrupts if they've been masked at 
the MPIC?

-Scott

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Chenhui Zhao]

On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood <scottwood@freescale.com> 
wrote:
> On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
>>  On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood <scottwood@freescale.com>
>>  wrote:
>>  > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
>>  > >  >
>>  >
>>  > >  On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood 
>> <scottwood@freescale.com>
>>  > >  wrote:
>>  >
>>  > >  >
>>  > >  > Could you explain irq_mask()?  Why would there still be IRQs
>>  > > destined
>>  > >  > for
>>  > >  > this CPU at this point?
>>  > >
>>  > >  This function just masks irq by setting the registers in RCPM 
>> (for
>>  > >  example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all irqs to 
>> this CPU
>>  > >  have been migrated to other CPUs.
>>  >
>>  > So why do we need to set those bits in RCPM?  Is it just caution?
>> 
>>  Setting these bits can mask interrupts signalled to RCPM from MPIC 
>> as a
>>  means of
>>  waking up from a lower power state. So, cores will not be waked up
>>  unexpectedly.
> 
> Why would the MPIC be signalling those interrupts if they've been 
> masked at
> the MPIC?
> 
> -Scott
> 

The interrupts to RCPM from MPIC are IRQ, Machine Check, NMI and 
Critical interrupts. Some of them didn't be masked in MPIC.

-Chenhui

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Scott Wood]

On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
> On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood <scottwood@freescale.com> 
> wrote:
> > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
> > >  On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood <scottwood@freescale.com>
> > >  wrote:
> > >  > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> > >  > >  >
> > >  >
> > >  > >  On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood 
> > > <scottwood@freescale.com>
> > >  > >  wrote:
> > >  >
> > >  > >  >
> > >  > >  > Could you explain irq_mask()?  Why would there still be IRQs
> > >  > > destined
> > >  > >  > for
> > >  > >  > this CPU at this point?
> > >  > >
> > >  > >  This function just masks irq by setting the registers in RCPM 
> > > (for
> > >  > >  example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all irqs to 
> > > this CPU
> > >  > >  have been migrated to other CPUs.
> > >  >
> > >  > So why do we need to set those bits in RCPM?  Is it just caution?
> > > 
> > >  Setting these bits can mask interrupts signalled to RCPM from MPIC 
> > > as a
> > >  means of
> > >  waking up from a lower power state. So, cores will not be waked up
> > >  unexpectedly.
> > 
> > Why would the MPIC be signalling those interrupts if they've been 
> > masked at
> > the MPIC?
> > 
> > -Scott
> > 
> 
> The interrupts to RCPM from MPIC are IRQ, Machine Check, NMI and 
> Critical interrupts. Some of them didn't be masked in MPIC.

What interrupt could actually happen to a sleeping cpu that this protects 
against?

-Scott

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Chenhui Zhao]

On Thu, Aug 6, 2015 at 1:46 PM, Scott Wood <scottwood@freescale.com> 
wrote:
> On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
>>  On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood 
>> <scottwood@freescale.com>
>>  wrote:
>>  > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
>>  > >  On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood 
>> <scottwood@freescale.com>
>>  > >  wrote:
>>  > >  > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
>>  > >  > >  >
>>  > >  >
>>  > >  > >  On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
>>  > > <scottwood@freescale.com>
>>  > >  > >  wrote:
>>  > >  >
>>  > >  > >  >
>>  > >  > >  > Could you explain irq_mask()?  Why would there still be 
>> IRQs
>>  > >  > > destined
>>  > >  > >  > for
>>  > >  > >  > this CPU at this point?
>>  > >  > >
>>  > >  > >  This function just masks irq by setting the registers in 
>> RCPM
>>  > > (for
>>  > >  > >  example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all irqs to
>>  > > this CPU
>>  > >  > >  have been migrated to other CPUs.
>>  > >  >
>>  > >  > So why do we need to set those bits in RCPM?  Is it just 
>> caution?
>>  > >
>>  > >  Setting these bits can mask interrupts signalled to RCPM from 
>> MPIC
>>  > > as a
>>  > >  means of
>>  > >  waking up from a lower power state. So, cores will not be 
>> waked up
>>  > >  unexpectedly.
>>  >
>>  > Why would the MPIC be signalling those interrupts if they've been
>>  > masked at
>>  > the MPIC?
>>  >
>>  > -Scott
>>  >
>> 
>>  The interrupts to RCPM from MPIC are IRQ, Machine Check, NMI and
>>  Critical interrupts. Some of them didn't be masked in MPIC.
> 
> What interrupt could actually happen to a sleeping cpu that this 
> protects
> against?
> 
> -Scott

Not sure. Maybe spurious interrupts or hardware exceptions. However, 
setting them make sure dead cpus can not be waked up unexpectedly.

-Chenhui

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Scott Wood]

On Thu, 2015-08-06 at 13:54 +0800, Chenhui Zhao wrote:
> On Thu, Aug 6, 2015 at 1:46 PM, Scott Wood <scottwood@freescale.com> 
> wrote:
> > On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
> > >  On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood 
> > > <scottwood@freescale.com>
> > >  wrote:
> > >  > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
> > >  > >  On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood 
> > > <scottwood@freescale.com>
> > >  > >  wrote:
> > >  > >  > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> > >  > >  > >  >
> > >  > >  >
> > >  > >  > >  On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
> > >  > > <scottwood@freescale.com>
> > >  > >  > >  wrote:
> > >  > >  >
> > >  > >  > >  >
> > >  > >  > >  > Could you explain irq_mask()?  Why would there still be 
> > > IRQs
> > >  > >  > > destined
> > >  > >  > >  > for
> > >  > >  > >  > this CPU at this point?
> > >  > >  > >
> > >  > >  > >  This function just masks irq by setting the registers in 
> > > RCPM
> > >  > > (for
> > >  > >  > >  example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all irqs to
> > >  > > this CPU
> > >  > >  > >  have been migrated to other CPUs.
> > >  > >  >
> > >  > >  > So why do we need to set those bits in RCPM?  Is it just 
> > > caution?
> > >  > >
> > >  > >  Setting these bits can mask interrupts signalled to RCPM from 
> > > MPIC
> > >  > > as a
> > >  > >  means of
> > >  > >  waking up from a lower power state. So, cores will not be 
> > > waked up
> > >  > >  unexpectedly.
> > >  >
> > >  > Why would the MPIC be signalling those interrupts if they've been
> > >  > masked at
> > >  > the MPIC?
> > >  >
> > >  > -Scott
> > >  >
> > > 
> > >  The interrupts to RCPM from MPIC are IRQ, Machine Check, NMI and
> > >  Critical interrupts. Some of them didn't be masked in MPIC.
> > 
> > What interrupt could actually happen to a sleeping cpu that this 
> > protects
> > against?
> > 
> > -Scott
> 
> Not sure. Maybe spurious interrupts or hardware exceptions.

Spurious interrupts happen due to race conditions.  They don't happen because 
the MPIC is bored and decides to ring a CPU's doorbell and hide in the bushes.

If by "hardware exceptions" you mean machine checks, how would such a machine 
check be generated by a core that is off?

>  However, setting them make sure dead cpus can not be waked up unexpectedly.

I'm not seeing enough value here to warrant resurrecting the old sleep node 
stuff.

-Scott

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Chenhui Zhao]

On Fri, Aug 7, 2015 at 2:02 AM, Scott Wood <scottwood@freescale.com> 
wrote:
> On Thu, 2015-08-06 at 13:54 +0800, Chenhui Zhao wrote:
>>  On Thu, Aug 6, 2015 at 1:46 PM, Scott Wood <scottwood@freescale.com>
>>  wrote:
>>  > On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
>>  > >  On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood
>>  > > <scottwood@freescale.com>
>>  > >  wrote:
>>  > >  > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
>>  > >  > >  On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood
>>  > > <scottwood@freescale.com>
>>  > >  > >  wrote:
>>  > >  > >  > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
>>  > >  > >  > >  >
>>  > >  > >  >
>>  > >  > >  > >  On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
>>  > >  > > <scottwood@freescale.com>
>>  > >  > >  > >  wrote:
>>  > >  > >  >
>>  > >  > >  > >  >
>>  > >  > >  > >  > Could you explain irq_mask()?  Why would there 
>> still be
>>  > > IRQs
>>  > >  > >  > > destined
>>  > >  > >  > >  > for
>>  > >  > >  > >  > this CPU at this point?
>>  > >  > >  > >
>>  > >  > >  > >  This function just masks irq by setting the 
>> registers in
>>  > > RCPM
>>  > >  > > (for
>>  > >  > >  > >  example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all 
>> irqs to
>>  > >  > > this CPU
>>  > >  > >  > >  have been migrated to other CPUs.
>>  > >  > >  >
>>  > >  > >  > So why do we need to set those bits in RCPM?  Is it just
>>  > > caution?
>>  > >  > >
>>  > >  > >  Setting these bits can mask interrupts signalled to RCPM 
>> from
>>  > > MPIC
>>  > >  > > as a
>>  > >  > >  means of
>>  > >  > >  waking up from a lower power state. So, cores will not be
>>  > > waked up
>>  > >  > >  unexpectedly.
>>  > >  >
>>  > >  > Why would the MPIC be signalling those interrupts if they've 
>> been
>>  > >  > masked at
>>  > >  > the MPIC?
>>  > >  >
>>  > >  > -Scott
>>  > >  >
>>  > >
>>  > >  The interrupts to RCPM from MPIC are IRQ, Machine Check, NMI 
>> and
>>  > >  Critical interrupts. Some of them didn't be masked in MPIC.
>>  >
>>  > What interrupt could actually happen to a sleeping cpu that this
>>  > protects
>>  > against?
>>  >
>>  > -Scott
>> 
>>  Not sure. Maybe spurious interrupts or hardware exceptions.
> 
> Spurious interrupts happen due to race conditions.  They don't happen 
> because
> the MPIC is bored and decides to ring a CPU's doorbell and hide in 
> the bushes.
> 
> If by "hardware exceptions" you mean machine checks, how would such a 
> machine
> check be generated by a core that is off?
> 
>>   However, setting them make sure dead cpus can not be waked up 
>> unexpectedly.
> 
> I'm not seeing enough value here to warrant resurrecting the old 
> sleep node
> stuff.
> 
> -Scott

My guess maybe not accurate. My point is that electronic parts don't 
always work as expected. Taking preventative measures can make the 
system more robust. In addition, this step is required in deep sleep 
procedure.

-Chenhui

Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations

[Scott Wood]

On Fri, 2015-08-07 at 11:19 +0800, Chenhui Zhao wrote:
> On Fri, Aug 7, 2015 at 2:02 AM, Scott Wood <scottwood@freescale.com> 
> wrote:
> > On Thu, 2015-08-06 at 13:54 +0800, Chenhui Zhao wrote:
> > >  On Thu, Aug 6, 2015 at 1:46 PM, Scott Wood <scottwood@freescale.com>
> > >  wrote:
> > >  > On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
> > >  > >  On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood
> > >  > > <scottwood@freescale.com>
> > >  > >  wrote:
> > >  > >  > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
> > >  > >  > >  On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood
> > >  > > <scottwood@freescale.com>
> > >  > >  > >  wrote:
> > >  > >  > >  > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> > >  > >  > >  > >  >
> > >  > >  > >  >
> > >  > >  > >  > >  On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
> > >  > >  > > <scottwood@freescale.com>
> > >  > >  > >  > >  wrote:
> > >  > >  > >  >
> > >  > >  > >  > >  >
> > >  > >  > >  > >  > Could you explain irq_mask()?  Why would there 
> > > still be
> > >  > > IRQs
> > >  > >  > >  > > destined
> > >  > >  > >  > >  > for
> > >  > >  > >  > >  > this CPU at this point?
> > >  > >  > >  > >
> > >  > >  > >  > >  This function just masks irq by setting the 
> > > registers in
> > >  > > RCPM
> > >  > >  > > (for
> > >  > >  > >  > >  example, RCPM_CPMIMR, RCPM_CPMCIMR). Actually, all 
> > > irqs to
> > >  > >  > > this CPU
> > >  > >  > >  > >  have been migrated to other CPUs.
> > >  > >  > >  >
> > >  > >  > >  > So why do we need to set those bits in RCPM?  Is it just
> > >  > > caution?
> > >  > >  > >
> > >  > >  > >  Setting these bits can mask interrupts signalled to RCPM 
> > > from
> > >  > > MPIC
> > >  > >  > > as a
> > >  > >  > >  means of
> > >  > >  > >  waking up from a lower power state. So, cores will not be
> > >  > > waked up
> > >  > >  > >  unexpectedly.
> > >  > >  >
> > >  > >  > Why would the MPIC be signalling those interrupts if they've 
> > > been
> > >  > >  > masked at
> > >  > >  > the MPIC?
> > >  > >  >
> > >  > >  > -Scott
> > >  > >  >
> > >  > >
> > >  > >  The interrupts to RCPM from MPIC are IRQ, Machine Check, NMI 
> > > and
> > >  > >  Critical interrupts. Some of them didn't be masked in MPIC.
> > >  >
> > >  > What interrupt could actually happen to a sleeping cpu that this
> > >  > protects
> > >  > against?
> > >  >
> > >  > -Scott
> > > 
> > >  Not sure. Maybe spurious interrupts or hardware exceptions.
> > 
> > Spurious interrupts happen due to race conditions.  They don't happen 
> > because
> > the MPIC is bored and decides to ring a CPU's doorbell and hide in 
> > the bushes.
> > 
> > If by "hardware exceptions" you mean machine checks, how would such a 
> > machine
> > check be generated by a core that is off?
> > 
> > >   However, setting them make sure dead cpus can not be waked up 
> > > unexpectedly.
> > 
> > I'm not seeing enough value here to warrant resurrecting the old 
> > sleep node
> > stuff.
> > 
> > -Scott
> 
> My guess maybe not accurate. My point is that electronic parts don't 
> always work as expected. Taking preventative measures can make the 
> system more robust. In addition, this step is required in deep sleep 
> procedure.

The deep sleep part is more convincing -- so MPIC masking is not effective 
during deep sleep?

-Scott