[PATCH V2 0/3] ARM: tegra114: cpuidle: add power down state

[PATCH V2 0/3] ARM: tegra114: cpuidle: add power down state

[Joseph Lo]

This series introduce CPU core power down state for CPU idle. When CPU go
into this state, it saves it's context and needs a proper configuration
in flow controller to power gate the CPU when CPU runs into WFI
instruction. And the CPU also needs to set the IRQ as CPU power down idle
wake up event in flow controller.

To prevent race conditions and ensure proper interrupt routing on
Cortex-A15 CPUs when they are power-gated, add a CPU PM notifier
call-back to reprogram the GIC CPU interface on PM entry. The
GIC CPU interface will be reset back to its normal state by
the common GIC CPU PM exit callback when the CPU wakes up.

This series depends on the series of "ARM: tegra: cpuidle: some clean up
for CPUidle driver".

V2:
* clean up the CPUidle driver to make it more generic

Joseph Lo (3):
  ARM: tegra114: Reprogram GIC CPU interface to bypass IRQ on CPU PM
    entry
  ARM: tegra114: add low level support for CPU idle powered-down mode
  ARM: tegra114: cpuidle: add powered-down state

 arch/arm/mach-tegra/cpuidle-tegra114.c | 51 +++++++++++++++++++++++++++++++++-
 arch/arm/mach-tegra/flowctrl.h         |  2 ++
 arch/arm/mach-tegra/irq.c              | 40 ++++++++++++++++++++++++++
 arch/arm/mach-tegra/sleep-tegra30.S    |  2 ++
 4 files changed, 94 insertions(+), 1 deletion(-)

-- 
1.8.3

[PATCH V2 0/3] ARM: tegra114: cpuidle: add power down state

[Joseph Lo]

This series introduce CPU core power down state for CPU idle. When CPU go
into this state, it saves it's context and needs a proper configuration
in flow controller to power gate the CPU when CPU runs into WFI
instruction. And the CPU also needs to set the IRQ as CPU power down idle
wake up event in flow controller.

To prevent race conditions and ensure proper interrupt routing on
Cortex-A15 CPUs when they are power-gated, add a CPU PM notifier
call-back to reprogram the GIC CPU interface on PM entry. The
GIC CPU interface will be reset back to its normal state by
the common GIC CPU PM exit callback when the CPU wakes up.

This series depends on the series of "ARM: tegra: cpuidle: some clean up
for CPUidle driver".

V2:
* clean up the CPUidle driver to make it more generic

Joseph Lo (3):
  ARM: tegra114: Reprogram GIC CPU interface to bypass IRQ on CPU PM
    entry
  ARM: tegra114: add low level support for CPU idle powered-down mode
  ARM: tegra114: cpuidle: add powered-down state

 arch/arm/mach-tegra/cpuidle-tegra114.c | 51 +++++++++++++++++++++++++++++++++-
 arch/arm/mach-tegra/flowctrl.h         |  2 ++
 arch/arm/mach-tegra/irq.c              | 40 ++++++++++++++++++++++++++
 arch/arm/mach-tegra/sleep-tegra30.S    |  2 ++
 4 files changed, 94 insertions(+), 1 deletion(-)

-- 
1.8.3

[PATCH V2 1/3] ARM: tegra114: Reprogram GIC CPU interface to bypass IRQ on CPU PM entry

[Joseph Lo]

There is a difference between GICv1 and v2 when CPU in power management
mode (aka CPU power down on Tegra). For GICv1, IRQ/FIQ interrupt lines
going to CPU are same lines which are also used for wake-interrupt.
Therefore, we cannot disable the GIC CPU interface if we need to use same
interrupts for CPU wake purpose. This creates a race condition for CPU
power off entry. Also, in GICv1, disabling GICv1 CPU interface puts GICv1
into bypass mode such that incoming legacy IRQ/FIQ are sent to CPU, which
means disabling GIC CPU interface doesn't really disable IRQ/FIQ to CPU.

GICv2 provides a wake IRQ/FIQ (for wake-event purpose), which are not
disabled by GIC CPU interface. This is done by adding a bypass override
capability when the interrupts are disabled at the CPU interface. To
support this, there are four bits about IRQ/FIQ BypassDisable in CPU
interface Control Register. When the IRQ/FIQ not being driver by the
CPU interface, each interrupt output signal can be deasserted rather
than being driven by the legacy interrupt input.

So the wake-event can be used as wakeup signals to SoC (system power
controller).

To prevent race conditions and ensure proper interrupt routing on
Cortex-A15 CPUs when they are power-gated, add a CPU PM notifier
call-back to reprogram the GIC CPU interface on PM entry. The
GIC CPU interface will be reset back to its normal state by
the common GIC CPU PM exit callback when the CPU wakes up.

Based on the work by: Scott Williams <scwilliams-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
V2:
* no change
---
 arch/arm/mach-tegra/irq.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index 0de4eed..1a74d56 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -18,10 +18,12 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/cpu_pm.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/irqchip/arm-gic.h>
 #include <linux/syscore_ops.h>
 
@@ -65,6 +67,7 @@ static u32 cpu_ier[TEGRA_MAX_NUM_ICTLRS];
 static u32 cpu_iep[TEGRA_MAX_NUM_ICTLRS];
 
 static u32 ictlr_wake_mask[TEGRA_MAX_NUM_ICTLRS];
+static void __iomem *tegra_gic_cpu_base;
 #endif
 
 bool tegra_pending_sgi(void)
@@ -213,8 +216,43 @@ int tegra_legacy_irq_syscore_init(void)
 
 	return 0;
 }
