Messages in this thread | | | Subject | Re: [PATCH 1/3] Powerpc: mpc85xx: refactor the PM operations | From | Scott Wood <> | Date | Fri, 31 Jul 2015 18:59:38 -0500 |
| |
On Fri, 2015-07-31 at 17:20 +0800, b29983@freescale.com wrote: > @@ -71,7 +56,7 @@ static void mpc85xx_give_timebase(void) > barrier(); > tb_req = 0; > > - mpc85xx_timebase_freeze(1); > + qoriq_pm_ops->freeze_time_base(1);
freeze_time_base() takes a bool. Use true/false.
> #ifdef CONFIG_PPC64 > /* > * e5500/e6500 have a workaround for erratum A-006958 in place > @@ -104,7 +89,7 @@ static void mpc85xx_give_timebase(void) > while (tb_valid) > barrier(); > > - mpc85xx_timebase_freeze(0); > + qoriq_pm_ops->freeze_time_base(0); > > local_irq_restore(flags); > } > @@ -127,20 +112,10 @@ static void mpc85xx_take_timebase(void) > } > > #ifdef CONFIG_HOTPLUG_CPU > -static void smp_85xx_mach_cpu_die(void) > +static void e500_cpu_idle(void)
This is not the function that gets called during normal cpu idle, and it shouldn't be named to look like it is.
> { > - unsigned int cpu = smp_processor_id(); > u32 tmp; > > - local_irq_disable(); > - idle_task_exit(); > - generic_set_cpu_dead(cpu); > - mb(); > - > - mtspr(SPRN_TCR, 0); > - > - cur_cpu_spec->cpu_down_flush(); > - > tmp = (mfspr(SPRN_HID0) & ~(HID0_DOZE|HID0_SLEEP)) | HID0_NAP; > mtspr(SPRN_HID0, tmp); > isync(); > @@ -151,6 +126,25 @@ static void smp_85xx_mach_cpu_die(void) > mb(); > mtmsr(tmp); > isync(); > +} > + > +static void qoriq_cpu_dying(void) > +{ > + unsigned int cpu = smp_processor_id(); > + > + hard_irq_disable(); > + /* mask all irqs to prevent cpu wakeup */ > + qoriq_pm_ops->irq_mask(cpu); > + idle_task_exit(); > + > + mtspr(SPRN_TCR, 0); > + mtspr(SPRN_TSR, mfspr(SPRN_TSR)); > + > + cur_cpu_spec->cpu_down_flush(); > + > + generic_set_cpu_dead(cpu); > + > + e500_cpu_idle();
Why is something that claims to be applicable to all qoriq directly calling an e500v2-specific function?
Could you explain irq_mask()? Why would there still be IRQs destined for this CPU at this point?
@@ -431,21 +415,9 @@ void __init mpc85xx_smp_init(void) > smp_85xx_ops.probe = NULL; > } > > - np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids); > - if (np) { > - guts = of_iomap(np, 0); > - of_node_put(np); > - if (!guts) { > - pr_err("%s: Could not map guts node address\n", > - __func__); > - return; > - } > - smp_85xx_ops.give_timebase = mpc85xx_give_timebase; > - smp_85xx_ops.take_timebase = mpc85xx_take_timebase; > #ifdef CONFIG_HOTPLUG_CPU > - ppc_md.cpu_die = smp_85xx_mach_cpu_die; > + ppc_md.cpu_die = qoriq_cpu_dying; > #endif
Shouldn't you make sure there's a valid qoriq_pm_ops before setting cpu_die()? Or make sure that qoriq_cpu_dying() works regardless.
-Scott
| |