| Message ID | 1411730703-25836-2-git-send-email-chenhui.zhao@freescale.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Sep 26, 2014 at 07:25:01PM +0800, Chenhui Zhao wrote: > +static inline void ls1_do_lowpower(unsigned int cpu, int *spurious) > +{ > + /* > + * there is no power-control hardware on this platform, so all > + * we can do is put the core into WFI; this is safe as the calling > + * code will have already disabled interrupts > + */ > + for (;;) { > + wfi(); > + > + if (pen_release == cpu_logical_map(cpu)) { > + /*OK, proper wakeup, we're done*/ > + break; > + } > + > + /* > + * Getting here, means that we have come out of WFI without > + * having been woken up - this shouldn't happen > + * > + * Just note it happening - when we're woken, we can report > + * its occurrence. > + */ > + (*spurious)++; > + } > +} This is pretty much unacceptable - this breaks kexec(), and suspend support because your secondary CPUs aren't really sleeping, they're sitting in a loop doing nothing. In the kexec case, the code which the secondary CPU is executing can be overwritten, which then means that the CPU ends up executing some random code instead. Do you really have no way to properly power down or reset the CPU being unplugged? If you don't, that's a *huge* oversight in this modern age, and it means that you're pretty much stuck with having these features disabled.
On Fri, Sep 26, 2014 at 01:20:04PM +0100, Russell King - ARM Linux wrote: > On Fri, Sep 26, 2014 at 07:25:01PM +0800, Chenhui Zhao wrote: > > +static inline void ls1_do_lowpower(unsigned int cpu, int *spurious) > > +{ > > + /* > > + * there is no power-control hardware on this platform, so all > > + * we can do is put the core into WFI; this is safe as the calling > > + * code will have already disabled interrupts > > + */ > > + for (;;) { > > + wfi(); > > + > > + if (pen_release == cpu_logical_map(cpu)) { > > + /*OK, proper wakeup, we're done*/ > > + break; > > + } > > + > > + /* > > + * Getting here, means that we have come out of WFI without > > + * having been woken up - this shouldn't happen > > + * > > + * Just note it happening - when we're woken, we can report > > + * its occurrence. > > + */ > > + (*spurious)++; > > + } > > +} > > This is pretty much unacceptable - this breaks kexec(), and suspend > support because your secondary CPUs aren't really sleeping, they're > sitting in a loop doing nothing. Agreed. This looks to be a carbon copy of the vexpress pseudo-hotplug in arch/arm/mach-vexpress/hotplug.c, which is obviously broken in the way you describe above. Perhaps we should go about ripping that out? Mark.
On Fri, Sep 26, 2014 at 01:46:14PM +0100, Mark Rutland wrote: > On Fri, Sep 26, 2014 at 01:20:04PM +0100, Russell King - ARM Linux wrote: > > On Fri, Sep 26, 2014 at 07:25:01PM +0800, Chenhui Zhao wrote: > > > +static inline void ls1_do_lowpower(unsigned int cpu, int *spurious) > > > +{ > > > + /* > > > + * there is no power-control hardware on this platform, so all > > > + * we can do is put the core into WFI; this is safe as the calling > > > + * code will have already disabled interrupts > > > + */ > > > + for (;;) { > > > + wfi(); > > > + > > > + if (pen_release == cpu_logical_map(cpu)) { > > > + /*OK, proper wakeup, we're done*/ > > > + break; > > > + } > > > + > > > + /* > > > + * Getting here, means that we have come out of WFI without > > > + * having been woken up - this shouldn't happen > > > + * > > > + * Just note it happening - when we're woken, we can report > > > + * its occurrence. > > > + */ > > > + (*spurious)++; > > > + } > > > +} > > > > This is pretty much unacceptable - this breaks kexec(), and suspend > > support because your secondary CPUs aren't really sleeping, they're > > sitting in a loop doing nothing. > > Agreed. > > This looks to be a carbon copy of the vexpress pseudo-hotplug in > arch/arm/mach-vexpress/hotplug.c, which is obviously broken in the way > you describe above. Perhaps we should go about ripping that out? The Versatile Express does not support suspend so the only problem case is kexec. However, isn't this support needed for big.LITTLE, and as the Versatile Express is the platform which these features get developed on, having working CPU hotplug seems rather fundamental for ARM kernel feature development. In that regard, Versatile Express is something of a special case.
> > This looks to be a carbon copy of the vexpress pseudo-hotplug in > > arch/arm/mach-vexpress/hotplug.c, which is obviously broken in the way > > you describe above. Perhaps we should go about ripping that out? > > The Versatile Express does not support suspend so the only problem case > is kexec. However, isn't this support needed for big.LITTLE, and as > the Versatile Express is the platform which these features get developed > on, having working CPU hotplug seems rather fundamental for ARM kernel > feature development. > > In that regard, Versatile Express is something of a special case. It is admittedly helpful during development to perform pseudo-hotplug on Versatile Express. I have a local patch adding vexpress_cpu_disable so I can test for bugs that only trigger if CPU0 is hotplugged. Given that, perhaps we should make it clearer that Versatile Express is not a reference implementation for CPU hotplug; add some Kconfig (e.g. VEXPRESS_PSEUDO_HOTPLUG) that depends on !KEXEC && !SUSPEND, and putting a note in hotplug.c stating it's not suitable as a reference implementation. ...but perhaps that's overkill. Mark.
On Fri, Sep 26, 2014 at 9:20 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> > This looks to be a carbon copy of the vexpress pseudo-hotplug in >> > arch/arm/mach-vexpress/hotplug.c, which is obviously broken in the way >> > you describe above. Perhaps we should go about ripping that out? >> >> The Versatile Express does not support suspend so the only problem case >> is kexec. However, isn't this support needed for big.LITTLE, and as >> the Versatile Express is the platform which these features get developed >> on, having working CPU hotplug seems rather fundamental for ARM kernel >> feature development. >> >> In that regard, Versatile Express is something of a special case. > > It is admittedly helpful during development to perform pseudo-hotplug on > Versatile Express. I have a local patch adding vexpress_cpu_disable so I > can test for bugs that only trigger if CPU0 is hotplugged. > > Given that, perhaps we should make it clearer that Versatile Express is > not a reference implementation for CPU hotplug; add some Kconfig (e.g. > VEXPRESS_PSEUDO_HOTPLUG) that depends on !KEXEC && !SUSPEND, and putting > a note in hotplug.c stating it's not suitable as a reference > implementation. > > ...but perhaps that's overkill. I agree that the pseudo-hotplug is breaking the kexec. However, I don't think it breaks the system suspend case as tasks and interrupts have been moved out of the non-booting CPU. The pseudo-hotplug would be even better than real hotplug as it introduces less latency. Regards, Leo
On Sun, Sep 28, 2014 at 06:57:18PM +0800, Li Yang wrote: > On Fri, Sep 26, 2014 at 9:20 PM, Mark Rutland <mark.rutland@arm.com> wrote: > >> > This looks to be a carbon copy of the vexpress pseudo-hotplug in > >> > arch/arm/mach-vexpress/hotplug.c, which is obviously broken in the way > >> > you describe above. Perhaps we should go about ripping that out? > >> > >> The Versatile Express does not support suspend so the only problem case > >> is kexec. However, isn't this support needed for big.LITTLE, and as > >> the Versatile Express is the platform which these features get developed > >> on, having working CPU hotplug seems rather fundamental for ARM kernel > >> feature development. > >> > >> In that regard, Versatile Express is something of a special case. > > > > It is admittedly helpful during development to perform pseudo-hotplug on > > Versatile Express. I have a local patch adding vexpress_cpu_disable so I > > can test for bugs that only trigger if CPU0 is hotplugged. > > > > Given that, perhaps we should make it clearer that Versatile Express is > > not a reference implementation for CPU hotplug; add some Kconfig (e.g. > > VEXPRESS_PSEUDO_HOTPLUG) that depends on !KEXEC && !SUSPEND, and putting > > a note in hotplug.c stating it's not suitable as a reference > > implementation. > > > > ...but perhaps that's overkill. > > I agree that the pseudo-hotplug is breaking the kexec. However, I > don't think it breaks the system suspend case as tasks and interrupts > have been moved out of the non-booting CPU. The pseudo-hotplug would > be even better than real hotplug as it introduces less latency. Sorry, that's not an acceptable reason to use it. NAK.
diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h index 203ee73..2ca32fe 100644 --- a/arch/arm/mach-imx/common.h +++ b/arch/arm/mach-imx/common.h @@ -115,6 +115,7 @@ void tzic_handle_irq(struct pt_regs *); extern void imx_enable_cpu(int cpu, bool enable); extern void imx_set_cpu_jump(int cpu, void *jump_addr); extern u32 imx_get_cpu_arg(int cpu); +extern u32 ls1_get_cpu_arg(int cpu); extern void imx_set_cpu_arg(int cpu, u32 arg); extern void v7_cpu_resume(void); #ifdef CONFIG_SMP @@ -145,6 +146,9 @@ extern void imx6q_set_chicken_bit(void); extern void imx_cpu_die(unsigned int cpu); extern int imx_cpu_kill(unsigned int cpu); +extern void ls1021a_cpu_die(unsigned int cpu); +extern int ls1021a_cpu_kill(unsigned int cpu); + #ifdef CONFIG_PM extern void imx6q_pm_init(void); extern void imx5_pm_init(void); diff --git a/arch/arm/mach-imx/hotplug.c b/arch/arm/mach-imx/hotplug.c index 3daf1ed..646034f 100644 --- a/arch/arm/mach-imx/hotplug.c +++ b/arch/arm/mach-imx/hotplug.c @@ -14,6 +14,9 @@ #include <linux/jiffies.h> #include <asm/cp15.h> #include <asm/proc-fns.h> +#include<asm/smp.h> +#include<asm/smp_plat.h> +#include<asm/cacheflush.h> #include "common.h" @@ -38,6 +41,22 @@ static inline void cpu_enter_lowpower(void) : "cc"); } +static inline void cpu_leave_lowpower(void) +{ + unsigned int v; + + asm volatile( + " mrc p15, 0, %0, c1, c0, 0\n" + " orr %0, %0, %1\n" + " mcr p15, 0, %0, c1, c0, 0\n" + " mrc p15, 0, %0, c1, c0, 1\n" + " orr %0, %0, %2\n" + " mcr p15, 0, %0, c1, c0, 1\n" + : "=&r" (v) + : "Ir" (CR_C), "Ir" (0x40) + : "cc"); +} + /* * platform-specific code to shutdown a CPU * @@ -66,3 +85,74 @@ int imx_cpu_kill(unsigned int cpu) imx_set_cpu_arg(cpu, 0); return 1; } + +static inline void ls1_do_lowpower(unsigned int cpu, int *spurious) +{ + /* + * there is no power-control hardware on this platform, so all + * we can do is put the core into WFI; this is safe as the calling + * code will have already disabled interrupts + */ + for (;;) { + wfi(); + + if (pen_release == cpu_logical_map(cpu)) { + /*OK, proper wakeup, we're done*/ + break; + } + + /* + * Getting here, means that we have come out of WFI without + * having been woken up - this shouldn't happen + * + * Just note it happening - when we're woken, we can report + * its occurrence. + */ + (*spurious)++; + } +} + +/* + * platform-specific code to shutdown a CPU + * + * Called with IRQs disabled + */ +void __ref ls1021a_cpu_die(unsigned int cpu) +{ + int spurious = 0; + + v7_exit_coherency_flush(louis); + + /*we're ready for shutdown now, so do it*/ + ls1_do_lowpower(cpu, &spurious); + + /* + * bring this CPU back into the world of cache + * coherency, and then restore interrupts + */ + cpu_leave_lowpower(); + + if (spurious) + pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, spurious); + + /* + * Do not return to the idle loop - jump back to the secondary + * cpu initialisation. There's some initialisation which needs + * to be repeated to undo the effects of taking the CPU offline. + */ + __asm__("mov sp, %0\n" + " mov fp, #0\n" + " b ls1021a_secondary_startup" + : + : "r" (task_stack_page(current) + THREAD_SIZE - 8)); +} + +int ls1021a_cpu_kill(unsigned int cpu) +{ + unsigned long timeout = jiffies + msecs_to_jiffies(50); + + while (ls1_get_cpu_arg(cpu)) + if (time_after(jiffies, timeout)) + return 0; + return 1; +} diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c index 5225b69..d262b32 100644 --- a/arch/arm/mach-imx/platsmp.c +++ b/arch/arm/mach-imx/platsmp.c @@ -29,6 +29,7 @@ u32 g_diag_reg; static void __iomem *scu_base; +void __iomem *dcfg_base; static struct map_desc scu_io_desc __initdata = { /* .virtual and .pfn are run-time assigned */ @@ -196,19 +197,26 @@ static void __init ls1021a_smp_init_cpus(void) set_cpu_possible(i, false); } +void ls1021a_set_secondary_entry(void) +{ + unsigned long paddr; + + if (dcfg_base) { + paddr = virt_to_phys(ls1021a_secondary_startup); + writel_relaxed(cpu_to_be32(paddr), + dcfg_base + DCFG_CCSR_SCRATCHRW1); + } +} + static void __init ls1021a_smp_prepare_cpus(unsigned int max_cpus) { struct device_node *np; - void __iomem *dcfg_base; - unsigned long paddr; np = of_find_compatible_node(NULL, NULL, "fsl,ls1021a-dcfg"); dcfg_base = of_iomap(np, 0); WARN_ON(!dcfg_base); - paddr = virt_to_phys(ls1021a_secondary_startup); - writel_relaxed(cpu_to_be32(paddr), dcfg_base + DCFG_CCSR_SCRATCHRW1); - + ls1021a_set_secondary_entry(); } struct smp_operations ls1021a_smp_ops __initdata = { @@ -216,4 +224,8 @@ struct smp_operations ls1021a_smp_ops __initdata = { .smp_prepare_cpus = ls1021a_smp_prepare_cpus, .smp_boot_secondary = ls1021a_boot_secondary, .smp_secondary_init = ls1021a_secondary_init, +#ifdef CONFIG_HOTPLUG_CPU + .cpu_die = ls1021a_cpu_die, + .cpu_kill = ls1021a_cpu_kill, +#endif }; diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c index 10a6b1a..49508d6 100644 --- a/arch/arm/mach-imx/src.c +++ b/arch/arm/mach-imx/src.c @@ -30,6 +30,8 @@ #define BP_SRC_SCR_CORE1_RST 14 #define BP_SRC_SCR_CORE1_ENABLE 22 +#define CCSR_TWAITSR0 0x04C + static void __iomem *src_base; static DEFINE_SPINLOCK(scr_lock); @@ -114,6 +116,25 @@ void imx_set_cpu_arg(int cpu, u32 arg) writel_relaxed(arg, src_base + SRC_GPR1 + cpu * 8 + 4); } +u32 ls1_get_cpu_arg(int cpu) +{ + struct device_node *np; + void __iomem *ls1_rcpm_base; + + np = of_find_compatible_node(NULL, NULL, "fsl,qoriq-rcpm-2.1"); + if (!np) { + pr_err("%s(): Can not find the RCPM node.\n", __func__); + return -ENODEV; + } + + ls1_rcpm_base = of_iomap(np, 0); + of_node_put(np); + WARN_ON(!ls1_rcpm_base); + + cpu = cpu_logical_map(cpu); + return readl_relaxed(ls1_rcpm_base + CCSR_TWAITSR0) & (1 << cpu); +} + void imx_src_prepare_restart(void) { u32 val;