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

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

[Scott Wood]

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

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

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

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

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

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

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


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

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

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

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

-Scott

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

[Chenhui Zhao]

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


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

Will change the max timeout to 10 seconds.

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

Will check if qoriq_pm_ops is valid.

-Chenhui

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

[Chenhui Zhao]

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

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

-Chenhui

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

[Scott Wood]

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

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

-Scott

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

[Chenhui Zhao]

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

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

-Chenhui

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

[Scott Wood]

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

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

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

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

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

-Scott

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

[Chenhui Zhao]

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

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

-Chenhui

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

[Scott Wood]

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

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

-Scott

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

[Chenhui Zhao]

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

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

-Chenhui

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

[Scott Wood]

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

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

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

-Scott