+
+static int tegra_gic_notifier(struct notifier_block *self,
+			      unsigned long cmd, void *v)
+{
+	switch (cmd) {
+	case CPU_PM_ENTER:
+		writel_relaxed(0x1E0, tegra_gic_cpu_base + GIC_CPU_CTRL);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block tegra_gic_notifier_block = {
+	.notifier_call = tegra_gic_notifier,
+};
+
+static const struct of_device_id tegra114_dt_gic_match[] __initconst = {
+	{ .compatible = "arm,cortex-a15-gic" },
+	{ }
+};
+
+static void tegra114_gic_cpu_pm_registration(void)
+{
+	struct device_node *dn;
+
+	dn = of_find_matching_node(NULL, tegra114_dt_gic_match);
+	if (!dn)
+		return;
+
+	tegra_gic_cpu_base = of_iomap(dn, 1);
+
+	cpu_pm_register_notifier(&tegra_gic_notifier_block);
+}
 #else
 #define tegra_set_wake NULL
+static void tegra114_gic_cpu_pm_registration(void) { }
 #endif
 
 void __init tegra_init_irq(void)
@@ -252,4 +290,6 @@ void __init tegra_init_irq(void)
 	if (!of_have_populated_dt())
 		gic_init(0, 29, distbase,
 			IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
+
+	tegra114_gic_cpu_pm_registration();
 }
-- 
1.8.3

[PATCH V2 1/3] ARM: tegra114: Reprogram GIC CPU interface to bypass IRQ on CPU PM entry

[Joseph Lo]

There is a difference between GICv1 and v2 when CPU in power management
mode (aka CPU power down on Tegra). For GICv1, IRQ/FIQ interrupt lines
going to CPU are same lines which are also used for wake-interrupt.
Therefore, we cannot disable the GIC CPU interface if we need to use same
interrupts for CPU wake purpose. This creates a race condition for CPU
power off entry. Also, in GICv1, disabling GICv1 CPU interface puts GICv1
into bypass mode such that incoming legacy IRQ/FIQ are sent to CPU, which
means disabling GIC CPU interface doesn't really disable IRQ/FIQ to CPU.

GICv2 provides a wake IRQ/FIQ (for wake-event purpose), which are not
disabled by GIC CPU interface. This is done by adding a bypass override
capability when the interrupts are disabled at the CPU interface. To
support this, there are four bits about IRQ/FIQ BypassDisable in CPU
interface Control Register. When the IRQ/FIQ not being driver by the
CPU interface, each interrupt output signal can be deasserted rather
than being driven by the legacy interrupt input.

So the wake-event can be used as wakeup signals to SoC (system power
controller).

To prevent race conditions and ensure proper interrupt routing on
Cortex-A15 CPUs when they are power-gated, add a CPU PM notifier
call-back to reprogram the GIC CPU interface on PM entry. The
GIC CPU interface will be reset back to its normal state by
the common GIC CPU PM exit callback when the CPU wakes up.

Based on the work by: Scott Williams <scwilliams@nvidia.com>

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
V2:
* no change
---
 arch/arm/mach-tegra/irq.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index 0de4eed..1a74d56 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -18,10 +18,12 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/cpu_pm.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/irqchip/arm-gic.h>
 #include <linux/syscore_ops.h>
 
@@ -65,6 +67,7 @@ static u32 cpu_ier[TEGRA_MAX_NUM_ICTLRS];
 static u32 cpu_iep[TEGRA_MAX_NUM_ICTLRS];
 
 static u32 ictlr_wake_mask[TEGRA_MAX_NUM_ICTLRS];
+static void __iomem *tegra_gic_cpu_base;
 #endif
 
 bool tegra_pending_sgi(void)
@@ -213,8 +216,43 @@ int tegra_legacy_irq_syscore_init(void)
 
 	return 0;
 }
+
+static int tegra_gic_notifier(struct notifier_block *self,
+			      unsigned long cmd, void *v)
+{
+	switch (cmd) {
+	case CPU_PM_ENTER:
+		writel_relaxed(0x1E0, tegra_gic_cpu_base + GIC_CPU_CTRL);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block tegra_gic_notifier_block = {
+	.notifier_call = tegra_gic_notifier,
+};
+
+static const struct of_device_id tegra114_dt_gic_match[] __initconst = {
+	{ .compatible = "arm,cortex-a15-gic" },
+	{ }
+};
+
+static void tegra114_gic_cpu_pm_registration(void)
+{
+	struct device_node *dn;
+
+	dn = of_find_matching_node(NULL, tegra114_dt_gic_match);
+	if (!dn)
+		return;
+
+	tegra_gic_cpu_base = of_iomap(dn, 1);
+
+	cpu_pm_register_notifier(&tegra_gic_notifier_block);
+}
 #else
 #define tegra_set_wake NULL
+static void tegra114_gic_cpu_pm_registration(void) { }
 #endif
 
 void __init tegra_init_irq(void)
@@ -252,4 +290,6 @@ void __init tegra_init_irq(void)
 	if (!of_have_populated_dt())
 		gic_init(0, 29, distbase,
 			IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
+
+	tegra114_gic_cpu_pm_registration();
 }
-- 
1.8.3

[PATCH V2 2/3] ARM: tegra114: add low level support for CPU idle powered-down mode

[Joseph Lo]

The flow controller would take care the power sequence when CPU idle in
powered-down mode. It powered gate the CPU when CPU runs into WFI
instruction. And wake up the CPU when event be triggered.

The sequence is below.
* setting wfi bitmap for the CPU as the halt event in the
  FLOW_CTRL_CPU_HALT_REG to monitor the CPU running into WFI,then power
  gate it
* setting IRQ and FIQ as wake up event to wake up CPU when event triggered

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
V2:
* no change
---
 arch/arm/mach-tegra/flowctrl.h      | 2 ++
 arch/arm/mach-tegra/sleep-tegra30.S | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm/mach-tegra/flowctrl.h b/arch/arm/mach-tegra/flowctrl.h
index 7a29bae..e56a950 100644
--- a/arch/arm/mach-tegra/flowctrl.h
+++ b/arch/arm/mach-tegra/flowctrl.h
@@ -28,6 +28,8 @@
 #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_HALT_GIC_IRQ		(1 << 9)
+#define FLOW_CTRL_HALT_GIC_FIQ		(1 << 8)
 #define FLOW_CTRL_CPU0_CSR		0x8
 #define	FLOW_CTRL_CSR_INTR_FLAG		(1 << 15)
 #define FLOW_CTRL_CSR_EVENT_FLAG	(1 << 14)
diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S
index ada8821..5877f26 100644
--- a/arch/arm/mach-tegra/sleep-tegra30.S
+++ b/arch/arm/mach-tegra/sleep-tegra30.S
@@ -99,6 +99,8 @@ flow_ctrl_setting_for_lp2:
 	cmp	r10, #TEGRA30
 	moveq   r3, #FLOW_CTRL_WAIT_FOR_INTERRUPT	@ For LP2
 	movne	r3, #FLOW_CTRL_WAITEVENT
+	orrne	r3, r3, #FLOW_CTRL_HALT_GIC_IRQ
+	orrne	r3, r3, #FLOW_CTRL_HALT_GIC_FIQ
 flow_ctrl_done:
 	cmp	r10, #TEGRA30
 	str	r3, [r2]
-- 
1.8.3

[PATCH V2 2/3] ARM: tegra114: add low level support for CPU idle powered-down mode

[Joseph Lo]

The flow controller would take care the power sequence when CPU idle in
powered-down mode. It powered gate the CPU when CPU runs into WFI
instruction. And wake up the CPU when event be triggered.

The sequence is below.
* setting wfi bitmap for the CPU as the halt event in the
  FLOW_CTRL_CPU_HALT_REG to monitor the CPU running into WFI,then power
  gate it
* setting IRQ and FIQ as wake up event to wake up CPU when event triggered

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
V2:
* no change
---
 arch/arm/mach-tegra/flowctrl.h      | 2 ++
 arch/arm/mach-tegra/sleep-tegra30.S | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm/mach-tegra/flowctrl.h b/arch/arm/mach-tegra/flowctrl.h
index 7a29bae..e56a950 100644
--- a/arch/arm/mach-tegra/flowctrl.h
+++ b/arch/arm/mach-tegra/flowctrl.h
@@ -28,6 +28,8 @@
 #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_HALT_GIC_IRQ		(1 << 9)
+#define FLOW_CTRL_HALT_GIC_FIQ		(1 << 8)
 #define FLOW_CTRL_CPU0_CSR		0x8
 #define	FLOW_CTRL_CSR_INTR_FLAG		(1 << 15)
 #define FLOW_CTRL_CSR_EVENT_FLAG	(1 << 14)
diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S
index ada8821..5877f26 100644
--- a/arch/arm/mach-tegra/sleep-tegra30.S
+++ b/arch/arm/mach-tegra/sleep-tegra30.S
@@ -99,6 +99,8 @@ flow_ctrl_setting_for_lp2:
 	cmp	r10, #TEGRA30
 	moveq   r3, #FLOW_CTRL_WAIT_FOR_INTERRUPT	@ For LP2
 	movne	r3, #FLOW_CTRL_WAITEVENT
+	orrne	r3, r3, #FLOW_CTRL_HALT_GIC_IRQ
+	orrne	r3, r3, #FLOW_CTRL_HALT_GIC_FIQ
 flow_ctrl_done:
 	cmp	r10, #TEGRA30
 	str	r3, [r2]
-- 
1.8.3

[PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Joseph Lo]

This supports CPU core power down on each CPU when CPU idle. When CPU go
into this state, it saves it's context and needs a proper configuration
in flow controller to power gate the CPU when CPU runs into WFI
instruction. And the CPU also needs to set the IRQ as CPU power down idle
wake up event in flow controller.

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
V2:
* remove some redundant code of memory barrier
* remove the function declaration by rearranging the coding sequence 
---
 arch/arm/mach-tegra/cpuidle-tegra114.c | 51 +++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
index 1d1c602..e0b8730 100644
--- a/arch/arm/mach-tegra/cpuidle-tegra114.c
+++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
@@ -17,15 +17,64 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/cpuidle.h>
+#include <linux/cpu_pm.h>
+#include <linux/clockchips.h>
 
 #include <asm/cpuidle.h>
+#include <asm/suspend.h>
+#include <asm/smp_plat.h>
+
+#include "pm.h"
+#include "sleep.h"
+
+#ifdef CONFIG_PM_SLEEP
+#define TEGRA114_MAX_STATES 2
+#else
+#define TEGRA114_MAX_STATES 1
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int tegra114_idle_power_down(struct cpuidle_device *dev,
+				    struct cpuidle_driver *drv,
+				    int index)
+{
+	local_fiq_disable();
+
+	tegra_set_cpu_in_lp2();
+	cpu_pm_enter();
+
+	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
+
+	cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
+
+	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
+
+	cpu_pm_exit();
+	tegra_clear_cpu_in_lp2();
+
+	local_fiq_enable();
+
+	return index;
+}
+#endif
 
 static struct cpuidle_driver tegra_idle_driver = {
 	.name = "tegra_idle",
 	.owner = THIS_MODULE,
-	.state_count = 1,
+	.state_count = TEGRA114_MAX_STATES,
 	.states = {
 		[0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
+#ifdef CONFIG_PM_SLEEP
+		[1] = {
+			.enter			= tegra114_idle_power_down,
+			.exit_latency		= 500,
+			.target_residency	= 1000,
+			.power_usage		= 0,
+			.flags			= CPUIDLE_FLAG_TIME_VALID,
+			.name			= "powered-down",
+			.desc			= "CPU power gated",
+		},
+#endif
 	},
 };
 
-- 
1.8.3

[PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Joseph Lo]

This supports CPU core power down on each CPU when CPU idle. When CPU go
into this state, it saves it's context and needs a proper configuration
in flow controller to power gate the CPU when CPU runs into WFI
instruction. And the CPU also needs to set the IRQ as CPU power down idle
wake up event in flow controller.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
V2:
* remove some redundant code of memory barrier
* remove the function declaration by rearranging the coding sequence 
---
 arch/arm/mach-tegra/cpuidle-tegra114.c | 51 +++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
index 1d1c602..e0b8730 100644
--- a/arch/arm/mach-tegra/cpuidle-tegra114.c
+++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
@@ -17,15 +17,64 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/cpuidle.h>
+#include <linux/cpu_pm.h>
+#include <linux/clockchips.h>
 
 #include <asm/cpuidle.h>
+#include <asm/suspend.h>
+#include <asm/smp_plat.h>
+
+#include "pm.h"
+#include "sleep.h"
+
+#ifdef CONFIG_PM_SLEEP
+#define TEGRA114_MAX_STATES 2
+#else
+#define TEGRA114_MAX_STATES 1
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int tegra114_idle_power_down(struct cpuidle_device *dev,
+				    struct cpuidle_driver *drv,
+				    int index)
+{
+	local_fiq_disable();
+
+	tegra_set_cpu_in_lp2();
+	cpu_pm_enter();
+
+	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
+
+	cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
+
+	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
+
+	cpu_pm_exit();
+	tegra_clear_cpu_in_lp2();
+
+	local_fiq_enable();
+
+	return index;
+}
+#endif
 
 static struct cpuidle_driver tegra_idle_driver = {
 	.name = "tegra_idle",
 	.owner = THIS_MODULE,
-	.state_count = 1,
+	.state_count = TEGRA114_MAX_STATES,
 	.states = {
 		[0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
+#ifdef CONFIG_PM_SLEEP
+		[1] = {
+			.enter			= tegra114_idle_power_down,
+			.exit_latency		= 500,
+			.target_residency	= 1000,
+			.power_usage		= 0,
+			.flags			= CPUIDLE_FLAG_TIME_VALID,
+			.name			= "powered-down",
+			.desc			= "CPU power gated",
+		},
+#endif
 	},
 };
 
-- 
1.8.3

Re: [PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Daniel Lezcano]

On 06/04/2013 12:48 PM, Joseph Lo wrote:
> This supports CPU core power down on each CPU when CPU idle. When CPU go
> into this state, it saves it's context and needs a proper configuration
> in flow controller to power gate the CPU when CPU runs into WFI
> instruction. And the CPU also needs to set the IRQ as CPU power down idle
> wake up event in flow controller.
> 
> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I would like to understand why there is a WARN with the
CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
is not the tree hiding the forest.

Thanks
  -- Daniel

> ---
> V2:
> * remove some redundant code of memory barrier
> * remove the function declaration by rearranging the coding sequence 
> ---
>  arch/arm/mach-tegra/cpuidle-tegra114.c | 51 +++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
> index 1d1c602..e0b8730 100644
> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
> @@ -17,15 +17,64 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/cpuidle.h>
> +#include <linux/cpu_pm.h>
> +#include <linux/clockchips.h>
>  
>  #include <asm/cpuidle.h>
> +#include <asm/suspend.h>
> +#include <asm/smp_plat.h>
> +
> +#include "pm.h"
> +#include "sleep.h"
> +
> +#ifdef CONFIG_PM_SLEEP
> +#define TEGRA114_MAX_STATES 2
> +#else
> +#define TEGRA114_MAX_STATES 1
> +#endif
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int tegra114_idle_power_down(struct cpuidle_device *dev,
> +				    struct cpuidle_driver *drv,
> +				    int index)
> +{
> +	local_fiq_disable();
> +
> +	tegra_set_cpu_in_lp2();
> +	cpu_pm_enter();
> +
> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> +
> +	cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
> +
> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> +
> +	cpu_pm_exit();
> +	tegra_clear_cpu_in_lp2();
> +
> +	local_fiq_enable();
> +
> +	return index;
> +}
> +#endif
>  
>  static struct cpuidle_driver tegra_idle_driver = {
>  	.name = "tegra_idle",
>  	.owner = THIS_MODULE,
> -	.state_count = 1,
> +	.state_count = TEGRA114_MAX_STATES,
>  	.states = {
>  		[0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
> +#ifdef CONFIG_PM_SLEEP
> +		[1] = {
> +			.enter			= tegra114_idle_power_down,
> +			.exit_latency		= 500,
> +			.target_residency	= 1000,
> +			.power_usage		= 0,
> +			.flags			= CPUIDLE_FLAG_TIME_VALID,
> +			.name			= "powered-down",
> +			.desc			= "CPU power gated",
> +		},
> +#endif
>  	},
>  };
>  
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Daniel Lezcano]

On 06/04/2013 12:48 PM, Joseph Lo wrote:
> This supports CPU core power down on each CPU when CPU idle. When CPU go
> into this state, it saves it's context and needs a proper configuration
> in flow controller to power gate the CPU when CPU runs into WFI
> instruction. And the CPU also needs to set the IRQ as CPU power down idle
> wake up event in flow controller.
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>

I would like to understand why there is a WARN with the
CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
is not the tree hiding the forest.

Thanks
  -- Daniel

> ---
> V2:
> * remove some redundant code of memory barrier
> * remove the function declaration by rearranging the coding sequence 
> ---
>  arch/arm/mach-tegra/cpuidle-tegra114.c | 51 +++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
> index 1d1c602..e0b8730 100644
> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
> @@ -17,15 +17,64 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/cpuidle.h>
> +#include <linux/cpu_pm.h>
> +#include <linux/clockchips.h>
>  
>  #include <asm/cpuidle.h>
> +#include <asm/suspend.h>
> +#include <asm/smp_plat.h>
> +
> +#include "pm.h"
> +#include "sleep.h"
> +
> +#ifdef CONFIG_PM_SLEEP
> +#define TEGRA114_MAX_STATES 2
> +#else
> +#define TEGRA114_MAX_STATES 1
> +#endif
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int tegra114_idle_power_down(struct cpuidle_device *dev,
> +				    struct cpuidle_driver *drv,
> +				    int index)
> +{
> +	local_fiq_disable();
> +
> +	tegra_set_cpu_in_lp2();
> +	cpu_pm_enter();
> +
> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> +
> +	cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
> +
> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> +
> +	cpu_pm_exit();
> +	tegra_clear_cpu_in_lp2();
> +
> +	local_fiq_enable();
> +
> +	return index;
> +}
> +#endif
>  
>  static struct cpuidle_driver tegra_idle_driver = {
>  	.name = "tegra_idle",
>  	.owner = THIS_MODULE,
> -	.state_count = 1,
> +	.state_count = TEGRA114_MAX_STATES,
>  	.states = {
>  		[0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
> +#ifdef CONFIG_PM_SLEEP
> +		[1] = {
> +			.enter			= tegra114_idle_power_down,
> +			.exit_latency		= 500,
> +			.target_residency	= 1000,
> +			.power_usage		= 0,
> +			.flags			= CPUIDLE_FLAG_TIME_VALID,
> +			.name			= "powered-down",
> +			.desc			= "CPU power gated",
> +		},
> +#endif
>  	},
>  };
>  
> 


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Stephen Warren]

On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
> On 06/04/2013 12:48 PM, Joseph Lo wrote:
>> This supports CPU core power down on each CPU when CPU idle. When CPU go
>> into this state, it saves it's context and needs a proper configuration
>> in flow controller to power gate the CPU when CPU runs into WFI
>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
>> wake up event in flow controller.
>>
>> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> I would like to understand why there is a WARN with the
> CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
> is not the tree hiding the forest.

Joseph, are you planning to post an updated series or respond to resolve
Daniel's question?

[PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Stephen Warren]

On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
> On 06/04/2013 12:48 PM, Joseph Lo wrote:
>> This supports CPU core power down on each CPU when CPU idle. When CPU go
>> into this state, it saves it's context and needs a proper configuration
>> in flow controller to power gate the CPU when CPU runs into WFI
>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
>> wake up event in flow controller.
>>
>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> 
> I would like to understand why there is a WARN with the
> CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
> is not the tree hiding the forest.

Joseph, are you planning to post an updated series or respond to resolve
Daniel's question?

Re: [PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Joseph Lo]

On Thu, 2013-06-13 at 01:41 +0800, Stephen Warren wrote:
> On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
> > On 06/04/2013 12:48 PM, Joseph Lo wrote:
> >> This supports CPU core power down on each CPU when CPU idle. When CPU go
> >> into this state, it saves it's context and needs a proper configuration
> >> in flow controller to power gate the CPU when CPU runs into WFI
> >> instruction. And the CPU also needs to set the IRQ as CPU power down idle
> >> wake up event in flow controller.
> >>
> >> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > I would like to understand why there is a WARN with the
> > CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
> > is not the tree hiding the forest.
> 
> Joseph, are you planning to post an updated series or respond to resolve
> Daniel's question?

I need more time to investigate the detail about what caused the WARN
when apply the flag. And what's the difference if we didn't enable
CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, then it only applies
CLOCK_EVT_NOTIFY_BROADCAST_ON on CPU0. Why if I am moving the
clockevent_notify of CLOCK_EVT_NOTIFY_BROADCAST_EXIT before
"local_irq_enable" in the
"cpuidle_enter_state" (drivers/cpuidle/cpuidle.c), then the warning
message gone?

Daniel,

Because I need more time to figure it out, are you OK if I can post
another patch to apply CPUIDLE_FLAG_TIMER_STOP flag for all Tegra CPU
Idle drivers later? (Once we know what's the problem behind this)

Stephen,

I don't have an update of this series, if Daniel is OK if I can post
another patch to apply TIMER_STOP flag for all Tegra idle drivers.

Thanks,
Joseph

[PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Joseph Lo]

On Thu, 2013-06-13 at 01:41 +0800, Stephen Warren wrote:
> On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
> > On 06/04/2013 12:48 PM, Joseph Lo wrote:
> >> This supports CPU core power down on each CPU when CPU idle. When CPU go
> >> into this state, it saves it's context and needs a proper configuration
> >> in flow controller to power gate the CPU when CPU runs into WFI
> >> instruction. And the CPU also needs to set the IRQ as CPU power down idle
> >> wake up event in flow controller.
> >>
> >> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> > 
> > I would like to understand why there is a WARN with the
> > CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
> > is not the tree hiding the forest.
> 
> Joseph, are you planning to post an updated series or respond to resolve
> Daniel's question?

I need more time to investigate the detail about what caused the WARN
when apply the flag. And what's the difference if we didn't enable
CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, then it only applies
CLOCK_EVT_NOTIFY_BROADCAST_ON on CPU0. Why if I am moving the
clockevent_notify of CLOCK_EVT_NOTIFY_BROADCAST_EXIT before
"local_irq_enable" in the
"cpuidle_enter_state" (drivers/cpuidle/cpuidle.c), then the warning
message gone?

Daniel,

Because I need more time to figure it out, are you OK if I can post
another patch to apply CPUIDLE_FLAG_TIMER_STOP flag for all Tegra CPU
Idle drivers later? (Once we know what's the problem behind this)

Stephen,

I don't have an update of this series, if Daniel is OK if I can post
another patch to apply TIMER_STOP flag for all Tegra idle drivers.

Thanks,
Joseph

Re: [PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Daniel Lezcano]

On 06/13/2013 04:13 AM, Joseph Lo wrote:
> On Thu, 2013-06-13 at 01:41 +0800, Stephen Warren wrote:
>> On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
>>> On 06/04/2013 12:48 PM, Joseph Lo wrote:
>>>> This supports CPU core power down on each CPU when CPU idle. When CPU go
>>>> into this state, it saves it's context and needs a proper configuration
>>>> in flow controller to power gate the CPU when CPU runs into WFI
>>>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
>>>> wake up event in flow controller.
>>>>
>>>> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> I would like to understand why there is a WARN with the
>>> CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
>>> is not the tree hiding the forest.
>>
>> Joseph, are you planning to post an updated series or respond to resolve
>> Daniel's question?
> 
> I need more time to investigate the detail about what caused the WARN
> when apply the flag. And what's the difference if we didn't enable
> CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, then it only applies
> CLOCK_EVT_NOTIFY_BROADCAST_ON on CPU0.

I think I got it for this one. It is a bug in the cpuidle driver's code.

cpuidle_register_driver does get_cpu => CPU0

Then __cpuidle_register_driver(drv, <CPU0>)

 => __cpuidle_driver_init(drv, <CPU0>)

Hence, the timer broadcast is initialized only for cpu0.

That should have been fixed with:

commit 82467a5a885ddd9f80309682159da8db510e7832
Author: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date:   Fri Jun 7 21:53:09 2013 +0000

    cpuidle: simplify multiple driver support

    Commit bf4d1b5 (cpuidle: support multiple drivers) introduced support
    for using multiple cpuidle drivers at the same time.  It added a
    couple of new APIs to register the driver per CPU, but that led to
    some unnecessary code complexity related to the kernel config options
    deciding whether or not the multiple driver support is enabled.  The
    code has to work as it did before when the multiple driver support is
    not enabled and the multiple driver support has to be compatible with
    the previously existing API.

    Remove the new API, not used by any driver in the tree yet (but
    needed for the HMP cpuidle drivers that will be submitted soon), and
    add a new cpumask pointer to the cpuidle driver structure that will
    point to the mask of CPUs handled by the given driver.  That will
    allow the cpuidle_[un]register_driver() API to be used for the
    multiple driver support along with the cpuidle_[un]register()
    functions added recently.

    [rjw: Changelog]
    Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>


> Why if I am moving the
> clockevent_notify of CLOCK_EVT_NOTIFY_BROADCAST_EXIT before
> "local_irq_enable" in the
> "cpuidle_enter_state" (drivers/cpuidle/cpuidle.c), then the warning
> message gone?

Is the warning coming immediately, at the first time
CLOCK_EVT_NOTIFY_BROADCAST_EXIT is invoked or can it occur at different
moment ?

> Daniel,
> 
> Because I need more time to figure it out, are you OK if I can post
> another patch to apply CPUIDLE_FLAG_TIMER_STOP flag for all Tegra CPU
> Idle drivers later? (Once we know what's the problem behind this)
> 
> Stephen,
> 
> I don't have an update of this series, if Daniel is OK if I can post
> another patch to apply TIMER_STOP flag for all Tegra idle drivers.
> 
> Thanks,
> Joseph
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Daniel Lezcano]

On 06/13/2013 04:13 AM, Joseph Lo wrote:
> On Thu, 2013-06-13 at 01:41 +0800, Stephen Warren wrote:
>> On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
>>> On 06/04/2013 12:48 PM, Joseph Lo wrote:
>>>> This supports CPU core power down on each CPU when CPU idle. When CPU go
>>>> into this state, it saves it's context and needs a proper configuration
>>>> in flow controller to power gate the CPU when CPU runs into WFI
>>>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
>>>> wake up event in flow controller.
>>>>
>>>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>>>
>>> I would like to understand why there is a WARN with the
>>> CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
>>> is not the tree hiding the forest.
>>
>> Joseph, are you planning to post an updated series or respond to resolve
>> Daniel's question?
> 
> I need more time to investigate the detail about what caused the WARN
> when apply the flag. And what's the difference if we didn't enable
> CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, then it only applies
> CLOCK_EVT_NOTIFY_BROADCAST_ON on CPU0.

I think I got it for this one. It is a bug in the cpuidle driver's code.

cpuidle_register_driver does get_cpu => CPU0

Then __cpuidle_register_driver(drv, <CPU0>)

 => __cpuidle_driver_init(drv, <CPU0>)

Hence, the timer broadcast is initialized only for cpu0.

That should have been fixed with:

commit 82467a5a885ddd9f80309682159da8db510e7832
Author: Daniel Lezcano <daniel.lezcano@linaro.org>
Date:   Fri Jun 7 21:53:09 2013 +0000

    cpuidle: simplify multiple driver support

    Commit bf4d1b5 (cpuidle: support multiple drivers) introduced support
    for using multiple cpuidle drivers at the same time.  It added a
    couple of new APIs to register the driver per CPU, but that led to
    some unnecessary code complexity related to the kernel config options
    deciding whether or not the multiple driver support is enabled.  The
    code has to work as it did before when the multiple driver support is
    not enabled and the multiple driver support has to be compatible with
    the previously existing API.

    Remove the new API, not used by any driver in the tree yet (but
    needed for the HMP cpuidle drivers that will be submitted soon), and
    add a new cpumask pointer to the cpuidle driver structure that will
    point to the mask of CPUs handled by the given driver.  That will
    allow the cpuidle_[un]register_driver() API to be used for the
    multiple driver support along with the cpuidle_[un]register()
    functions added recently.

    [rjw: Changelog]
    Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>


> Why if I am moving the
> clockevent_notify of CLOCK_EVT_NOTIFY_BROADCAST_EXIT before
> "local_irq_enable" in the
> "cpuidle_enter_state" (drivers/cpuidle/cpuidle.c), then the warning
> message gone?

Is the warning coming immediately, at the first time
CLOCK_EVT_NOTIFY_BROADCAST_EXIT is invoked or can it occur at different
moment ?

> Daniel,
> 
> Because I need more time to figure it out, are you OK if I can post
> another patch to apply CPUIDLE_FLAG_TIMER_STOP flag for all Tegra CPU
> Idle drivers later? (Once we know what's the problem behind this)
> 
> Stephen,
> 
> I don't have an update of this series, if Daniel is OK if I can post
> another patch to apply TIMER_STOP flag for all Tegra idle drivers.
> 
> Thanks,
> Joseph
> 


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Joseph Lo]

On Thu, 2013-06-13 at 17:30 +0800, Daniel Lezcano wrote:
> On 06/13/2013 04:13 AM, Joseph Lo wrote:
> > On Thu, 2013-06-13 at 01:41 +0800, Stephen Warren wrote:
> >> On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
> >>> On 06/04/2013 12:48 PM, Joseph Lo wrote:
> >>>> This supports CPU core power down on each CPU when CPU idle. When CPU go
> >>>> into this state, it saves it's context and needs a proper configuration
> >>>> in flow controller to power gate the CPU when CPU runs into WFI
> >>>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
> >>>> wake up event in flow controller.
> >>>>
> >>>> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>>
> >>> I would like to understand why there is a WARN with the
> >>> CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
> >>> is not the tree hiding the forest.
> >>
> >> Joseph, are you planning to post an updated series or respond to resolve
> >> Daniel's question?
> > 
> > I need more time to investigate the detail about what caused the WARN
> > when apply the flag. And what's the difference if we didn't enable
> > CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, then it only applies
> > CLOCK_EVT_NOTIFY_BROADCAST_ON on CPU0.
> 
> I think I got it for this one. It is a bug in the cpuidle driver's code.
> 
> cpuidle_register_driver does get_cpu => CPU0
> 
> Then __cpuidle_register_driver(drv, <CPU0>)
> 
>  => __cpuidle_driver_init(drv, <CPU0>)
> 
> Hence, the timer broadcast is initialized only for cpu0.
> 
> That should have been fixed with:
> 
OK, thanks.

> commit 82467a5a885ddd9f80309682159da8db510e7832
> Author: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Date:   Fri Jun 7 21:53:09 2013 +0000
> 
>     cpuidle: simplify multiple driver support
> 
>     Commit bf4d1b5 (cpuidle: support multiple drivers) introduced support
>     for using multiple cpuidle drivers at the same time.  It added a
>     couple of new APIs to register the driver per CPU, but that led to
>     some unnecessary code complexity related to the kernel config options
>     deciding whether or not the multiple driver support is enabled.  The
>     code has to work as it did before when the multiple driver support is
>     not enabled and the multiple driver support has to be compatible with
>     the previously existing API.
> 
>     Remove the new API, not used by any driver in the tree yet (but
>     needed for the HMP cpuidle drivers that will be submitted soon), and
>     add a new cpumask pointer to the cpuidle driver structure that will
>     point to the mask of CPUs handled by the given driver.  That will
>     allow the cpuidle_[un]register_driver() API to be used for the
>     multiple driver support along with the cpuidle_[un]register()
>     functions added recently.
> 
>     [rjw: Changelog]
>     Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> 
> > Why if I am moving the
> > clockevent_notify of CLOCK_EVT_NOTIFY_BROADCAST_EXIT before
> > "local_irq_enable" in the
> > "cpuidle_enter_state" (drivers/cpuidle/cpuidle.c), then the warning
> > message gone?
> 
> Is the warning coming immediately, at the first time
> CLOCK_EVT_NOTIFY_BROADCAST_EXIT is invoked or can it occur at different
> moment ?
> 
Not exactly at the 1st time, it may happen in the 3rd to 10th time. But
it only happens once. I mean I did check the
tick_broadcast_pending_mask, it only happens once the CPU didn't clear
the tick_broadcast_pending_mask after CLOCK_EVT_NOTIFY_BROADCAST_EXIT.
And cause the warning message. Then I didn't see it happen anymore.

[PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Joseph Lo]

On Thu, 2013-06-13 at 17:30 +0800, Daniel Lezcano wrote:
> On 06/13/2013 04:13 AM, Joseph Lo wrote:
> > On Thu, 2013-06-13 at 01:41 +0800, Stephen Warren wrote:
> >> On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
> >>> On 06/04/2013 12:48 PM, Joseph Lo wrote:
> >>>> This supports CPU core power down on each CPU when CPU idle. When CPU go
> >>>> into this state, it saves it's context and needs a proper configuration
> >>>> in flow controller to power gate the CPU when CPU runs into WFI
> >>>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
> >>>> wake up event in flow controller.
> >>>>
> >>>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> >>>
> >>> I would like to understand why there is a WARN with the
> >>> CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
> >>> is not the tree hiding the forest.
> >>
> >> Joseph, are you planning to post an updated series or respond to resolve
> >> Daniel's question?
> > 
> > I need more time to investigate the detail about what caused the WARN
> > when apply the flag. And what's the difference if we didn't enable
> > CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, then it only applies
> > CLOCK_EVT_NOTIFY_BROADCAST_ON on CPU0.
> 
> I think I got it for this one. It is a bug in the cpuidle driver's code.
> 
> cpuidle_register_driver does get_cpu => CPU0
> 
> Then __cpuidle_register_driver(drv, <CPU0>)
> 
>  => __cpuidle_driver_init(drv, <CPU0>)
> 
> Hence, the timer broadcast is initialized only for cpu0.
> 
> That should have been fixed with:
> 
OK, thanks.

> commit 82467a5a885ddd9f80309682159da8db510e7832
> Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> Date:   Fri Jun 7 21:53:09 2013 +0000
> 
>     cpuidle: simplify multiple driver support
> 
>     Commit bf4d1b5 (cpuidle: support multiple drivers) introduced support
>     for using multiple cpuidle drivers at the same time.  It added a
>     couple of new APIs to register the driver per CPU, but that led to
>     some unnecessary code complexity related to the kernel config options
>     deciding whether or not the multiple driver support is enabled.  The
>     code has to work as it did before when the multiple driver support is
>     not enabled and the multiple driver support has to be compatible with
>     the previously existing API.
> 
>     Remove the new API, not used by any driver in the tree yet (but
>     needed for the HMP cpuidle drivers that will be submitted soon), and
>     add a new cpumask pointer to the cpuidle driver structure that will
>     point to the mask of CPUs handled by the given driver.  That will
>     allow the cpuidle_[un]register_driver() API to be used for the
>     multiple driver support along with the cpuidle_[un]register()
>     functions added recently.
> 
>     [rjw: Changelog]
>     Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 
> > Why if I am moving the
> > clockevent_notify of CLOCK_EVT_NOTIFY_BROADCAST_EXIT before
> > "local_irq_enable" in the
> > "cpuidle_enter_state" (drivers/cpuidle/cpuidle.c), then the warning
> > message gone?
> 
> Is the warning coming immediately, at the first time
> CLOCK_EVT_NOTIFY_BROADCAST_EXIT is invoked or can it occur at different
> moment ?
> 
Not exactly at the 1st time, it may happen in the 3rd to 10th time. But
it only happens once. I mean I did check the
tick_broadcast_pending_mask, it only happens once the CPU didn't clear
the tick_broadcast_pending_mask after CLOCK_EVT_NOTIFY_BROADCAST_EXIT.
And cause the warning message. Then I didn't see it happen anymore.

Re: [PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Daniel Lezcano]

On 06/13/2013 04:32 PM, Joseph Lo wrote:
> On Thu, 2013-06-13 at 17:30 +0800, Daniel Lezcano wrote:
>> On 06/13/2013 04:13 AM, Joseph Lo wrote:
>>> On Thu, 2013-06-13 at 01:41 +0800, Stephen Warren wrote:
>>>> On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
>>>>> On 06/04/2013 12:48 PM, Joseph Lo wrote:
>>>>>> This supports CPU core power down on each CPU when CPU idle. When CPU go
>>>>>> into this state, it saves it's context and needs a proper configuration
>>>>>> in flow controller to power gate the CPU when CPU runs into WFI
>>>>>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
>>>>>> wake up event in flow controller.
>>>>>>
>>>>>> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> I would like to understand why there is a WARN with the
>>>>> CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
>>>>> is not the tree hiding the forest.
>>>>
>>>> Joseph, are you planning to post an updated series or respond to resolve
>>>> Daniel's question?
>>>
>>> I need more time to investigate the detail about what caused the WARN
>>> when apply the flag. And what's the difference if we didn't enable
>>> CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, then it only applies
>>> CLOCK_EVT_NOTIFY_BROADCAST_ON on CPU0.
>>
>> I think I got it for this one. It is a bug in the cpuidle driver's code.
>>
>> cpuidle_register_driver does get_cpu => CPU0
>>
>> Then __cpuidle_register_driver(drv, <CPU0>)
>>
>>  => __cpuidle_driver_init(drv, <CPU0>)
>>
>> Hence, the timer broadcast is initialized only for cpu0.
>>
>> That should have been fixed with:
>>
> OK, thanks.
> 
>> commit 82467a5a885ddd9f80309682159da8db510e7832
>> Author: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Date:   Fri Jun 7 21:53:09 2013 +0000
>>
>>     cpuidle: simplify multiple driver support
>>
>>     Commit bf4d1b5 (cpuidle: support multiple drivers) introduced support
>>     for using multiple cpuidle drivers at the same time.  It added a
>>     couple of new APIs to register the driver per CPU, but that led to
>>     some unnecessary code complexity related to the kernel config options
>>     deciding whether or not the multiple driver support is enabled.  The
>>     code has to work as it did before when the multiple driver support is
>>     not enabled and the multiple driver support has to be compatible with
>>     the previously existing API.
>>
>>     Remove the new API, not used by any driver in the tree yet (but
>>     needed for the HMP cpuidle drivers that will be submitted soon), and
>>     add a new cpumask pointer to the cpuidle driver structure that will
>>     point to the mask of CPUs handled by the given driver.  That will
>>     allow the cpuidle_[un]register_driver() API to be used for the
>>     multiple driver support along with the cpuidle_[un]register()
>>     functions added recently.
>>
>>     [rjw: Changelog]
>>     Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>>
>>> Why if I am moving the
>>> clockevent_notify of CLOCK_EVT_NOTIFY_BROADCAST_EXIT before
>>> "local_irq_enable" in the
>>> "cpuidle_enter_state" (drivers/cpuidle/cpuidle.c), then the warning
>>> message gone?
>>
>> Is the warning coming immediately, at the first time
>> CLOCK_EVT_NOTIFY_BROADCAST_EXIT is invoked or can it occur at different
>> moment ?
>>
> Not exactly at the 1st time, it may happen in the 3rd to 10th time. But
> it only happens once. I mean I did check the
> tick_broadcast_pending_mask, it only happens once the CPU didn't clear
> the tick_broadcast_pending_mask after CLOCK_EVT_NOTIFY_BROADCAST_EXIT.
> And cause the warning message. Then I didn't see it happen anymore.

I suspect when the CLOCK_EVENT_NOTIFY_EXIT is after the
local_irq_enable, we have the timer handler executed before where it
sets the tick_broadcast_pending_mask.

Hence in the tick_broadcast_oneshot_control function when we fall into
the condition:

 if (dev->next_event.tv64 == KTIME_MAX)
	goto out;

The tick_broadcast_pending_mask is not cleared and when re-entering with
CLOCK_EVENT_NOTIFY_ENTER, this mask is set and that raises the warning.

So, if I am not wrong, the condition to raise this warning would be:

 1. use the flag (of course), because the
CLOCK_EVT_NOTIFY_BROADCAST_EXIT is called after the local_irq_enabled

 2. there is no timer planned after the expired one.

 3. a new timer is created and the cpu enters the low power state again

The first step would be to create a simple program to reproduce easily
the warning.

First boot the kernel with init=/bin/bash (or whatever, the init process
is replaced by a shell), thus reducing considerably the number of timers.

Then try to raise the warning with cpu2:

	taskset 4 /bin/sleep 0.5

... several times until the warning appears.

As I don't have the hardware, this is based on assumptions, so may be I
am wrong.

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Daniel Lezcano]

On 06/13/2013 04:32 PM, Joseph Lo wrote:
> On Thu, 2013-06-13 at 17:30 +0800, Daniel Lezcano wrote:
>> On 06/13/2013 04:13 AM, Joseph Lo wrote:
>>> On Thu, 2013-06-13 at 01:41 +0800, Stephen Warren wrote:
>>>> On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
>>>>> On 06/04/2013 12:48 PM, Joseph Lo wrote:
>>>>>> This supports CPU core power down on each CPU when CPU idle. When CPU go
>>>>>> into this state, it saves it's context and needs a proper configuration
>>>>>> in flow controller to power gate the CPU when CPU runs into WFI
>>>>>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
>>>>>> wake up event in flow controller.
>>>>>>
>>>>>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>>>>>
>>>>> I would like to understand why there is a WARN with the
>>>>> CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
>>>>> is not the tree hiding the forest.
>>>>
>>>> Joseph, are you planning to post an updated series or respond to resolve
>>>> Daniel's question?
>>>
>>> I need more time to investigate the detail about what caused the WARN
>>> when apply the flag. And what's the difference if we didn't enable
>>> CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, then it only applies
>>> CLOCK_EVT_NOTIFY_BROADCAST_ON on CPU0.
>>
>> I think I got it for this one. It is a bug in the cpuidle driver's code.
>>
>> cpuidle_register_driver does get_cpu => CPU0
>>
>> Then __cpuidle_register_driver(drv, <CPU0>)
>>
>>  => __cpuidle_driver_init(drv, <CPU0>)
>>
>> Hence, the timer broadcast is initialized only for cpu0.
>>
>> That should have been fixed with:
>>
> OK, thanks.
> 
>> commit 82467a5a885ddd9f80309682159da8db510e7832
>> Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Date:   Fri Jun 7 21:53:09 2013 +0000
>>
>>     cpuidle: simplify multiple driver support
>>
>>     Commit bf4d1b5 (cpuidle: support multiple drivers) introduced support
>>     for using multiple cpuidle drivers at the same time.  It added a
>>     couple of new APIs to register the driver per CPU, but that led to
>>     some unnecessary code complexity related to the kernel config options
>>     deciding whether or not the multiple driver support is enabled.  The
>>     code has to work as it did before when the multiple driver support is
>>     not enabled and the multiple driver support has to be compatible with
>>     the previously existing API.
>>
>>     Remove the new API, not used by any driver in the tree yet (but
>>     needed for the HMP cpuidle drivers that will be submitted soon), and
>>     add a new cpumask pointer to the cpuidle driver structure that will
>>     point to the mask of CPUs handled by the given driver.  That will
>>     allow the cpuidle_[un]register_driver() API to be used for the
>>     multiple driver support along with the cpuidle_[un]register()
>>     functions added recently.
>>
>>     [rjw: Changelog]
>>     Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>>
>>> Why if I am moving the
>>> clockevent_notify of CLOCK_EVT_NOTIFY_BROADCAST_EXIT before
>>> "local_irq_enable" in the
>>> "cpuidle_enter_state" (drivers/cpuidle/cpuidle.c), then the warning
>>> message gone?
>>
>> Is the warning coming immediately, at the first time
>> CLOCK_EVT_NOTIFY_BROADCAST_EXIT is invoked or can it occur at different
>> moment ?
>>
> Not exactly at the 1st time, it may happen in the 3rd to 10th time. But
> it only happens once. I mean I did check the
> tick_broadcast_pending_mask, it only happens once the CPU didn't clear
> the tick_broadcast_pending_mask after CLOCK_EVT_NOTIFY_BROADCAST_EXIT.
> And cause the warning message. Then I didn't see it happen anymore.

I suspect when the CLOCK_EVENT_NOTIFY_EXIT is after the
local_irq_enable, we have the timer handler executed before where it
sets the tick_broadcast_pending_mask.

Hence in the tick_broadcast_oneshot_control function when we fall into
the condition:

 if (dev->next_event.tv64 == KTIME_MAX)
	goto out;

The tick_broadcast_pending_mask is not cleared and when re-entering with
CLOCK_EVENT_NOTIFY_ENTER, this mask is set and that raises the warning.

So, if I am not wrong, the condition to raise this warning would be:

 1. use the flag (of course), because the
CLOCK_EVT_NOTIFY_BROADCAST_EXIT is called after the local_irq_enabled

 2. there is no timer planned after the expired one.

 3. a new timer is created and the cpu enters the low power state again

The first step would be to create a simple program to reproduce easily
the warning.

First boot the kernel with init=/bin/bash (or whatever, the init process
is replaced by a shell), thus reducing considerably the number of timers.

Then try to raise the warning with cpu2:

	taskset 4 /bin/sleep 0.5

... several times until the warning appears.

As I don't have the hardware, this is based on assumptions, so may be I
am wrong.

-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog