Message ID | 1336737235-15370-2-git-send-email-chenhui.zhao@freescale.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Kumar Gala |
Headers | show |
On 05/11/2012 06:53 AM, Zhao Chenhui wrote: \> +#if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx) > +extern void __flush_disable_L1(void); > +#endif Prototypes aren't normally guarded by ifdefs. > +static void __cpuinit smp_85xx_mach_cpu_die(void) > +{ > + unsigned int cpu = smp_processor_id(); > + u32 tmp; > + > + local_irq_disable(); > + idle_task_exit(); > + generic_set_cpu_dead(cpu); > + mb(); > + > + mtspr(SPRN_TCR, 0); > + > + __flush_disable_L1(); > + tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP; > + mtspr(SPRN_HID0, tmp); > + > + /* Enter NAP mode. */ > + tmp = mfmsr(); > + tmp |= MSR_WE; > + mb(); > + mtmsr(tmp); > + isync(); Need isync after writing to HID0. > + /* > + * We don't set the BPTR register here upon it points > + * to the boot page properly. > + */ > + mpic_reset_core(hw_cpu); That comment's wording is hard to follow -- maybe s/upon it points/since it already points/ > + /* wait until core is ready... */ > + if (!spin_event_timeout(in_be32(&spin_table->addr_l) == 1, > + 10000, 100)) { > + pr_err("%s: timeout waiting for core %d to reset\n", > + __func__, hw_cpu); > + ret = -ENOENT; > + goto out; > + } We need to fix U-Boot to write addr_l last (with an msync beforehand). > -#ifdef CONFIG_KEXEC > +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU) Let's not grow lists like this. Is there any harm in building it unconditionally? -Scott
On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote: > On 05/11/2012 06:53 AM, Zhao Chenhui wrote: > \> +#if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx) > > +extern void __flush_disable_L1(void); > > +#endif > > Prototypes aren't normally guarded by ifdefs. OK. Thanks. > > > +static void __cpuinit smp_85xx_mach_cpu_die(void) > > +{ > > + unsigned int cpu = smp_processor_id(); > > + u32 tmp; > > + > > + local_irq_disable(); > > + idle_task_exit(); > > + generic_set_cpu_dead(cpu); > > + mb(); > > + > > + mtspr(SPRN_TCR, 0); > > + > > + __flush_disable_L1(); > > + tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP; > > + mtspr(SPRN_HID0, tmp); > > + > > + /* Enter NAP mode. */ > > + tmp = mfmsr(); > > + tmp |= MSR_WE; > > + mb(); > > + mtmsr(tmp); > > + isync(); > > Need isync after writing to HID0. > > > + /* > > + * We don't set the BPTR register here upon it points > > + * to the boot page properly. > > + */ > > + mpic_reset_core(hw_cpu); > > That comment's wording is hard to follow -- maybe s/upon it points/since > it already points/ > > > + /* wait until core is ready... */ > > + if (!spin_event_timeout(in_be32(&spin_table->addr_l) == 1, > > + 10000, 100)) { > > + pr_err("%s: timeout waiting for core %d to reset\n", > > + __func__, hw_cpu); > > + ret = -ENOENT; > > + goto out; > > + } > > We need to fix U-Boot to write addr_l last (with an msync beforehand). I agree. > > > -#ifdef CONFIG_KEXEC > > +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU) > > Let's not grow lists like this. Is there any harm in building it > unconditionally? > > -Scott We need this ifdef. We only set give_timebase/take_timebase when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined. -Chenhui
On 06/04/2012 06:04 AM, Zhao Chenhui wrote: > On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote: >> On 05/11/2012 06:53 AM, Zhao Chenhui wrote: >>> -#ifdef CONFIG_KEXEC >>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU) >> >> Let's not grow lists like this. Is there any harm in building it >> unconditionally? >> >> -Scott > > We need this ifdef. We only set give_timebase/take_timebase > when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined. If we really need this to be a compile-time decision, make a new symbol for it, but I really think this should be decided at runtime. Just because we have kexec or hotplug support enabled doesn't mean that's actually what we're doing at the moment. -Scott
On Mon, Jun 04, 2012 at 11:32:47AM -0500, Scott Wood wrote: > On 06/04/2012 06:04 AM, Zhao Chenhui wrote: > > On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote: > >> On 05/11/2012 06:53 AM, Zhao Chenhui wrote: > >>> -#ifdef CONFIG_KEXEC > >>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU) > >> > >> Let's not grow lists like this. Is there any harm in building it > >> unconditionally? > >> > >> -Scott > > > > We need this ifdef. We only set give_timebase/take_timebase > > when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined. > > If we really need this to be a compile-time decision, make a new symbol > for it, but I really think this should be decided at runtime. Just > because we have kexec or hotplug support enabled doesn't mean that's > actually what we're doing at the moment. > > -Scott If user does not enable kexec or hotplug, these codes are redundant. So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them. -Chenhui
On 06/05/2012 06:18 AM, Zhao Chenhui wrote: > On Mon, Jun 04, 2012 at 11:32:47AM -0500, Scott Wood wrote: >> On 06/04/2012 06:04 AM, Zhao Chenhui wrote: >>> On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote: >>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote: >>>>> -#ifdef CONFIG_KEXEC >>>>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU) >>>> >>>> Let's not grow lists like this. Is there any harm in building it >>>> unconditionally? >>>> >>>> -Scott >>> >>> We need this ifdef. We only set give_timebase/take_timebase >>> when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined. >> >> If we really need this to be a compile-time decision, make a new symbol >> for it, but I really think this should be decided at runtime. Just >> because we have kexec or hotplug support enabled doesn't mean that's >> actually what we're doing at the moment. >> >> -Scott > > If user does not enable kexec or hotplug, these codes are redundant. > So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them. My point is that these lists tend to grow and be a maintenance pain. For small things it's often better to not worry about saving a few bytes. For larger things that need to be conditional, define a new symbol rather than growing ORed lists like this. -Scott
On Tue, Jun 05, 2012 at 11:15:52AM -0500, Scott Wood wrote: > On 06/05/2012 06:18 AM, Zhao Chenhui wrote: > > On Mon, Jun 04, 2012 at 11:32:47AM -0500, Scott Wood wrote: > >> On 06/04/2012 06:04 AM, Zhao Chenhui wrote: > >>> On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote: > >>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote: > >>>>> -#ifdef CONFIG_KEXEC > >>>>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU) > >>>> > >>>> Let's not grow lists like this. Is there any harm in building it > >>>> unconditionally? > >>>> > >>>> -Scott > >>> > >>> We need this ifdef. We only set give_timebase/take_timebase > >>> when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined. > >> > >> If we really need this to be a compile-time decision, make a new symbol > >> for it, but I really think this should be decided at runtime. Just > >> because we have kexec or hotplug support enabled doesn't mean that's > >> actually what we're doing at the moment. > >> > >> -Scott > > > > If user does not enable kexec or hotplug, these codes are redundant. > > So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them. > > My point is that these lists tend to grow and be a maintenance pain. > For small things it's often better to not worry about saving a few > bytes. For larger things that need to be conditional, define a new > symbol rather than growing ORed lists like this. > > -Scott I agree with you in principle. But there are only two config options in this patch, and it is unlikely to grow. -Chenhui
On 06/06/2012 04:59 AM, Zhao Chenhui wrote: > On Tue, Jun 05, 2012 at 11:15:52AM -0500, Scott Wood wrote: >> On 06/05/2012 06:18 AM, Zhao Chenhui wrote: >>> If user does not enable kexec or hotplug, these codes are redundant. >>> So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them. >> >> My point is that these lists tend to grow and be a maintenance pain. >> For small things it's often better to not worry about saving a few >> bytes. For larger things that need to be conditional, define a new >> symbol rather than growing ORed lists like this. >> >> -Scott > > I agree with you in principle. But there are only two config options > in this patch, and it is unlikely to grow. That's what everybody says when these things start. :-) -Scott
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index feab3ba..d65ae35 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -220,7 +220,8 @@ config ARCH_HIBERNATION_POSSIBLE config ARCH_SUSPEND_POSSIBLE def_bool y depends on ADB_PMU || PPC_EFIKA || PPC_LITE5200 || PPC_83xx || \ - (PPC_85xx && !SMP) || PPC_86xx || PPC_PSERIES || 44x || 40x + (PPC_85xx && !PPC_E500MC) || PPC_86xx || PPC_PSERIES \ + || 44x || 40x config PPC_DCR_NATIVE bool @@ -331,7 +332,8 @@ config SWIOTLB config HOTPLUG_CPU bool "Support for enabling/disabling CPUs" - depends on SMP && HOTPLUG && EXPERIMENTAL && (PPC_PSERIES || PPC_PMAC || PPC_POWERNV) + depends on SMP && HOTPLUG && EXPERIMENTAL && (PPC_PSERIES || \ + PPC_PMAC || PPC_POWERNV || (PPC_85xx && !PPC_E500MC)) ---help--- Say Y here to be able to disable and re-enable individual CPUs at runtime on SMP machines. diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h index ab9e402..94ec20a 100644 --- a/arch/powerpc/include/asm/cacheflush.h +++ b/arch/powerpc/include/asm/cacheflush.h @@ -30,6 +30,10 @@ extern void flush_dcache_page(struct page *page); #define flush_dcache_mmap_lock(mapping) do { } while (0) #define flush_dcache_mmap_unlock(mapping) do { } while (0) +#if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_6xx) +extern void __flush_disable_L1(void); +#endif + extern void __flush_icache_range(unsigned long, unsigned long); static inline void flush_icache_range(unsigned long start, unsigned long stop) { diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h index ebc24dc..e807e9d 100644 --- a/arch/powerpc/include/asm/smp.h +++ b/arch/powerpc/include/asm/smp.h @@ -65,6 +65,7 @@ int generic_cpu_disable(void); void generic_cpu_die(unsigned int cpu); void generic_mach_cpu_die(void); void generic_set_cpu_dead(unsigned int cpu); +void generic_set_cpu_up(unsigned int cpu); int generic_check_cpu_restart(unsigned int cpu); #endif @@ -190,6 +191,7 @@ extern unsigned long __secondary_hold_spinloop; extern unsigned long __secondary_hold_acknowledge; extern char __secondary_hold; +extern void __early_start(void); #endif /* __ASSEMBLY__ */ #endif /* __KERNEL__ */ diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S index 28e6259..0cc6f02 100644 --- a/arch/powerpc/kernel/head_fsl_booke.S +++ b/arch/powerpc/kernel/head_fsl_booke.S @@ -1004,6 +1004,34 @@ _GLOBAL(flush_dcache_L1) blr +/* Flush L1 d-cache, invalidate and disable d-cache and i-cache */ +_GLOBAL(__flush_disable_L1) + mflr r10 + bl flush_dcache_L1 /* Flush L1 d-cache */ + mtlr r10 + + mfspr r4, SPRN_L1CSR0 /* Invalidate and disable d-cache */ + li r5, 2 + rlwimi r4, r5, 0, 3 + + msync + isync + mtspr SPRN_L1CSR0, r4 + isync + +1: mfspr r4, SPRN_L1CSR0 /* Wait for the invalidate to finish */ + andi. r4, r4, 2 + bne 1b + + mfspr r4, SPRN_L1CSR1 /* Invalidate and disable i-cache */ + li r5, 2 + rlwimi r4, r5, 0, 3 + + mtspr SPRN_L1CSR1, r4 + isync + + blr + #ifdef CONFIG_SMP /* When we get here, r24 needs to hold the CPU # */ .globl __secondary_start diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index d9f9441..e0ffe03 100644 --- a/arch/powerpc/kernel/smp.c +++ b/arch/powerpc/kernel/smp.c @@ -423,6 +423,16 @@ void generic_set_cpu_dead(unsigned int cpu) per_cpu(cpu_state, cpu) = CPU_DEAD; } +/* + * The cpu_state should be set to CPU_UP_PREPARE in kick_cpu(), otherwise + * the cpu_state is always CPU_DEAD after calling generic_set_cpu_dead(), + * which makes the delay in generic_cpu_die() not happen. + */ +void generic_set_cpu_up(unsigned int cpu) +{ + per_cpu(cpu_state, cpu) = CPU_UP_PREPARE; +} + int generic_check_cpu_restart(unsigned int cpu) { return per_cpu(cpu_state, cpu) == CPU_UP_PREPARE; diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c index 6862dda..6b95d4a 100644 --- a/arch/powerpc/platforms/85xx/smp.c +++ b/arch/powerpc/platforms/85xx/smp.c @@ -2,7 +2,7 @@ * Author: Andy Fleming <afleming@freescale.com> * Kumar Gala <galak@kernel.crashing.org> * - * Copyright 2006-2008, 2011 Freescale Semiconductor Inc. + * Copyright 2006-2008, 2011-2012 Freescale Semiconductor Inc. * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the @@ -17,6 +17,7 @@ #include <linux/of.h> #include <linux/kexec.h> #include <linux/highmem.h> +#include <linux/cpu.h> #include <asm/machdep.h> #include <asm/pgtable.h> @@ -30,28 +31,55 @@ #include <sysdev/mpic.h> #include "smp.h" -extern void __early_start(void); - -#define BOOT_ENTRY_ADDR_UPPER 0 -#define BOOT_ENTRY_ADDR_LOWER 1 -#define BOOT_ENTRY_R3_UPPER 2 -#define BOOT_ENTRY_R3_LOWER 3 -#define BOOT_ENTRY_RESV 4 -#define BOOT_ENTRY_PIR 5 -#define BOOT_ENTRY_R6_UPPER 6 -#define BOOT_ENTRY_R6_LOWER 7 -#define NUM_BOOT_ENTRY 8 -#define SIZE_BOOT_ENTRY (NUM_BOOT_ENTRY * sizeof(u32)) - -static int __init -smp_85xx_kick_cpu(int nr) +struct epapr_spin_table { + u32 addr_h; + u32 addr_l; + u32 r3_h; + u32 r3_l; + u32 reserved; + u32 pir; +}; + +static void __cpuinit smp_85xx_setup_cpu(int cpu_nr); + +#ifdef CONFIG_HOTPLUG_CPU +static void __cpuinit smp_85xx_mach_cpu_die(void) +{ + unsigned int cpu = smp_processor_id(); + u32 tmp; + + local_irq_disable(); + idle_task_exit(); + generic_set_cpu_dead(cpu); + mb(); + + mtspr(SPRN_TCR, 0); + + __flush_disable_L1(); + tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP; + mtspr(SPRN_HID0, tmp); + + /* Enter NAP mode. */ + tmp = mfmsr(); + tmp |= MSR_WE; + mb(); + mtmsr(tmp); + isync(); + + while (1) + ; +} +#endif + +static int __cpuinit smp_85xx_kick_cpu(int nr) { unsigned long flags; const u64 *cpu_rel_addr; - __iomem u32 *bptr_vaddr; + __iomem struct epapr_spin_table *spin_table; struct device_node *np; - int n = 0, hw_cpu = get_hard_smp_processor_id(nr); + int hw_cpu = get_hard_smp_processor_id(nr); int ioremappable; + int ret = 0; WARN_ON(nr < 0 || nr >= NR_CPUS); WARN_ON(hw_cpu < 0 || hw_cpu >= NR_CPUS); @@ -76,49 +104,84 @@ smp_85xx_kick_cpu(int nr) /* Map the spin table */ if (ioremappable) - bptr_vaddr = ioremap(*cpu_rel_addr, SIZE_BOOT_ENTRY); + spin_table = ioremap(*cpu_rel_addr, + sizeof(struct epapr_spin_table)); else - bptr_vaddr = phys_to_virt(*cpu_rel_addr); + spin_table = phys_to_virt(*cpu_rel_addr); local_irq_save(flags); - - out_be32(bptr_vaddr + BOOT_ENTRY_PIR, hw_cpu); #ifdef CONFIG_PPC32 - out_be32(bptr_vaddr + BOOT_ENTRY_ADDR_LOWER, __pa(__early_start)); +#ifdef CONFIG_HOTPLUG_CPU + /* Corresponding to generic_set_cpu_dead() */ + generic_set_cpu_up(nr); + + if (system_state == SYSTEM_RUNNING) { + out_be32(&spin_table->addr_l, 0); + + /* + * We don't set the BPTR register here upon it points + * to the boot page properly. + */ + mpic_reset_core(hw_cpu); + + /* wait until core is ready... */ + if (!spin_event_timeout(in_be32(&spin_table->addr_l) == 1, + 10000, 100)) { + pr_err("%s: timeout waiting for core %d to reset\n", + __func__, hw_cpu); + ret = -ENOENT; + goto out; + } + + /* clear the acknowledge status */ + __secondary_hold_acknowledge = -1; + } +#endif + out_be32(&spin_table->pir, hw_cpu); + out_be32(&spin_table->addr_l, __pa(__early_start)); if (!ioremappable) - flush_dcache_range((ulong)bptr_vaddr, - (ulong)(bptr_vaddr + SIZE_BOOT_ENTRY)); + flush_dcache_range((ulong)spin_table, + (ulong)spin_table + sizeof(struct epapr_spin_table)); /* Wait a bit for the CPU to ack. */ - while ((__secondary_hold_acknowledge != hw_cpu) && (++n < 1000)) - mdelay(1); + if (!spin_event_timeout(__secondary_hold_acknowledge == hw_cpu, + 10000, 100)) { + pr_err("%s: timeout waiting for core %d to ack\n", + __func__, hw_cpu); + ret = -ENOENT; + goto out; + } +out: #else smp_generic_kick_cpu(nr); - out_be64((u64 *)(bptr_vaddr + BOOT_ENTRY_ADDR_UPPER), - __pa((u64)*((unsigned long long *) generic_secondary_smp_init))); + out_be32(&spin_table->pir, hw_cpu); + out_be64((u64 *)(&spin_table->addr_h), + __pa((u64)*((unsigned long long *)generic_secondary_smp_init))); if (!ioremappable) - flush_dcache_range((ulong)bptr_vaddr, - (ulong)(bptr_vaddr + SIZE_BOOT_ENTRY)); + flush_dcache_range((ulong)spin_table, + (ulong)spin_table + sizeof(struct epapr_spin_table)); #endif local_irq_restore(flags); if (ioremappable) - iounmap(bptr_vaddr); - - pr_debug("waited %d msecs for CPU #%d.\n", n, nr); + iounmap(spin_table); - return 0; + return ret; } struct smp_ops_t smp_85xx_ops = { .kick_cpu = smp_85xx_kick_cpu, +#ifdef CONFIG_HOTPLUG_CPU + .cpu_disable = generic_cpu_disable, + .cpu_die = generic_cpu_die, +#endif }; -#ifdef CONFIG_KEXEC +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU) static struct ccsr_guts __iomem *guts; static u64 timebase; static int tb_req; @@ -179,7 +242,9 @@ static void mpc85xx_take_timebase(void) local_irq_restore(flags); } +#endif +#ifdef CONFIG_KEXEC atomic_t kexec_down_cpus = ATOMIC_INIT(0); void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary) @@ -276,8 +341,7 @@ static void mpc85xx_smp_machine_kexec(struct kimage *image) } #endif /* CONFIG_KEXEC */ -static void __init -smp_85xx_setup_cpu(int cpu_nr) +static void __cpuinit smp_85xx_setup_cpu(int cpu_nr) { if (smp_85xx_ops.probe == smp_mpic_probe) mpic_setup_this_cpu(); @@ -286,7 +350,7 @@ smp_85xx_setup_cpu(int cpu_nr) doorbell_setup_this_cpu(); } -#ifdef CONFIG_KEXEC +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU) static const struct of_device_id guts_ids[] = { { .compatible = "fsl,mpc8572-guts", }, { .compatible = "fsl,mpc8560-guts", }, @@ -321,10 +385,11 @@ void __init mpc85xx_smp_init(void) smp_85xx_ops.cause_ipi = doorbell_cause_ipi; } -#ifdef CONFIG_KEXEC +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU) np = of_find_matching_node(NULL, guts_ids); if (np) { guts = of_iomap(np, 0); + ppc_md.cpu_die = smp_85xx_mach_cpu_die; smp_85xx_ops.give_timebase = mpc85xx_give_timebase; smp_85xx_ops.take_timebase = mpc85xx_take_timebase; of_node_put(np);