Re: [3/4] powerpc: support CPU hotplug for e500mc, e5500 and e6500
From: Wood Scott-B07421
Sent: Friday, April 3, 2015 0:03
To: Zhao Chenhui-B35336
Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org;
linux-kernel@vger.kernel.org; Jin Zhengxiong-R64188
Subject: Re: [3/4] powerpc: support CPU hotplug for e500mc, e5500 and e6500
On Thu, 2015-04-02 at 06:16 -0500, Zhao Chenhui-B35336 wrote:
>
>
> From: Wood Scott-B07421
> Sent: Tuesday, March 31, 2015 10:07
> To: Zhao Chenhui-B35336
> Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Jin Zhengxiong-R64188
> Subject: Re: [3/4] powerpc: support CPU hotplug for e500mc, e5500 and e6500
>
> On Thu, Mar 26, 2015 at 06:18:14PM +0800, chenhui zhao wrote:
> > @@ -189,16 +193,14 @@ _GLOBAL(fsl_secondary_thread_init)
> > isync
> >
> > /*
> > - * Fix PIR to match the linear numbering in the device tree.
> > - *
> > - * On e6500, the reset value of PIR uses the low three bits for
> > - * the thread within a core, and the upper bits for the core
> > - * number. There are two threads per core, so shift everything
> > - * but the low bit right by two bits so that the cpu numbering is
> > - * continuous.
>
> Why are you getting rid of this? If it's to avoid doing it twice on the
> same thread, in my work-in-progress kexec patches I instead check to see
> whether BUCSR has already been set up -- if it has, I assume we've
> already been here.
>
> [chenhui] I didn't delete the branch prediction related code.
I didn't say you did. I'm saying that you can check whether BUCSR has
been set up, to determine whether PIR has already been adjusted, if your
concern is avoiding running this twice on a thread between core resets.
If that's not your concern, then please explain.
[chenhui] If no need to change PIR in CPU hotplug, I will change the code as
you mentioned.
> > + /*
> > + * 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.
> > + */
>
> What if the core is not in a PM state that requires a reset?
> Where does this reset occur?
>
> [chenhui] Reset occurs in the function mpic_reset_core().
>
> > + 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);
>
> Why are you swapping the hard smp ids?
>
> [chenhui] For example, Core1 has two threads, Thread0 and Thread1. In normal
> boot, Thread0 is CPU2, and Thread1 is CPU3.
> But, if CPU2 and CPU3 are all off, user wants CPU3 up first. we need to call
> Thread0 as CPU3 and Thead1 as CPU2, considering
> the limitation, after core is reset, only Thread0 is up, then Thread0 kicks
> up Thread1.
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.
[chenhui] Great. If you have completed your patches, can we merge our code?
> > @@ -252,11 +340,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);
> > -
>
> Why did you move this?
>
> [chenhui] It would be better to set this after CPU is really up.
Please make it a separate patch with an explanation.
-Scott
[chenhui] OK.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [4/4] powerpc/85xx: support sleep feature on QorIQ SoCs with RCPM
From: Wood Scott-B07421
Sent: Tuesday, March 31, 2015 10:35
To: Zhao Chenhui-B35336
Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org;
linux-kernel@vger.kernel.org; Jin Zhengxiong-R64188
Subject: Re: [4/4] powerpc/85xx: support sleep feature on QorIQ SoCs with RCPM
On Thu, Mar 26, 2015 at 06:18:15PM +0800, chenhui zhao wrote:
> In sleep mode, the clocks of e500 cores and unused IP blocks is
> turned off. The IP blocks which are allowed to wake up the processor
> are still running.
>
> The sleep mode is equal to the Standby state in Linux. Use the
> command to enter sleep mode:
> echo standby > /sys/power/state
>
> Signed-off-by: Chenhui Zhao
> ---
> arch/powerpc/Kconfig | 3 +-
> arch/powerpc/platforms/85xx/Kconfig| 5 +++
> arch/powerpc/platforms/85xx/Makefile | 1 +
> arch/powerpc/platforms/85xx/qoriq_pm.c | 59
> ++
> arch/powerpc/platforms/86xx/Kconfig| 1 +
> 5 files changed, 67 insertions(+), 2 deletions(-)
> create mode 100644 arch/powerpc/platforms/85xx/qoriq_pm.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 9846c83..162eb53 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -233,7 +233,7 @@ config ARCH_HIBERNATION_POSSIBLE
> config ARCH_SUSPEND_POSSIBLE
> def_bool y
> depends on ADB_PMU || PPC_EFIKA || PPC_LITE5200 || PPC_83xx || \
> -(PPC_85xx && !PPC_E500MC) || PPC_86xx || PPC_PSERIES \
> +FSL_SOC_BOOKE || PPC_86xx || PPC_PSERIES \
> || 44x || 40x
>
> config PPC_DCR_NATIVE
> @@ -747,7 +747,6 @@ config FSL_PCI
>
> config FSL_PMC
> bool
> - default y
> depends on SUSPEND && (PPC_85xx || PPC_86xx)
Get rid of this depends line if you're going to use select instead.
> +static int qoriq_suspend_valid(suspend_state_t state)
> +{
> + unsigned int pm_modes;
> +
> + pm_modes = qoriq_pm_ops->get_pm_modes();
> +
> + if ((state == PM_SUSPEND_STANDBY) && (pm_modes & FSL_PM_SLEEP))
> + return 1;
Unnecessary parentheses around ==
-Scott
[chenhui] OK--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/4] powerpc: support CPU hotplug for e500mc, e5500 and e6500
From: Wood Scott-B07421
Sent: Tuesday, March 31, 2015 10:07
To: Zhao Chenhui-B35336
Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org;
linux-kernel@vger.kernel.org; Jin Zhengxiong-R64188
Subject: Re: [3/4] powerpc: support CPU hotplug for e500mc, e5500 and e6500
On Thu, Mar 26, 2015 at 06:18:14PM +0800, chenhui zhao wrote:
> Implemented CPU hotplug on e500mc, e5500 and e6500, and support
> multiple threads mode and 64-bits mode.
>
> For e6500 with two threads, if one thread is online, it can
> enable/disable the other thread in the same core. If two threads of
> one core are offline, the core will enter the PH20 state (a low power
> state). When the core is up again, Thread0 is up first, and it will be
> bound with the present booting cpu. This way, all CPUs can hotplug
> separately.
>
> Signed-off-by: Chenhui Zhao
> ---
> arch/powerpc/Kconfig | 2 +-
> arch/powerpc/include/asm/fsl_pm.h | 4 +
> arch/powerpc/include/asm/smp.h| 2 +
> arch/powerpc/kernel/head_64.S | 20 +++--
> arch/powerpc/kernel/smp.c | 5 ++
> arch/powerpc/platforms/85xx/smp.c | 182
> +-
> arch/powerpc/sysdev/fsl_rcpm.c| 56
> 7 files changed, 220 insertions(+), 51 deletions(-)
Please factor out changes to generic code (including but not limited to
cur_boot_cpu and PIR handling) into separate patches with clear
explanations.
[chenhui] OK.
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 22b0940..9846c83 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -380,7 +380,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/fsl_pm.h
> b/arch/powerpc/include/asm/fsl_pm.h
> index bbe6089..579f495 100644
> --- a/arch/powerpc/include/asm/fsl_pm.h
> +++ b/arch/powerpc/include/asm/fsl_pm.h
> @@ -34,6 +34,10 @@ struct fsl_pm_ops {
> void (*cpu_enter_state)(int cpu, int state);
> /* exit the CPU from the specified state */
> void (*cpu_exit_state)(int cpu, int state);
> + /* cpu up */
> + void (*cpu_up)(int cpu);
Again, this sort of comment is useless. Tell us what "cpu up" *does*,
when it should be called, etc.
> @@ -189,16 +193,14 @@ _GLOBAL(fsl_secondary_thread_init)
> isync
>
> /*
> - * Fix PIR to match the linear numbering in the device tree.
> - *
> - * On e6500, the reset value of PIR uses the low three bits for
> - * the thread within a core, and the upper bits for the core
> - * number. There are two threads per core, so shift everything
> - * but the low bit right by two bits so that the cpu numbering is
> - * continuous.
Why are you getting rid of this? If it's to avoid doing it twice on the
same thread, in my work-in-progress kexec patches I instead check to see
whether BUCSR has already been set up -- if it has, I assume we've
already been here.
[chenhui] I didn't delete the branch prediction related code.
> + * The current thread has been in 64-bit mode,
> + * see the value of TMRN_IMSR.
I don't see what the relevance of this comment is here.
[chenhui] Will explain it more clear.
> + * compute the address of __cur_boot_cpu
>*/
> - mfspr r3, SPRN_PIR
> - rlwimi r3, r3, 30, 2, 30
> + bl 10f
> +10: mflrr22
> + addir22,r22,(__cur_boot_cpu - 10b)
> + lwz r3,0(r22)
Please save non-volatile registers for things that need to stick around
for a while.
[chenhui] OK.
> mtspr SPRN_PIR, r3
If __cur_boot_cpu is meant to be the PIR of the currently booting CPU,
it's a misleading. It looks like it's supposed to have something to do
with the boot cpu (not "booting").
[chenhui] I mean the PIR of the currently booting CPU. Change to
"booting_cpu_hwid".
Also please don't put leading underscores on symbols just because the
adjacent symbols have them.
> -#ifdef CONFIG_HOTPLUG_CPU
> +#ifdef CONFIG_PPC_E500MC
> +static void qoriq_cpu_wait_die(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_flush_caches();
> +
> + generic_set_cpu_dead(cpu);
> + smp_mb();
Comment memory barriers, as checkpatch says.
> + while (1)
> + ;
Indent the ;
[chenhui] OK
> @@ -174,17 +232,29 @@ static inline u32 read_spin_table_addr_l(void
> *spin_table)
> static void wake_hw_thread(void *info)
> {
>
Re: [2/4] powerpc/rcpm: add RCPM driver
From: Wood Scott-B07421
Sent: Tuesday, March 31, 2015 9:30
To: Zhao Chenhui-B35336
Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org;
linux-kernel@vger.kernel.org; Jin Zhengxiong-R64188
Subject: Re: [2/4] powerpc/rcpm: add RCPM driver
On Thu, Mar 26, 2015 at 06:18:13PM +0800, chenhui zhao wrote:
> There is a RCPM (Run Control/Power Management) in Freescale QorIQ
> series processors. The device performs tasks associated with device
> run control and power management.
>
> The driver implements some features: mask/unmask irq, enter/exit low
> power states, freeze time base, etc.
>
> Signed-off-by: Chenhui Zhao
> ---
> Documentation/devicetree/bindings/soc/fsl/rcpm.txt | 23 ++
> arch/powerpc/include/asm/fsl_guts.h| 105 ++
> arch/powerpc/include/asm/fsl_pm.h | 49 +++
> arch/powerpc/platforms/85xx/Kconfig| 1 +
> arch/powerpc/sysdev/Kconfig| 5 +
> arch/powerpc/sysdev/Makefile | 1 +
> arch/powerpc/sysdev/fsl_rcpm.c | 353
> +
> 7 files changed, 537 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> create mode 100644 arch/powerpc/include/asm/fsl_pm.h
> create mode 100644 arch/powerpc/sysdev/fsl_rcpm.c
>
> diff --git a/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> new file mode 100644
> index 000..8c21b6c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> @@ -0,0 +1,23 @@
> +* Run Control and Power Management
> +
> +The RCPM performs all device-level tasks associated with device run control
> +and power management.
> +
> +Required properites:
> + - reg : Offset and length of the register set of RCPM block.
> + - compatible : Specifies the compatibility list for the RCPM. The type
> +should be string, such as "fsl,qoriq-rcpm-1.0", "fsl,qoriq-rcpm-2.0".
> +
> +Example:
> +The RCPM node for T4240:
> + rcpm: global-utilities@e2000 {
> + compatible = "fsl,t4240-rcpm", "fsl,qoriq-rcpm-2.0";
> + reg = <0xe2000 0x1000>;
> + };
> +
> +The RCPM node for P4080:
> + rcpm: global-utilities@e2000 {
> + compatible = "fsl,qoriq-rcpm-1.0";
> + reg = <0xe2000 0x1000>;
> + #sleep-cells = <1>;
> + };
Where is #sleep-cells documented? It's copy-and-paste from something
that was never finished from many years ago.
[chenhui] Will get rid of them.
> diff --git a/arch/powerpc/include/asm/fsl_pm.h
> b/arch/powerpc/include/asm/fsl_pm.h
> new file mode 100644
> index 000..bbe6089
> --- /dev/null
> +++ b/arch/powerpc/include/asm/fsl_pm.h
> @@ -0,0 +1,49 @@
> +/*
> + * Support Power Management
> + *
> + * Copyright 2014-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.
> + */
> +#ifndef __PPC_FSL_PM_H
> +#define __PPC_FSL_PM_H
> +#ifdef __KERNEL__
Put a space after #ifdef, not a tab.
[Chenhui] Will change it.
> +#define E500_PM_PH10 1
> +#define E500_PM_PH15 2
> +#define E500_PM_PH20 3
> +#define E500_PM_PH30 4
> +#define E500_PM_DOZE E500_PM_PH10
> +#define E500_PM_NAP E500_PM_PH15
> +
> +#define PLAT_PM_SLEEP20
> +#define PLAT_PM_LPM2030
> +
> +#define FSL_PM_SLEEP (1 << 0)
> +#define FSL_PM_DEEP_SLEEP(1 << 1)
> +
> +struct fsl_pm_ops {
> + /* mask pending interrupts to the RCPM from MPIC */
> + void (*irq_mask)(int cpu);
> + /* unmask pending interrupts to the RCPM from MPIC */
> + void (*irq_unmask)(int cpu);
> + /* place the CPU in the specified state */
> + void (*cpu_enter_state)(int cpu, int state);
> + /* exit the CPU from the specified state */
> + void (*cpu_exit_state)(int cpu, int state);
> + /* place the platform in the sleep state */
> + int (*plat_enter_sleep)(void);
> + /* freeze the time base */
> + void (*freeze_time_base)(int freeze);
> + /* keep the power of IP blocks during sleep/deep sleep */
> + void (*set_ip_power)(int enable, u32 *mask);
> + /* get platform supported power management modes */
> + unsigned int (*get_pm_modes)(void);
> +};
Drop the comments that are basically just a restatement of the function
name. Where there are comments, it'd be easier to read with a blank line
between a function and the next comment.
s/int enable/bool enable/
s/int freeze/bool freeze/
[chenhui] Yes, you are right.
> +#endif /* __KERNEL__ */
> +#endif /* __PPC_FSL_PM_H */
Please be consistent with whitespace.
> + default:
> + pr_err("%s: Unknown cpu PM state (%d)\n", __func__, state);
WARN?
> +static int rcpm_v2_plat_e
Re: [1/4] powerpc/cache: add cache flush operation for various e500
From: Wood Scott-B07421
Sent: Tuesday, March 31, 2015 9:10
To: Zhao Chenhui-B35336
Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org;
linux-kernel@vger.kernel.org; Jin Zhengxiong-R64188
Subject: Re: [1/4] powerpc/cache: add cache flush operation for various e500
On Thu, Mar 26, 2015 at 06:18:12PM +0800, chenhui zhao wrote:
> Various e500 core have different cache architecture, so they
> need different cache flush operations. Therefore, add a callback
> function cpu_flush_caches to the struct cpu_spec. The cache flush
> operation for the specific kind of e500 is selected at init time.
> The callback function will flush all caches inside the current cpu.
>
> Signed-off-by: Chenhui Zhao
> ---
> arch/powerpc/include/asm/cacheflush.h | 2 -
> arch/powerpc/include/asm/cputable.h | 11 +++
> arch/powerpc/kernel/asm-offsets.c | 3 +
> arch/powerpc/kernel/cpu_setup_fsl_booke.S | 114
> +-
> arch/powerpc/kernel/cputable.c| 4 ++
> arch/powerpc/kernel/head_fsl_booke.S | 74 ---
> arch/powerpc/platforms/85xx/smp.c | 3 +-
> 7 files changed, 133 insertions(+), 78 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/cacheflush.h
> b/arch/powerpc/include/asm/cacheflush.h
> index 30b35ff..729fde4 100644
> --- a/arch/powerpc/include/asm/cacheflush.h
> +++ b/arch/powerpc/include/asm/cacheflush.h
> @@ -30,8 +30,6 @@ 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)
>
> -extern void __flush_disable_L1(void);
> -
> extern void flush_icache_range(unsigned long, unsigned long);
> extern void flush_icache_user_range(struct vm_area_struct *vma,
> struct page *page, unsigned long addr,
> diff --git a/arch/powerpc/include/asm/cputable.h
> b/arch/powerpc/include/asm/cputable.h
> index 5cf5a6d..c776efe4 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -43,6 +43,13 @@ extern int machine_check_e500(struct pt_regs *regs);
> extern int machine_check_e200(struct pt_regs *regs);
> extern int machine_check_47x(struct pt_regs *regs);
>
> +#if defined(CONFIG_E500) || defined(CONFIG_PPC_E500MC)
> +extern void __flush_caches_e500v2(void);
> +extern void __flush_caches_e500mc(void);
> +extern void __flush_caches_e5500(void);
> +extern void __flush_caches_e6500(void);
> +#endif
Why the leading underscores?
[chenhui] Will get rid of them.
> /* NOTE WELL: Update identify_cpu() if fields are added or removed! */
> struct cpu_spec {
> /* CPU is matched via (PVR & pvr_mask) == pvr_value */
> @@ -59,6 +66,10 @@ struct cpu_spec {
> unsigned inticache_bsize;
> unsigned intdcache_bsize;
>
> +#if defined(CONFIG_E500) || defined(CONFIG_PPC_E500MC)
CONFIG_PPC_E500MC implies CONFIG_E500. Why do we need this ifdef?
[chenhui] Change to "#ifdef CONFIG_E500".
> + /* flush caches inside the current cpu */
> + void (*cpu_flush_caches)(void);
> +#endif
It seems you literally mean "in the cpu" -- If it's a threaded core, then
by "cpu" do you mean "thread" (like we usually do) and thus no caches get
flushed (ignore the fact that it's moot on e6500 -- this is an interface
and needs to be clear).
Also, no-oping L1 flush on e6500 is not compliant with the claim that
you're flushing the cache. You're relying on an unstated assumption that
you'll invalidate that cache later instead.
If you want to make this "flush whatever needs to be flushed for
suspend/hotplug", call it that.
-Scott
[chenhui] OK. Then, call "cpu_down_flush".
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
