| Message ID | 1368613644-11863-7-git-send-email-josephl@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/15/2013 04:27 AM, Joseph Lo wrote: > The Tegra114 is a quad cores SoC. Each core can be hotplugged including > CPU0. The hotplug sequence can be controlled by setting event trigger in > flow controller. Then the flow controller will take care all the power > sequence that include CPU up and down. > diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile > +obj-$(CONFIG_ARCH_TEGRA_114_SOC) += sleep-tegra30.o CONFIG_ARCH_TEGRA_3x_SOC also adds that to obj-y. Will including it twice cause problems? > diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S > * CPU boot vector when restarting the a CPU following > * an LP2 transition. Also branched to by LP0 and LP1 resume after > * re-enabling sdram. > + * > + * r6: SoC ID << 8 I meant to ask in patch 1: Perhaps the macro could do a >>8 so that code can compare directly against the SoC IDs instead of having to compare against shifted values. > + tegra_check_soc_id TEGRA114, TEGRA_APB_MISC_BASE, r6, r7 > + beq no_cpu0_chk Does that need a THUMB(it ...) added? Same elsewhere. > + cmp r6, #(TEGRA114 <<8) Needs a space after <<. > diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S > @@ -45,12 +46,17 @@ ENDPROC(tegra30_hotplug_shutdown) > * and powergates it -- flags (in R0) indicate the request type. > * Must never be called for CPU 0. That comment is wrong now, I think. > tst r0, #TEGRA30_POWER_HOTPLUG_SHUTDOWN > + beq flow_ctrl_setting_for_lp2 > + > + /* flow controller set up for hotplug */ > + mov r3, #FLOW_CTRL_WAITEVENT @ For hotplug > + b flow_ctrl_done > +flow_ctrl_setting_for_lp2: > + /* flow controller set up for LP2 */ > + cmp r10, #(TEGRA30 << 8) > moveq r3, #FLOW_CTRL_WAIT_FOR_INTERRUPT @ For LP2 Do you need e.g. "movne r3, #0" or similar here? Otherwise, I think r3 ends up not being initialized. > - movne r3, #FLOW_CTRL_WAITEVENT @ For hotplug > +flow_ctrl_done: > + cmp r10, #(TEGRA30 << 8) > str r3, [r2] I wonder whether creating a separate tegra114_cpu_shutdown() wouldn't be better, to avoid all the runtime conditional code.
On Thu, 2013-05-16 at 07:11 +0800, Stephen Warren wrote: > On 05/15/2013 04:27 AM, Joseph Lo wrote: > > The Tegra114 is a quad cores SoC. Each core can be hotplugged including > > CPU0. The hotplug sequence can be controlled by setting event trigger in > > flow controller. Then the flow controller will take care all the power > > sequence that include CPU up and down. > > > diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile > > > +obj-$(CONFIG_ARCH_TEGRA_114_SOC) += sleep-tegra30.o > > CONFIG_ARCH_TEGRA_3x_SOC also adds that to obj-y. Will including it > twice cause problems? > Looks the compiler can take care of this. I didn't see any problem here. > > diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S > > > * CPU boot vector when restarting the a CPU following > > * an LP2 transition. Also branched to by LP0 and LP1 resume after > > * re-enabling sdram. > > + * > > + * r6: SoC ID << 8 > > I meant to ask in patch 1: Perhaps the macro could do a >>8 so that code > can compare directly against the SoC IDs instead of having to compare > against shifted values. > That's more easy to read the code indeed. Will do. > > + tegra_check_soc_id TEGRA114, TEGRA_APB_MISC_BASE, r6, r7 > > + beq no_cpu0_chk > > Does that need a THUMB(it ...) added? Same elsewhere. > Will double confirm again later. > > + cmp r6, #(TEGRA114 <<8) > > Needs a space after <<. > > > diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S > > > @@ -45,12 +46,17 @@ ENDPROC(tegra30_hotplug_shutdown) > > * and powergates it -- flags (in R0) indicate the request type. > > * Must never be called for CPU 0. > > That comment is wrong now, I think. Only true for Tegra30 now. Will fix. > > > tst r0, #TEGRA30_POWER_HOTPLUG_SHUTDOWN > > + beq flow_ctrl_setting_for_lp2 > > + > > + /* flow controller set up for hotplug */ > > + mov r3, #FLOW_CTRL_WAITEVENT @ For hotplug > > + b flow_ctrl_done > > +flow_ctrl_setting_for_lp2: > > + /* flow controller set up for LP2 */ > > + cmp r10, #(TEGRA30 << 8) > > moveq r3, #FLOW_CTRL_WAIT_FOR_INTERRUPT @ For LP2 > > Do you need e.g. "movne r3, #0" or similar here? Otherwise, I think r3 > ends up not being initialized. Yes, I will add the code when I add LP2 support for Tegra114 later. Currently the LP2 code flow only for Tegra30, so it's OK. > > > - movne r3, #FLOW_CTRL_WAITEVENT @ For hotplug > > +flow_ctrl_done: > > + cmp r10, #(TEGRA30 << 8) > > str r3, [r2] > > I wonder whether creating a separate tegra114_cpu_shutdown() wouldn't be > better, to avoid all the runtime conditional code. I personally think the conditional code is OK. And the ARM CPU also had hardware for branch detection also. I had finished some further features for both Tegra30 and Tegra114. And most of the code here can be shared each other at least until now I could see. Once I see if there is a significant difference, then I would prepare maintain the code separately too.
On 05/16/2013 05:14 AM, Joseph Lo wrote: > On Thu, 2013-05-16 at 07:11 +0800, Stephen Warren wrote: >> On 05/15/2013 04:27 AM, Joseph Lo wrote: >>> The Tegra114 is a quad cores SoC. Each core can be hotplugged including >>> CPU0. The hotplug sequence can be controlled by setting event trigger in >>> flow controller. Then the flow controller will take care all the power >>> sequence that include CPU up and down. >>> tst r0, #TEGRA30_POWER_HOTPLUG_SHUTDOWN >>> + beq flow_ctrl_setting_for_lp2 >>> + >>> + /* flow controller set up for hotplug */ >>> + mov r3, #FLOW_CTRL_WAITEVENT @ For hotplug >>> + b flow_ctrl_done >>> +flow_ctrl_setting_for_lp2: >>> + /* flow controller set up for LP2 */ >>> + cmp r10, #(TEGRA30 << 8) >>> moveq r3, #FLOW_CTRL_WAIT_FOR_INTERRUPT @ For LP2 >> >> Do you need e.g. "movne r3, #0" or similar here? Otherwise, I think r3 >> ends up not being initialized. > > Yes, I will add the code when I add LP2 support for Tegra114 later. > Currently the LP2 code flow only for Tegra30, so it's OK. Ah, I see. Can we add the extra move to make sure the register is always initialized now even though the path won't be used. That way, nobody will be confused by this. >>> - movne r3, #FLOW_CTRL_WAITEVENT @ For hotplug >>> +flow_ctrl_done: >>> + cmp r10, #(TEGRA30 << 8) >>> str r3, [r2] >> >> I wonder whether creating a separate tegra114_cpu_shutdown() wouldn't be >> better, to avoid all the runtime conditional code. > > I personally think the conditional code is OK. And the ARM CPU also had > hardware for branch detection also. > > I had finished some further features for both Tegra30 and Tegra114. And > most of the code here can be shared each other at least until now I > could see. Once I see if there is a significant difference, then I would > prepare maintain the code separately too. OK, if the function gets larger with more shared, I guess it's fine. Right now, the conditional-to-non-conditional code ratio is pretty high.
diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile index d011f0a..98b184e 100644 --- a/arch/arm/mach-tegra/Makefile +++ b/arch/arm/mach-tegra/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_HOTPLUG_CPU) += hotplug.o obj-$(CONFIG_TEGRA_PCI) += pcie.o obj-$(CONFIG_ARCH_TEGRA_114_SOC) += tegra114_speedo.o +obj-$(CONFIG_ARCH_TEGRA_114_SOC) += sleep-tegra30.o ifeq ($(CONFIG_CPU_IDLE),y) obj-$(CONFIG_ARCH_TEGRA_114_SOC) += cpuidle-tegra114.o endif diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c index 184914a..d07f152 100644 --- a/arch/arm/mach-tegra/hotplug.c +++ b/arch/arm/mach-tegra/hotplug.c @@ -55,4 +55,6 @@ void __init tegra_hotplug_init(void) tegra_hotplug_shutdown = tegra20_hotplug_shutdown; if (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC) && tegra_chip_id == TEGRA30) tegra_hotplug_shutdown = tegra30_hotplug_shutdown; + if (IS_ENABLED(CONFIG_ARCH_TEGRA_114_SOC) && tegra_chip_id == TEGRA114) + tegra_hotplug_shutdown = tegra30_hotplug_shutdown; } diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S index 893f6b7..5af3a7d 100644 --- a/arch/arm/mach-tegra/reset-handler.S +++ b/arch/arm/mach-tegra/reset-handler.S @@ -38,18 +38,24 @@ * CPU boot vector when restarting the a CPU following * an LP2 transition. Also branched to by LP0 and LP1 resume after * re-enabling sdram. + * + * r6: SoC ID << 8 */ ENTRY(tegra_resume) bl v7_invalidate_l1 cpu_id r0 + tegra_check_soc_id TEGRA114, TEGRA_APB_MISC_BASE, r6, r7 + beq no_cpu0_chk + cmp r0, #0 @ CPU0? THUMB( it ne ) bne cpu_resume @ no +no_cpu0_chk: #ifndef CONFIG_ARCH_TEGRA_2x_SOC /* Are we on Tegra20? */ - tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r7 + cmp r6, #(TEGRA20 << 8) beq 1f @ Yes /* Clear the flow controller flags for this CPU. */ cpu_to_csr_req r1, r0 @@ -186,8 +192,11 @@ __is_not_lp2: * Can only be secondary boot (initial or hotplug) but CPU 0 * cannot be here. */ + cmp r6, #(TEGRA114 <<8) + beq __no_cpu0_chk cmp r10, #0 bleq __die @ CPU0 cannot be here +__no_cpu0_chk: ldr lr, [r12, #RESET_DATA(STARTUP_SECONDARY)] cmp lr, #0 bleq __die @ no secondary startup handler diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S index d29dfcc..ae36fbb 100644 --- a/arch/arm/mach-tegra/sleep-tegra30.S +++ b/arch/arm/mach-tegra/sleep-tegra30.S @@ -19,6 +19,7 @@ #include <asm/assembler.h> #include <asm/asm-offsets.h> +#include "fuse.h" #include "sleep.h" #include "flowctrl.h" @@ -45,12 +46,17 @@ ENDPROC(tegra30_hotplug_shutdown) * and powergates it -- flags (in R0) indicate the request type. * Must never be called for CPU 0. * - * corrupts r0-r4, r12 + * r10 = SoC ID << 8 + * corrupts r0-r4, r10-r12 */ ENTRY(tegra30_cpu_shutdown) cpu_id r3 + tegra_check_soc_id TEGRA30, TEGRA_APB_MISC_VIRT, r10, r11 + bne _no_cpu0_chk @ It's not Tegra30 + cmp r3, #0 moveq pc, lr @ Must never be called for CPU 0 +_no_cpu0_chk: ldr r12, =TEGRA_FLOW_CTRL_VIRT cpu_to_csr_reg r1, r3 @@ -65,7 +71,9 @@ ENTRY(tegra30_cpu_shutdown) movw r12, \ FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG | \ FLOW_CTRL_CSR_ENABLE - mov r4, #(1 << 4) + cmp r10, #(TEGRA30 << 8) + moveq r4, #(1 << 4) @ wfe bitmap + movne r4, #(1 << 8) @ wfi bitmap ARM( orr r12, r12, r4, lsl r3 ) THUMB( lsl r4, r4, r3 ) THUMB( orr r12, r12, r4 ) @@ -79,9 +87,19 @@ delay_1: cpsid a @ disable imprecise aborts. ldr r3, [r1] @ read CSR str r3, [r1] @ clear CSR + tst r0, #TEGRA30_POWER_HOTPLUG_SHUTDOWN + beq flow_ctrl_setting_for_lp2 + + /* flow controller set up for hotplug */ + mov r3, #FLOW_CTRL_WAITEVENT @ For hotplug + b flow_ctrl_done +flow_ctrl_setting_for_lp2: + /* flow controller set up for LP2 */ + cmp r10, #(TEGRA30 << 8) moveq r3, #FLOW_CTRL_WAIT_FOR_INTERRUPT @ For LP2 - movne r3, #FLOW_CTRL_WAITEVENT @ For hotplug +flow_ctrl_done: + cmp r10, #(TEGRA30 << 8) str r3, [r2] ldr r0, [r2] b wfe_war @@ -89,7 +107,8 @@ delay_1: __cpu_reset_again: dsb .align 5 - wfe @ CPU should be power gated here + wfeeq @ CPU should be power gated here + wfine wfe_war: b __cpu_reset_again diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h index c2cac9a..92a3f0c 100644 --- a/arch/arm/mach-tegra/sleep.h +++ b/arch/arm/mach-tegra/sleep.h @@ -25,6 +25,8 @@ + IO_PPSB_VIRT) #define TEGRA_CLK_RESET_VIRT (TEGRA_CLK_RESET_BASE - IO_PPSB_PHYS \ + IO_PPSB_VIRT) +#define TEGRA_APB_MISC_VIRT (TEGRA_APB_MISC_BASE - IO_APB_PHYS \ + + IO_APB_VIRT) #define TEGRA_PMC_VIRT (TEGRA_PMC_BASE - IO_APB_PHYS + IO_APB_VIRT) /* PMC_SCRATCH37-39 and 41 are used for tegra_pen_lock and idle */
The Tegra114 is a quad cores SoC. Each core can be hotplugged including CPU0. The hotplug sequence can be controlled by setting event trigger in flow controller. Then the flow controller will take care all the power sequence that include CPU up and down. Signed-off-by: Joseph Lo <josephl@nvidia.com> --- arch/arm/mach-tegra/Makefile | 1 + arch/arm/mach-tegra/hotplug.c | 2 ++ arch/arm/mach-tegra/reset-handler.S | 11 ++++++++++- arch/arm/mach-tegra/sleep-tegra30.S | 27 +++++++++++++++++++++++---- arch/arm/mach-tegra/sleep.h | 2 ++ 5 files changed, 38 insertions(+), 5 deletions(-)