From: Tang Yuantian <[email protected]>
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 <[email protected]>
Signed-off-by: Tang Yuantian <[email protected]>
---
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 <[email protected]>
* Kumar Gala <[email protected]>
*
- * 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
From: Tang Yuantian <[email protected]>
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 <[email protected]>
Signed-off-by: Tang Yuantian <[email protected]>
---
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
From: Tang Yuantian <[email protected]>
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 <[email protected]>
Signed-off-by: Tang Yuantian <[email protected]>
---
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
On Fri, 2015-07-31 at 17:20 +0800, [email protected] 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
On Fri, 2015-07-31 at 17:20 +0800, [email protected] wrote:
> From: Tang Yuantian <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Tang Yuantian <[email protected]>
> ---
> 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
On Fri, 2015-07-31 at 17:20 +0800, [email protected] 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
On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood <[email protected]>
wrote:
> On Fri, 2015-07-31 at 17:20 +0800, [email protected] 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
On Sat, Aug 1, 2015 at 8:14 AM, Scott Wood <[email protected]>
wrote:
> On Fri, 2015-07-31 at 17:20 +0800, [email protected] wrote:
>> From: Tang Yuantian <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Tang Yuantian <[email protected]>
>> ---
>> 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
On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> >
> On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood <[email protected]>
> 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
[Added [email protected]. 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 <[email protected]>
> wrote:
> > On Fri, 2015-07-31 at 17:20 +0800, [email protected]:
> > > From: Tang Yuantian <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Tang Yuantian <[email protected]>
> > > ---
> > > 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
On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood <[email protected]>
wrote:
> On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
>> >
>
>> On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood <[email protected]>
>> 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
On Tue, Aug 4, 2015 at 5:18 AM, Scott Wood <[email protected]>
wrote:
> [Added [email protected]. 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 <[email protected]>
>> wrote:
>> > On Fri, 2015-07-31 at 17:20 +0800, [email protected]:
>> > > From: Tang Yuantian <[email protected]>
>> > >
>> > > 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 <[email protected]>
>> > > Signed-off-by: Tang Yuantian <[email protected]>
>> +{
>> > > + 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
On Sat, Aug 1, 2015 at 8:22 AM, Scott Wood <[email protected]>
wrote:
> On Fri, 2015-07-31 at 17:20 +0800, [email protected] 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
On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
> On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood <[email protected]>
> wrote:
> > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> > > >
> >
> > > On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood <[email protected]>
> > > 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
On Wed, 2015-08-05 at 19:08 +0800, Chenhui Zhao wrote:
> On Sat, Aug 1, 2015 at 8:22 AM, Scott Wood <[email protected]>
> wrote:
> > On Fri, 2015-07-31 at 17:20 +0800, [email protected]:
> > > + /*
> > > + * 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
On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood <[email protected]>
wrote:
> On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
>> On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood <[email protected]>
>> wrote:
>> > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
>> > > >
>> >
>> > > On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
>> <[email protected]>
>> > > 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
On Thu, Aug 6, 2015 at 11:16 AM, Scott Wood <[email protected]>
wrote:
> On Wed, 2015-08-05 at 19:08 +0800, Chenhui Zhao wrote:
>> On Sat, Aug 1, 2015 at 8:22 AM, Scott Wood <[email protected]>
>> wrote:
>> > On Fri, 2015-07-31 at 17:20 +0800, [email protected]:
>> > > + /*
>> > > + * 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
On Thu, 2015-08-06 at 12:32 +0800, Chenhui Zhao wrote:
> On Thu, Aug 6, 2015 at 11:16 AM, Scott Wood <[email protected]>
> wrote:
> > On Wed, 2015-08-05 at 19:08 +0800, Chenhui Zhao wrote:
> > > On Sat, Aug 1, 2015 at 8:22 AM, Scott Wood <[email protected]>
> > > wrote:
> > > > On Fri, 2015-07-31 at 17:20 +0800, [email protected]:
> > > > > + /*
> > > > > + * 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
On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
> On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood <[email protected]>
> wrote:
> > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
> > > On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood <[email protected]>
> > > wrote:
> > > > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> > > > > >
> > > >
> > > > > On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
> > > <[email protected]>
> > > > > 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
On Thu, Aug 6, 2015 at 1:46 PM, Scott Wood <[email protected]>
wrote:
> On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
>> On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood
>> <[email protected]>
>> wrote:
>> > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
>> > > On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood
>> <[email protected]>
>> > > wrote:
>> > > > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
>> > > > > >
>> > > >
>> > > > > On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
>> > > <[email protected]>
>> > > > > 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
On Thu, 2015-08-06 at 13:54 +0800, Chenhui Zhao wrote:
> On Thu, Aug 6, 2015 at 1:46 PM, Scott Wood <[email protected]>
> wrote:
> > On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
> > > On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood
> > > <[email protected]>
> > > wrote:
> > > > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
> > > > > On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood
> > > <[email protected]>
> > > > > wrote:
> > > > > > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> > > > > > > >
> > > > > >
> > > > > > > On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
> > > > > <[email protected]>
> > > > > > > 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
On Fri, Aug 7, 2015 at 2:02 AM, Scott Wood <[email protected]>
wrote:
> On Thu, 2015-08-06 at 13:54 +0800, Chenhui Zhao wrote:
>> On Thu, Aug 6, 2015 at 1:46 PM, Scott Wood <[email protected]>
>> wrote:
>> > On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
>> > > On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood
>> > > <[email protected]>
>> > > wrote:
>> > > > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
>> > > > > On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood
>> > > <[email protected]>
>> > > > > wrote:
>> > > > > > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
>> > > > > > > >
>> > > > > >
>> > > > > > > On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
>> > > > > <[email protected]>
>> > > > > > > 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
On Fri, 2015-08-07 at 11:19 +0800, Chenhui Zhao wrote:
> On Fri, Aug 7, 2015 at 2:02 AM, Scott Wood <[email protected]>
> wrote:
> > On Thu, 2015-08-06 at 13:54 +0800, Chenhui Zhao wrote:
> > > On Thu, Aug 6, 2015 at 1:46 PM, Scott Wood <[email protected]>
> > > wrote:
> > > > On Thu, 2015-08-06 at 12:20 +0800, Chenhui Zhao wrote:
> > > > > On Thu, Aug 6, 2015 at 10:57 AM, Scott Wood
> > > > > <[email protected]>
> > > > > wrote:
> > > > > > On Wed, 2015-08-05 at 18:11 +0800, Chenhui Zhao wrote:
> > > > > > > On Tue, Aug 4, 2015 at 4:26 AM, Scott Wood
> > > > > <[email protected]>
> > > > > > > wrote:
> > > > > > > > On Mon, 2015-08-03 at 19:32 +0800, Chenhui Zhao wrote:
> > > > > > > > > >
> > > > > > > >
> > > > > > > > > On Sat, Aug 1, 2015 at 7:59 AM, Scott Wood
> > > > > > > <[email protected]>
> > > > > > > > > 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