[PATCH 0/6] ARM: tegra114: add CPU hotplug support

[PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
as CPU core and a enhanced flow controller for CPU power control. So
we need to skip some code that was for Contex-A9 and some other support
code that was for other Tegra SoC chips. Then adding the proper power up
and hot plug control for Tegra114.

Joseph Lo (6):
  ARM: tegra: add an assembly marco to check Tegra SoC ID
  ARM: tegra: skip SCU and PL310 code when CPU is not Cortex-A9
  ARM: tegra: make tegra_resume can work for Tegra114
  ARM: tegra114: add power up sequence for warm boot CPU
  clk: tegra114: implement wait_for_reset and disable_clock for
    tegra_cpu_car_ops
  ARM: tegra114: add CPU hotplug support

 arch/arm/mach-tegra/Makefile        |  1 +
 arch/arm/mach-tegra/flowctrl.h      |  1 +
 arch/arm/mach-tegra/fuse.h          | 22 +++++++++---------
 arch/arm/mach-tegra/hotplug.c       |  2 ++
 arch/arm/mach-tegra/platsmp.c       | 25 ++++++++++++++++++++-
 arch/arm/mach-tegra/reset-handler.S | 45 ++++++++++++++++++++-----------------
 arch/arm/mach-tegra/sleep-tegra30.S | 27 ++++++++++++++++++----
 arch/arm/mach-tegra/sleep.S         |  8 ++++---
 arch/arm/mach-tegra/sleep.h         | 35 ++++++++++++++++++++++++-----
 drivers/clk/tegra/clk-tegra114.c    | 23 ++++++++++++++++++-
 10 files changed, 143 insertions(+), 46 deletions(-)

-- 
1.8.2.2

[PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
as CPU core and a enhanced flow controller for CPU power control. So
we need to skip some code that was for Contex-A9 and some other support
code that was for other Tegra SoC chips. Then adding the proper power up
and hot plug control for Tegra114.

Joseph Lo (6):
  ARM: tegra: add an assembly marco to check Tegra SoC ID
  ARM: tegra: skip SCU and PL310 code when CPU is not Cortex-A9
  ARM: tegra: make tegra_resume can work for Tegra114
  ARM: tegra114: add power up sequence for warm boot CPU
  clk: tegra114: implement wait_for_reset and disable_clock for
    tegra_cpu_car_ops
  ARM: tegra114: add CPU hotplug support

 arch/arm/mach-tegra/Makefile        |  1 +
 arch/arm/mach-tegra/flowctrl.h      |  1 +
 arch/arm/mach-tegra/fuse.h          | 22 +++++++++---------
 arch/arm/mach-tegra/hotplug.c       |  2 ++
 arch/arm/mach-tegra/platsmp.c       | 25 ++++++++++++++++++++-
 arch/arm/mach-tegra/reset-handler.S | 45 ++++++++++++++++++++-----------------
 arch/arm/mach-tegra/sleep-tegra30.S | 27 ++++++++++++++++++----
 arch/arm/mach-tegra/sleep.S         |  8 ++++---
 arch/arm/mach-tegra/sleep.h         | 35 ++++++++++++++++++++++++-----
 drivers/clk/tegra/clk-tegra114.c    | 23 ++++++++++++++++++-
 10 files changed, 143 insertions(+), 46 deletions(-)

-- 
1.8.2.2

[PATCH 1/6] ARM: tegra: add an assembly marco to check Tegra SoC ID

[Joseph Lo]

There are some Tegra SoC ID checking code around the low level assembly
code. Adding a marco to replace them. For the single image to support all
the Tegra series, we may also need the marco in other common code. So we
make it become a marco for the usage.

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/fuse.h          | 22 ++++++++++++----------
 arch/arm/mach-tegra/reset-handler.S | 24 +++++++-----------------
 arch/arm/mach-tegra/sleep.h         |  9 +++++++++
 3 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/arch/arm/mach-tegra/fuse.h b/arch/arm/mach-tegra/fuse.h
index aacc00d..def7968 100644
--- a/arch/arm/mach-tegra/fuse.h
+++ b/arch/arm/mach-tegra/fuse.h
@@ -19,16 +19,6 @@
 #ifndef __MACH_TEGRA_FUSE_H
 #define __MACH_TEGRA_FUSE_H
 
-enum tegra_revision {
-	TEGRA_REVISION_UNKNOWN = 0,
-	TEGRA_REVISION_A01,
-	TEGRA_REVISION_A02,
-	TEGRA_REVISION_A03,
-	TEGRA_REVISION_A03p,
-	TEGRA_REVISION_A04,
-	TEGRA_REVISION_MAX,
-};
-
 #define SKU_ID_T20	8
 #define SKU_ID_T25SE	20
 #define SKU_ID_AP25	23
@@ -40,6 +30,17 @@ enum tegra_revision {
 #define TEGRA30		0x30
 #define TEGRA114	0x35
 
+#ifndef __ASSEMBLY__
+enum tegra_revision {
+	TEGRA_REVISION_UNKNOWN = 0,
+	TEGRA_REVISION_A01,
+	TEGRA_REVISION_A02,
+	TEGRA_REVISION_A03,
+	TEGRA_REVISION_A03p,
+	TEGRA_REVISION_A04,
+	TEGRA_REVISION_MAX,
+};
+
 extern int tegra_sku_id;
 extern int tegra_cpu_process_id;
 extern int tegra_core_process_id;
@@ -72,5 +73,6 @@ void tegra114_init_speedo_data(void);
 #else
 static inline void tegra114_init_speedo_data(void) {}
 #endif
+#endif /* __ASSEMBLY__ */
 
 #endif
diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S
index e6de88a..525f1b9 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -22,11 +22,11 @@
 #include <asm/hardware/cache-l2x0.h>
 
 #include "flowctrl.h"
+#include "fuse.h"
 #include "iomap.h"
 #include "reset.h"
 #include "sleep.h"
 
-#define APB_MISC_GP_HIDREV	0x804
 #define PMC_SCRATCH41	0x140
 
 #define RESET_DATA(x)	((TEGRA_RESET_##x)*4)
@@ -49,10 +49,7 @@ ENTRY(tegra_resume)
 
 #ifdef CONFIG_ARCH_TEGRA_3x_SOC
 	/* Are we on Tegra20? */
-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r0, [r6, #APB_MISC_GP_HIDREV]
-	and	r0, r0, #0xff00
-	cmp	r0, #(0x20 << 8)
+	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r7
 	beq	1f				@ Yes
 	/* Clear the flow controller flags for this CPU. */
 	mov32	r2, TEGRA_FLOW_CTRL_BASE + FLOW_CTRL_CPU0_CSR	@ CPU0 CSR
@@ -115,13 +112,9 @@ ENTRY(__tegra_cpu_reset_handler)
 
 	cpsid	aif, 0x13			@ SVC mode, interrupts disabled
 
-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
-	and	r6, r6, #0xff00
-#ifdef CONFIG_ARCH_TEGRA_2x_SOC
-t20_check:
-	cmp	r6, #(0x20 << 8)
+	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r5
 	bne	after_t20_check
+#ifdef CONFIG_ARCH_TEGRA_2x_SOC
 t20_errata:
 	# Tegra20 is a Cortex-A9 r1p1
 	mrc	p15, 0, r0, c1, c0, 0   @ read system control register
@@ -132,8 +125,8 @@ t20_errata:
 	orr	r0, r0, #1 << 11        @ erratum 751472
 	mcr	p15, 0, r0, c15, c0, 1  @ write diagnostic register
 	b	after_errata
-after_t20_check:
 #endif
+after_t20_check:
 #ifdef CONFIG_ARCH_TEGRA_3x_SOC
 t30_check:
 	cmp	r6, #(0x30 << 8)
@@ -163,7 +156,7 @@ after_errata:
 
 #ifdef CONFIG_ARCH_TEGRA_2x_SOC
 	/* Are we on Tegra20? */
-	cmp	r6, #(0x20 << 8)
+	cmp	r6, #(TEGRA20 << 8)
 	bne	1f
 	/* If not CPU0, don't let CPU0 reset CPU1 now that CPU1 is coming up. */
 	mov32	r5, TEGRA_PMC_BASE
@@ -210,10 +203,7 @@ __die:
 	mov32	r7, TEGRA_CLK_RESET_BASE
 
 	/* Are we on Tegra20? */
-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r0, [r6, #APB_MISC_GP_HIDREV]
-	and	r0, r0, #0xff00
-	cmp	r0, #(0x20 << 8)
+	cmp	r6, #(TEGRA20 << 8)
 	bne	1f
 
 #ifdef CONFIG_ARCH_TEGRA_2x_SOC
diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h
index 2080fb1..7e9c9fe 100644
--- a/arch/arm/mach-tegra/sleep.h
+++ b/arch/arm/mach-tegra/sleep.h
@@ -85,6 +85,15 @@
 	dsb
 .endm
 
+/* Macro to check Tegra revision */
+#define APB_MISC_GP_HIDREV	0x804
+.macro tegra_check_soc_id rev, base, tmp1, tmp2
+	mov32	\tmp2, \base
+	ldr	\tmp1, [\tmp2, #APB_MISC_GP_HIDREV]
+	and	\tmp1, \tmp1, #0xff00
+	cmp	\tmp1, #(\rev << 8)
+.endm
+
 /* Macro to resume & re-enable L2 cache */
 #ifndef L2X0_CTRL_EN
 #define L2X0_CTRL_EN	1
-- 
1.8.2.2

[PATCH 1/6] ARM: tegra: add an assembly marco to check Tegra SoC ID

[Joseph Lo]

There are some Tegra SoC ID checking code around the low level assembly
code. Adding a marco to replace them. For the single image to support all
the Tegra series, we may also need the marco in other common code. So we
make it become a marco for the usage.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/fuse.h          | 22 ++++++++++++----------
 arch/arm/mach-tegra/reset-handler.S | 24 +++++++-----------------
 arch/arm/mach-tegra/sleep.h         |  9 +++++++++
 3 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/arch/arm/mach-tegra/fuse.h b/arch/arm/mach-tegra/fuse.h
index aacc00d..def7968 100644
--- a/arch/arm/mach-tegra/fuse.h
+++ b/arch/arm/mach-tegra/fuse.h
@@ -19,16 +19,6 @@
 #ifndef __MACH_TEGRA_FUSE_H
 #define __MACH_TEGRA_FUSE_H
 
-enum tegra_revision {
-	TEGRA_REVISION_UNKNOWN = 0,
-	TEGRA_REVISION_A01,
-	TEGRA_REVISION_A02,
-	TEGRA_REVISION_A03,
-	TEGRA_REVISION_A03p,
-	TEGRA_REVISION_A04,
-	TEGRA_REVISION_MAX,
-};
-
 #define SKU_ID_T20	8
 #define SKU_ID_T25SE	20
 #define SKU_ID_AP25	23
@@ -40,6 +30,17 @@ enum tegra_revision {
 #define TEGRA30		0x30
 #define TEGRA114	0x35
 
+#ifndef __ASSEMBLY__
+enum tegra_revision {
+	TEGRA_REVISION_UNKNOWN = 0,
+	TEGRA_REVISION_A01,
+	TEGRA_REVISION_A02,
+	TEGRA_REVISION_A03,
+	TEGRA_REVISION_A03p,
+	TEGRA_REVISION_A04,
+	TEGRA_REVISION_MAX,
+};
+
 extern int tegra_sku_id;
 extern int tegra_cpu_process_id;
 extern int tegra_core_process_id;
@@ -72,5 +73,6 @@ void tegra114_init_speedo_data(void);
 #else
 static inline void tegra114_init_speedo_data(void) {}
 #endif
+#endif /* __ASSEMBLY__ */
 
 #endif
diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S
index e6de88a..525f1b9 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -22,11 +22,11 @@
 #include <asm/hardware/cache-l2x0.h>
 
 #include "flowctrl.h"
+#include "fuse.h"
 #include "iomap.h"
 #include "reset.h"
 #include "sleep.h"
 
-#define APB_MISC_GP_HIDREV	0x804
 #define PMC_SCRATCH41	0x140
 
 #define RESET_DATA(x)	((TEGRA_RESET_##x)*4)
@@ -49,10 +49,7 @@ ENTRY(tegra_resume)
 
 #ifdef CONFIG_ARCH_TEGRA_3x_SOC
 	/* Are we on Tegra20? */
-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r0, [r6, #APB_MISC_GP_HIDREV]
-	and	r0, r0, #0xff00
-	cmp	r0, #(0x20 << 8)
+	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r7
 	beq	1f				@ Yes
 	/* Clear the flow controller flags for this CPU. */
 	mov32	r2, TEGRA_FLOW_CTRL_BASE + FLOW_CTRL_CPU0_CSR	@ CPU0 CSR
@@ -115,13 +112,9 @@ ENTRY(__tegra_cpu_reset_handler)
 
 	cpsid	aif, 0x13			@ SVC mode, interrupts disabled
 
-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
-	and	r6, r6, #0xff00
-#ifdef CONFIG_ARCH_TEGRA_2x_SOC
-t20_check:
-	cmp	r6, #(0x20 << 8)
+	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r5
 	bne	after_t20_check
+#ifdef CONFIG_ARCH_TEGRA_2x_SOC
 t20_errata:
 	# Tegra20 is a Cortex-A9 r1p1
 	mrc	p15, 0, r0, c1, c0, 0   @ read system control register
@@ -132,8 +125,8 @@ t20_errata:
 	orr	r0, r0, #1 << 11        @ erratum 751472
 	mcr	p15, 0, r0, c15, c0, 1  @ write diagnostic register
 	b	after_errata
-after_t20_check:
 #endif
+after_t20_check:
 #ifdef CONFIG_ARCH_TEGRA_3x_SOC
 t30_check:
 	cmp	r6, #(0x30 << 8)
@@ -163,7 +156,7 @@ after_errata:
 
 #ifdef CONFIG_ARCH_TEGRA_2x_SOC
 	/* Are we on Tegra20? */
-	cmp	r6, #(0x20 << 8)
+	cmp	r6, #(TEGRA20 << 8)
 	bne	1f
 	/* If not CPU0, don't let CPU0 reset CPU1 now that CPU1 is coming up. */
 	mov32	r5, TEGRA_PMC_BASE
@@ -210,10 +203,7 @@ __die:
 	mov32	r7, TEGRA_CLK_RESET_BASE
 
 	/* Are we on Tegra20? */
-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r0, [r6, #APB_MISC_GP_HIDREV]
-	and	r0, r0, #0xff00
-	cmp	r0, #(0x20 << 8)
+	cmp	r6, #(TEGRA20 << 8)
 	bne	1f
 
 #ifdef CONFIG_ARCH_TEGRA_2x_SOC
diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h
index 2080fb1..7e9c9fe 100644
--- a/arch/arm/mach-tegra/sleep.h
+++ b/arch/arm/mach-tegra/sleep.h
@@ -85,6 +85,15 @@
 	dsb
 .endm
 
+/* Macro to check Tegra revision */
+#define APB_MISC_GP_HIDREV	0x804
+.macro tegra_check_soc_id rev, base, tmp1, tmp2
+	mov32	\tmp2, \base
+	ldr	\tmp1, [\tmp2, #APB_MISC_GP_HIDREV]
+	and	\tmp1, \tmp1, #0xff00
+	cmp	\tmp1, #(\rev << 8)
+.endm
+
 /* Macro to resume & re-enable L2 cache */
 #ifndef L2X0_CTRL_EN
 #define L2X0_CTRL_EN	1
-- 
1.8.2.2

Re: [PATCH 1/6] ARM: tegra: add an assembly marco to check Tegra SoC ID

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> There are some Tegra SoC ID checking code around the low level assembly
> code. Adding a marco to replace them. For the single image to support all
> the Tegra series, we may also need the marco in other common code. So we
> make it become a marco for the usage.

I'm not sure this patch doesn't obfuscate the code too much. The big
issue I see is:

> @@ -115,13 +112,9 @@ ENTRY(__tegra_cpu_reset_handler)
>  
>  	cpsid	aif, 0x13			@ SVC mode, interrupts disabled
>  
> -	mov32	r6, TEGRA_APB_MISC_BASE
> -	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
> -	and	r6, r6, #0xff00
> -#ifdef CONFIG_ARCH_TEGRA_2x_SOC
> -t20_check:
> -	cmp	r6, #(0x20 << 8)
> +	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r5

Here, we replace all the code with the new macro, ...

>  #ifdef CONFIG_ARCH_TEGRA_2x_SOC
>  	/* Are we on Tegra20? */
> -	cmp	r6, #(0x20 << 8)
> +	cmp	r6, #(TEGRA20 << 8)

But here we still have to write out the cmp instructions.

It may be reasonable to create a macro to read/mask/shift the SoC ID,
similar to the existing cpu_id macro, i.e. roughly:

/* Macro to check Tegra revision */
+#define APB_MISC_GP_HIDREV	0x804
+.macro tegra_soc_id base, tmp1, tmp2
+	mov32	\tmp2, \base
+	ldr	\tmp1, [\tmp2, #APB_MISC_GP_HIDREV]
+	and	\tmp1, \tmp1, #0xff00
+.endm

Also, I wonder:

(a) why the need to pass TEGRA_APB_MISC_BASE into the macro; can't the
macro just hard-code that value since it's always the same?

(b) Why need two registers passed to the macro. At least one of the code
segments you've replaced with the macro uses a single register instead:

-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
-	and	r6, r6, #0xff00

Wouldn't that be a better implementation of the macro? I don't think
relying on \tmp2 containing "base" after the macro invocation would be a
good idea, since that's rather like looking inside the black box.

[PATCH 1/6] ARM: tegra: add an assembly marco to check Tegra SoC ID

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> There are some Tegra SoC ID checking code around the low level assembly
> code. Adding a marco to replace them. For the single image to support all
> the Tegra series, we may also need the marco in other common code. So we
> make it become a marco for the usage.

I'm not sure this patch doesn't obfuscate the code too much. The big
issue I see is:

> @@ -115,13 +112,9 @@ ENTRY(__tegra_cpu_reset_handler)
>  
>  	cpsid	aif, 0x13			@ SVC mode, interrupts disabled
>  
> -	mov32	r6, TEGRA_APB_MISC_BASE
> -	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
> -	and	r6, r6, #0xff00
> -#ifdef CONFIG_ARCH_TEGRA_2x_SOC
> -t20_check:
> -	cmp	r6, #(0x20 << 8)
> +	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r5

Here, we replace all the code with the new macro, ...

>  #ifdef CONFIG_ARCH_TEGRA_2x_SOC
>  	/* Are we on Tegra20? */
> -	cmp	r6, #(0x20 << 8)
> +	cmp	r6, #(TEGRA20 << 8)

But here we still have to write out the cmp instructions.

It may be reasonable to create a macro to read/mask/shift the SoC ID,
similar to the existing cpu_id macro, i.e. roughly:

/* Macro to check Tegra revision */
+#define APB_MISC_GP_HIDREV	0x804
+.macro tegra_soc_id base, tmp1, tmp2
+	mov32	\tmp2, \base
+	ldr	\tmp1, [\tmp2, #APB_MISC_GP_HIDREV]
+	and	\tmp1, \tmp1, #0xff00
+.endm

Also, I wonder:

(a) why the need to pass TEGRA_APB_MISC_BASE into the macro; can't the
macro just hard-code that value since it's always the same?

(b) Why need two registers passed to the macro. At least one of the code
segments you've replaced with the macro uses a single register instead:

-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
-	and	r6, r6, #0xff00

Wouldn't that be a better implementation of the macro? I don't think
relying on \tmp2 containing "base" after the macro invocation would be a
good idea, since that's rather like looking inside the black box.

Re: [PATCH 1/6] ARM: tegra: add an assembly marco to check Tegra SoC ID

[Joseph Lo]

On Thu, 2013-05-16 at 06:43 +0800, Stephen Warren wrote:
> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> > There are some Tegra SoC ID checking code around the low level assembly
> > code. Adding a marco to replace them. For the single image to support all
> > the Tegra series, we may also need the marco in other common code. So we
> > make it become a marco for the usage.
> 
> I'm not sure this patch doesn't obfuscate the code too much. The big
> issue I see is:
> 
> > @@ -115,13 +112,9 @@ ENTRY(__tegra_cpu_reset_handler)
> >  
> >  	cpsid	aif, 0x13			@ SVC mode, interrupts disabled
> >  
> > -	mov32	r6, TEGRA_APB_MISC_BASE
> > -	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
> > -	and	r6, r6, #0xff00
> > -#ifdef CONFIG_ARCH_TEGRA_2x_SOC
> > -t20_check:
> > -	cmp	r6, #(0x20 << 8)
> > +	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r5
> 
> Here, we replace all the code with the new macro, ...
> 
> >  #ifdef CONFIG_ARCH_TEGRA_2x_SOC
> >  	/* Are we on Tegra20? */
> > -	cmp	r6, #(0x20 << 8)
> > +	cmp	r6, #(TEGRA20 << 8)
> 
> But here we still have to write out the cmp instructions.
> 
The marco did "cmp" once and move the soc_id into r6. But the flags may
be changed after running some other codes, we need to "cmp" again to get
the correct flag.

> It may be reasonable to create a macro to read/mask/shift the SoC ID,
> similar to the existing cpu_id macro, i.e. roughly:
> 
> /* Macro to check Tegra revision */
> +#define APB_MISC_GP_HIDREV	0x804
> +.macro tegra_soc_id base, tmp1, tmp2
> +	mov32	\tmp2, \base
> +	ldr	\tmp1, [\tmp2, #APB_MISC_GP_HIDREV]
> +	and	\tmp1, \tmp1, #0xff00
> +.endm
> 
> Also, I wonder:
> 
> (a) why the need to pass TEGRA_APB_MISC_BASE into the macro; can't the
> macro just hard-code that value since it's always the same?
> 
Because this marco will be used in virtual address space too, I need to
pass different base addr here.

> (b) Why need two registers passed to the macro. At least one of the code
> segments you've replaced with the macro uses a single register instead:
> 
> -	mov32	r6, TEGRA_APB_MISC_BASE
> -	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
> -	and	r6, r6, #0xff00
> 
> Wouldn't that be a better implementation of the macro? I don't think
> relying on \tmp2 containing "base" after the macro invocation would be a
> good idea, since that's rather like looking inside the black box.

OK. I can remove one tmp register here.

[PATCH 1/6] ARM: tegra: add an assembly marco to check Tegra SoC ID

[Joseph Lo]

On Thu, 2013-05-16 at 06:43 +0800, Stephen Warren wrote:
> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> > There are some Tegra SoC ID checking code around the low level assembly
> > code. Adding a marco to replace them. For the single image to support all
> > the Tegra series, we may also need the marco in other common code. So we
> > make it become a marco for the usage.
> 
> I'm not sure this patch doesn't obfuscate the code too much. The big
> issue I see is:
> 
> > @@ -115,13 +112,9 @@ ENTRY(__tegra_cpu_reset_handler)
> >  
> >  	cpsid	aif, 0x13			@ SVC mode, interrupts disabled
> >  
> > -	mov32	r6, TEGRA_APB_MISC_BASE
> > -	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
> > -	and	r6, r6, #0xff00
> > -#ifdef CONFIG_ARCH_TEGRA_2x_SOC
> > -t20_check:
> > -	cmp	r6, #(0x20 << 8)
> > +	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r5
> 
> Here, we replace all the code with the new macro, ...
> 
> >  #ifdef CONFIG_ARCH_TEGRA_2x_SOC
> >  	/* Are we on Tegra20? */
> > -	cmp	r6, #(0x20 << 8)
> > +	cmp	r6, #(TEGRA20 << 8)
> 
> But here we still have to write out the cmp instructions.
> 
The marco did "cmp" once and move the soc_id into r6. But the flags may
be changed after running some other codes, we need to "cmp" again to get
the correct flag.

> It may be reasonable to create a macro to read/mask/shift the SoC ID,
> similar to the existing cpu_id macro, i.e. roughly:
> 
> /* Macro to check Tegra revision */
> +#define APB_MISC_GP_HIDREV	0x804
> +.macro tegra_soc_id base, tmp1, tmp2
> +	mov32	\tmp2, \base
> +	ldr	\tmp1, [\tmp2, #APB_MISC_GP_HIDREV]
> +	and	\tmp1, \tmp1, #0xff00
> +.endm
> 
> Also, I wonder:
> 
> (a) why the need to pass TEGRA_APB_MISC_BASE into the macro; can't the
> macro just hard-code that value since it's always the same?
> 
Because this marco will be used in virtual address space too, I need to
pass different base addr here.

> (b) Why need two registers passed to the macro. At least one of the code
> segments you've replaced with the macro uses a single register instead:
> 
> -	mov32	r6, TEGRA_APB_MISC_BASE
> -	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
> -	and	r6, r6, #0xff00
> 
> Wouldn't that be a better implementation of the macro? I don't think
> relying on \tmp2 containing "base" after the macro invocation would be a
> good idea, since that's rather like looking inside the black box.

OK. I can remove one tmp register here.

Re: [PATCH 1/6] ARM: tegra: add an assembly marco to check Tegra SoC ID

[Stephen Warren]

On 05/16/2013 04:09 AM, Joseph Lo wrote:
> On Thu, 2013-05-16 at 06:43 +0800, Stephen Warren wrote:
>> On 05/15/2013 04:27 AM, Joseph Lo wrote:
>>> There are some Tegra SoC ID checking code around the low level assembly
>>> code. Adding a marco to replace them. For the single image to support all
>>> the Tegra series, we may also need the marco in other common code. So we
>>> make it become a marco for the usage.
>>
>> I'm not sure this patch doesn't obfuscate the code too much. The big
>> issue I see is:
>>
>>> @@ -115,13 +112,9 @@ ENTRY(__tegra_cpu_reset_handler)
>>>  
>>>  	cpsid	aif, 0x13			@ SVC mode, interrupts disabled
>>>  
>>> -	mov32	r6, TEGRA_APB_MISC_BASE
>>> -	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
>>> -	and	r6, r6, #0xff00
>>> -#ifdef CONFIG_ARCH_TEGRA_2x_SOC
>>> -t20_check:
>>> -	cmp	r6, #(0x20 << 8)
>>> +	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r5
>>
>> Here, we replace all the code with the new macro, ...
>>
>>>  #ifdef CONFIG_ARCH_TEGRA_2x_SOC
>>>  	/* Are we on Tegra20? */
>>> -	cmp	r6, #(0x20 << 8)
>>> +	cmp	r6, #(TEGRA20 << 8)
>>
>> But here we still have to write out the cmp instructions.
>>
> The marco did "cmp" once and move the soc_id into r6. But the flags may
> be changed after running some other codes, we need to "cmp" again to get
> the correct flag.

Yes, I understand that. The issue is that writing the macro didn't
remove all the code, so it seems like it'd be best if the macro only
included the common code, and anything that needs to be repeated at each
test site not be part of the macro.

[PATCH 1/6] ARM: tegra: add an assembly marco to check Tegra SoC ID

[Stephen Warren]

On 05/16/2013 04:09 AM, Joseph Lo wrote:
> On Thu, 2013-05-16 at 06:43 +0800, Stephen Warren wrote:
>> On 05/15/2013 04:27 AM, Joseph Lo wrote:
>>> There are some Tegra SoC ID checking code around the low level assembly
>>> code. Adding a marco to replace them. For the single image to support all
>>> the Tegra series, we may also need the marco in other common code. So we
>>> make it become a marco for the usage.
>>
>> I'm not sure this patch doesn't obfuscate the code too much. The big
>> issue I see is:
>>
>>> @@ -115,13 +112,9 @@ ENTRY(__tegra_cpu_reset_handler)
>>>  
>>>  	cpsid	aif, 0x13			@ SVC mode, interrupts disabled
>>>  
>>> -	mov32	r6, TEGRA_APB_MISC_BASE
>>> -	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
>>> -	and	r6, r6, #0xff00
>>> -#ifdef CONFIG_ARCH_TEGRA_2x_SOC
>>> -t20_check:
>>> -	cmp	r6, #(0x20 << 8)
>>> +	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r5
>>
>> Here, we replace all the code with the new macro, ...
>>
>>>  #ifdef CONFIG_ARCH_TEGRA_2x_SOC
>>>  	/* Are we on Tegra20? */
>>> -	cmp	r6, #(0x20 << 8)
>>> +	cmp	r6, #(TEGRA20 << 8)
>>
>> But here we still have to write out the cmp instructions.
>>
> The marco did "cmp" once and move the soc_id into r6. But the flags may
> be changed after running some other codes, we need to "cmp" again to get
> the correct flag.

Yes, I understand that. The issue is that writing the macro didn't
remove all the code, so it seems like it'd be best if the macro only
included the common code, and anything that needs to be repeated at each
test site not be part of the macro.

[PATCH 2/6] ARM: tegra: skip SCU and PL310 code when CPU is not Cortex-A9

[Joseph Lo]

For supporting single image on all Tegra series, we need to skip some HW
support code for Cortex-A9 only.

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/sleep.S |  8 +++++---
 arch/arm/mach-tegra/sleep.h | 24 ++++++++++++++++++------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-tegra/sleep.S b/arch/arm/mach-tegra/sleep.S
index 364d845..9daaef2 100644
--- a/arch/arm/mach-tegra/sleep.S
+++ b/arch/arm/mach-tegra/sleep.S
@@ -106,9 +106,11 @@ ENTRY(tegra_shut_off_mmu)
 	isb
 #ifdef CONFIG_CACHE_L2X0
 	/* Disable L2 cache */
-	mov32	r4, TEGRA_ARM_PERIF_BASE + 0x3000
-	mov	r5, #0
-	str	r5, [r4, #L2X0_CTRL]
+	check_cpu_part_num 0xc09, r9, r10
+	movweq	r4, #:lower16:(TEGRA_ARM_PERIF_BASE + 0x3000)
+	movteq	r4, #:upper16:(TEGRA_ARM_PERIF_BASE + 0x3000)
+	moveq	r5, #0
+	streq	r5, [r4, #L2X0_CTRL]
 #endif
 	mov	pc, r0
 ENDPROC(tegra_shut_off_mmu)
diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h
index 7e9c9fe..c2cac9a 100644
--- a/arch/arm/mach-tegra/sleep.h
+++ b/arch/arm/mach-tegra/sleep.h
@@ -70,19 +70,31 @@
 	movt	\reg, #:upper16:\val
 .endm
 
+/* Marco to check CPU part num */
+.macro check_cpu_part_num part_num, tmp1, tmp2
+	mrc	p15, 0, \tmp1, c0, c0, 0
+	ubfx	\tmp1, \tmp1, #4, #12
+	mov32	\tmp2, \part_num
+	cmp	\tmp1, \tmp2
+.endm
+
 /* Macro to exit SMP coherency. */
 .macro exit_smp, tmp1, tmp2
 	mrc	p15, 0, \tmp1, c1, c0, 1	@ ACTLR
 	bic	\tmp1, \tmp1, #(1<<6) | (1<<0)	@ clear ACTLR.SMP | ACTLR.FW
 	mcr	p15, 0, \tmp1, c1, c0, 1	@ ACTLR
 	isb
-	cpu_id	\tmp1
-	mov	\tmp1, \tmp1, lsl #2
-	mov	\tmp2, #0xf
-	mov	\tmp2, \tmp2, lsl \tmp1
-	mov32	\tmp1, TEGRA_ARM_PERIF_VIRT + 0xC
-	str	\tmp2, [\tmp1]			@ invalidate SCU tags for CPU
+#ifdef CONFIG_HAVE_ARM_SCU
+	check_cpu_part_num 0xc09, \tmp1, \tmp2
+	mrceq	p15, 0, \tmp1, c0, c0, 5
+	andeq	\tmp1, \tmp1, #0xF
+	moveq	\tmp1, \tmp1, lsl #2
+	moveq	\tmp2, #0xf
+	moveq	\tmp2, \tmp2, lsl \tmp1
+	ldreq	\tmp1, =(TEGRA_ARM_PERIF_VIRT + 0xC)
+	streq	\tmp2, [\tmp1]			@ invalidate SCU tags for CPU
 	dsb
+#endif
 .endm
 
 /* Macro to check Tegra revision */
-- 
1.8.2.2

[PATCH 2/6] ARM: tegra: skip SCU and PL310 code when CPU is not Cortex-A9

[Joseph Lo]

For supporting single image on all Tegra series, we need to skip some HW
support code for Cortex-A9 only.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/sleep.S |  8 +++++---
 arch/arm/mach-tegra/sleep.h | 24 ++++++++++++++++++------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-tegra/sleep.S b/arch/arm/mach-tegra/sleep.S
index 364d845..9daaef2 100644
--- a/arch/arm/mach-tegra/sleep.S
+++ b/arch/arm/mach-tegra/sleep.S
@@ -106,9 +106,11 @@ ENTRY(tegra_shut_off_mmu)
 	isb
 #ifdef CONFIG_CACHE_L2X0
 	/* Disable L2 cache */
-	mov32	r4, TEGRA_ARM_PERIF_BASE + 0x3000
-	mov	r5, #0
-	str	r5, [r4, #L2X0_CTRL]
+	check_cpu_part_num 0xc09, r9, r10
+	movweq	r4, #:lower16:(TEGRA_ARM_PERIF_BASE + 0x3000)
+	movteq	r4, #:upper16:(TEGRA_ARM_PERIF_BASE + 0x3000)
+	moveq	r5, #0
+	streq	r5, [r4, #L2X0_CTRL]
 #endif
 	mov	pc, r0
 ENDPROC(tegra_shut_off_mmu)
diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h
index 7e9c9fe..c2cac9a 100644
--- a/arch/arm/mach-tegra/sleep.h
+++ b/arch/arm/mach-tegra/sleep.h
@@ -70,19 +70,31 @@
 	movt	\reg, #:upper16:\val
 .endm
 
+/* Marco to check CPU part num */
+.macro check_cpu_part_num part_num, tmp1, tmp2
+	mrc	p15, 0, \tmp1, c0, c0, 0
+	ubfx	\tmp1, \tmp1, #4, #12
+	mov32	\tmp2, \part_num
+	cmp	\tmp1, \tmp2
+.endm
+
 /* Macro to exit SMP coherency. */
 .macro exit_smp, tmp1, tmp2
 	mrc	p15, 0, \tmp1, c1, c0, 1	@ ACTLR
 	bic	\tmp1, \tmp1, #(1<<6) | (1<<0)	@ clear ACTLR.SMP | ACTLR.FW
 	mcr	p15, 0, \tmp1, c1, c0, 1	@ ACTLR
 	isb
-	cpu_id	\tmp1
-	mov	\tmp1, \tmp1, lsl #2
-	mov	\tmp2, #0xf
-	mov	\tmp2, \tmp2, lsl \tmp1
-	mov32	\tmp1, TEGRA_ARM_PERIF_VIRT + 0xC
-	str	\tmp2, [\tmp1]			@ invalidate SCU tags for CPU
+#ifdef CONFIG_HAVE_ARM_SCU
+	check_cpu_part_num 0xc09, \tmp1, \tmp2
+	mrceq	p15, 0, \tmp1, c0, c0, 5
+	andeq	\tmp1, \tmp1, #0xF
+	moveq	\tmp1, \tmp1, lsl #2
+	moveq	\tmp2, #0xf
+	moveq	\tmp2, \tmp2, lsl \tmp1
+	ldreq	\tmp1, =(TEGRA_ARM_PERIF_VIRT + 0xC)
+	streq	\tmp2, [\tmp1]			@ invalidate SCU tags for CPU
 	dsb
+#endif
 .endm
 
 /* Macro to check Tegra revision */
-- 
1.8.2.2

Re: [PATCH 2/6] ARM: tegra: skip SCU and PL310 code when CPU is not Cortex-A9

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> For supporting single image on all Tegra series, we need to skip some HW
> support code for Cortex-A9 only.

> diff --git a/arch/arm/mach-tegra/sleep.S b/arch/arm/mach-tegra/sleep.S

> +	check_cpu_part_num 0xc09, r9, r10
> +	movweq	r4, #:lower16:(TEGRA_ARM_PERIF_BASE + 0x3000)
> +	movteq	r4, #:upper16:(TEGRA_ARM_PERIF_BASE + 0x3000)
> +	moveq	r5, #0
> +	streq	r5, [r4, #L2X0_CTRL]

Do those conditional instructions need a Thumb iteq wrapped around them
in order to compile in Thumb2 mode? Same comment for the other change,
although IIRC iteq only supports 4 instructions at a time, so maybe a
branch would be better there. Using branches might also reduce the size
of the diff, and make the change more obvious.

[PATCH 2/6] ARM: tegra: skip SCU and PL310 code when CPU is not Cortex-A9

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> For supporting single image on all Tegra series, we need to skip some HW
> support code for Cortex-A9 only.

> diff --git a/arch/arm/mach-tegra/sleep.S b/arch/arm/mach-tegra/sleep.S

> +	check_cpu_part_num 0xc09, r9, r10
> +	movweq	r4, #:lower16:(TEGRA_ARM_PERIF_BASE + 0x3000)
> +	movteq	r4, #:upper16:(TEGRA_ARM_PERIF_BASE + 0x3000)
> +	moveq	r5, #0
> +	streq	r5, [r4, #L2X0_CTRL]

Do those conditional instructions need a Thumb iteq wrapped around them
in order to compile in Thumb2 mode? Same comment for the other change,
although IIRC iteq only supports 4 instructions at a time, so maybe a
branch would be better there. Using branches might also reduce the size
of the diff, and make the change more obvious.

Re: [PATCH 2/6] ARM: tegra: skip SCU and PL310 code when CPU is not Cortex-A9

[Joseph Lo]

On Thu, 2013-05-16 at 06:48 +0800, Stephen Warren wrote:
> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> > For supporting single image on all Tegra series, we need to skip some HW
> > support code for Cortex-A9 only.
> 
> > diff --git a/arch/arm/mach-tegra/sleep.S b/arch/arm/mach-tegra/sleep.S
> 
> > +	check_cpu_part_num 0xc09, r9, r10
> > +	movweq	r4, #:lower16:(TEGRA_ARM_PERIF_BASE + 0x3000)
> > +	movteq	r4, #:upper16:(TEGRA_ARM_PERIF_BASE + 0x3000)
> > +	moveq	r5, #0
> > +	streq	r5, [r4, #L2X0_CTRL]
> 
> Do those conditional instructions need a Thumb iteq wrapped around them
> in order to compile in Thumb2 mode? Same comment for the other change,
> although IIRC iteq only supports 4 instructions at a time, so maybe a
> branch would be better there. Using branches might also reduce the size
> of the diff, and make the change more obvious.

IIRC, I had tested this patch series with THUMB2_KERNEL enabled. It's
OK. I can double confirm again later.

Thanks,
Joseph

[PATCH 2/6] ARM: tegra: skip SCU and PL310 code when CPU is not Cortex-A9

[Joseph Lo]

On Thu, 2013-05-16 at 06:48 +0800, Stephen Warren wrote:
> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> > For supporting single image on all Tegra series, we need to skip some HW
> > support code for Cortex-A9 only.
> 
> > diff --git a/arch/arm/mach-tegra/sleep.S b/arch/arm/mach-tegra/sleep.S
> 
> > +	check_cpu_part_num 0xc09, r9, r10
> > +	movweq	r4, #:lower16:(TEGRA_ARM_PERIF_BASE + 0x3000)
> > +	movteq	r4, #:upper16:(TEGRA_ARM_PERIF_BASE + 0x3000)
> > +	moveq	r5, #0
> > +	streq	r5, [r4, #L2X0_CTRL]
> 
> Do those conditional instructions need a Thumb iteq wrapped around them
> in order to compile in Thumb2 mode? Same comment for the other change,
> although IIRC iteq only supports 4 instructions at a time, so maybe a
> branch would be better there. Using branches might also reduce the size
> of the diff, and make the change more obvious.

IIRC, I had tested this patch series with THUMB2_KERNEL enabled. It's
OK. I can double confirm again later.

Thanks,
Joseph

[PATCH 3/6] ARM: tegra: make tegra_resume can work for Tegra114

[Joseph Lo]

Tegra114 is an ARM Cortex-A15 based SoC and some of the flow controller
hardware behavior and configurations are different with other Tegra series.
We fix the common resume function of tegra_resume to make it can work on
Tegra114 by checking SoC ID. And also checking CPU primary part number to
isolate the support code for Cortex A9 and A15.

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/reset-handler.S | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S
index 525f1b9..893f6b7 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -47,22 +47,25 @@ ENTRY(tegra_resume)
  THUMB(	it	ne )
 	bne	cpu_resume			@ no
 
-#ifdef CONFIG_ARCH_TEGRA_3x_SOC
+#ifndef CONFIG_ARCH_TEGRA_2x_SOC
 	/* Are we on Tegra20? */
 	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r7
 	beq	1f				@ Yes
 	/* Clear the flow controller flags for this CPU. */
-	mov32	r2, TEGRA_FLOW_CTRL_BASE + FLOW_CTRL_CPU0_CSR	@ CPU0 CSR
-	ldr	r1, [r2]
+	cpu_to_csr_req r1, r0
+	mov32	r2, TEGRA_FLOW_CTRL_BASE
+	ldr	r1, [r2, r1]
 	/* Clear event & intr flag */
 	orr	r1, r1, \
 		#FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG
-	movw	r0, #0x0FFD	@ enable, cluster_switch, immed, & bitmaps
+	movw	r0, #0x3FFD	@ enable, cluster_switch, immed, & bitmaps
 	bic	r1, r1, r0
 	str	r1, [r2]
 1:
 #endif
 
+	check_cpu_part_num 0xc09, r8, r9
+	bne	not_ca9
 #ifdef CONFIG_HAVE_ARM_SCU
 	/* enable SCU */
 	mov32	r0, TEGRA_ARM_PERIF_BASE
@@ -73,6 +76,7 @@ ENTRY(tegra_resume)
 
 	/* L2 cache resume & re-enable */
 	l2_cache_resume r0, r1, r2, l2x0_saved_regs_addr
+not_ca9:
 
 	b	cpu_resume
 ENDPROC(tegra_resume)
-- 
1.8.2.2

[PATCH 3/6] ARM: tegra: make tegra_resume can work for Tegra114

[Joseph Lo]

Tegra114 is an ARM Cortex-A15 based SoC and some of the flow controller
hardware behavior and configurations are different with other Tegra series.
We fix the common resume function of tegra_resume to make it can work on
Tegra114 by checking SoC ID. And also checking CPU primary part number to
isolate the support code for Cortex A9 and A15.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/reset-handler.S | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S
index 525f1b9..893f6b7 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -47,22 +47,25 @@ ENTRY(tegra_resume)
  THUMB(	it	ne )
 	bne	cpu_resume			@ no
 
-#ifdef CONFIG_ARCH_TEGRA_3x_SOC
+#ifndef CONFIG_ARCH_TEGRA_2x_SOC
 	/* Are we on Tegra20? */
 	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r7
 	beq	1f				@ Yes
 	/* Clear the flow controller flags for this CPU. */
-	mov32	r2, TEGRA_FLOW_CTRL_BASE + FLOW_CTRL_CPU0_CSR	@ CPU0 CSR
-	ldr	r1, [r2]
+	cpu_to_csr_req r1, r0
+	mov32	r2, TEGRA_FLOW_CTRL_BASE
+	ldr	r1, [r2, r1]
 	/* Clear event & intr flag */
 	orr	r1, r1, \
 		#FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG
-	movw	r0, #0x0FFD	@ enable, cluster_switch, immed, & bitmaps
+	movw	r0, #0x3FFD	@ enable, cluster_switch, immed, & bitmaps
 	bic	r1, r1, r0
 	str	r1, [r2]
 1:
 #endif
 
+	check_cpu_part_num 0xc09, r8, r9
+	bne	not_ca9
 #ifdef CONFIG_HAVE_ARM_SCU
 	/* enable SCU */
 	mov32	r0, TEGRA_ARM_PERIF_BASE
@@ -73,6 +76,7 @@ ENTRY(tegra_resume)
 
 	/* L2 cache resume & re-enable */
 	l2_cache_resume r0, r1, r2, l2x0_saved_regs_addr
+not_ca9:
 
 	b	cpu_resume
 ENDPROC(tegra_resume)
-- 
1.8.2.2

Re: [PATCH 3/6] ARM: tegra: make tegra_resume can work for Tegra114

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> Tegra114 is an ARM Cortex-A15 based SoC and some of the flow controller

I don't think the CPU type is the issue here. The issue is simply that
Tegra114's flow controller is different. Mentioning the CPU type seems
misleading.

> hardware behavior and configurations are different with other Tegra series.
> We fix the common resume function of tegra_resume to make it can work on
> Tegra114 by checking SoC ID. And also checking CPU primary part number to
> isolate the support code for Cortex A9 and A15.

> diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S

> -#ifdef CONFIG_ARCH_TEGRA_3x_SOC
> +#ifndef CONFIG_ARCH_TEGRA_2x_SOC
>  	/* Are we on Tegra20? */
>  	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r7
>  	beq	1f				@ Yes
>  	/* Clear the flow controller flags for this CPU. */
> -	mov32	r2, TEGRA_FLOW_CTRL_BASE + FLOW_CTRL_CPU0_CSR	@ CPU0 CSR
> -	ldr	r1, [r2]
> +	cpu_to_csr_req r1, r0

Where is cpu_to_csr_req defined? grep can't find it in next-20130513,
and I don't see it added in this series.

This presumably changes behaviour on Tegra30; will this cause problems?

> +	mov32	r2, TEGRA_FLOW_CTRL_BASE
> +	ldr	r1, [r2, r1]
>  	/* Clear event & intr flag */
>  	orr	r1, r1, \
>  		#FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG
> -	movw	r0, #0x0FFD	@ enable, cluster_switch, immed, & bitmaps
> +	movw	r0, #0x3FFD	@ enable, cluster_switch, immed, & bitmaps

What does this change do; does the commend need updating to describe the
new bits that are set?

[PATCH 3/6] ARM: tegra: make tegra_resume can work for Tegra114

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> Tegra114 is an ARM Cortex-A15 based SoC and some of the flow controller

I don't think the CPU type is the issue here. The issue is simply that
Tegra114's flow controller is different. Mentioning the CPU type seems
misleading.

> hardware behavior and configurations are different with other Tegra series.
> We fix the common resume function of tegra_resume to make it can work on
> Tegra114 by checking SoC ID. And also checking CPU primary part number to
> isolate the support code for Cortex A9 and A15.

> diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S

> -#ifdef CONFIG_ARCH_TEGRA_3x_SOC
> +#ifndef CONFIG_ARCH_TEGRA_2x_SOC
>  	/* Are we on Tegra20? */
>  	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r7
>  	beq	1f				@ Yes
>  	/* Clear the flow controller flags for this CPU. */
> -	mov32	r2, TEGRA_FLOW_CTRL_BASE + FLOW_CTRL_CPU0_CSR	@ CPU0 CSR
> -	ldr	r1, [r2]
> +	cpu_to_csr_req r1, r0

Where is cpu_to_csr_req defined? grep can't find it in next-20130513,
and I don't see it added in this series.

This presumably changes behaviour on Tegra30; will this cause problems?

> +	mov32	r2, TEGRA_FLOW_CTRL_BASE
> +	ldr	r1, [r2, r1]
>  	/* Clear event & intr flag */
>  	orr	r1, r1, \
>  		#FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG
> -	movw	r0, #0x0FFD	@ enable, cluster_switch, immed, & bitmaps
> +	movw	r0, #0x3FFD	@ enable, cluster_switch, immed, & bitmaps

What does this change do; does the commend need updating to describe the
new bits that are set?

Re: [PATCH 3/6] ARM: tegra: make tegra_resume can work for Tegra114

[Joseph Lo]

On Thu, 2013-05-16 at 06:57 +0800, Stephen Warren wrote:
> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> > Tegra114 is an ARM Cortex-A15 based SoC and some of the flow controller
> 
> I don't think the CPU type is the issue here. The issue is simply that
> Tegra114's flow controller is different. Mentioning the CPU type seems
> misleading.
> 
> > hardware behavior and configurations are different with other Tegra series.
> > We fix the common resume function of tegra_resume to make it can work on
> > Tegra114 by checking SoC ID. And also checking CPU primary part number to
> > isolate the support code for Cortex A9 and A15.
> 
> > diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S
> 
> > -#ifdef CONFIG_ARCH_TEGRA_3x_SOC
> > +#ifndef CONFIG_ARCH_TEGRA_2x_SOC
> >  	/* Are we on Tegra20? */
> >  	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r7
> >  	beq	1f				@ Yes
> >  	/* Clear the flow controller flags for this CPU. */
> > -	mov32	r2, TEGRA_FLOW_CTRL_BASE + FLOW_CTRL_CPU0_CSR	@ CPU0 CSR
> > -	ldr	r1, [r2]
> > +	cpu_to_csr_req r1, r0
> 
> Where is cpu_to_csr_req defined? grep can't find it in next-20130513,
> and I don't see it added in this series.
> 
The cpu_to_csr_reg macro was defined in "sleep.h". I was used to
translate the bit offset of CPU number in CSR register. The code here
was used for CPU0 on Tegra30 only before. Because the Tegra114 had more
advance CPU power control. Each Core need run through here when power
gate resume.

> This presumably changes behaviour on Tegra30; will this cause problems?
> 
No, I had verified on Tegra20, Tegra30 and Tegra114 of this series.

> > +	mov32	r2, TEGRA_FLOW_CTRL_BASE
> > +	ldr	r1, [r2, r1]
> >  	/* Clear event & intr flag */
> >  	orr	r1, r1, \
> >  		#FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG
> > -	movw	r0, #0x0FFD	@ enable, cluster_switch, immed, & bitmaps
> > +	movw	r0, #0x3FFD	@ enable, cluster_switch, immed, & bitmaps
> 
> What does this change do; does the commend need updating to describe the
> new bits that are set?
It clear the some extensions for CPU power control on Tegra114.

[PATCH 3/6] ARM: tegra: make tegra_resume can work for Tegra114

[Joseph Lo]

On Thu, 2013-05-16 at 06:57 +0800, Stephen Warren wrote:
> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> > Tegra114 is an ARM Cortex-A15 based SoC and some of the flow controller
> 
> I don't think the CPU type is the issue here. The issue is simply that
> Tegra114's flow controller is different. Mentioning the CPU type seems
> misleading.
> 
> > hardware behavior and configurations are different with other Tegra series.
> > We fix the common resume function of tegra_resume to make it can work on
> > Tegra114 by checking SoC ID. And also checking CPU primary part number to
> > isolate the support code for Cortex A9 and A15.
> 
> > diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S
> 
> > -#ifdef CONFIG_ARCH_TEGRA_3x_SOC
> > +#ifndef CONFIG_ARCH_TEGRA_2x_SOC
> >  	/* Are we on Tegra20? */
> >  	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r7
> >  	beq	1f				@ Yes
> >  	/* Clear the flow controller flags for this CPU. */
> > -	mov32	r2, TEGRA_FLOW_CTRL_BASE + FLOW_CTRL_CPU0_CSR	@ CPU0 CSR
> > -	ldr	r1, [r2]
> > +	cpu_to_csr_req r1, r0
> 
> Where is cpu_to_csr_req defined? grep can't find it in next-20130513,
> and I don't see it added in this series.
> 
The cpu_to_csr_reg macro was defined in "sleep.h". I was used to
translate the bit offset of CPU number in CSR register. The code here
was used for CPU0 on Tegra30 only before. Because the Tegra114 had more
advance CPU power control. Each Core need run through here when power
gate resume.

> This presumably changes behaviour on Tegra30; will this cause problems?
> 
No, I had verified on Tegra20, Tegra30 and Tegra114 of this series.

> > +	mov32	r2, TEGRA_FLOW_CTRL_BASE
> > +	ldr	r1, [r2, r1]
> >  	/* Clear event & intr flag */
> >  	orr	r1, r1, \
> >  		#FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG
> > -	movw	r0, #0x0FFD	@ enable, cluster_switch, immed, & bitmaps
> > +	movw	r0, #0x3FFD	@ enable, cluster_switch, immed, & bitmaps
> 
> What does this change do; does the commend need updating to describe the
> new bits that are set?
It clear the some extensions for CPU power control on Tegra114.

Re: [PATCH 3/6] ARM: tegra: make tegra_resume can work for Tegra114

[Stephen Warren]

On 05/16/2013 04:35 AM, Joseph Lo wrote:
> On Thu, 2013-05-16 at 06:57 +0800, Stephen Warren wrote:
>> On 05/15/2013 04:27 AM, Joseph Lo wrote:

>>> diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S

>>> +	cpu_to_csr_req r1, r0
>>
>> Where is cpu_to_csr_req defined? grep can't find it in next-20130513,
>> and I don't see it added in this series.
>
> The cpu_to_csr_reg macro was defined in "sleep.h".

So it is. I fail at grep.

> I was used to
> translate the bit offset of CPU number in CSR register. The code here
> was used for CPU0 on Tegra30 only before.
...
>> This presumably changes behaviour on Tegra30; will this cause problems?

Ah, OK.

>>> +	mov32	r2, TEGRA_FLOW_CTRL_BASE
>>> +	ldr	r1, [r2, r1]
>>>  	/* Clear event & intr flag */
>>>  	orr	r1, r1, \
>>>  		#FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG
>>> -	movw	r0, #0x0FFD	@ enable, cluster_switch, immed, & bitmaps
>>> +	movw	r0, #0x3FFD	@ enable, cluster_switch, immed, & bitmaps
>>
>> What does this change do; does the commend need updating to describe the
>> new bits that are set?
>
> It clear the some extensions for CPU power control on Tegra114.

OK; I was implying that perhaps the comment needs updating?

[PATCH 3/6] ARM: tegra: make tegra_resume can work for Tegra114

[Stephen Warren]

On 05/16/2013 04:35 AM, Joseph Lo wrote:
> On Thu, 2013-05-16 at 06:57 +0800, Stephen Warren wrote:
>> On 05/15/2013 04:27 AM, Joseph Lo wrote:

>>> diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S

>>> +	cpu_to_csr_req r1, r0
>>
>> Where is cpu_to_csr_req defined? grep can't find it in next-20130513,
>> and I don't see it added in this series.
>
> The cpu_to_csr_reg macro was defined in "sleep.h".

So it is. I fail at grep.

> I was used to
> translate the bit offset of CPU number in CSR register. The code here
> was used for CPU0 on Tegra30 only before.
...
>> This presumably changes behaviour on Tegra30; will this cause problems?

Ah, OK.

>>> +	mov32	r2, TEGRA_FLOW_CTRL_BASE
>>> +	ldr	r1, [r2, r1]
>>>  	/* Clear event & intr flag */
>>>  	orr	r1, r1, \
>>>  		#FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG
>>> -	movw	r0, #0x0FFD	@ enable, cluster_switch, immed, & bitmaps
>>> +	movw	r0, #0x3FFD	@ enable, cluster_switch, immed, & bitmaps
>>
>> What does this change do; does the commend need updating to describe the
>> new bits that are set?
>
> It clear the some extensions for CPU power control on Tegra114.

OK; I was implying that perhaps the comment needs updating?

[PATCH 4/6] ARM: tegra114: add power up sequence for warm boot CPU

[Joseph Lo]

For Tegra114, once the CPUs were powered up by PMC in cold boot flow. The
flow controller will maintain the power state and control power sequence
for each CPU by setting event trigger (e.g. CPU hotplug ,idle and
suspend power down/up).

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/flowctrl.h |  1 +
 arch/arm/mach-tegra/platsmp.c  | 25 ++++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/flowctrl.h b/arch/arm/mach-tegra/flowctrl.h
index 67eab56..7a29bae 100644
--- a/arch/arm/mach-tegra/flowctrl.h
+++ b/arch/arm/mach-tegra/flowctrl.h
@@ -25,6 +25,7 @@
 #define FLOW_CTRL_WAITEVENT		(2 << 29)
 #define FLOW_CTRL_WAIT_FOR_INTERRUPT	(4 << 29)
 #define FLOW_CTRL_JTAG_RESUME		(1 << 28)
+#define FLOW_CTRL_SCLK_RESUME		(1 << 27)
 #define FLOW_CTRL_HALT_CPU_IRQ		(1 << 10)
 #define	FLOW_CTRL_HALT_CPU_FIQ		(1 << 8)
 #define FLOW_CTRL_CPU0_CSR		0x8
diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index fad4226..554aedc 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -140,8 +140,31 @@ remove_clamps:
 
 static int tegra114_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
+	int ret = 0;
+
 	cpu = cpu_logical_map(cpu);
-	return tegra_pmc_cpu_power_on(cpu);
+
+	if (cpumask_test_cpu(cpu, &tegra_cpu_init_mask)) {
+		/*
+		 * Warm boot flow
+		 * The flow controller in charge of the power state and
+		 * control for each CPU.
+		 */
+		/* set SCLK as event trigger for flow controller */
+		flowctrl_write_cpu_csr(cpu, 1);
+		flowctrl_write_cpu_halt(cpu,
+				FLOW_CTRL_WAITEVENT | FLOW_CTRL_SCLK_RESUME);
+	} else {
+		/*
+		 * Cold boot flow
+		 * The CPU is powered up by toggling PMC directly. It will
+		 * also initial power state in flow controller. After that,
+		 * the CPU's power state is maintained by flow controller.
+		 */
+		ret = tegra_pmc_cpu_power_on(cpu);
+	}
+
+	return ret;
 }
 
 static int __cpuinit tegra_boot_secondary(unsigned int cpu,
-- 
1.8.2.2

[PATCH 4/6] ARM: tegra114: add power up sequence for warm boot CPU

[Joseph Lo]

For Tegra114, once the CPUs were powered up by PMC in cold boot flow. The
flow controller will maintain the power state and control power sequence
for each CPU by setting event trigger (e.g. CPU hotplug ,idle and
suspend power down/up).

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/flowctrl.h |  1 +
 arch/arm/mach-tegra/platsmp.c  | 25 ++++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/flowctrl.h b/arch/arm/mach-tegra/flowctrl.h
index 67eab56..7a29bae 100644
--- a/arch/arm/mach-tegra/flowctrl.h
+++ b/arch/arm/mach-tegra/flowctrl.h
@@ -25,6 +25,7 @@
 #define FLOW_CTRL_WAITEVENT		(2 << 29)
 #define FLOW_CTRL_WAIT_FOR_INTERRUPT	(4 << 29)
 #define FLOW_CTRL_JTAG_RESUME		(1 << 28)
+#define FLOW_CTRL_SCLK_RESUME		(1 << 27)
 #define FLOW_CTRL_HALT_CPU_IRQ		(1 << 10)
 #define	FLOW_CTRL_HALT_CPU_FIQ		(1 << 8)
 #define FLOW_CTRL_CPU0_CSR		0x8
diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index fad4226..554aedc 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -140,8 +140,31 @@ remove_clamps:
 
 static int tegra114_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
+	int ret = 0;
+
 	cpu = cpu_logical_map(cpu);
-	return tegra_pmc_cpu_power_on(cpu);
+
+	if (cpumask_test_cpu(cpu, &tegra_cpu_init_mask)) {
+		/*
+		 * Warm boot flow
+		 * The flow controller in charge of the power state and
+		 * control for each CPU.
+		 */
+		/* set SCLK as event trigger for flow controller */
+		flowctrl_write_cpu_csr(cpu, 1);
+		flowctrl_write_cpu_halt(cpu,
+				FLOW_CTRL_WAITEVENT | FLOW_CTRL_SCLK_RESUME);
+	} else {
+		/*
+		 * Cold boot flow
+		 * The CPU is powered up by toggling PMC directly. It will
+		 * also initial power state in flow controller. After that,
+		 * the CPU's power state is maintained by flow controller.
+		 */
+		ret = tegra_pmc_cpu_power_on(cpu);
+	}
+
+	return ret;
 }
 
 static int __cpuinit tegra_boot_secondary(unsigned int cpu,
-- 
1.8.2.2

[PATCH 5/6] clk: tegra114: implement wait_for_reset and disable_clock for tegra_cpu_car_ops

[Joseph Lo]

The conventional CPU hotplug sequence on the other Tegra chips, we will also
clock gate the CPU in tegra_cpu_kill() after the CPU was power gated. For
Tegra114, the flow controller will clock gate the CPU after the power down
sequence. But we still need to implement a empty function for disable_clock
to avoid kernel warning message.

Cc: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/clk/tegra/clk-tegra114.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
index d78e16e..40d939d 100644
--- a/drivers/clk/tegra/clk-tegra114.c
+++ b/drivers/clk/tegra/clk-tegra114.c
@@ -250,6 +250,9 @@
 #define CLK_SOURCE_XUSB_DEV_SRC 0x60c
 #define CLK_SOURCE_EMC 0x19c
 
+/* Tegra CPU clock and reset control regs */
+#define CLK_RST_CONTROLLER_CPU_CMPLX_STATUS	0x470
+
 static int periph_clk_enb_refcnt[CLK_OUT_ENB_NUM * 32];
 
 static void __iomem *clk_base;
@@ -2000,7 +2003,25 @@ static __init void tegra114_periph_clk_init(void __iomem *clk_base)
 	}
 }
 
-static struct tegra_cpu_car_ops tegra114_cpu_car_ops;
+/* Tegra114 CPU clock and reset control functions */
+static void tegra114_wait_cpu_in_reset(u32 cpu)
+{
+	unsigned int reg;
+
+	do {
+		reg = readl(clk_base + CLK_RST_CONTROLLER_CPU_CMPLX_STATUS);
+		cpu_relax();
+	} while (!(reg & (1 << cpu)));  /* check CPU been reset or not */
+}
+static void tegra114_disable_cpu_clock(u32 cpu)
+{
+	/* flow controller would take care in the power sequence. */
+}
+
+static struct tegra_cpu_car_ops tegra114_cpu_car_ops = {
+	.wait_for_reset	= tegra114_wait_cpu_in_reset,
+	.disable_clock	= tegra114_disable_cpu_clock,
+};
 
 static const struct of_device_id pmc_match[] __initconst = {
 	{ .compatible = "nvidia,tegra114-pmc" },
-- 
1.8.2.2

[PATCH 5/6] clk: tegra114: implement wait_for_reset and disable_clock for tegra_cpu_car_ops

[Joseph Lo]

The conventional CPU hotplug sequence on the other Tegra chips, we will also
clock gate the CPU in tegra_cpu_kill() after the CPU was power gated. For
Tegra114, the flow controller will clock gate the CPU after the power down
sequence. But we still need to implement a empty function for disable_clock
to avoid kernel warning message.

Cc: Mike Turquette <mturquette@linaro.org>
Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 drivers/clk/tegra/clk-tegra114.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
index d78e16e..40d939d 100644
--- a/drivers/clk/tegra/clk-tegra114.c
+++ b/drivers/clk/tegra/clk-tegra114.c
@@ -250,6 +250,9 @@
 #define CLK_SOURCE_XUSB_DEV_SRC 0x60c
 #define CLK_SOURCE_EMC 0x19c
 
+/* Tegra CPU clock and reset control regs */
+#define CLK_RST_CONTROLLER_CPU_CMPLX_STATUS	0x470
+
 static int periph_clk_enb_refcnt[CLK_OUT_ENB_NUM * 32];
 
 static void __iomem *clk_base;
@@ -2000,7 +2003,25 @@ static __init void tegra114_periph_clk_init(void __iomem *clk_base)
 	}
 }
 
-static struct tegra_cpu_car_ops tegra114_cpu_car_ops;
+/* Tegra114 CPU clock and reset control functions */
+static void tegra114_wait_cpu_in_reset(u32 cpu)
+{
+	unsigned int reg;
+
+	do {
+		reg = readl(clk_base + CLK_RST_CONTROLLER_CPU_CMPLX_STATUS);
+		cpu_relax();
+	} while (!(reg & (1 << cpu)));  /* check CPU been reset or not */
+}
+static void tegra114_disable_cpu_clock(u32 cpu)
+{
+	/* flow controller would take care in the power sequence. */
+}
+
+static struct tegra_cpu_car_ops tegra114_cpu_car_ops = {
+	.wait_for_reset	= tegra114_wait_cpu_in_reset,
+	.disable_clock	= tegra114_disable_cpu_clock,
+};
 
 static const struct of_device_id pmc_match[] __initconst = {
 	{ .compatible = "nvidia,tegra114-pmc" },
-- 
1.8.2.2

Re: [PATCH 5/6] clk: tegra114: implement wait_for_reset and disable_clock for tegra_cpu_car_ops

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> The conventional CPU hotplug sequence on the other Tegra chips, we will also
> clock gate the CPU in tegra_cpu_kill() after the CPU was power gated. For
> Tegra114, the flow controller will clock gate the CPU after the power down
> sequence. But we still need to implement a empty function for disable_clock
> to avoid kernel warning message.

I assume I need to apply this patch in the Tegra tree along with all the
other patches in this series? i.e. Mike can't apply it on its own to the
clk tree?

If so, I'll wait for an ack from Mike.

[PATCH 5/6] clk: tegra114: implement wait_for_reset and disable_clock for tegra_cpu_car_ops

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> The conventional CPU hotplug sequence on the other Tegra chips, we will also
> clock gate the CPU in tegra_cpu_kill() after the CPU was power gated. For
> Tegra114, the flow controller will clock gate the CPU after the power down
> sequence. But we still need to implement a empty function for disable_clock
> to avoid kernel warning message.

I assume I need to apply this patch in the Tegra tree along with all the
other patches in this series? i.e. Mike can't apply it on its own to the
clk tree?

If so, I'll wait for an ack from Mike.

Re: [PATCH 5/6] clk: tegra114: implement wait_for_reset and disable_clock for tegra_cpu_car_ops

[Mike Turquette]

Quoting Joseph Lo (2013-05-15 03:27:23)
> The conventional CPU hotplug sequence on the other Tegra chips, we will also
> clock gate the CPU in tegra_cpu_kill() after the CPU was power gated. For
> Tegra114, the flow controller will clock gate the CPU after the power down
> sequence. But we still need to implement a empty function for disable_clock
> to avoid kernel warning message.
> 
> Cc: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Acked-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> ---
>  drivers/clk/tegra/clk-tegra114.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
> index d78e16e..40d939d 100644
> --- a/drivers/clk/tegra/clk-tegra114.c
> +++ b/drivers/clk/tegra/clk-tegra114.c
> @@ -250,6 +250,9 @@
>  #define CLK_SOURCE_XUSB_DEV_SRC 0x60c
>  #define CLK_SOURCE_EMC 0x19c
>  
> +/* Tegra CPU clock and reset control regs */
> +#define CLK_RST_CONTROLLER_CPU_CMPLX_STATUS    0x470
> +
>  static int periph_clk_enb_refcnt[CLK_OUT_ENB_NUM * 32];
>  
>  static void __iomem *clk_base;
> @@ -2000,7 +2003,25 @@ static __init void tegra114_periph_clk_init(void __iomem *clk_base)
>         }
>  }
>  
> -static struct tegra_cpu_car_ops tegra114_cpu_car_ops;
> +/* Tegra114 CPU clock and reset control functions */
> +static void tegra114_wait_cpu_in_reset(u32 cpu)
> +{
> +       unsigned int reg;
> +
> +       do {
> +               reg = readl(clk_base + CLK_RST_CONTROLLER_CPU_CMPLX_STATUS);
> +               cpu_relax();
> +       } while (!(reg & (1 << cpu)));  /* check CPU been reset or not */
> +}
> +static void tegra114_disable_cpu_clock(u32 cpu)
> +{
> +       /* flow controller would take care in the power sequence. */
> +}
> +
> +static struct tegra_cpu_car_ops tegra114_cpu_car_ops = {
> +       .wait_for_reset = tegra114_wait_cpu_in_reset,
> +       .disable_clock  = tegra114_disable_cpu_clock,
> +};
>  
>  static const struct of_device_id pmc_match[] __initconst = {
>         { .compatible = "nvidia,tegra114-pmc" },
> -- 
> 1.8.2.2

[PATCH 5/6] clk: tegra114: implement wait_for_reset and disable_clock for tegra_cpu_car_ops

[Mike Turquette]

Quoting Joseph Lo (2013-05-15 03:27:23)
> The conventional CPU hotplug sequence on the other Tegra chips, we will also
> clock gate the CPU in tegra_cpu_kill() after the CPU was power gated. For
> Tegra114, the flow controller will clock gate the CPU after the power down
> sequence. But we still need to implement a empty function for disable_clock
> to avoid kernel warning message.
> 
> Cc: Mike Turquette <mturquette@linaro.org>
> Signed-off-by: Joseph Lo <josephl@nvidia.com>

Acked-by: Mike Turquette <mturquette@linaro.org>

> ---
>  drivers/clk/tegra/clk-tegra114.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
> index d78e16e..40d939d 100644
> --- a/drivers/clk/tegra/clk-tegra114.c
> +++ b/drivers/clk/tegra/clk-tegra114.c
> @@ -250,6 +250,9 @@
>  #define CLK_SOURCE_XUSB_DEV_SRC 0x60c
>  #define CLK_SOURCE_EMC 0x19c
>  
> +/* Tegra CPU clock and reset control regs */
> +#define CLK_RST_CONTROLLER_CPU_CMPLX_STATUS    0x470
> +
>  static int periph_clk_enb_refcnt[CLK_OUT_ENB_NUM * 32];
>  
>  static void __iomem *clk_base;
> @@ -2000,7 +2003,25 @@ static __init void tegra114_periph_clk_init(void __iomem *clk_base)
>         }
>  }
>  
> -static struct tegra_cpu_car_ops tegra114_cpu_car_ops;
> +/* Tegra114 CPU clock and reset control functions */
> +static void tegra114_wait_cpu_in_reset(u32 cpu)
> +{
> +       unsigned int reg;
> +
> +       do {
> +               reg = readl(clk_base + CLK_RST_CONTROLLER_CPU_CMPLX_STATUS);
> +               cpu_relax();
> +       } while (!(reg & (1 << cpu)));  /* check CPU been reset or not */
> +}
> +static void tegra114_disable_cpu_clock(u32 cpu)
> +{
> +       /* flow controller would take care in the power sequence. */
> +}
> +
> +static struct tegra_cpu_car_ops tegra114_cpu_car_ops = {
> +       .wait_for_reset = tegra114_wait_cpu_in_reset,
> +       .disable_clock  = tegra114_disable_cpu_clock,
> +};
>  
>  static const struct of_device_id pmc_match[] __initconst = {
>         { .compatible = "nvidia,tegra114-pmc" },
> -- 
> 1.8.2.2

[PATCH 6/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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(-)

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 */
-- 
1.8.2.2

[PATCH 6/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

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(-)

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 */
-- 
1.8.2.2

Re: [PATCH 6/6] ARM: tegra114: add CPU hotplug support

[Stephen Warren]

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.

[PATCH 6/6] ARM: tegra114: add CPU hotplug support

[Stephen Warren]

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.

Re: [PATCH 6/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

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.

[PATCH 6/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

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.

Re: [PATCH 6/6] ARM: tegra114: add CPU hotplug support

[Stephen Warren]

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.

[PATCH 6/6] ARM: tegra114: add CPU hotplug support

[Stephen Warren]

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.

Re: [PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
> as CPU core and a enhanced flow controller for CPU power control. So
> we need to skip some code that was for Contex-A9 and some other support
> code that was for other Tegra SoC chips. Then adding the proper power up
> and hot plug control for Tegra114.

This series mostly works OK, but I see one problem: I can't hotunplug
CPU0, which the commit descriptions and code changes imply I should be
able to do:

root@localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
-bash: echo: write error: Operation not permitted

[PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Stephen Warren]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
> as CPU core and a enhanced flow controller for CPU power control. So
> we need to skip some code that was for Contex-A9 and some other support
> code that was for other Tegra SoC chips. Then adding the proper power up
> and hot plug control for Tegra114.

This series mostly works OK, but I see one problem: I can't hotunplug
CPU0, which the commit descriptions and code changes imply I should be
able to do:

root at localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
-bash: echo: write error: Operation not permitted

Re: [PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

On Thu, 2013-05-16 at 07:38 +0800, Stephen Warren wrote:
> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> > Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
> > as CPU core and a enhanced flow controller for CPU power control. So
> > we need to skip some code that was for Contex-A9 and some other support
> > code that was for other Tegra SoC chips. Then adding the proper power up
> > and hot plug control for Tegra114.
> 
> This series mostly works OK, but I see one problem: I can't hotunplug
> CPU0, which the commit descriptions and code changes imply I should be
> able to do:
> 
> root@localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
> -bash: echo: write error: Operation not permitted

I want to provide this function originally. But I found the
tegra_cpu_disable() was removed recently. It was replaced by the common
cpu_disable() function that didn't allow CPU0 to be un-plugged.

But I had verified the CPU0 is OK to be un-plugged on the older
linux-next branch that tegra_cpu_disable() watn't removed yet.

Do you want me to add them back to support this function for Tegra114?

Thanks,
Joseph

[PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

On Thu, 2013-05-16 at 07:38 +0800, Stephen Warren wrote:
> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> > Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
> > as CPU core and a enhanced flow controller for CPU power control. So
> > we need to skip some code that was for Contex-A9 and some other support
> > code that was for other Tegra SoC chips. Then adding the proper power up
> > and hot plug control for Tegra114.
> 
> This series mostly works OK, but I see one problem: I can't hotunplug
> CPU0, which the commit descriptions and code changes imply I should be
> able to do:
> 
> root at localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
> -bash: echo: write error: Operation not permitted

I want to provide this function originally. But I found the
tegra_cpu_disable() was removed recently. It was replaced by the common
cpu_disable() function that didn't allow CPU0 to be un-plugged.

But I had verified the CPU0 is OK to be un-plugged on the older
linux-next branch that tegra_cpu_disable() watn't removed yet.

Do you want me to add them back to support this function for Tegra114?

Thanks,
Joseph

Re: [PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Stephen Warren]

On 05/16/2013 03:53 AM, Joseph Lo wrote:
> On Thu, 2013-05-16 at 07:38 +0800, Stephen Warren wrote:
>> On 05/15/2013 04:27 AM, Joseph Lo wrote:
>>> Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
>>> as CPU core and a enhanced flow controller for CPU power control. So
>>> we need to skip some code that was for Contex-A9 and some other support
>>> code that was for other Tegra SoC chips. Then adding the proper power up
>>> and hot plug control for Tegra114.
>>
>> This series mostly works OK, but I see one problem: I can't hotunplug
>> CPU0, which the commit descriptions and code changes imply I should be
>> able to do:
>>
>> root@localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
>> -bash: echo: write error: Operation not permitted
> 
> I want to provide this function originally. But I found the
> tegra_cpu_disable() was removed recently. It was replaced by the common
> cpu_disable() function that didn't allow CPU0 to be un-plugged.

Is there a specific reason for that; is there some problem in the core
ARM code that implies CPU0 should never be disabled?

> But I had verified the CPU0 is OK to be un-plugged on the older
> linux-next branch that tegra_cpu_disable() watn't removed yet.
> 
> Do you want me to add them back to support this function for Tegra114?

If there is a problem removing CPU0 in the core code, the functionality
of this series is OK, although it's probably worth removing the parts
that attempt to make CPU0 hot-unpluggable to reduce the diff size.

If there's no problem removing CPU0, it'd be good to make it work,
although that could be done in followon patches.

[PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Stephen Warren]

On 05/16/2013 03:53 AM, Joseph Lo wrote:
> On Thu, 2013-05-16 at 07:38 +0800, Stephen Warren wrote:
>> On 05/15/2013 04:27 AM, Joseph Lo wrote:
>>> Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
>>> as CPU core and a enhanced flow controller for CPU power control. So
>>> we need to skip some code that was for Contex-A9 and some other support
>>> code that was for other Tegra SoC chips. Then adding the proper power up
>>> and hot plug control for Tegra114.
>>
>> This series mostly works OK, but I see one problem: I can't hotunplug
>> CPU0, which the commit descriptions and code changes imply I should be
>> able to do:
>>
>> root at localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
>> -bash: echo: write error: Operation not permitted
> 
> I want to provide this function originally. But I found the
> tegra_cpu_disable() was removed recently. It was replaced by the common
> cpu_disable() function that didn't allow CPU0 to be un-plugged.

Is there a specific reason for that; is there some problem in the core
ARM code that implies CPU0 should never be disabled?

> But I had verified the CPU0 is OK to be un-plugged on the older
> linux-next branch that tegra_cpu_disable() watn't removed yet.
> 
> Do you want me to add them back to support this function for Tegra114?

If there is a problem removing CPU0 in the core code, the functionality
of this series is OK, although it's probably worth removing the parts
that attempt to make CPU0 hot-unpluggable to reduce the diff size.

If there's no problem removing CPU0, it'd be good to make it work,
although that could be done in followon patches.

Re: [PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

On Fri, 2013-05-17 at 02:19 +0800, Stephen Warren wrote:
> On 05/16/2013 03:53 AM, Joseph Lo wrote:
> > On Thu, 2013-05-16 at 07:38 +0800, Stephen Warren wrote:
> >> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> >>> Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
> >>> as CPU core and a enhanced flow controller for CPU power control. So
> >>> we need to skip some code that was for Contex-A9 and some other support
> >>> code that was for other Tegra SoC chips. Then adding the proper power up
> >>> and hot plug control for Tegra114.
> >>
> >> This series mostly works OK, but I see one problem: I can't hotunplug
> >> CPU0, which the commit descriptions and code changes imply I should be
> >> able to do:
> >>
> >> root@localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
> >> -bash: echo: write error: Operation not permitted
> > 
> > I want to provide this function originally. But I found the
> > tegra_cpu_disable() was removed recently. It was replaced by the common
> > cpu_disable() function that didn't allow CPU0 to be un-plugged.
> 
> Is there a specific reason for that; is there some problem in the core
> ARM code that implies CPU0 should never be disabled?
> 
I think the CPU0 is the boot CPU for most of the case. We always need at
least one CPU online. That assumes to be the boot CPU.

> > But I had verified the CPU0 is OK to be un-plugged on the older
> > linux-next branch that tegra_cpu_disable() watn't removed yet.
> > 
> > Do you want me to add them back to support this function for Tegra114?
> 
> If there is a problem removing CPU0 in the core code, the functionality
> of this series is OK, although it's probably worth removing the parts
> that attempt to make CPU0 hot-unpluggable to reduce the diff size.
> 
> If there's no problem removing CPU0, it'd be good to make it work,
> although that could be done in followon patches.

OK. Thanks.

[PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Joseph Lo]

On Fri, 2013-05-17 at 02:19 +0800, Stephen Warren wrote:
> On 05/16/2013 03:53 AM, Joseph Lo wrote:
> > On Thu, 2013-05-16 at 07:38 +0800, Stephen Warren wrote:
> >> On 05/15/2013 04:27 AM, Joseph Lo wrote:
> >>> Tegra 114 is different with other Tegra SoC chips. It using ARM Cortex-A15
> >>> as CPU core and a enhanced flow controller for CPU power control. So
> >>> we need to skip some code that was for Contex-A9 and some other support
> >>> code that was for other Tegra SoC chips. Then adding the proper power up
> >>> and hot plug control for Tegra114.
> >>
> >> This series mostly works OK, but I see one problem: I can't hotunplug
> >> CPU0, which the commit descriptions and code changes imply I should be
> >> able to do:
> >>
> >> root at localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
> >> -bash: echo: write error: Operation not permitted
> > 
> > I want to provide this function originally. But I found the
> > tegra_cpu_disable() was removed recently. It was replaced by the common
> > cpu_disable() function that didn't allow CPU0 to be un-plugged.
> 
> Is there a specific reason for that; is there some problem in the core
> ARM code that implies CPU0 should never be disabled?
> 
I think the CPU0 is the boot CPU for most of the case. We always need at
least one CPU online. That assumes to be the boot CPU.

> > But I had verified the CPU0 is OK to be un-plugged on the older
> > linux-next branch that tegra_cpu_disable() watn't removed yet.
> > 
> > Do you want me to add them back to support this function for Tegra114?
> 
> If there is a problem removing CPU0 in the core code, the functionality
> of this series is OK, although it's probably worth removing the parts
> that attempt to make CPU0 hot-unpluggable to reduce the diff size.
> 
> If there's no problem removing CPU0, it'd be good to make it work,
> although that could be done in followon patches.

OK. Thanks.

Re: [PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Russell King - ARM Linux]

On Thu, May 16, 2013 at 12:19:48PM -0600, Stephen Warren wrote:
> Is there a specific reason for that; is there some problem in the core
> ARM code that implies CPU0 should never be disabled?

No.  This is the code I removed:

-int tegra_cpu_disable(unsigned int cpu)
-{
-       /*
-        * we don't allow CPU 0 to be shutdown (it is still too special
-        * e.g. clock tick interrupts)
-        */
-       return cpu == 0 ? -EPERM : 0;
-}

This gets called from here:

static int platform_cpu_disable(unsigned int cpu)
{
        if (smp_ops.cpu_disable)
                return smp_ops.cpu_disable(cpu);

        /*
         * By default, allow disabling all CPUs except the first one,
         * since this is special on a lot of platforms, e.g. because
         * of clock tick interrupts.
         */
        return cpu == 0 ? -EPERM : 0;
}

Notice the useless utterly nature of tegra_cpu_disable() given that it's
doing _precisely_ the same thing as the common code.  It might as well
not even exist - and it doesn't for that very reason.  Never provide
implementations for methods which are copies of the default action if
no method is provided.

[PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Russell King - ARM Linux]

On Thu, May 16, 2013 at 12:19:48PM -0600, Stephen Warren wrote:
> Is there a specific reason for that; is there some problem in the core
> ARM code that implies CPU0 should never be disabled?

No.  This is the code I removed:

-int tegra_cpu_disable(unsigned int cpu)
-{
-       /*
-        * we don't allow CPU 0 to be shutdown (it is still too special
-        * e.g. clock tick interrupts)
-        */
-       return cpu == 0 ? -EPERM : 0;
-}

This gets called from here:

static int platform_cpu_disable(unsigned int cpu)
{
        if (smp_ops.cpu_disable)
                return smp_ops.cpu_disable(cpu);

        /*
         * By default, allow disabling all CPUs except the first one,
         * since this is special on a lot of platforms, e.g. because
         * of clock tick interrupts.
         */
        return cpu == 0 ? -EPERM : 0;
}

Notice the useless utterly nature of tegra_cpu_disable() given that it's
doing _precisely_ the same thing as the common code.  It might as well
not even exist - and it doesn't for that very reason.  Never provide
implementations for methods which are copies of the default action if
no method is provided.

Re: [PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Russell King - ARM Linux]

On Thu, May 16, 2013 at 05:53:39PM +0800, Joseph Lo wrote:
> On Thu, 2013-05-16 at 07:38 +0800, Stephen Warren wrote:
> > This series mostly works OK, but I see one problem: I can't hotunplug
> > CPU0, which the commit descriptions and code changes imply I should be
> > able to do:
> > 
> > root@localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
> > -bash: echo: write error: Operation not permitted
> 
> I want to provide this function originally. But I found the
> tegra_cpu_disable() was removed recently. It was replaced by the common
> cpu_disable() function that didn't allow CPU0 to be un-plugged.

It got removed for the simple reason that the implementation of that
function was exactly the same as the default path in cpu_disable().
If you have a need to do something else other than the default, then
provide the function (and *only* provide it if you need to do something
different.)

However, if the default suffices, do not provide extra code even if you
later intend to change the function.  Only add the function when you
need to do something else.

I see too much of this "I'll add this function just becase, and maybe
I'll change it later" crap getting into the kernel; this needs to stop.
Things like providing an empty function for every single method in a
struct even through the behaviour would be to do nothing had that
function not been provided.  So whenever I see this, I remove the
offending functions if I'm touching that code.

[PATCH 0/6] ARM: tegra114: add CPU hotplug support

[Russell King - ARM Linux]

On Thu, May 16, 2013 at 05:53:39PM +0800, Joseph Lo wrote:
> On Thu, 2013-05-16 at 07:38 +0800, Stephen Warren wrote:
> > This series mostly works OK, but I see one problem: I can't hotunplug
> > CPU0, which the commit descriptions and code changes imply I should be
> > able to do:
> > 
> > root at localhost:~# echo 0 > /sys/devices/system/cpu/cpu0/online
> > -bash: echo: write error: Operation not permitted
> 
> I want to provide this function originally. But I found the
> tegra_cpu_disable() was removed recently. It was replaced by the common
> cpu_disable() function that didn't allow CPU0 to be un-plugged.

It got removed for the simple reason that the implementation of that
function was exactly the same as the default path in cpu_disable().
If you have a need to do something else other than the default, then
provide the function (and *only* provide it if you need to do something
different.)

However, if the default suffices, do not provide extra code even if you
later intend to change the function.  Only add the function when you
need to do something else.

I see too much of this "I'll add this function just becase, and maybe
I'll change it later" crap getting into the kernel; this needs to stop.
Things like providing an empty function for every single method in a
struct even through the behaviour would be to do nothing had that
function not been provided.  So whenever I see this, I remove the
offending functions if I'm touching that code.