[PATCH 0/5] ARM: Few patches for PM enablement.

[PATCH 0/5] ARM: Few patches for PM enablement.

[Santosh Shilimkar]

The series consist of few patches to address some common PM issues
on Cortex-A9 based ARM SoCs. The skip calibration on secondary cores
is still under discussion.
On OMAP, the GIC save restore is done differently and hence that
part isn't included. Collin already have a patch for the same which
should address other SOCs.

The following changes since commit 1bae4ce27c9c90344f23c65ea6966c50ffeae2f5:
  Linus Torvalds (1):
        Linux 2.6.38-rc2

Santosh Shilimkar (5):
      ARM: gic: Add hooks for architecture specific extensions
      ARM: gic: Add distributor and interface enable/disable accessory api
      ARM: twd: Add context save restore support
      ARM: scu: Move register defines to header file
      ARM: smp: Skip secondary cpu calibration to speed-up boot

 arch/arm/common/gic.c               |   52 +++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/hardware/gic.h |    3 ++
 arch/arm/include/asm/localtimer.h   |   13 +++++++++
 arch/arm/include/asm/smp.h          |    8 +++++
 arch/arm/include/asm/smp_scu.h      |    6 ++++
 arch/arm/include/asm/smp_twd.h      |    2 +
 arch/arm/kernel/smp.c               |   35 +++++++++++++++++------
 arch/arm/kernel/smp_scu.c           |    6 ----
 arch/arm/kernel/smp_twd.c           |   24 ++++++++++++++++
 arch/arm/mach-omap2/omap-smp.c      |    3 ++
 10 files changed, 137 insertions(+), 15 deletions(-)

[PATCH 0/5] ARM: Few patches for PM enablement.

[Santosh Shilimkar]

The series consist of few patches to address some common PM issues
on Cortex-A9 based ARM SoCs. The skip calibration on secondary cores
is still under discussion.
On OMAP, the GIC save restore is done differently and hence that
part isn't included. Collin already have a patch for the same which
should address other SOCs.

The following changes since commit 1bae4ce27c9c90344f23c65ea6966c50ffeae2f5:
  Linus Torvalds (1):
        Linux 2.6.38-rc2

Santosh Shilimkar (5):
      ARM: gic: Add hooks for architecture specific extensions
      ARM: gic: Add distributor and interface enable/disable accessory api
      ARM: twd: Add context save restore support
      ARM: scu: Move register defines to header file
      ARM: smp: Skip secondary cpu calibration to speed-up boot

 arch/arm/common/gic.c               |   52 +++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/hardware/gic.h |    3 ++
 arch/arm/include/asm/localtimer.h   |   13 +++++++++
 arch/arm/include/asm/smp.h          |    8 +++++
 arch/arm/include/asm/smp_scu.h      |    6 ++++
 arch/arm/include/asm/smp_twd.h      |    2 +
 arch/arm/kernel/smp.c               |   35 +++++++++++++++++------
 arch/arm/kernel/smp_scu.c           |    6 ----
 arch/arm/kernel/smp_twd.c           |   24 ++++++++++++++++
 arch/arm/mach-omap2/omap-smp.c      |    3 ++
 10 files changed, 137 insertions(+), 15 deletions(-)

[PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Santosh Shilimkar]

Few architectures combine the GIC with an external interrupt controller.
On such systems it may be necessary to update both the GIC registers
and the external controller's registers to control IRQ behavior.

This can be addressed in couple of possible methods.
 1.	Export common GIC routines along with 'struct irq_chip gic_chip'
	and allow architectures to have custom function by override.

 2.	Provide architecture specific function pointer hooks
	within GIC library and leave platforms to add the necessary
	code as part of these hooks.

First one might be non-intrusive but have few shortcomings like arch needs
to have there own custom gic library. Locks used should be common since it
caters to same IRQs etc. Maintenance point of view also it leads to
multiple file fixes.

The second probably is cleaner and portable. It ensures that all the
common GIC infrastructure is not touched and also provides archs to
address their specific issue.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/common/gic.c               |   28 ++++++++++++++++++++++++++++
 arch/arm/include/asm/hardware/gic.h |    1 +
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 2243772..b4a9ea7 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -44,6 +44,15 @@ struct gic_chip_data {
 	void __iomem *cpu_base;
 };
 
+/* Default make arch specific GIC functions NULL */
+struct irq_chip gic_arch_extn = {
+	.irq_mask	= NULL,
+	.irq_unmask	= NULL,
+#ifdef CONFIG_PM
+	.irq_set_wake	= NULL,
+#endif
+};
+
 #ifndef MAX_GIC_NR
 #define MAX_GIC_NR	1
 #endif
@@ -84,6 +93,8 @@ static void gic_mask_irq(struct irq_data *d)
 
 	spin_lock(&irq_controller_lock);
 	writel(mask, gic_dist_base(d) + GIC_DIST_ENABLE_CLEAR + (gic_irq(d) / 32) * 4);
+	if (gic_arch_extn.irq_mask)
+		gic_arch_extn.irq_mask(d);
 	spin_unlock(&irq_controller_lock);
 }
 
@@ -92,6 +103,8 @@ static void gic_unmask_irq(struct irq_data *d)
 	u32 mask = 1 << (d->irq % 32);
 
 	spin_lock(&irq_controller_lock);
+	if (gic_arch_extn.irq_unmask)
+		gic_arch_extn.irq_unmask(d);
 	writel(mask, gic_dist_base(d) + GIC_DIST_ENABLE_SET + (gic_irq(d) / 32) * 4);
 	spin_unlock(&irq_controller_lock);
 }
@@ -167,6 +180,18 @@ gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val, bool force)
 }
 #endif
 
+#ifdef CONFIG_PM
+static int gic_set_wake(struct irq_data *d, unsigned int on)
+{
+	int ret = -ENXIO;
+
+	if (gic_arch_extn.irq_set_wake)
+		ret = gic_arch_extn.irq_set_wake(d, on);
+
+	return ret;
+}
+#endif
+
 static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 {
 	struct gic_chip_data *chip_data = get_irq_data(irq);
@@ -205,6 +230,9 @@ static struct irq_chip gic_chip = {
 #ifdef CONFIG_SMP
 	.irq_set_affinity	= gic_set_cpu,
 #endif
+#ifdef CONFIG_PM
+	.irq_set_wake	= gic_set_wake,
+#endif
 };
 
 void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 84557d3..0691f9d 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -34,6 +34,7 @@
 
 #ifndef __ASSEMBLY__
 extern void __iomem *gic_cpu_base_addr;
+extern struct irq_chip gic_arch_extn;
 
 void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
 void gic_secondary_init(unsigned int);
-- 
1.6.0.4

[PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Santosh Shilimkar]

Few architectures combine the GIC with an external interrupt controller.
On such systems it may be necessary to update both the GIC registers
and the external controller's registers to control IRQ behavior.

This can be addressed in couple of possible methods.
 1.	Export common GIC routines along with 'struct irq_chip gic_chip'
	and allow architectures to have custom function by override.

 2.	Provide architecture specific function pointer hooks
	within GIC library and leave platforms to add the necessary
	code as part of these hooks.

First one might be non-intrusive but have few shortcomings like arch needs
to have there own custom gic library. Locks used should be common since it
caters to same IRQs etc. Maintenance point of view also it leads to
multiple file fixes.

The second probably is cleaner and portable. It ensures that all the
common GIC infrastructure is not touched and also provides archs to
address their specific issue.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/common/gic.c               |   28 ++++++++++++++++++++++++++++
 arch/arm/include/asm/hardware/gic.h |    1 +
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 2243772..b4a9ea7 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -44,6 +44,15 @@ struct gic_chip_data {
 	void __iomem *cpu_base;
 };
 
+/* Default make arch specific GIC functions NULL */
+struct irq_chip gic_arch_extn = {
+	.irq_mask	= NULL,
+	.irq_unmask	= NULL,
+#ifdef CONFIG_PM
+	.irq_set_wake	= NULL,
+#endif
+};
+
 #ifndef MAX_GIC_NR
 #define MAX_GIC_NR	1
 #endif
@@ -84,6 +93,8 @@ static void gic_mask_irq(struct irq_data *d)
 
 	spin_lock(&irq_controller_lock);
 	writel(mask, gic_dist_base(d) + GIC_DIST_ENABLE_CLEAR + (gic_irq(d) / 32) * 4);
+	if (gic_arch_extn.irq_mask)
+		gic_arch_extn.irq_mask(d);
 	spin_unlock(&irq_controller_lock);
 }
 
@@ -92,6 +103,8 @@ static void gic_unmask_irq(struct irq_data *d)
 	u32 mask = 1 << (d->irq % 32);
 
 	spin_lock(&irq_controller_lock);
+	if (gic_arch_extn.irq_unmask)
+		gic_arch_extn.irq_unmask(d);
 	writel(mask, gic_dist_base(d) + GIC_DIST_ENABLE_SET + (gic_irq(d) / 32) * 4);
 	spin_unlock(&irq_controller_lock);
 }
@@ -167,6 +180,18 @@ gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val, bool force)
 }
 #endif
 
+#ifdef CONFIG_PM
+static int gic_set_wake(struct irq_data *d, unsigned int on)
+{
+	int ret = -ENXIO;
+
+	if (gic_arch_extn.irq_set_wake)
+		ret = gic_arch_extn.irq_set_wake(d, on);
+
+	return ret;
+}
+#endif
+
 static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 {
 	struct gic_chip_data *chip_data = get_irq_data(irq);
@@ -205,6 +230,9 @@ static struct irq_chip gic_chip = {
 #ifdef CONFIG_SMP
 	.irq_set_affinity	= gic_set_cpu,
 #endif
+#ifdef CONFIG_PM
+	.irq_set_wake	= gic_set_wake,
+#endif
 };
 
 void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 84557d3..0691f9d 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -34,6 +34,7 @@
 
 #ifndef __ASSEMBLY__
 extern void __iomem *gic_cpu_base_addr;
+extern struct irq_chip gic_arch_extn;
 
 void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
 void gic_secondary_init(unsigned int);
-- 
1.6.0.4

Re: [PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Colin Cross]

On Mon, Jan 24, 2011 at 12:51 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> Few architectures combine the GIC with an external interrupt controller.
> On such systems it may be necessary to update both the GIC registers
> and the external controller's registers to control IRQ behavior.
>
> This can be addressed in couple of possible methods.
>  1.     Export common GIC routines along with 'struct irq_chip gic_chip'
>        and allow architectures to have custom function by override.
>
>  2.     Provide architecture specific function pointer hooks
>        within GIC library and leave platforms to add the necessary
>        code as part of these hooks.
>
> First one might be non-intrusive but have few shortcomings like arch needs
> to have there own custom gic library. Locks used should be common since it
> caters to same IRQs etc. Maintenance point of view also it leads to
> multiple file fixes.
>
> The second probably is cleaner and portable. It ensures that all the
> common GIC infrastructure is not touched and also provides archs to
> address their specific issue.

This method would work for most of Tegra's needs, although we would
need gic_set_type and gic_ack_irq to have arch extensions as well.
However, it does not allow for irq_retrigger, which can be implemented
on Tegra.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Colin Cross]

On Mon, Jan 24, 2011 at 12:51 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> Few architectures combine the GIC with an external interrupt controller.
> On such systems it may be necessary to update both the GIC registers
> and the external controller's registers to control IRQ behavior.
>
> This can be addressed in couple of possible methods.
> ?1. ? ? Export common GIC routines along with 'struct irq_chip gic_chip'
> ? ? ? ?and allow architectures to have custom function by override.
>
> ?2. ? ? Provide architecture specific function pointer hooks
> ? ? ? ?within GIC library and leave platforms to add the necessary
> ? ? ? ?code as part of these hooks.
>
> First one might be non-intrusive but have few shortcomings like arch needs
> to have there own custom gic library. Locks used should be common since it
> caters to same IRQs etc. Maintenance point of view also it leads to
> multiple file fixes.
>
> The second probably is cleaner and portable. It ensures that all the
> common GIC infrastructure is not touched and also provides archs to
> address their specific issue.

This method would work for most of Tegra's needs, although we would
need gic_set_type and gic_ack_irq to have arch extensions as well.
However, it does not allow for irq_retrigger, which can be implemented
on Tegra.

Re: [PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Colin Cross]

On Mon, Jan 24, 2011 at 7:03 PM, Colin Cross <ccross@android.com> wrote:
> On Mon, Jan 24, 2011 at 12:51 AM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
>> Few architectures combine the GIC with an external interrupt controller.
>> On such systems it may be necessary to update both the GIC registers
>> and the external controller's registers to control IRQ behavior.
>>
>> This can be addressed in couple of possible methods.
>>  1.     Export common GIC routines along with 'struct irq_chip gic_chip'
>>        and allow architectures to have custom function by override.
>>
>>  2.     Provide architecture specific function pointer hooks
>>        within GIC library and leave platforms to add the necessary
>>        code as part of these hooks.
>>
>> First one might be non-intrusive but have few shortcomings like arch needs
>> to have there own custom gic library. Locks used should be common since it
>> caters to same IRQs etc. Maintenance point of view also it leads to
>> multiple file fixes.
>>
>> The second probably is cleaner and portable. It ensures that all the
>> common GIC infrastructure is not touched and also provides archs to
>> address their specific issue.
>
> This method would work for most of Tegra's needs, although we would
> need gic_set_type and gic_ack_irq to have arch extensions as well.
> However, it does not allow for irq_retrigger, which can be implemented
> on Tegra.

irq_retrigger does work with this method, the core IRQ code checks for
a return value if the retrigger was successful.  Tegra works with your
patch along with these changes:

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 0b6c043..7993f07 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct irq_data *d)
 static void gic_ack_irq(struct irq_data *d)
 {
 	spin_lock(&irq_controller_lock);
+	if (gic_arch_extn.irq_ack)
+		gic_arch_extn.irq_ack(d);
 	writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
 	spin_unlock(&irq_controller_lock);
 }
@@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
unsigned int type)
 	return 0;
 }

+static int gic_retrigger(struct irq_data *d)
+{
+	if (gic_arch_extn.irq_retrigger)
+		return gic_arch_extn.irq_retrigger(d);
+
+	return 0;
+}
+
 #ifdef CONFIG_SMP
 static int
 gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val, bool force)
@@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
 	.irq_mask		= gic_mask_irq,
 	.irq_unmask		= gic_unmask_irq,
 	.irq_set_type		= gic_set_type,
+	.irq_retrigger		= gic_retrigger,
 #ifdef CONFIG_SMP
 	.irq_set_affinity	= gic_set_cpu,
 #endif
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Colin Cross]

On Mon, Jan 24, 2011 at 7:03 PM, Colin Cross <ccross@android.com> wrote:
> On Mon, Jan 24, 2011 at 12:51 AM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
>> Few architectures combine the GIC with an external interrupt controller.
>> On such systems it may be necessary to update both the GIC registers
>> and the external controller's registers to control IRQ behavior.
>>
>> This can be addressed in couple of possible methods.
>> ?1. ? ? Export common GIC routines along with 'struct irq_chip gic_chip'
>> ? ? ? ?and allow architectures to have custom function by override.
>>
>> ?2. ? ? Provide architecture specific function pointer hooks
>> ? ? ? ?within GIC library and leave platforms to add the necessary
>> ? ? ? ?code as part of these hooks.
>>
>> First one might be non-intrusive but have few shortcomings like arch needs
>> to have there own custom gic library. Locks used should be common since it
>> caters to same IRQs etc. Maintenance point of view also it leads to
>> multiple file fixes.
>>
>> The second probably is cleaner and portable. It ensures that all the
>> common GIC infrastructure is not touched and also provides archs to
>> address their specific issue.
>
> This method would work for most of Tegra's needs, although we would
> need gic_set_type and gic_ack_irq to have arch extensions as well.
> However, it does not allow for irq_retrigger, which can be implemented
> on Tegra.

irq_retrigger does work with this method, the core IRQ code checks for
a return value if the retrigger was successful.  Tegra works with your
patch along with these changes:

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 0b6c043..7993f07 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct irq_data *d)
 static void gic_ack_irq(struct irq_data *d)
 {
 	spin_lock(&irq_controller_lock);
+	if (gic_arch_extn.irq_ack)
+		gic_arch_extn.irq_ack(d);
 	writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
 	spin_unlock(&irq_controller_lock);
 }
@@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
unsigned int type)
 	return 0;
 }

+static int gic_retrigger(struct irq_data *d)
+{
+	if (gic_arch_extn.irq_retrigger)
+		return gic_arch_extn.irq_retrigger(d);
+
+	return 0;
+}
+
 #ifdef CONFIG_SMP
 static int
 gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val, bool force)
@@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
 	.irq_mask		= gic_mask_irq,
 	.irq_unmask		= gic_unmask_irq,
 	.irq_set_type		= gic_set_type,
+	.irq_retrigger		= gic_retrigger,
 #ifdef CONFIG_SMP
 	.irq_set_affinity	= gic_set_cpu,
 #endif

RE: [PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Santosh Shilimkar]

> -----Original Message-----
> From: ccross@google.com [mailto:ccross@google.com] On Behalf Of
> Colin Cross
> Sent: Wednesday, January 26, 2011 2:25 AM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel@lists.infradead.org; linux-
> omap@vger.kernel.org; catalin.marinas@arm.com;
> linux@arm.linux.org.uk; linus.ml.walleij@gmail.com; Russell King
> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
> specific extensions
>
> On Mon, Jan 24, 2011 at 7:03 PM, Colin Cross <ccross@android.com>
> wrote:
> > On Mon, Jan 24, 2011 at 12:51 AM, Santosh Shilimkar
> > <santosh.shilimkar@ti.com> wrote:
> >> Few architectures combine the GIC with an external interrupt
> controller.
> >> On such systems it may be necessary to update both the GIC
> registers
> >> and the external controller's registers to control IRQ behavior.
> >>
> >> This can be addressed in couple of possible methods.
> >>  1.     Export common GIC routines along with 'struct irq_chip
> gic_chip'
> >>        and allow architectures to have custom function by
> override.
> >>
> >>  2.     Provide architecture specific function pointer hooks
> >>        within GIC library and leave platforms to add the
> necessary
> >>        code as part of these hooks.
> >>
> >> First one might be non-intrusive but have few shortcomings like
> arch needs
> >> to have there own custom gic library. Locks used should be common
> since it
> >> caters to same IRQs etc. Maintenance point of view also it leads
> to
> >> multiple file fixes.
> >>
> >> The second probably is cleaner and portable. It ensures that all
> the
> >> common GIC infrastructure is not touched and also provides archs
> to
> >> address their specific issue.
> >
> > This method would work for most of Tegra's needs, although we
> would
> > need gic_set_type and gic_ack_irq to have arch extensions as well.
> > However, it does not allow for irq_retrigger, which can be
> implemented
> > on Tegra.
>
> irq_retrigger does work with this method, the core IRQ code checks
> for
> a return value if the retrigger was successful.  Tegra works with
> your
> patch along with these changes:
>
Great.
Can I fold below changes in my patch and add you ack and tested-by?

> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index 0b6c043..7993f07 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct irq_data
> *d)
>  static void gic_ack_irq(struct irq_data *d)
>  {
>  	spin_lock(&irq_controller_lock);
> +	if (gic_arch_extn.irq_ack)
> +		gic_arch_extn.irq_ack(d);
>  	writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>  	spin_unlock(&irq_controller_lock);
>  }
> @@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
> unsigned int type)
>  	return 0;
>  }
>
> +static int gic_retrigger(struct irq_data *d)
> +{
> +	if (gic_arch_extn.irq_retrigger)
> +		return gic_arch_extn.irq_retrigger(d);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_SMP
>  static int
>  gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val,
> bool force)
> @@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
>  	.irq_mask		= gic_mask_irq,
>  	.irq_unmask		= gic_unmask_irq,
>  	.irq_set_type		= gic_set_type,
> +	.irq_retrigger		= gic_retrigger,
>  #ifdef CONFIG_SMP
>  	.irq_set_affinity	= gic_set_cpu,
>  #endif
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Santosh Shilimkar]

> -----Original Message-----
> From: ccross at google.com [mailto:ccross at google.com] On Behalf Of
> Colin Cross
> Sent: Wednesday, January 26, 2011 2:25 AM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; linux-
> omap at vger.kernel.org; catalin.marinas at arm.com;
> linux at arm.linux.org.uk; linus.ml.walleij at gmail.com; Russell King
> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
> specific extensions
>
> On Mon, Jan 24, 2011 at 7:03 PM, Colin Cross <ccross@android.com>
> wrote:
> > On Mon, Jan 24, 2011 at 12:51 AM, Santosh Shilimkar
> > <santosh.shilimkar@ti.com> wrote:
> >> Few architectures combine the GIC with an external interrupt
> controller.
> >> On such systems it may be necessary to update both the GIC
> registers
> >> and the external controller's registers to control IRQ behavior.
> >>
> >> This can be addressed in couple of possible methods.
> >> ?1. ? ? Export common GIC routines along with 'struct irq_chip
> gic_chip'
> >> ? ? ? ?and allow architectures to have custom function by
> override.
> >>
> >> ?2. ? ? Provide architecture specific function pointer hooks
> >> ? ? ? ?within GIC library and leave platforms to add the
> necessary
> >> ? ? ? ?code as part of these hooks.
> >>
> >> First one might be non-intrusive but have few shortcomings like
> arch needs
> >> to have there own custom gic library. Locks used should be common
> since it
> >> caters to same IRQs etc. Maintenance point of view also it leads
> to
> >> multiple file fixes.
> >>
> >> The second probably is cleaner and portable. It ensures that all
> the
> >> common GIC infrastructure is not touched and also provides archs
> to
> >> address their specific issue.
> >
> > This method would work for most of Tegra's needs, although we
> would
> > need gic_set_type and gic_ack_irq to have arch extensions as well.
> > However, it does not allow for irq_retrigger, which can be
> implemented
> > on Tegra.
>
> irq_retrigger does work with this method, the core IRQ code checks
> for
> a return value if the retrigger was successful.  Tegra works with
> your
> patch along with these changes:
>
Great.
Can I fold below changes in my patch and add you ack and tested-by?

> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index 0b6c043..7993f07 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct irq_data
> *d)
>  static void gic_ack_irq(struct irq_data *d)
>  {
>  	spin_lock(&irq_controller_lock);
> +	if (gic_arch_extn.irq_ack)
> +		gic_arch_extn.irq_ack(d);
>  	writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>  	spin_unlock(&irq_controller_lock);
>  }
> @@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
> unsigned int type)
>  	return 0;
>  }
>
> +static int gic_retrigger(struct irq_data *d)
> +{
> +	if (gic_arch_extn.irq_retrigger)
> +		return gic_arch_extn.irq_retrigger(d);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_SMP
>  static int
>  gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val,
> bool force)
> @@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
>  	.irq_mask		= gic_mask_irq,
>  	.irq_unmask		= gic_unmask_irq,
>  	.irq_set_type		= gic_set_type,
> +	.irq_retrigger		= gic_retrigger,
>  #ifdef CONFIG_SMP
>  	.irq_set_affinity	= gic_set_cpu,
>  #endif

Re: [PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Colin Cross]

On Tue, Jan 25, 2011 at 11:22 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
>> -----Original Message-----
>> From: ccross@google.com [mailto:ccross@google.com] On Behalf Of
>> Colin Cross
>> Sent: Wednesday, January 26, 2011 2:25 AM
>> To: Santosh Shilimkar
>> Cc: linux-arm-kernel@lists.infradead.org; linux-
>> omap@vger.kernel.org; catalin.marinas@arm.com;
>> linux@arm.linux.org.uk; linus.ml.walleij@gmail.com; Russell King
>> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
>> specific extensions
>>
>> On Mon, Jan 24, 2011 at 7:03 PM, Colin Cross <ccross@android.com>
>> wrote:
>> > On Mon, Jan 24, 2011 at 12:51 AM, Santosh Shilimkar
>> > <santosh.shilimkar@ti.com> wrote:
>> >> Few architectures combine the GIC with an external interrupt
>> controller.
>> >> On such systems it may be necessary to update both the GIC
>> registers
>> >> and the external controller's registers to control IRQ behavior.
>> >>
>> >> This can be addressed in couple of possible methods.
>> >>  1.     Export common GIC routines along with 'struct irq_chip
>> gic_chip'
>> >>        and allow architectures to have custom function by
>> override.
>> >>
>> >>  2.     Provide architecture specific function pointer hooks
>> >>        within GIC library and leave platforms to add the
>> necessary
>> >>        code as part of these hooks.
>> >>
>> >> First one might be non-intrusive but have few shortcomings like
>> arch needs
>> >> to have there own custom gic library. Locks used should be common
>> since it
>> >> caters to same IRQs etc. Maintenance point of view also it leads
>> to
>> >> multiple file fixes.
>> >>
>> >> The second probably is cleaner and portable. It ensures that all
>> the
>> >> common GIC infrastructure is not touched and also provides archs
>> to
>> >> address their specific issue.
>> >
>> > This method would work for most of Tegra's needs, although we
>> would
>> > need gic_set_type and gic_ack_irq to have arch extensions as well.
>> > However, it does not allow for irq_retrigger, which can be
>> implemented
>> > on Tegra.
>>
>> irq_retrigger does work with this method, the core IRQ code checks
>> for
>> a return value if the retrigger was successful.  Tegra works with
>> your
>> patch along with these changes:
>>
> Great.
> Can I fold below changes in my patch and add you ack and tested-by?

Sure

>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> index 0b6c043..7993f07 100644
>> --- a/arch/arm/common/gic.c
>> +++ b/arch/arm/common/gic.c
>> @@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct irq_data
>> *d)
>>  static void gic_ack_irq(struct irq_data *d)
>>  {
>>       spin_lock(&irq_controller_lock);
>> +     if (gic_arch_extn.irq_ack)
>> +             gic_arch_extn.irq_ack(d);
>>       writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>>       spin_unlock(&irq_controller_lock);
>>  }
>> @@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
>> unsigned int type)
>>       return 0;
>>  }
>>
>> +static int gic_retrigger(struct irq_data *d)
>> +{
>> +     if (gic_arch_extn.irq_retrigger)
>> +             return gic_arch_extn.irq_retrigger(d);
>> +
>> +     return 0;
>> +}
>> +
>>  #ifdef CONFIG_SMP
>>  static int
>>  gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val,
>> bool force)
>> @@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
>>       .irq_mask               = gic_mask_irq,
>>       .irq_unmask             = gic_unmask_irq,
>>       .irq_set_type           = gic_set_type,
>> +     .irq_retrigger          = gic_retrigger,
>>  #ifdef CONFIG_SMP
>>       .irq_set_affinity       = gic_set_cpu,
>>  #endif
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Colin Cross]

On Tue, Jan 25, 2011 at 11:22 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
>> -----Original Message-----
>> From: ccross at google.com [mailto:ccross at google.com] On Behalf Of
>> Colin Cross
>> Sent: Wednesday, January 26, 2011 2:25 AM
>> To: Santosh Shilimkar
>> Cc: linux-arm-kernel at lists.infradead.org; linux-
>> omap at vger.kernel.org; catalin.marinas at arm.com;
>> linux at arm.linux.org.uk; linus.ml.walleij at gmail.com; Russell King
>> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
>> specific extensions
>>
>> On Mon, Jan 24, 2011 at 7:03 PM, Colin Cross <ccross@android.com>
>> wrote:
>> > On Mon, Jan 24, 2011 at 12:51 AM, Santosh Shilimkar
>> > <santosh.shilimkar@ti.com> wrote:
>> >> Few architectures combine the GIC with an external interrupt
>> controller.
>> >> On such systems it may be necessary to update both the GIC
>> registers
>> >> and the external controller's registers to control IRQ behavior.
>> >>
>> >> This can be addressed in couple of possible methods.
>> >> ?1. ? ? Export common GIC routines along with 'struct irq_chip
>> gic_chip'
>> >> ? ? ? ?and allow architectures to have custom function by
>> override.
>> >>
>> >> ?2. ? ? Provide architecture specific function pointer hooks
>> >> ? ? ? ?within GIC library and leave platforms to add the
>> necessary
>> >> ? ? ? ?code as part of these hooks.
>> >>
>> >> First one might be non-intrusive but have few shortcomings like
>> arch needs
>> >> to have there own custom gic library. Locks used should be common
>> since it
>> >> caters to same IRQs etc. Maintenance point of view also it leads
>> to
>> >> multiple file fixes.
>> >>
>> >> The second probably is cleaner and portable. It ensures that all
>> the
>> >> common GIC infrastructure is not touched and also provides archs
>> to
>> >> address their specific issue.
>> >
>> > This method would work for most of Tegra's needs, although we
>> would
>> > need gic_set_type and gic_ack_irq to have arch extensions as well.
>> > However, it does not allow for irq_retrigger, which can be
>> implemented
>> > on Tegra.
>>
>> irq_retrigger does work with this method, the core IRQ code checks
>> for
>> a return value if the retrigger was successful. ?Tegra works with
>> your
>> patch along with these changes:
>>
> Great.
> Can I fold below changes in my patch and add you ack and tested-by?

Sure

>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> index 0b6c043..7993f07 100644
>> --- a/arch/arm/common/gic.c
>> +++ b/arch/arm/common/gic.c
>> @@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct irq_data
>> *d)
>> ?static void gic_ack_irq(struct irq_data *d)
>> ?{
>> ? ? ? spin_lock(&irq_controller_lock);
>> + ? ? if (gic_arch_extn.irq_ack)
>> + ? ? ? ? ? ? gic_arch_extn.irq_ack(d);
>> ? ? ? writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>> ? ? ? spin_unlock(&irq_controller_lock);
>> ?}
>> @@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
>> unsigned int type)
>> ? ? ? return 0;
>> ?}
>>
>> +static int gic_retrigger(struct irq_data *d)
>> +{
>> + ? ? if (gic_arch_extn.irq_retrigger)
>> + ? ? ? ? ? ? return gic_arch_extn.irq_retrigger(d);
>> +
>> + ? ? return 0;
>> +}
>> +
>> ?#ifdef CONFIG_SMP
>> ?static int
>> ?gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val,
>> bool force)
>> @@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
>> ? ? ? .irq_mask ? ? ? ? ? ? ? = gic_mask_irq,
>> ? ? ? .irq_unmask ? ? ? ? ? ? = gic_unmask_irq,
>> ? ? ? .irq_set_type ? ? ? ? ? = gic_set_type,
>> + ? ? .irq_retrigger ? ? ? ? ?= gic_retrigger,
>> ?#ifdef CONFIG_SMP
>> ? ? ? .irq_set_affinity ? ? ? = gic_set_cpu,
>> ?#endif
>

RE: [PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Santosh Shilimkar]

> -----Original Message-----
> From: ccross@google.com [mailto:ccross@google.com] On Behalf Of
> Colin Cross
> Sent: Wednesday, January 26, 2011 12:54 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel@lists.infradead.org; linux-
> omap@vger.kernel.org; catalin.marinas@arm.com;
> linux@arm.linux.org.uk; linus.ml.walleij@gmail.com; Russell King
> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
> specific extensions
>
[....]

> >>
> > Great.
> > Can I fold below changes in my patch and add you ack and tested-
> by?
>
> Sure

After reading your initial comment, you mentioned you need to have
'gic_set_type' as well. Is this still true. If yes then we need to
have arch_extn call for that as well.
>
> >> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> >> index 0b6c043..7993f07 100644
> >> --- a/arch/arm/common/gic.c
> >> +++ b/arch/arm/common/gic.c
> >> @@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct
> irq_data
> >> *d)
> >>  static void gic_ack_irq(struct irq_data *d)
> >>  {
> >>       spin_lock(&irq_controller_lock);
> >> +     if (gic_arch_extn.irq_ack)
> >> +             gic_arch_extn.irq_ack(d);
> >>       writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
> >>       spin_unlock(&irq_controller_lock);
> >>  }
> >> @@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
> >> unsigned int type)
> >>       return 0;
> >>  }
> >>
> >> +static int gic_retrigger(struct irq_data *d)
> >> +{
> >> +     if (gic_arch_extn.irq_retrigger)
> >> +             return gic_arch_extn.irq_retrigger(d);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >>  #ifdef CONFIG_SMP
> >>  static int
> >>  gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val,
> >> bool force)
> >> @@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
> >>       .irq_mask               = gic_mask_irq,
> >>       .irq_unmask             = gic_unmask_irq,
> >>       .irq_set_type           = gic_set_type,
> >> +     .irq_retrigger          = gic_retrigger,
> >>  #ifdef CONFIG_SMP
> >>       .irq_set_affinity       = gic_set_cpu,
> >>  #endif
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Santosh Shilimkar]

> -----Original Message-----
> From: ccross at google.com [mailto:ccross at google.com] On Behalf Of
> Colin Cross
> Sent: Wednesday, January 26, 2011 12:54 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; linux-
> omap at vger.kernel.org; catalin.marinas at arm.com;
> linux at arm.linux.org.uk; linus.ml.walleij at gmail.com; Russell King
> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
> specific extensions
>
[....]

> >>
> > Great.
> > Can I fold below changes in my patch and add you ack and tested-
> by?
>
> Sure

After reading your initial comment, you mentioned you need to have
'gic_set_type' as well. Is this still true. If yes then we need to
have arch_extn call for that as well.
>
> >> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> >> index 0b6c043..7993f07 100644
> >> --- a/arch/arm/common/gic.c
> >> +++ b/arch/arm/common/gic.c
> >> @@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct
> irq_data
> >> *d)
> >> ?static void gic_ack_irq(struct irq_data *d)
> >> ?{
> >> ? ? ? spin_lock(&irq_controller_lock);
> >> + ? ? if (gic_arch_extn.irq_ack)
> >> + ? ? ? ? ? ? gic_arch_extn.irq_ack(d);
> >> ? ? ? writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
> >> ? ? ? spin_unlock(&irq_controller_lock);
> >> ?}
> >> @@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
> >> unsigned int type)
> >> ? ? ? return 0;
> >> ?}
> >>
> >> +static int gic_retrigger(struct irq_data *d)
> >> +{
> >> + ? ? if (gic_arch_extn.irq_retrigger)
> >> + ? ? ? ? ? ? return gic_arch_extn.irq_retrigger(d);
> >> +
> >> + ? ? return 0;
> >> +}
> >> +
> >> ?#ifdef CONFIG_SMP
> >> ?static int
> >> ?gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val,
> >> bool force)
> >> @@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
> >> ? ? ? .irq_mask ? ? ? ? ? ? ? = gic_mask_irq,
> >> ? ? ? .irq_unmask ? ? ? ? ? ? = gic_unmask_irq,
> >> ? ? ? .irq_set_type ? ? ? ? ? = gic_set_type,
> >> + ? ? .irq_retrigger ? ? ? ? ?= gic_retrigger,
> >> ?#ifdef CONFIG_SMP
> >> ? ? ? .irq_set_affinity ? ? ? = gic_set_cpu,
> >> ?#endif
> >

Re: [PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Colin Cross]

On Tue, Jan 25, 2011 at 11:31 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
>> -----Original Message-----
>> From: ccross@google.com [mailto:ccross@google.com] On Behalf Of
>> Colin Cross
>> Sent: Wednesday, January 26, 2011 12:54 PM
>> To: Santosh Shilimkar
>> Cc: linux-arm-kernel@lists.infradead.org; linux-
>> omap@vger.kernel.org; catalin.marinas@arm.com;
>> linux@arm.linux.org.uk; linus.ml.walleij@gmail.com; Russell King
>> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
>> specific extensions
>>
> [....]
>
>> >>
>> > Great.
>> > Can I fold below changes in my patch and add you ack and tested-
>> by?
>>
>> Sure
>
> After reading your initial comment, you mentioned you need to have
> 'gic_set_type' as well. Is this still true. If yes then we need to
> have arch_extn call for that as well.

You are right, I missed adding the extension for gic_set_type.  My
testing doesn't cover that case right now, because I don't have any
drivers updated to linux-next that use a wake source that is
compatible with Tegra's lowest power suspend mode, and that is the
only time the extension to gic_set_type is necessary.

>> >> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> >> index 0b6c043..7993f07 100644
>> >> --- a/arch/arm/common/gic.c
>> >> +++ b/arch/arm/common/gic.c
>> >> @@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct
>> irq_data
>> >> *d)
>> >>  static void gic_ack_irq(struct irq_data *d)
>> >>  {
>> >>       spin_lock(&irq_controller_lock);
>> >> +     if (gic_arch_extn.irq_ack)
>> >> +             gic_arch_extn.irq_ack(d);
>> >>       writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>> >>       spin_unlock(&irq_controller_lock);
>> >>  }
>> >> @@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
>> >> unsigned int type)
>> >>       return 0;
>> >>  }
>> >>
>> >> +static int gic_retrigger(struct irq_data *d)
>> >> +{
>> >> +     if (gic_arch_extn.irq_retrigger)
>> >> +             return gic_arch_extn.irq_retrigger(d);
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +
>> >>  #ifdef CONFIG_SMP
>> >>  static int
>> >>  gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val,
>> >> bool force)
>> >> @@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
>> >>       .irq_mask               = gic_mask_irq,
>> >>       .irq_unmask             = gic_unmask_irq,
>> >>       .irq_set_type           = gic_set_type,
>> >> +     .irq_retrigger          = gic_retrigger,
>> >>  #ifdef CONFIG_SMP
>> >>       .irq_set_affinity       = gic_set_cpu,
>> >>  #endif
>> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Colin Cross]

On Tue, Jan 25, 2011 at 11:31 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
>> -----Original Message-----
>> From: ccross at google.com [mailto:ccross at google.com] On Behalf Of
>> Colin Cross
>> Sent: Wednesday, January 26, 2011 12:54 PM
>> To: Santosh Shilimkar
>> Cc: linux-arm-kernel at lists.infradead.org; linux-
>> omap at vger.kernel.org; catalin.marinas at arm.com;
>> linux at arm.linux.org.uk; linus.ml.walleij at gmail.com; Russell King
>> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
>> specific extensions
>>
> [....]
>
>> >>
>> > Great.
>> > Can I fold below changes in my patch and add you ack and tested-
>> by?
>>
>> Sure
>
> After reading your initial comment, you mentioned you need to have
> 'gic_set_type' as well. Is this still true. If yes then we need to
> have arch_extn call for that as well.

You are right, I missed adding the extension for gic_set_type.  My
testing doesn't cover that case right now, because I don't have any
drivers updated to linux-next that use a wake source that is
compatible with Tegra's lowest power suspend mode, and that is the
only time the extension to gic_set_type is necessary.

>> >> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> >> index 0b6c043..7993f07 100644
>> >> --- a/arch/arm/common/gic.c
>> >> +++ b/arch/arm/common/gic.c
>> >> @@ -90,6 +90,8 @@ static inline unsigned int gic_irq(struct
>> irq_data
>> >> *d)
>> >> ?static void gic_ack_irq(struct irq_data *d)
>> >> ?{
>> >> ? ? ? spin_lock(&irq_controller_lock);
>> >> + ? ? if (gic_arch_extn.irq_ack)
>> >> + ? ? ? ? ? ? gic_arch_extn.irq_ack(d);
>> >> ? ? ? writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>> >> ? ? ? spin_unlock(&irq_controller_lock);
>> >> ?}
>> >> @@ -161,6 +163,14 @@ static int gic_set_type(struct irq_data *d,
>> >> unsigned int type)
>> >> ? ? ? return 0;
>> >> ?}
>> >>
>> >> +static int gic_retrigger(struct irq_data *d)
>> >> +{
>> >> + ? ? if (gic_arch_extn.irq_retrigger)
>> >> + ? ? ? ? ? ? return gic_arch_extn.irq_retrigger(d);
>> >> +
>> >> + ? ? return 0;
>> >> +}
>> >> +
>> >> ?#ifdef CONFIG_SMP
>> >> ?static int
>> >> ?gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val,
>> >> bool force)
>> >> @@ -234,6 +244,7 @@ static struct irq_chip gic_chip = {
>> >> ? ? ? .irq_mask ? ? ? ? ? ? ? = gic_mask_irq,
>> >> ? ? ? .irq_unmask ? ? ? ? ? ? = gic_unmask_irq,
>> >> ? ? ? .irq_set_type ? ? ? ? ? = gic_set_type,
>> >> + ? ? .irq_retrigger ? ? ? ? ?= gic_retrigger,
>> >> ?#ifdef CONFIG_SMP
>> >> ? ? ? .irq_set_affinity ? ? ? = gic_set_cpu,
>> >> ?#endif
>> >
>

RE: [PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Santosh Shilimkar]

> -----Original Message-----
> From: ccross@google.com [mailto:ccross@google.com] On Behalf Of
> Colin Cross
> Sent: Wednesday, January 26, 2011 1:23 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel@lists.infradead.org; linux-
> omap@vger.kernel.org; catalin.marinas@arm.com;
> linux@arm.linux.org.uk; linus.ml.walleij@gmail.com; Russell King
> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
> specific extensions
>
> On Tue, Jan 25, 2011 at 11:31 PM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
> >> -----Original Message-----
> >> From: ccross@google.com [mailto:ccross@google.com] On Behalf Of
> >> Colin Cross
> >> Sent: Wednesday, January 26, 2011 12:54 PM
> >> To: Santosh Shilimkar
> >> Cc: linux-arm-kernel@lists.infradead.org; linux-
> >> omap@vger.kernel.org; catalin.marinas@arm.com;
> >> linux@arm.linux.org.uk; linus.ml.walleij@gmail.com; Russell King
> >> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
> >> specific extensions
> >>
> > [....]
> >
> >> >>
> >> > Great.
> >> > Can I fold below changes in my patch and add you ack and
> tested-
> >> by?
> >>
> >> Sure
> >
> > After reading your initial comment, you mentioned you need to have
> > 'gic_set_type' as well. Is this still true. If yes then we need to
> > have arch_extn call for that as well.
>
> You are right, I missed adding the extension for gic_set_type.  My
> testing doesn't cover that case right now, because I don't have any
> drivers updated to linux-next that use a wake source that is
> compatible with Tegra's lowest power suspend mode, and that is the
> only time the extension to gic_set_type is necessary.
>
Ok.
So I will go ahead and add an extension for the same so that we have
most of the usecases covered.

Regards,
Santosh

[PATCH 1/5] ARM: gic: Add hooks for architecture specific extensions

[Santosh Shilimkar]

> -----Original Message-----
> From: ccross at google.com [mailto:ccross at google.com] On Behalf Of
> Colin Cross
> Sent: Wednesday, January 26, 2011 1:23 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; linux-
> omap at vger.kernel.org; catalin.marinas at arm.com;
> linux at arm.linux.org.uk; linus.ml.walleij at gmail.com; Russell King
> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
> specific extensions
>
> On Tue, Jan 25, 2011 at 11:31 PM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
> >> -----Original Message-----
> >> From: ccross at google.com [mailto:ccross at google.com] On Behalf Of
> >> Colin Cross
> >> Sent: Wednesday, January 26, 2011 12:54 PM
> >> To: Santosh Shilimkar
> >> Cc: linux-arm-kernel at lists.infradead.org; linux-
> >> omap at vger.kernel.org; catalin.marinas at arm.com;
> >> linux at arm.linux.org.uk; linus.ml.walleij at gmail.com; Russell King
> >> Subject: Re: [PATCH 1/5] ARM: gic: Add hooks for architecture
> >> specific extensions
> >>
> > [....]
> >
> >> >>
> >> > Great.
> >> > Can I fold below changes in my patch and add you ack and
> tested-
> >> by?
> >>
> >> Sure
> >
> > After reading your initial comment, you mentioned you need to have
> > 'gic_set_type' as well. Is this still true. If yes then we need to
> > have arch_extn call for that as well.
>
> You are right, I missed adding the extension for gic_set_type.  My
> testing doesn't cover that case right now, because I don't have any
> drivers updated to linux-next that use a wake source that is
> compatible with Tegra's lowest power suspend mode, and that is the
> only time the extension to gic_set_type is necessary.
>
Ok.
So I will go ahead and add an extension for the same so that we have
most of the usecases covered.

Regards,
Santosh

[PATCH 2/5] ARM: gic: Add distributor and interface enable/disable accessory api

[Santosh Shilimkar]

The power management code needs to have access to enable/disable the
gic cpu interface and distributor based on targetted low power
states.

This patch adds and exports one API each for distributor and cpu
interface enable/disable.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/common/gic.c               |   24 ++++++++++++++++++++++++
 arch/arm/include/asm/hardware/gic.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index b4a9ea7..6384bb7 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -369,6 +369,30 @@ void __cpuinit gic_enable_ppi(unsigned int irq)
 	local_irq_restore(flags);
 }
 
+/* Used in power management paths */
+void gic_secondary_set(unsigned int gic_nr, unsigned int on)
+{
+	BUG_ON(gic_nr >= MAX_GIC_NR);
+
+	if (on) {
+		__raw_writel(0xf0, gic_data[gic_nr].cpu_base + GIC_CPU_PRIMASK);
+		__raw_writel(1, gic_data[gic_nr].cpu_base + GIC_CPU_CTRL);
+
+	} else {
+		__raw_writel(0, gic_data[gic_nr].cpu_base + GIC_CPU_CTRL);
+	}
+}
+
+void gic_dist_set(unsigned int gic_nr, unsigned int on)
+{
+	BUG_ON(gic_nr >= MAX_GIC_NR);
+
+	if (on)
+		__raw_writel(0x1, gic_data[gic_nr].dist_base + GIC_DIST_CTRL);
+	else
+		__raw_writel(0, gic_data[gic_nr].cpu_base + GIC_CPU_CTRL);
+}
+
 #ifdef CONFIG_SMP
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 {
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 0691f9d..638d9dc 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -41,6 +41,8 @@ void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
 void gic_enable_ppi(unsigned int);
+void gic_secondary_set(unsigned int gic_nr, unsigned int on);
+void gic_dist_set(unsigned int gic_nr, unsigned int on);
 #endif
 
 #endif
-- 
1.6.0.4

[PATCH 2/5] ARM: gic: Add distributor and interface enable/disable accessory api

[Santosh Shilimkar]

The power management code needs to have access to enable/disable the
gic cpu interface and distributor based on targetted low power
states.

This patch adds and exports one API each for distributor and cpu
interface enable/disable.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/common/gic.c               |   24 ++++++++++++++++++++++++
 arch/arm/include/asm/hardware/gic.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index b4a9ea7..6384bb7 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -369,6 +369,30 @@ void __cpuinit gic_enable_ppi(unsigned int irq)
 	local_irq_restore(flags);
 }
 
+/* Used in power management paths */
+void gic_secondary_set(unsigned int gic_nr, unsigned int on)
+{
+	BUG_ON(gic_nr >= MAX_GIC_NR);
+
+	if (on) {
+		__raw_writel(0xf0, gic_data[gic_nr].cpu_base + GIC_CPU_PRIMASK);
+		__raw_writel(1, gic_data[gic_nr].cpu_base + GIC_CPU_CTRL);
+
+	} else {
+		__raw_writel(0, gic_data[gic_nr].cpu_base + GIC_CPU_CTRL);
+	}
+}
+
+void gic_dist_set(unsigned int gic_nr, unsigned int on)
+{
+	BUG_ON(gic_nr >= MAX_GIC_NR);
+
+	if (on)
+		__raw_writel(0x1, gic_data[gic_nr].dist_base + GIC_DIST_CTRL);
+	else
+		__raw_writel(0, gic_data[gic_nr].cpu_base + GIC_CPU_CTRL);
+}
+
 #ifdef CONFIG_SMP
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 {
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 0691f9d..638d9dc 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -41,6 +41,8 @@ void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
 void gic_enable_ppi(unsigned int);
+void gic_secondary_set(unsigned int gic_nr, unsigned int on);
+void gic_dist_set(unsigned int gic_nr, unsigned int on);
 #endif
 
 #endif
-- 
1.6.0.4

RE: [PATCH 2/5] ARM: gic: Add distributor and interface enable/disable accessory api

[Santosh Shilimkar]

> -----Original Message-----
> From: Santosh Shilimkar [mailto:santosh.shilimkar@ti.com]
> Sent: Monday, January 24, 2011 2:21 PM
> To: linux-arm-kernel@lists.infradead.org
> Cc: linux-omap@vger.kernel.org; ccross@android.com;
> catalin.marinas@arm.com; linux@arm.linux.org.uk;
> linus.ml.walleij@gmail.com; Santosh Shilimkar; Russell King
> Subject: [PATCH 2/5] ARM: gic: Add distributor and interface
> enable/disable accessory api
>
> The power management code needs to have access to enable/disable the
> gic cpu interface and distributor based on targetted low power
> states.
>
> This patch adds and exports one API each for distributor and cpu
> interface enable/disable.
>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
As per off-the list discussion with Russell, I am going to drop
this patch for time being and handle this in OMAP code instead
till common GIC code refactoring is sorted out.

[PATCH 2/5] ARM: gic: Add distributor and interface enable/disable accessory api

[Santosh Shilimkar]

> -----Original Message-----
> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
> Sent: Monday, January 24, 2011 2:21 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: linux-omap at vger.kernel.org; ccross at android.com;
> catalin.marinas at arm.com; linux at arm.linux.org.uk;
> linus.ml.walleij at gmail.com; Santosh Shilimkar; Russell King
> Subject: [PATCH 2/5] ARM: gic: Add distributor and interface
> enable/disable accessory api
>
> The power management code needs to have access to enable/disable the
> gic cpu interface and distributor based on targetted low power
> states.
>
> This patch adds and exports one API each for distributor and cpu
> interface enable/disable.
>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
As per off-the list discussion with Russell, I am going to drop
this patch for time being and handle this in OMAP code instead
till common GIC code refactoring is sorted out.

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

In CPU low power state, local timer looses its register context. This
patch adds context save restore hooks which can be used by platforms
appropriately.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/localtimer.h |   13 +++++++++++++
 arch/arm/include/asm/smp_twd.h    |    2 ++
 arch/arm/kernel/smp_twd.c         |   24 ++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/localtimer.h b/arch/arm/include/asm/localtimer.h
index 6bc63ab..6753754 100644
--- a/arch/arm/include/asm/localtimer.h
+++ b/arch/arm/include/asm/localtimer.h
@@ -31,6 +31,9 @@ asmlinkage void do_local_timer(struct pt_regs *);
 
 #define local_timer_ack()	twd_timer_ack()
 
+#define local_timer_save(cpu)		twd_context_save(cpu);
+#define local_timer_restore(cpu)	twd_context_save(cpu);
+
 #else
 
 /*
@@ -46,6 +49,16 @@ int local_timer_ack(void);
  */
 void local_timer_setup(struct clock_event_device *);
 
+/*
+ * Save local timer register context
+ */
+void local_timer_save(unsigned int cpu);
+
+/*
+ * Restore local timer register context
+ */
+void local_timer_restoree(unsigned int cpu);
+
 #endif
 
 #endif
diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index fed9981..7828f29 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -24,5 +24,7 @@ extern void __iomem *twd_base;
 
 int twd_timer_ack(void);
 void twd_timer_setup(struct clock_event_device *);
+void twd_context_save(unsigned int cpu);
+void twd_context_restore(unsigned int cpu);
 
 #endif
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..f3098e4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -24,6 +24,12 @@
 /* set up by the platform code */
 void __iomem *twd_base;
 
+/* Timer context to be saved in low power modes */
+struct twd_registers {
+	unsigned long timer_ctrl;
+	unsigned long timer_load;
+};
+static DEFINE_PER_CPU(struct twd_registers, twd_context);
 static unsigned long twd_timer_rate;
 
 static void twd_set_mode(enum clock_event_mode mode,
@@ -145,3 +151,21 @@ void __cpuinit twd_timer_setup(struct clock_event_device *clk)
 
 	clockevents_register_device(clk);
 }
+
+/* Low power context save */
+void twd_context_save(unsigned int cpu)
+{
+	struct twd_registers *regs = &per_cpu(twd_context, cpu);
+
+	regs->timer_ctrl = __raw_readl(twd_base + TWD_TIMER_CONTROL);
+	regs->timer_load = __raw_readl(twd_base + TWD_TIMER_LOAD);
+}
+
+/* Low power context restore */
+void twd_context_restore(unsigned int cpu)
+{
+	struct twd_registers *regs = &per_cpu(twd_context, cpu);
+
+	__raw_writel(regs->timer_load, twd_base + TWD_TIMER_LOAD);
+	__raw_writel(regs->timer_ctrl, twd_base + TWD_TIMER_CONTROL);
+}
-- 
1.6.0.4

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

In CPU low power state, local timer looses its register context. This
patch adds context save restore hooks which can be used by platforms
appropriately.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/localtimer.h |   13 +++++++++++++
 arch/arm/include/asm/smp_twd.h    |    2 ++
 arch/arm/kernel/smp_twd.c         |   24 ++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/localtimer.h b/arch/arm/include/asm/localtimer.h
index 6bc63ab..6753754 100644
--- a/arch/arm/include/asm/localtimer.h
+++ b/arch/arm/include/asm/localtimer.h
@@ -31,6 +31,9 @@ asmlinkage void do_local_timer(struct pt_regs *);
 
 #define local_timer_ack()	twd_timer_ack()
 
+#define local_timer_save(cpu)		twd_context_save(cpu);
+#define local_timer_restore(cpu)	twd_context_save(cpu);
+
 #else
 
 /*
@@ -46,6 +49,16 @@ int local_timer_ack(void);
  */
 void local_timer_setup(struct clock_event_device *);
 
+/*
+ * Save local timer register context
+ */
+void local_timer_save(unsigned int cpu);
+
+/*
+ * Restore local timer register context
+ */
+void local_timer_restoree(unsigned int cpu);
+
 #endif
 
 #endif
diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index fed9981..7828f29 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -24,5 +24,7 @@ extern void __iomem *twd_base;
 
 int twd_timer_ack(void);
 void twd_timer_setup(struct clock_event_device *);
+void twd_context_save(unsigned int cpu);
+void twd_context_restore(unsigned int cpu);
 
 #endif
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..f3098e4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -24,6 +24,12 @@
 /* set up by the platform code */
 void __iomem *twd_base;
 
+/* Timer context to be saved in low power modes */
+struct twd_registers {
+	unsigned long timer_ctrl;
+	unsigned long timer_load;
+};
+static DEFINE_PER_CPU(struct twd_registers, twd_context);
 static unsigned long twd_timer_rate;
 
 static void twd_set_mode(enum clock_event_mode mode,
@@ -145,3 +151,21 @@ void __cpuinit twd_timer_setup(struct clock_event_device *clk)
 
 	clockevents_register_device(clk);
 }
+
+/* Low power context save */
+void twd_context_save(unsigned int cpu)
+{
+	struct twd_registers *regs = &per_cpu(twd_context, cpu);
+
+	regs->timer_ctrl = __raw_readl(twd_base + TWD_TIMER_CONTROL);
+	regs->timer_load = __raw_readl(twd_base + TWD_TIMER_LOAD);
+}
+
+/* Low power context restore */
+void twd_context_restore(unsigned int cpu)
+{
+	struct twd_registers *regs = &per_cpu(twd_context, cpu);
+
+	__raw_writel(regs->timer_load, twd_base + TWD_TIMER_LOAD);
+	__raw_writel(regs->timer_ctrl, twd_base + TWD_TIMER_CONTROL);
+}
-- 
1.6.0.4

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> In CPU low power state, local timer looses its register context. This
> patch adds context save restore hooks which can be used by platforms
> appropriately.

I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
timer stuff wouldn't rely on it being kept alive?

Hmm, it looks like we bypass the clockevents code by only setting the
TWD load value at initialization time, not when we switch to periodic
mode.  We really ought to rewrite it whenever we switch back to periodic
mode.

I suspect fixing that means you won't need this save/restore support.

[PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> In CPU low power state, local timer looses its register context. This
> patch adds context save restore hooks which can be used by platforms
> appropriately.

I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
timer stuff wouldn't rely on it being kept alive?

Hmm, it looks like we bypass the clockevents code by only setting the
TWD load value at initialization time, not when we switch to periodic
mode.  We really ought to rewrite it whenever we switch back to periodic
mode.

I suspect fixing that means you won't need this save/restore support.

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > In CPU low power state, local timer looses its register context. This
> > patch adds context save restore hooks which can be used by platforms
> > appropriately.
> 
> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
> timer stuff wouldn't rely on it being kept alive?
> 
> Hmm, it looks like we bypass the clockevents code by only setting the
> TWD load value at initialization time, not when we switch to periodic
> mode.  We really ought to rewrite it whenever we switch back to periodic
> mode.
> 
> I suspect fixing that means you won't need this save/restore support.

Untested, but should do what's required.

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
 		/* timer load already set up */
 		ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
 			| TWD_TIMER_CONTROL_PERIODIC;
+		__raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
 
 static void __cpuinit twd_calibrate_rate(void)
 {
-	unsigned long load, count;
+	unsigned long count;
 	u64 waitjiffies;
 
 	/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
 		printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
 			(twd_timer_rate / 1000000) % 100);
 	}
-
-	load = twd_timer_rate / HZ;
-
-	__raw_writel(load, twd_base + TWD_TIMER_LOAD);
 }
 
 /*

[PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > In CPU low power state, local timer looses its register context. This
> > patch adds context save restore hooks which can be used by platforms
> > appropriately.
> 
> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
> timer stuff wouldn't rely on it being kept alive?
> 
> Hmm, it looks like we bypass the clockevents code by only setting the
> TWD load value at initialization time, not when we switch to periodic
> mode.  We really ought to rewrite it whenever we switch back to periodic
> mode.
> 
> I suspect fixing that means you won't need this save/restore support.

Untested, but should do what's required.

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
 		/* timer load already set up */
 		ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
 			| TWD_TIMER_CONTROL_PERIODIC;
+		__raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
 
 static void __cpuinit twd_calibrate_rate(void)
 {
-	unsigned long load, count;
+	unsigned long count;
 	u64 waitjiffies;
 
 	/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
 		printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
 			(twd_timer_rate / 1000000) % 100);
 	}
-
-	load = twd_timer_rate / HZ;
-
-	__raw_writel(load, twd_base + TWD_TIMER_LOAD);
 }
 
 /*

RE: [PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Monday, January 24, 2011 4:41 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel@lists.infradead.org; catalin.marinas@arm.com;
> ccross@android.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux
> wrote:
> > On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > > In CPU low power state, local timer looses its register context.
> This
> > > patch adds context save restore hooks which can be used by
> platforms
> > > appropriately.
> >
> > I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the
> generic
> > timer stuff wouldn't rely on it being kept alive?
> >
> > Hmm, it looks like we bypass the clockevents code by only setting
> the
> > TWD load value at initialization time, not when we switch to
> periodic
> > mode.  We really ought to rewrite it whenever we switch back to
> periodic
> > mode.
> >
> > I suspect fixing that means you won't need this save/restore
> support.
>
> Untested, but should do what's required.
:) I was just typing an email and you sent a patch. Will test this
and update you.

>
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index fd91566..60636f4 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode
> mode,
>  		/* timer load already set up */
>  		ctrl = TWD_TIMER_CONTROL_ENABLE |
> TWD_TIMER_CONTROL_IT_ENABLE
>  			| TWD_TIMER_CONTROL_PERIODIC;
> +		__raw_writel(twd_timer_rate / HZ, twd_base +
> TWD_TIMER_LOAD);
>  		break;
>  	case CLOCK_EVT_MODE_ONESHOT:
>  		/* period set, and timer enabled in 'next_event' hook */
> @@ -81,7 +82,7 @@ int twd_timer_ack(void)
>
>  static void __cpuinit twd_calibrate_rate(void)
>  {
> -	unsigned long load, count;
> +	unsigned long count;
>  	u64 waitjiffies;
>
>  	/*
> @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
>  		printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
>  			(twd_timer_rate / 1000000) % 100);
>  	}
> -
> -	load = twd_timer_rate / HZ;
> -
> -	__raw_writel(load, twd_base + TWD_TIMER_LOAD);
>  }
>
>  /*

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Monday, January 24, 2011 4:41 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; catalin.marinas at arm.com;
> ccross at android.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux
> wrote:
> > On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > > In CPU low power state, local timer looses its register context.
> This
> > > patch adds context save restore hooks which can be used by
> platforms
> > > appropriately.
> >
> > I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the
> generic
> > timer stuff wouldn't rely on it being kept alive?
> >
> > Hmm, it looks like we bypass the clockevents code by only setting
> the
> > TWD load value at initialization time, not when we switch to
> periodic
> > mode.  We really ought to rewrite it whenever we switch back to
> periodic
> > mode.
> >
> > I suspect fixing that means you won't need this save/restore
> support.
>
> Untested, but should do what's required.
:) I was just typing an email and you sent a patch. Will test this
and update you.

>
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index fd91566..60636f4 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode
> mode,
>  		/* timer load already set up */
>  		ctrl = TWD_TIMER_CONTROL_ENABLE |
> TWD_TIMER_CONTROL_IT_ENABLE
>  			| TWD_TIMER_CONTROL_PERIODIC;
> +		__raw_writel(twd_timer_rate / HZ, twd_base +
> TWD_TIMER_LOAD);
>  		break;
>  	case CLOCK_EVT_MODE_ONESHOT:
>  		/* period set, and timer enabled in 'next_event' hook */
> @@ -81,7 +82,7 @@ int twd_timer_ack(void)
>
>  static void __cpuinit twd_calibrate_rate(void)
>  {
> -	unsigned long load, count;
> +	unsigned long count;
>  	u64 waitjiffies;
>
>  	/*
> @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
>  		printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
>  			(twd_timer_rate / 1000000) % 100);
>  	}
> -
> -	load = twd_timer_rate / HZ;
> -
> -	__raw_writel(load, twd_base + TWD_TIMER_LOAD);
>  }
>
>  /*

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Colin Cross]

On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
>> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
>> > In CPU low power state, local timer looses its register context. This
>> > patch adds context save restore hooks which can be used by platforms
>> > appropriately.
>>
>> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
>> timer stuff wouldn't rely on it being kept alive?
>>
>> Hmm, it looks like we bypass the clockevents code by only setting the
>> TWD load value at initialization time, not when we switch to periodic
>> mode.  We really ought to rewrite it whenever we switch back to periodic
>> mode.
>>
>> I suspect fixing that means you won't need this save/restore support.
>
> Untested, but should do what's required.
>
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index fd91566..60636f4 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
>                /* timer load already set up */
>                ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
>                        | TWD_TIMER_CONTROL_PERIODIC;
> +               __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
>                break;
>        case CLOCK_EVT_MODE_ONESHOT:
>                /* period set, and timer enabled in 'next_event' hook */
> @@ -81,7 +82,7 @@ int twd_timer_ack(void)
>
>  static void __cpuinit twd_calibrate_rate(void)
>  {
> -       unsigned long load, count;
> +       unsigned long count;
>        u64 waitjiffies;
>
>        /*
> @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
>                printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
>                        (twd_timer_rate / 1000000) % 100);
>        }
> -
> -       load = twd_timer_rate / HZ;
> -
> -       __raw_writel(load, twd_base + TWD_TIMER_LOAD);
>  }
>
>  /*

This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
gets reset by cpu idle between twd_set_mode and twd_set_next_event.
Shadowing ctrl in a percpu variable and rewriting it during every call
to twd_set_next_event does work, but that's not much different, and a
little less efficient, than just saving and restoring the control and
load registers in idle.  It does have the advantage that platforms
don't need any extra calls.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] ARM: twd: Add context save restore support

[Colin Cross]

On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
>> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
>> > In CPU low power state, local timer looses its register context. This
>> > patch adds context save restore hooks which can be used by platforms
>> > appropriately.
>>
>> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
>> timer stuff wouldn't rely on it being kept alive?
>>
>> Hmm, it looks like we bypass the clockevents code by only setting the
>> TWD load value at initialization time, not when we switch to periodic
>> mode. ?We really ought to rewrite it whenever we switch back to periodic
>> mode.
>>
>> I suspect fixing that means you won't need this save/restore support.
>
> Untested, but should do what's required.
>
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index fd91566..60636f4 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
> ? ? ? ? ? ? ? ?/* timer load already set up */
> ? ? ? ? ? ? ? ?ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
> ? ? ? ? ? ? ? ? ? ? ? ?| TWD_TIMER_CONTROL_PERIODIC;
> + ? ? ? ? ? ? ? __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
> ? ? ? ? ? ? ? ?break;
> ? ? ? ?case CLOCK_EVT_MODE_ONESHOT:
> ? ? ? ? ? ? ? ?/* period set, and timer enabled in 'next_event' hook */
> @@ -81,7 +82,7 @@ int twd_timer_ack(void)
>
> ?static void __cpuinit twd_calibrate_rate(void)
> ?{
> - ? ? ? unsigned long load, count;
> + ? ? ? unsigned long count;
> ? ? ? ?u64 waitjiffies;
>
> ? ? ? ?/*
> @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
> ? ? ? ? ? ? ? ?printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
> ? ? ? ? ? ? ? ? ? ? ? ?(twd_timer_rate / 1000000) % 100);
> ? ? ? ?}
> -
> - ? ? ? load = twd_timer_rate / HZ;
> -
> - ? ? ? __raw_writel(load, twd_base + TWD_TIMER_LOAD);
> ?}
>
> ?/*

This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
gets reset by cpu idle between twd_set_mode and twd_set_next_event.
Shadowing ctrl in a percpu variable and rewriting it during every call
to twd_set_next_event does work, but that's not much different, and a
little less efficient, than just saving and restoring the control and
load registers in idle.  It does have the advantage that platforms
don't need any extra calls.

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
> >> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> >> > In CPU low power state, local timer looses its register context. This
> >> > patch adds context save restore hooks which can be used by platforms
> >> > appropriately.
> >>
> >> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
> >> timer stuff wouldn't rely on it being kept alive?
> >>
> >> Hmm, it looks like we bypass the clockevents code by only setting the
> >> TWD load value at initialization time, not when we switch to periodic
> >> mode.  We really ought to rewrite it whenever we switch back to periodic
> >> mode.
> >>
> >> I suspect fixing that means you won't need this save/restore support.
> >
> > Untested, but should do what's required.
> >
> > diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> > index fd91566..60636f4 100644
> > --- a/arch/arm/kernel/smp_twd.c
> > +++ b/arch/arm/kernel/smp_twd.c
> > @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
> >                /* timer load already set up */
> >                ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
> >                        | TWD_TIMER_CONTROL_PERIODIC;
> > +               __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
> >                break;
> >        case CLOCK_EVT_MODE_ONESHOT:
> >                /* period set, and timer enabled in 'next_event' hook */
> > @@ -81,7 +82,7 @@ int twd_timer_ack(void)
> >
> >  static void __cpuinit twd_calibrate_rate(void)
> >  {
> > -       unsigned long load, count;
> > +       unsigned long count;
> >        u64 waitjiffies;
> >
> >        /*
> > @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
> >                printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
> >                        (twd_timer_rate / 1000000) % 100);
> >        }
> > -
> > -       load = twd_timer_rate / HZ;
> > -
> > -       __raw_writel(load, twd_base + TWD_TIMER_LOAD);
> >  }
> >
> >  /*
> 
> This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> gets reset by cpu idle between twd_set_mode and twd_set_next_event.
> Shadowing ctrl in a percpu variable and rewriting it during every call
> to twd_set_next_event does work, but that's not much different, and a
> little less efficient, than just saving and restoring the control and
> load registers in idle.  It does have the advantage that platforms
> don't need any extra calls.

The next question is can we teach the generic time infrastructure about
this so we don't have to modify every clock event driver for it?  We
really need to get away from having this kind of knowledge buried down
in the lowest levels of every driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
> >> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> >> > In CPU low power state, local timer looses its register context. This
> >> > patch adds context save restore hooks which can be used by platforms
> >> > appropriately.
> >>
> >> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
> >> timer stuff wouldn't rely on it being kept alive?
> >>
> >> Hmm, it looks like we bypass the clockevents code by only setting the
> >> TWD load value at initialization time, not when we switch to periodic
> >> mode. ?We really ought to rewrite it whenever we switch back to periodic
> >> mode.
> >>
> >> I suspect fixing that means you won't need this save/restore support.
> >
> > Untested, but should do what's required.
> >
> > diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> > index fd91566..60636f4 100644
> > --- a/arch/arm/kernel/smp_twd.c
> > +++ b/arch/arm/kernel/smp_twd.c
> > @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
> > ? ? ? ? ? ? ? ?/* timer load already set up */
> > ? ? ? ? ? ? ? ?ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
> > ? ? ? ? ? ? ? ? ? ? ? ?| TWD_TIMER_CONTROL_PERIODIC;
> > + ? ? ? ? ? ? ? __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
> > ? ? ? ? ? ? ? ?break;
> > ? ? ? ?case CLOCK_EVT_MODE_ONESHOT:
> > ? ? ? ? ? ? ? ?/* period set, and timer enabled in 'next_event' hook */
> > @@ -81,7 +82,7 @@ int twd_timer_ack(void)
> >
> > ?static void __cpuinit twd_calibrate_rate(void)
> > ?{
> > - ? ? ? unsigned long load, count;
> > + ? ? ? unsigned long count;
> > ? ? ? ?u64 waitjiffies;
> >
> > ? ? ? ?/*
> > @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
> > ? ? ? ? ? ? ? ?printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
> > ? ? ? ? ? ? ? ? ? ? ? ?(twd_timer_rate / 1000000) % 100);
> > ? ? ? ?}
> > -
> > - ? ? ? load = twd_timer_rate / HZ;
> > -
> > - ? ? ? __raw_writel(load, twd_base + TWD_TIMER_LOAD);
> > ?}
> >
> > ?/*
> 
> This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> gets reset by cpu idle between twd_set_mode and twd_set_next_event.
> Shadowing ctrl in a percpu variable and rewriting it during every call
> to twd_set_next_event does work, but that's not much different, and a
> little less efficient, than just saving and restoring the control and
> load registers in idle.  It does have the advantage that platforms
> don't need any extra calls.

The next question is can we teach the generic time infrastructure about
this so we don't have to modify every clock event driver for it?  We
really need to get away from having this kind of knowledge buried down
in the lowest levels of every driver.

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3302 bytes --]

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> > On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
> > >> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > >> > In CPU low power state, local timer looses its register context. This
> > >> > patch adds context save restore hooks which can be used by platforms
> > >> > appropriately.
> > >>
> > >> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
> > >> timer stuff wouldn't rely on it being kept alive?
> > >>
> > >> Hmm, it looks like we bypass the clockevents code by only setting the
> > >> TWD load value at initialization time, not when we switch to periodic
> > >> mode.  We really ought to rewrite it whenever we switch back to periodic
> > >> mode.
> > >>
> > >> I suspect fixing that means you won't need this save/restore support.
> > >
> > > Untested, but should do what's required.
> > >
> > > diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> > > index fd91566..60636f4 100644
> > > --- a/arch/arm/kernel/smp_twd.c
> > > +++ b/arch/arm/kernel/smp_twd.c
> > > @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
> > >                /* timer load already set up */
> > >                ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
> > >                        | TWD_TIMER_CONTROL_PERIODIC;
> > > +               __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
> > >                break;
> > >        case CLOCK_EVT_MODE_ONESHOT:
> > >                /* period set, and timer enabled in 'next_event' hook */
> > > @@ -81,7 +82,7 @@ int twd_timer_ack(void)
> > >
> > >  static void __cpuinit twd_calibrate_rate(void)
> > >  {
> > > -       unsigned long load, count;
> > > +       unsigned long count;
> > >        u64 waitjiffies;
> > >
> > >        /*
> > > @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
> > >                printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
> > >                        (twd_timer_rate / 1000000) % 100);
> > >        }
> > > -
> > > -       load = twd_timer_rate / HZ;
> > > -
> > > -       __raw_writel(load, twd_base + TWD_TIMER_LOAD);
> > >  }
> > >
> > >  /*
> > 
> > This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> > gets reset by cpu idle between twd_set_mode and twd_set_next_event.
> > Shadowing ctrl in a percpu variable and rewriting it during every call
> > to twd_set_next_event does work, but that's not much different, and a
> > little less efficient, than just saving and restoring the control and
> > load registers in idle.  It does have the advantage that platforms
> > don't need any extra calls.
> 
> The next question is can we teach the generic time infrastructure about
> this so we don't have to modify every clock event driver for it?  We
> really need to get away from having this kind of knowledge buried down
> in the lowest levels of every driver.

In which way? I mean the generic code issues a call to the set_mode
function when we leave the broadcast mode. So what should the generic
code do more ?

Thanks,

	tglx

[PATCH 3/5] ARM: twd: Add context save restore support

[Thomas Gleixner]

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> > On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
> > >> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > >> > In CPU low power state, local timer looses its register context. This
> > >> > patch adds context save restore hooks which can be used by platforms
> > >> > appropriately.
> > >>
> > >> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
> > >> timer stuff wouldn't rely on it being kept alive?
> > >>
> > >> Hmm, it looks like we bypass the clockevents code by only setting the
> > >> TWD load value at initialization time, not when we switch to periodic
> > >> mode. ?We really ought to rewrite it whenever we switch back to periodic
> > >> mode.
> > >>
> > >> I suspect fixing that means you won't need this save/restore support.
> > >
> > > Untested, but should do what's required.
> > >
> > > diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> > > index fd91566..60636f4 100644
> > > --- a/arch/arm/kernel/smp_twd.c
> > > +++ b/arch/arm/kernel/smp_twd.c
> > > @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
> > > ? ? ? ? ? ? ? ?/* timer load already set up */
> > > ? ? ? ? ? ? ? ?ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
> > > ? ? ? ? ? ? ? ? ? ? ? ?| TWD_TIMER_CONTROL_PERIODIC;
> > > + ? ? ? ? ? ? ? __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
> > > ? ? ? ? ? ? ? ?break;
> > > ? ? ? ?case CLOCK_EVT_MODE_ONESHOT:
> > > ? ? ? ? ? ? ? ?/* period set, and timer enabled in 'next_event' hook */
> > > @@ -81,7 +82,7 @@ int twd_timer_ack(void)
> > >
> > > ?static void __cpuinit twd_calibrate_rate(void)
> > > ?{
> > > - ? ? ? unsigned long load, count;
> > > + ? ? ? unsigned long count;
> > > ? ? ? ?u64 waitjiffies;
> > >
> > > ? ? ? ?/*
> > > @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
> > > ? ? ? ? ? ? ? ?printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
> > > ? ? ? ? ? ? ? ? ? ? ? ?(twd_timer_rate / 1000000) % 100);
> > > ? ? ? ?}
> > > -
> > > - ? ? ? load = twd_timer_rate / HZ;
> > > -
> > > - ? ? ? __raw_writel(load, twd_base + TWD_TIMER_LOAD);
> > > ?}
> > >
> > > ?/*
> > 
> > This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> > gets reset by cpu idle between twd_set_mode and twd_set_next_event.
> > Shadowing ctrl in a percpu variable and rewriting it during every call
> > to twd_set_next_event does work, but that's not much different, and a
> > little less efficient, than just saving and restoring the control and
> > load registers in idle.  It does have the advantage that platforms
> > don't need any extra calls.
> 
> The next question is can we teach the generic time infrastructure about
> this so we don't have to modify every clock event driver for it?  We
> really need to get away from having this kind of knowledge buried down
> in the lowest levels of every driver.

In which way? I mean the generic code issues a call to the set_mode
function when we leave the broadcast mode. So what should the generic
code do more ?

Thanks,

	tglx

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> > On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> > > On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
> > > <linux@arm.linux.org.uk> wrote:
> > > > On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
> > > >> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > > >> > In CPU low power state, local timer looses its register context. This
> > > >> > patch adds context save restore hooks which can be used by platforms
> > > >> > appropriately.
> > > >>
> > > >> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
> > > >> timer stuff wouldn't rely on it being kept alive?
> > > >>
> > > >> Hmm, it looks like we bypass the clockevents code by only setting the
> > > >> TWD load value at initialization time, not when we switch to periodic
> > > >> mode.  We really ought to rewrite it whenever we switch back to periodic
> > > >> mode.
> > > >>
> > > >> I suspect fixing that means you won't need this save/restore support.
> > > >
> > > > Untested, but should do what's required.
> > > >
> > > > diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> > > > index fd91566..60636f4 100644
> > > > --- a/arch/arm/kernel/smp_twd.c
> > > > +++ b/arch/arm/kernel/smp_twd.c
> > > > @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
> > > >                /* timer load already set up */
> > > >                ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
> > > >                        | TWD_TIMER_CONTROL_PERIODIC;
> > > > +               __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
> > > >                break;
> > > >        case CLOCK_EVT_MODE_ONESHOT:
> > > >                /* period set, and timer enabled in 'next_event' hook */
> > > > @@ -81,7 +82,7 @@ int twd_timer_ack(void)
> > > >
> > > >  static void __cpuinit twd_calibrate_rate(void)
> > > >  {
> > > > -       unsigned long load, count;
> > > > +       unsigned long count;
> > > >        u64 waitjiffies;
> > > >
> > > >        /*
> > > > @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
> > > >                printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
> > > >                        (twd_timer_rate / 1000000) % 100);
> > > >        }
> > > > -
> > > > -       load = twd_timer_rate / HZ;
> > > > -
> > > > -       __raw_writel(load, twd_base + TWD_TIMER_LOAD);
> > > >  }
> > > >
> > > >  /*
> > > 
> > > This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> > > gets reset by cpu idle between twd_set_mode and twd_set_next_event.
> > > Shadowing ctrl in a percpu variable and rewriting it during every call
> > > to twd_set_next_event does work, but that's not much different, and a
> > > little less efficient, than just saving and restoring the control and
> > > load registers in idle.  It does have the advantage that platforms
> > > don't need any extra calls.
> > 
> > The next question is can we teach the generic time infrastructure about
> > this so we don't have to modify every clock event driver for it?  We
> > really need to get away from having this kind of knowledge buried down
> > in the lowest levels of every driver.
> 
> In which way? I mean the generic code issues a call to the set_mode
> function when we leave the broadcast mode. So what should the generic
> code do more ?

I can't say because these patches only add the hooks, there's no
implementation yet which uses the hooks.

Given the description about _why_ those hooks are necessary, it seems
that something is required.  Either we start adding custom hacks to
each clockevent driver as is done with this patch, or we get some
generic help in place.

I'm not thrilled by the custom hack approach - and I thought the
clockevent stuff was created to stop this kind of thing happening.

I suggest we defer this until there's a visible use case available.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> > On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> > > On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
> > > <linux@arm.linux.org.uk> wrote:
> > > > On Mon, Jan 24, 2011 at 11:06:09AM +0000, Russell King - ARM Linux wrote:
> > > >> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > > >> > In CPU low power state, local timer looses its register context. This
> > > >> > patch adds context save restore hooks which can be used by platforms
> > > >> > appropriately.
> > > >>
> > > >> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
> > > >> timer stuff wouldn't rely on it being kept alive?
> > > >>
> > > >> Hmm, it looks like we bypass the clockevents code by only setting the
> > > >> TWD load value at initialization time, not when we switch to periodic
> > > >> mode. ?We really ought to rewrite it whenever we switch back to periodic
> > > >> mode.
> > > >>
> > > >> I suspect fixing that means you won't need this save/restore support.
> > > >
> > > > Untested, but should do what's required.
> > > >
> > > > diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> > > > index fd91566..60636f4 100644
> > > > --- a/arch/arm/kernel/smp_twd.c
> > > > +++ b/arch/arm/kernel/smp_twd.c
> > > > @@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
> > > > ? ? ? ? ? ? ? ?/* timer load already set up */
> > > > ? ? ? ? ? ? ? ?ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
> > > > ? ? ? ? ? ? ? ? ? ? ? ?| TWD_TIMER_CONTROL_PERIODIC;
> > > > + ? ? ? ? ? ? ? __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
> > > > ? ? ? ? ? ? ? ?break;
> > > > ? ? ? ?case CLOCK_EVT_MODE_ONESHOT:
> > > > ? ? ? ? ? ? ? ?/* period set, and timer enabled in 'next_event' hook */
> > > > @@ -81,7 +82,7 @@ int twd_timer_ack(void)
> > > >
> > > > ?static void __cpuinit twd_calibrate_rate(void)
> > > > ?{
> > > > - ? ? ? unsigned long load, count;
> > > > + ? ? ? unsigned long count;
> > > > ? ? ? ?u64 waitjiffies;
> > > >
> > > > ? ? ? ?/*
> > > > @@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
> > > > ? ? ? ? ? ? ? ?printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
> > > > ? ? ? ? ? ? ? ? ? ? ? ?(twd_timer_rate / 1000000) % 100);
> > > > ? ? ? ?}
> > > > -
> > > > - ? ? ? load = twd_timer_rate / HZ;
> > > > -
> > > > - ? ? ? __raw_writel(load, twd_base + TWD_TIMER_LOAD);
> > > > ?}
> > > >
> > > > ?/*
> > > 
> > > This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> > > gets reset by cpu idle between twd_set_mode and twd_set_next_event.
> > > Shadowing ctrl in a percpu variable and rewriting it during every call
> > > to twd_set_next_event does work, but that's not much different, and a
> > > little less efficient, than just saving and restoring the control and
> > > load registers in idle.  It does have the advantage that platforms
> > > don't need any extra calls.
> > 
> > The next question is can we teach the generic time infrastructure about
> > this so we don't have to modify every clock event driver for it?  We
> > really need to get away from having this kind of knowledge buried down
> > in the lowest levels of every driver.
> 
> In which way? I mean the generic code issues a call to the set_mode
> function when we leave the broadcast mode. So what should the generic
> code do more ?

I can't say because these patches only add the hooks, there's no
implementation yet which uses the hooks.

Given the description about _why_ those hooks are necessary, it seems
that something is required.  Either we start adding custom hacks to
each clockevent driver as is done with this patch, or we get some
generic help in place.

I'm not thrilled by the custom hack approach - and I thought the
clockevent stuff was created to stop this kind of thing happening.

I suggest we defer this until there's a visible use case available.

RE: [PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 7:08 PM
> To: Thomas Gleixner
> Cc: Colin Cross; Santosh Shilimkar; catalin.marinas@arm.com;
> linus.ml.walleij@gmail.com; linux-omap@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> > On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:

[...]
> > > The next question is can we teach the generic time
> infrastructure about
> > > this so we don't have to modify every clock event driver for it?
> We
> > > really need to get away from having this kind of knowledge
> buried down
> > > in the lowest levels of every driver.
> >
> > In which way? I mean the generic code issues a call to the
> set_mode
> > function when we leave the broadcast mode. So what should the
> generic
> > code do more ?
>
> I can't say because these patches only add the hooks, there's no
> implementation yet which uses the hooks.
>
> Given the description about _why_ those hooks are necessary, it
> seems
> that something is required.  Either we start adding custom hacks to
> each clockevent driver as is done with this patch, or we get some
> generic help in place.
>
> I'm not thrilled by the custom hack approach - and I thought the
> clockevent stuff was created to stop this kind of thing happening.
>
> I suggest we defer this until there's a visible use case available.
Just for clarification. Without the TWD save restore patch the PM
won't work. So what you mean by visible usecase.
We need this to be fixed and that's what was done with the my
patch. Ofcourse it's a custom hack approach but does the job.

Regards,
Santosh

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 7:08 PM
> To: Thomas Gleixner
> Cc: Colin Cross; Santosh Shilimkar; catalin.marinas at arm.com;
> linus.ml.walleij at gmail.com; linux-omap at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> > On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:

[...]
> > > The next question is can we teach the generic time
> infrastructure about
> > > this so we don't have to modify every clock event driver for it?
> We
> > > really need to get away from having this kind of knowledge
> buried down
> > > in the lowest levels of every driver.
> >
> > In which way? I mean the generic code issues a call to the
> set_mode
> > function when we leave the broadcast mode. So what should the
> generic
> > code do more ?
>
> I can't say because these patches only add the hooks, there's no
> implementation yet which uses the hooks.
>
> Given the description about _why_ those hooks are necessary, it
> seems
> that something is required.  Either we start adding custom hacks to
> each clockevent driver as is done with this patch, or we get some
> generic help in place.
>
> I'm not thrilled by the custom hack approach - and I thought the
> clockevent stuff was created to stop this kind of thing happening.
>
> I suggest we defer this until there's a visible use case available.
Just for clarification. Without the TWD save restore patch the PM
won't work. So what you mean by visible usecase.
We need this to be fixed and that's what was done with the my
patch. Ofcourse it's a custom hack approach but does the job.

Regards,
Santosh

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Thomas Gleixner]

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> > In which way? I mean the generic code issues a call to the set_mode
> > function when we leave the broadcast mode. So what should the generic
> > code do more ?
> 
> I can't say because these patches only add the hooks, there's no
> implementation yet which uses the hooks.
> 
> Given the description about _why_ those hooks are necessary, it seems
> that something is required.  Either we start adding custom hacks to
> each clockevent driver as is done with this patch, or we get some
> generic help in place.
> 
> I'm not thrilled by the custom hack approach - and I thought the
> clockevent stuff was created to stop this kind of thing happening.

Yes, and it does the right thing:

     idle enter (where the cpu local tick device stops)

     	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_ENTER)

		clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN)
		tick_broadcast_set_event(dev->next_event, 1)

    idle exit

     	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_EXIT)

		clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT)
		tick_program_event(dev->next_event, 1)

So the generic code has all the calls in place. If a clock chip
implementation misses to set control registers in the
CLOCK_EVT_MODE_ONESHOT case, then it's not a short coming of the
generic code which needs magic hooks in the arch code.

The same applies for the periodic mode switch, which is handled via
tick_broadcast_on_off().

Am I missing something ?

Thanks,

	tglx

[PATCH 3/5] ARM: twd: Add context save restore support

[Thomas Gleixner]

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> > In which way? I mean the generic code issues a call to the set_mode
> > function when we leave the broadcast mode. So what should the generic
> > code do more ?
> 
> I can't say because these patches only add the hooks, there's no
> implementation yet which uses the hooks.
> 
> Given the description about _why_ those hooks are necessary, it seems
> that something is required.  Either we start adding custom hacks to
> each clockevent driver as is done with this patch, or we get some
> generic help in place.
> 
> I'm not thrilled by the custom hack approach - and I thought the
> clockevent stuff was created to stop this kind of thing happening.

Yes, and it does the right thing:

     idle enter (where the cpu local tick device stops)

     	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_ENTER)

		clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN)
		tick_broadcast_set_event(dev->next_event, 1)

    idle exit

     	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_EXIT)

		clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT)
		tick_program_event(dev->next_event, 1)

So the generic code has all the calls in place. If a clock chip
implementation misses to set control registers in the
CLOCK_EVT_MODE_ONESHOT case, then it's not a short coming of the
generic code which needs magic hooks in the arch code.

The same applies for the periodic mode switch, which is handled via
tick_broadcast_on_off().

Am I missing something ?

Thanks,

	tglx

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 03:12:24PM +0100, Thomas Gleixner wrote:
> On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> > On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> > > In which way? I mean the generic code issues a call to the set_mode
> > > function when we leave the broadcast mode. So what should the generic
> > > code do more ?
> > 
> > I can't say because these patches only add the hooks, there's no
> > implementation yet which uses the hooks.
> > 
> > Given the description about _why_ those hooks are necessary, it seems
> > that something is required.  Either we start adding custom hacks to
> > each clockevent driver as is done with this patch, or we get some
> > generic help in place.
> > 
> > I'm not thrilled by the custom hack approach - and I thought the
> > clockevent stuff was created to stop this kind of thing happening.
> 
> Yes, and it does the right thing:
> 
>      idle enter (where the cpu local tick device stops)
> 
>      	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_ENTER)
> 
> 		clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN)
> 		tick_broadcast_set_event(dev->next_event, 1)
> 
>     idle exit
> 
>      	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_EXIT)
> 
> 		clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT)
> 		tick_program_event(dev->next_event, 1)
> 
> So the generic code has all the calls in place. If a clock chip
> implementation misses to set control registers in the
> CLOCK_EVT_MODE_ONESHOT case, then it's not a short coming of the
> generic code which needs magic hooks in the arch code.
> 
> The same applies for the periodic mode switch, which is handled via
> tick_broadcast_on_off().
> 
> Am I missing something ?

I quote Colin:

| This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
| gets reset by cpu idle between twd_set_mode and twd_set_next_event.

I quote the code:

static void twd_set_mode(enum clock_event_mode mode,
                        struct clock_event_device *clk)
{
        unsigned long ctrl;
        switch (mode) {
        case CLOCK_EVT_MODE_PERIODIC:
                /* timer load already set up */
                ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
                        | TWD_TIMER_CONTROL_PERIODIC;
                break;
        case CLOCK_EVT_MODE_ONESHOT:
                /* period set, and timer enabled in 'next_event' hook */
                ctrl = TWD_TIMER_CONTROL_IT_ENABLE | TWD_TIMER_CONTROL_ONESHOT;
                break;
        case CLOCK_EVT_MODE_UNUSED:
        case CLOCK_EVT_MODE_SHUTDOWN:
        default:
                ctrl = 0;
        }

        __raw_writel(ctrl, twd_base + TWD_TIMER_CONTROL);
}

[PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 03:12:24PM +0100, Thomas Gleixner wrote:
> On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> > On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> > > In which way? I mean the generic code issues a call to the set_mode
> > > function when we leave the broadcast mode. So what should the generic
> > > code do more ?
> > 
> > I can't say because these patches only add the hooks, there's no
> > implementation yet which uses the hooks.
> > 
> > Given the description about _why_ those hooks are necessary, it seems
> > that something is required.  Either we start adding custom hacks to
> > each clockevent driver as is done with this patch, or we get some
> > generic help in place.
> > 
> > I'm not thrilled by the custom hack approach - and I thought the
> > clockevent stuff was created to stop this kind of thing happening.
> 
> Yes, and it does the right thing:
> 
>      idle enter (where the cpu local tick device stops)
> 
>      	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_ENTER)
> 
> 		clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN)
> 		tick_broadcast_set_event(dev->next_event, 1)
> 
>     idle exit
> 
>      	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_EXIT)
> 
> 		clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT)
> 		tick_program_event(dev->next_event, 1)
> 
> So the generic code has all the calls in place. If a clock chip
> implementation misses to set control registers in the
> CLOCK_EVT_MODE_ONESHOT case, then it's not a short coming of the
> generic code which needs magic hooks in the arch code.
> 
> The same applies for the periodic mode switch, which is handled via
> tick_broadcast_on_off().
> 
> Am I missing something ?

I quote Colin:

| This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
| gets reset by cpu idle between twd_set_mode and twd_set_next_event.

I quote the code:

static void twd_set_mode(enum clock_event_mode mode,
                        struct clock_event_device *clk)
{
        unsigned long ctrl;
        switch (mode) {
        case CLOCK_EVT_MODE_PERIODIC:
                /* timer load already set up */
                ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
                        | TWD_TIMER_CONTROL_PERIODIC;
                break;
        case CLOCK_EVT_MODE_ONESHOT:
                /* period set, and timer enabled in 'next_event' hook */
                ctrl = TWD_TIMER_CONTROL_IT_ENABLE | TWD_TIMER_CONTROL_ONESHOT;
                break;
        case CLOCK_EVT_MODE_UNUSED:
        case CLOCK_EVT_MODE_SHUTDOWN:
        default:
                ctrl = 0;
        }

        __raw_writel(ctrl, twd_base + TWD_TIMER_CONTROL);
}

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Thomas Gleixner]

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 03:12:24PM +0100, Thomas Gleixner wrote:
> > On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> > > On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> > > > In which way? I mean the generic code issues a call to the set_mode
> > > > function when we leave the broadcast mode. So what should the generic
> > > > code do more ?
> > > 
> > > I can't say because these patches only add the hooks, there's no
> > > implementation yet which uses the hooks.
> > > 
> > > Given the description about _why_ those hooks are necessary, it seems
> > > that something is required.  Either we start adding custom hacks to
> > > each clockevent driver as is done with this patch, or we get some
> > > generic help in place.
> > > 
> > > I'm not thrilled by the custom hack approach - and I thought the
> > > clockevent stuff was created to stop this kind of thing happening.
> > 
> > Yes, and it does the right thing:
> > 
> >      idle enter (where the cpu local tick device stops)
> > 
> >      	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_ENTER)
> > 
> > 		clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN)
> > 		tick_broadcast_set_event(dev->next_event, 1)
> > 
> >     idle exit
> > 
> >      	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_EXIT)
> > 
> > 		clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT)
> > 		tick_program_event(dev->next_event, 1)
> > 
> > So the generic code has all the calls in place. If a clock chip
> > implementation misses to set control registers in the
> > CLOCK_EVT_MODE_ONESHOT case, then it's not a short coming of the
> > generic code which needs magic hooks in the arch code.
> > 
> > The same applies for the periodic mode switch, which is handled via
> > tick_broadcast_on_off().
> > 
> > Am I missing something ?
> 
> I quote Colin:
> 
> | This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> | gets reset by cpu idle between twd_set_mode and twd_set_next_event.

And I fear that he means the twd_set_mode(ONESHOT) call which happens
during boot/cpuonline.

The point is that the above function needs to be called from the cpu
idle code to notify the clock events layer. The correct mechanism is
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT). The
existing callers are in:

arch/x86/kernel/process.c
drivers/acpi/acpi_pad.c
drivers/acpi/processor_idle.c
drivers/idle/intel_idle.c

So i'm not surprised, that this wont work on ARM :)

Thanks,

	tglx

[PATCH 3/5] ARM: twd: Add context save restore support

[Thomas Gleixner]

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 03:12:24PM +0100, Thomas Gleixner wrote:
> > On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> > > On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> > > > In which way? I mean the generic code issues a call to the set_mode
> > > > function when we leave the broadcast mode. So what should the generic
> > > > code do more ?
> > > 
> > > I can't say because these patches only add the hooks, there's no
> > > implementation yet which uses the hooks.
> > > 
> > > Given the description about _why_ those hooks are necessary, it seems
> > > that something is required.  Either we start adding custom hacks to
> > > each clockevent driver as is done with this patch, or we get some
> > > generic help in place.
> > > 
> > > I'm not thrilled by the custom hack approach - and I thought the
> > > clockevent stuff was created to stop this kind of thing happening.
> > 
> > Yes, and it does the right thing:
> > 
> >      idle enter (where the cpu local tick device stops)
> > 
> >      	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_ENTER)
> > 
> > 		clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN)
> > 		tick_broadcast_set_event(dev->next_event, 1)
> > 
> >     idle exit
> > 
> >      	  tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_EXIT)
> > 
> > 		clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT)
> > 		tick_program_event(dev->next_event, 1)
> > 
> > So the generic code has all the calls in place. If a clock chip
> > implementation misses to set control registers in the
> > CLOCK_EVT_MODE_ONESHOT case, then it's not a short coming of the
> > generic code which needs magic hooks in the arch code.
> > 
> > The same applies for the periodic mode switch, which is handled via
> > tick_broadcast_on_off().
> > 
> > Am I missing something ?
> 
> I quote Colin:
> 
> | This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> | gets reset by cpu idle between twd_set_mode and twd_set_next_event.

And I fear that he means the twd_set_mode(ONESHOT) call which happens
during boot/cpuonline.

The point is that the above function needs to be called from the cpu
idle code to notify the clock events layer. The correct mechanism is
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT). The
existing callers are in:

arch/x86/kernel/process.c
drivers/acpi/acpi_pad.c
drivers/acpi/processor_idle.c
drivers/idle/intel_idle.c

So i'm not surprised, that this wont work on ARM :)

Thanks,

	tglx

RE: [PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

[-- Attachment #1: Type: text/plain, Size: 3057 bytes --]

Russell, Thomas
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 7:08 PM
> To: Thomas Gleixner
> Cc: Colin Cross; Santosh Shilimkar; catalin.marinas@arm.com;
> linus.ml.walleij@gmail.com; linux-omap@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
[...]
>
> I can't say because these patches only add the hooks, there's no
> implementation yet which uses the hooks.
>
> Given the description about _why_ those hooks are necessary, it
> seems
> that something is required.  Either we start adding custom hacks to
> each clockevent driver as is done with this patch, or we get some
> generic help in place.
>
> I'm not thrilled by the custom hack approach - and I thought the
> clockevent stuff was created to stop this kind of thing happening.
>
> I suggest we defer this until there's a visible use case available.
Got some time to debug this issue. OMAP idle code already has the
broad-cast notifiers working for sometime. So I removed the existing
save restore twd code and just used the below patch which Russell
posted on this thread.
And things just work and I guess we are done with this issue !!
Timer framework is doing all right things with notifiers.

Am going to use this patch for my further work and drop the
save/restore patch. Will update you if there is any other
issue on this. I should have validated this patch on my own
earlier :(

Thanks for the discussion.

-----------------
>From 9084dd3e68a8528172d97fb49c42437ae873af43 Mon Sep 17 00:00:00 2001
From: Russell King <rmk+kernel@arm.linux.org.uk>
Date: Tue, 25 Jan 2011 14:26:41 +0530
Subject: [PATCH] ARM: smp_twd: Always program the loadvalue when switching
to periodic mode.

Always program the load value when switching to periodic mode so that
we don't need to save restore twd load register.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/kernel/smp_twd.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
 		/* timer load already set up */
 		ctrl = TWD_TIMER_CONTROL_ENABLE |
TWD_TIMER_CONTROL_IT_ENABLE
 			| TWD_TIMER_CONTROL_PERIODIC;
+		__raw_writel(twd_timer_rate / HZ, twd_base +
TWD_TIMER_LOAD);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)

 static void __cpuinit twd_calibrate_rate(void)
 {
-	unsigned long load, count;
+	unsigned long count;
 	u64 waitjiffies;

 	/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
 		printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
 			(twd_timer_rate / 1000000) % 100);
 	}
-
-	load = twd_timer_rate / HZ;
-
-	__raw_writel(load, twd_base + TWD_TIMER_LOAD);
 }

 /*
-- 
1.6.0.4

[-- Attachment #2: 0001-ARM-smp_twd-Always-program-the-loadvalue-when-swit.patch --]
[-- Type: application/octet-stream, Size: 1503 bytes --]

From 9084dd3e68a8528172d97fb49c42437ae873af43 Mon Sep 17 00:00:00 2001
From: Russell King <rmk+kernel@arm.linux.org.uk>
Date: Tue, 25 Jan 2011 14:26:41 +0530
Subject: [PATCH] ARM: smp_twd: Always program the loadvalue when switching to periodic mode.

Always program the load value when switching to periodic mode so that
we don't need to save restore twd load register.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/kernel/smp_twd.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
 		/* timer load already set up */
 		ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
 			| TWD_TIMER_CONTROL_PERIODIC;
+		__raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
 
 static void __cpuinit twd_calibrate_rate(void)
 {
-	unsigned long load, count;
+	unsigned long count;
 	u64 waitjiffies;
 
 	/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
 		printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
 			(twd_timer_rate / 1000000) % 100);
 	}
-
-	load = twd_timer_rate / HZ;
-
-	__raw_writel(load, twd_base + TWD_TIMER_LOAD);
 }
 
 /*
-- 
1.6.0.4


[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

Russell, Thomas
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 7:08 PM
> To: Thomas Gleixner
> Cc: Colin Cross; Santosh Shilimkar; catalin.marinas at arm.com;
> linus.ml.walleij at gmail.com; linux-omap at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
[...]
>
> I can't say because these patches only add the hooks, there's no
> implementation yet which uses the hooks.
>
> Given the description about _why_ those hooks are necessary, it
> seems
> that something is required.  Either we start adding custom hacks to
> each clockevent driver as is done with this patch, or we get some
> generic help in place.
>
> I'm not thrilled by the custom hack approach - and I thought the
> clockevent stuff was created to stop this kind of thing happening.
>
> I suggest we defer this until there's a visible use case available.
Got some time to debug this issue. OMAP idle code already has the
broad-cast notifiers working for sometime. So I removed the existing
save restore twd code and just used the below patch which Russell
posted on this thread.
And things just work and I guess we are done with this issue !!
Timer framework is doing all right things with notifiers.

Am going to use this patch for my further work and drop the
save/restore patch. Will update you if there is any other
issue on this. I should have validated this patch on my own
earlier :(

Thanks for the discussion.

-----------------

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 09:34:15PM +0530, Santosh Shilimkar wrote:
> Russell, Thomas
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> > Sent: Tuesday, January 25, 2011 7:08 PM
> > To: Thomas Gleixner
> > Cc: Colin Cross; Santosh Shilimkar; catalin.marinas@arm.com;
> > linus.ml.walleij@gmail.com; linux-omap@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
> >
> [...]
> >
> > I can't say because these patches only add the hooks, there's no
> > implementation yet which uses the hooks.
> >
> > Given the description about _why_ those hooks are necessary, it
> > seems
> > that something is required.  Either we start adding custom hacks to
> > each clockevent driver as is done with this patch, or we get some
> > generic help in place.
> >
> > I'm not thrilled by the custom hack approach - and I thought the
> > clockevent stuff was created to stop this kind of thing happening.
> >
> > I suggest we defer this until there's a visible use case available.
> Got some time to debug this issue. OMAP idle code already has the
> broad-cast notifiers working for sometime. So I removed the existing
> save restore twd code and just used the below patch which Russell
> posted on this thread.
> And things just work and I guess we are done with this issue !!
> Timer framework is doing all right things with notifiers.
> 
> Am going to use this patch for my further work and drop the
> save/restore patch. Will update you if there is any other
> issue on this. I should have validated this patch on my own
> earlier :(
> 
> Thanks for the discussion.

Thanks - I guess that's a tested-by then?

[PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 09:34:15PM +0530, Santosh Shilimkar wrote:
> Russell, Thomas
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > Sent: Tuesday, January 25, 2011 7:08 PM
> > To: Thomas Gleixner
> > Cc: Colin Cross; Santosh Shilimkar; catalin.marinas at arm.com;
> > linus.ml.walleij at gmail.com; linux-omap at vger.kernel.org; linux-arm-
> > kernel at lists.infradead.org
> > Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
> >
> [...]
> >
> > I can't say because these patches only add the hooks, there's no
> > implementation yet which uses the hooks.
> >
> > Given the description about _why_ those hooks are necessary, it
> > seems
> > that something is required.  Either we start adding custom hacks to
> > each clockevent driver as is done with this patch, or we get some
> > generic help in place.
> >
> > I'm not thrilled by the custom hack approach - and I thought the
> > clockevent stuff was created to stop this kind of thing happening.
> >
> > I suggest we defer this until there's a visible use case available.
> Got some time to debug this issue. OMAP idle code already has the
> broad-cast notifiers working for sometime. So I removed the existing
> save restore twd code and just used the below patch which Russell
> posted on this thread.
> And things just work and I guess we are done with this issue !!
> Timer framework is doing all right things with notifiers.
> 
> Am going to use this patch for my further work and drop the
> save/restore patch. Will update you if there is any other
> issue on this. I should have validated this patch on my own
> earlier :(
> 
> Thanks for the discussion.

Thanks - I guess that's a tested-by then?

RE: [PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 9:43 PM
> To: Santosh Shilimkar
> Cc: Thomas Gleixner; Colin Cross; catalin.marinas@arm.com;
> linus.ml.walleij@gmail.com; linux-omap@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Tue, Jan 25, 2011 at 09:34:15PM +0530, Santosh Shilimkar wrote:
> > Russell, Thomas
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> > > Sent: Tuesday, January 25, 2011 7:08 PM
> > > To: Thomas Gleixner
> > > Cc: Colin Cross; Santosh Shilimkar; catalin.marinas@arm.com;
> > > linus.ml.walleij@gmail.com; linux-omap@vger.kernel.org; linux-
> arm-
> > > kernel@lists.infradead.org
> > > Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore
> support
> > >
> > [...]
> > >
> > > I can't say because these patches only add the hooks, there's no
> > > implementation yet which uses the hooks.
> > >
> > > Given the description about _why_ those hooks are necessary, it
> > > seems
> > > that something is required.  Either we start adding custom hacks
> to
> > > each clockevent driver as is done with this patch, or we get
> some
> > > generic help in place.
> > >
> > > I'm not thrilled by the custom hack approach - and I thought the
> > > clockevent stuff was created to stop this kind of thing
> happening.
> > >
> > > I suggest we defer this until there's a visible use case
> available.
> > Got some time to debug this issue. OMAP idle code already has the
> > broad-cast notifiers working for sometime. So I removed the
> existing
> > save restore twd code and just used the below patch which Russell
> > posted on this thread.
> > And things just work and I guess we are done with this issue !!
> > Timer framework is doing all right things with notifiers.
> >
> > Am going to use this patch for my further work and drop the
> > save/restore patch. Will update you if there is any other
> > issue on this. I should have validated this patch on my own
> > earlier :(
> >
> > Thanks for the discussion.
>
> Thanks - I guess that's a tested-by then?

Sure.
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 9:43 PM
> To: Santosh Shilimkar
> Cc: Thomas Gleixner; Colin Cross; catalin.marinas at arm.com;
> linus.ml.walleij at gmail.com; linux-omap at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Tue, Jan 25, 2011 at 09:34:15PM +0530, Santosh Shilimkar wrote:
> > Russell, Thomas
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > > Sent: Tuesday, January 25, 2011 7:08 PM
> > > To: Thomas Gleixner
> > > Cc: Colin Cross; Santosh Shilimkar; catalin.marinas at arm.com;
> > > linus.ml.walleij at gmail.com; linux-omap at vger.kernel.org; linux-
> arm-
> > > kernel at lists.infradead.org
> > > Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore
> support
> > >
> > [...]
> > >
> > > I can't say because these patches only add the hooks, there's no
> > > implementation yet which uses the hooks.
> > >
> > > Given the description about _why_ those hooks are necessary, it
> > > seems
> > > that something is required.  Either we start adding custom hacks
> to
> > > each clockevent driver as is done with this patch, or we get
> some
> > > generic help in place.
> > >
> > > I'm not thrilled by the custom hack approach - and I thought the
> > > clockevent stuff was created to stop this kind of thing
> happening.
> > >
> > > I suggest we defer this until there's a visible use case
> available.
> > Got some time to debug this issue. OMAP idle code already has the
> > broad-cast notifiers working for sometime. So I removed the
> existing
> > save restore twd code and just used the below patch which Russell
> > posted on this thread.
> > And things just work and I guess we are done with this issue !!
> > Timer framework is doing all right things with notifiers.
> >
> > Am going to use this patch for my further work and drop the
> > save/restore patch. Will update you if there is any other
> > issue on this. I should have validated this patch on my own
> > earlier :(
> >
> > Thanks for the discussion.
>
> Thanks - I guess that's a tested-by then?

Sure.
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> gets reset by cpu idle between twd_set_mode and twd_set_next_event.
> Shadowing ctrl in a percpu variable and rewriting it during every call
> to twd_set_next_event does work, but that's not much different, and a
> little less efficient, than just saving and restoring the control and
> load registers in idle.  It does have the advantage that platforms
> don't need any extra calls.

BTW, do you think the patch is, nevertheless, an improvement and something
we should do?  If so, please can I have your ack for it?

Thanks.

[PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> gets reset by cpu idle between twd_set_mode and twd_set_next_event.
> Shadowing ctrl in a percpu variable and rewriting it during every call
> to twd_set_next_event does work, but that's not much different, and a
> little less efficient, than just saving and restoring the control and
> load registers in idle.  It does have the advantage that platforms
> don't need any extra calls.

BTW, do you think the patch is, nevertheless, an improvement and something
we should do?  If so, please can I have your ack for it?

Thanks.

RE: [PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:00 PM
> To: Colin Cross
> Cc: Santosh Shilimkar; catalin.marinas@arm.com;
> linus.ml.walleij@gmail.com; linux-omap@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> > This doesn't work for oneshot timers if the TWD_TIMER_CONTROL
> register
> > gets reset by cpu idle between twd_set_mode and
> twd_set_next_event.
> > Shadowing ctrl in a percpu variable and rewriting it during every
> call
> > to twd_set_next_event does work, but that's not much different,
> and a
> > little less efficient, than just saving and restoring the control
> and
> > load registers in idle.  It does have the advantage that platforms
> > don't need any extra calls.
>
> BTW, do you think the patch is, nevertheless, an improvement and
> something
> we should do?  If so, please can I have your ack for it?
>
As I was suspecting the one shot mode wouldn't work it. Collin
just confirmed it.
To make it fully work it needs control register save restore.
And that can be managed within TWD library so that every platform
doesn't have to bother of calling it from their idle code.

Do you prefer that as a separate patch or I can post a new version
in which can add your fix + per cpu control register shadowing ?

Regards,
Santosh

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:00 PM
> To: Colin Cross
> Cc: Santosh Shilimkar; catalin.marinas at arm.com;
> linus.ml.walleij at gmail.com; linux-omap at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
> > This doesn't work for oneshot timers if the TWD_TIMER_CONTROL
> register
> > gets reset by cpu idle between twd_set_mode and
> twd_set_next_event.
> > Shadowing ctrl in a percpu variable and rewriting it during every
> call
> > to twd_set_next_event does work, but that's not much different,
> and a
> > little less efficient, than just saving and restoring the control
> and
> > load registers in idle.  It does have the advantage that platforms
> > don't need any extra calls.
>
> BTW, do you think the patch is, nevertheless, an improvement and
> something
> we should do?  If so, please can I have your ack for it?
>
As I was suspecting the one shot mode wouldn't work it. Collin
just confirmed it.
To make it fully work it needs control register save restore.
And that can be managed within TWD library so that every platform
doesn't have to bother of calling it from their idle code.

Do you prefer that as a separate patch or I can post a new version
in which can add your fix + per cpu control register shadowing ?

Regards,
Santosh

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 05:10:37PM +0530, Santosh Shilimkar wrote:
> As I was suspecting the one shot mode wouldn't work it. Collin
> just confirmed it.
> To make it fully work it needs control register save restore.
> And that can be managed within TWD library so that every platform
> doesn't have to bother of calling it from their idle code.
> 
> Do you prefer that as a separate patch or I can post a new version
> in which can add your fix + per cpu control register shadowing ?

I said earlier:
| The next question is can we teach the generic time infrastructure about
| this so we don't have to modify every clock event driver for it?  We
| really need to get away from having this kind of knowledge buried down
| in the lowest levels of every driver.

IOW, if we go into a PM idle mode, when we come back out we need to
call clockevent set_mode to ensure that the control register is properly
reset.  I don't think its right to have this kind of knowledge buried
in each and every clockevent driver.

[PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 05:10:37PM +0530, Santosh Shilimkar wrote:
> As I was suspecting the one shot mode wouldn't work it. Collin
> just confirmed it.
> To make it fully work it needs control register save restore.
> And that can be managed within TWD library so that every platform
> doesn't have to bother of calling it from their idle code.
> 
> Do you prefer that as a separate patch or I can post a new version
> in which can add your fix + per cpu control register shadowing ?

I said earlier:
| The next question is can we teach the generic time infrastructure about
| this so we don't have to modify every clock event driver for it?  We
| really need to get away from having this kind of knowledge buried down
| in the lowest levels of every driver.

IOW, if we go into a PM idle mode, when we come back out we need to
call clockevent set_mode to ensure that the control register is properly
reset.  I don't think its right to have this kind of knowledge buried
in each and every clockevent driver.

RE: [PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

Looping Thomas G
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:19 PM
> To: Santosh Shilimkar
> Cc: Colin Cross; catalin.marinas@arm.com;
> linus.ml.walleij@gmail.com; linux-omap@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Tue, Jan 25, 2011 at 05:10:37PM +0530, Santosh Shilimkar wrote:
> > As I was suspecting the one shot mode wouldn't work it. Collin
> > just confirmed it.
> > To make it fully work it needs control register save restore.
> > And that can be managed within TWD library so that every platform
> > doesn't have to bother of calling it from their idle code.
> >
> > Do you prefer that as a separate patch or I can post a new version
> > in which can add your fix + per cpu control register shadowing ?
>
> I said earlier:
> | The next question is can we teach the generic time infrastructure
> about
> | this so we don't have to modify every clock event driver for it?
> We
> | really need to get away from having this kind of knowledge buried
> down
> | in the lowest levels of every driver.
>
> IOW, if we go into a PM idle mode, when we come back out we need to
> call clockevent set_mode to ensure that the control register is
> properly
> reset.  I don't think its right to have this kind of knowledge
> buried
> in each and every clockevent driver.
May be Thomas can comment on this.

Regards,
Santosh

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

Looping Thomas G
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:19 PM
> To: Santosh Shilimkar
> Cc: Colin Cross; catalin.marinas at arm.com;
> linus.ml.walleij at gmail.com; linux-omap at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Tue, Jan 25, 2011 at 05:10:37PM +0530, Santosh Shilimkar wrote:
> > As I was suspecting the one shot mode wouldn't work it. Collin
> > just confirmed it.
> > To make it fully work it needs control register save restore.
> > And that can be managed within TWD library so that every platform
> > doesn't have to bother of calling it from their idle code.
> >
> > Do you prefer that as a separate patch or I can post a new version
> > in which can add your fix + per cpu control register shadowing ?
>
> I said earlier:
> | The next question is can we teach the generic time infrastructure
> about
> | this so we don't have to modify every clock event driver for it?
> We
> | really need to get away from having this kind of knowledge buried
> down
> | in the lowest levels of every driver.
>
> IOW, if we go into a PM idle mode, when we come back out we need to
> call clockevent set_mode to ensure that the control register is
> properly
> reset.  I don't think its right to have this kind of knowledge
> buried
> in each and every clockevent driver.
May be Thomas can comment on this.

Regards,
Santosh

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 05:38:49PM +0530, Santosh Shilimkar wrote:
> Looping Thomas G
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> > Sent: Tuesday, January 25, 2011 5:19 PM
> > To: Santosh Shilimkar
> > Cc: Colin Cross; catalin.marinas@arm.com;
> > linus.ml.walleij@gmail.com; linux-omap@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
> >
> > On Tue, Jan 25, 2011 at 05:10:37PM +0530, Santosh Shilimkar wrote:
> > > As I was suspecting the one shot mode wouldn't work it. Collin
> > > just confirmed it.
> > > To make it fully work it needs control register save restore.
> > > And that can be managed within TWD library so that every platform
> > > doesn't have to bother of calling it from their idle code.
> > >
> > > Do you prefer that as a separate patch or I can post a new version
> > > in which can add your fix + per cpu control register shadowing ?
> >
> > I said earlier:
> > | The next question is can we teach the generic time infrastructure
> > about
> > | this so we don't have to modify every clock event driver for it?
> > We
> > | really need to get away from having this kind of knowledge buried
> > down
> > | in the lowest levels of every driver.
> >
> > IOW, if we go into a PM idle mode, when we come back out we need to
> > call clockevent set_mode to ensure that the control register is
> > properly
> > reset.  I don't think its right to have this kind of knowledge
> > buried
> > in each and every clockevent driver.
> May be Thomas can comment on this.

He was already added on my previous email... we're going round in circles.

[PATCH 3/5] ARM: twd: Add context save restore support

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 05:38:49PM +0530, Santosh Shilimkar wrote:
> Looping Thomas G
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > Sent: Tuesday, January 25, 2011 5:19 PM
> > To: Santosh Shilimkar
> > Cc: Colin Cross; catalin.marinas at arm.com;
> > linus.ml.walleij at gmail.com; linux-omap at vger.kernel.org; linux-arm-
> > kernel at lists.infradead.org
> > Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
> >
> > On Tue, Jan 25, 2011 at 05:10:37PM +0530, Santosh Shilimkar wrote:
> > > As I was suspecting the one shot mode wouldn't work it. Collin
> > > just confirmed it.
> > > To make it fully work it needs control register save restore.
> > > And that can be managed within TWD library so that every platform
> > > doesn't have to bother of calling it from their idle code.
> > >
> > > Do you prefer that as a separate patch or I can post a new version
> > > in which can add your fix + per cpu control register shadowing ?
> >
> > I said earlier:
> > | The next question is can we teach the generic time infrastructure
> > about
> > | this so we don't have to modify every clock event driver for it?
> > We
> > | really need to get away from having this kind of knowledge buried
> > down
> > | in the lowest levels of every driver.
> >
> > IOW, if we go into a PM idle mode, when we come back out we need to
> > call clockevent set_mode to ensure that the control register is
> > properly
> > reset.  I don't think its right to have this kind of knowledge
> > buried
> > in each and every clockevent driver.
> May be Thomas can comment on this.

He was already added on my previous email... we're going round in circles.

RE: [PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:48 PM
> To: Santosh Shilimkar
> Cc: Thomas Gleixner; Colin Cross; catalin.marinas@arm.com;
> linus.ml.walleij@gmail.com; linux-omap@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>

> > > call clockevent set_mode to ensure that the control register is
> > > properly
> > > reset.  I don't think its right to have this kind of knowledge
> > > buried
> > > in each and every clockevent driver.
> > May be Thomas can comment on this.
>
> He was already added on my previous email... we're going round in
> circles.
Sorry about that. Didn't noticed earlier

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:48 PM
> To: Santosh Shilimkar
> Cc: Thomas Gleixner; Colin Cross; catalin.marinas at arm.com;
> linus.ml.walleij at gmail.com; linux-omap at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>

> > > call clockevent set_mode to ensure that the control register is
> > > properly
> > > reset.  I don't think its right to have this kind of knowledge
> > > buried
> > > in each and every clockevent driver.
> > May be Thomas can comment on this.
>
> He was already added on my previous email... we're going round in
> circles.
Sorry about that. Didn't noticed earlier

Re: [PATCH 3/5] ARM: twd: Add context save restore support

[Colin Cross]

On Tue, Jan 25, 2011 at 3:29 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
>> This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
>> gets reset by cpu idle between twd_set_mode and twd_set_next_event.
>> Shadowing ctrl in a percpu variable and rewriting it during every call
>> to twd_set_next_event does work, but that's not much different, and a
>> little less efficient, than just saving and restoring the control and
>> load registers in idle.  It does have the advantage that platforms
>> don't need any extra calls.
>
> BTW, do you think the patch is, nevertheless, an improvement and something
> we should do?  If so, please can I have your ack for it?

This patch does work after adding the missing clockevent broadcasts to
the Tegra idle driver, and allows me to remove all references to twd
registers in the idle and suspend code, so:
Acked-by: Colin Cross <ccross@android.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] ARM: twd: Add context save restore support

[Colin Cross]

On Tue, Jan 25, 2011 at 3:29 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
>> This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
>> gets reset by cpu idle between twd_set_mode and twd_set_next_event.
>> Shadowing ctrl in a percpu variable and rewriting it during every call
>> to twd_set_next_event does work, but that's not much different, and a
>> little less efficient, than just saving and restoring the control and
>> load registers in idle. ?It does have the advantage that platforms
>> don't need any extra calls.
>
> BTW, do you think the patch is, nevertheless, an improvement and something
> we should do? ?If so, please can I have your ack for it?

This patch does work after adding the missing clockevent broadcasts to
the Tegra idle driver, and allows me to remove all references to twd
registers in the idle and suspend code, so:
Acked-by: Colin Cross <ccross@android.com>

RE: [PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Monday, January 24, 2011 4:36 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel@lists.infradead.org; catalin.marinas@arm.com;
> ccross@android.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > In CPU low power state, local timer looses its register context.
> This
> > patch adds context save restore hooks which can be used by
> platforms
> > appropriately.
>
> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the
> generic
> timer stuff wouldn't rely on it being kept alive?
>
> Hmm, it looks like we bypass the clockevents code by only setting
> the
> TWD load value at initialization time, not when we switch to
> periodic
> mode.  We really ought to rewrite it whenever we switch back to
> periodic
> mode.
>
Idea looks good though am not sure the one shot mode.

> I suspect fixing that means you won't need this save/restore
> support.
Will try it out.

Regards,
Santosh

[PATCH 3/5] ARM: twd: Add context save restore support

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Monday, January 24, 2011 4:36 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; catalin.marinas at arm.com;
> ccross at android.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org
> Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
>
> On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
> > In CPU low power state, local timer looses its register context.
> This
> > patch adds context save restore hooks which can be used by
> platforms
> > appropriately.
>
> I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the
> generic
> timer stuff wouldn't rely on it being kept alive?
>
> Hmm, it looks like we bypass the clockevents code by only setting
> the
> TWD load value at initialization time, not when we switch to
> periodic
> mode.  We really ought to rewrite it whenever we switch back to
> periodic
> mode.
>
Idea looks good though am not sure the one shot mode.

> I suspect fixing that means you won't need this save/restore
> support.
Will try it out.

Regards,
Santosh

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

This patch moves SCU register defines from smp_scu.c to smp_scu.h
so that its available for platforms to use

The SCU CPU power status registers is used for power management
on OMAP4.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/smp_scu.h |    6 ++++++
 arch/arm/kernel/smp_scu.c      |    6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..b6f42c9 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,6 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define SCU_CTRL		0x00
+#define SCU_CONFIG		0x04
+#define SCU_CPU_STATUS		0x08
+#define SCU_INVALIDATE		0x0c
+#define SCU_FPGA_REVISION	0x10
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
 
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..ee7bf47 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -14,12 +14,6 @@
 #include <asm/smp_scu.h>
 #include <asm/cacheflush.h>
 
-#define SCU_CTRL		0x00
-#define SCU_CONFIG		0x04
-#define SCU_CPU_STATUS		0x08
-#define SCU_INVALIDATE		0x0c
-#define SCU_FPGA_REVISION	0x10
-
 /*
  * Get the number of CPU cores from the SCU configuration
  */
-- 
1.6.0.4

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

This patch moves SCU register defines from smp_scu.c to smp_scu.h
so that its available for platforms to use

The SCU CPU power status registers is used for power management
on OMAP4.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/smp_scu.h |    6 ++++++
 arch/arm/kernel/smp_scu.c      |    6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..b6f42c9 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,6 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define SCU_CTRL		0x00
+#define SCU_CONFIG		0x04
+#define SCU_CPU_STATUS		0x08
+#define SCU_INVALIDATE		0x0c
+#define SCU_FPGA_REVISION	0x10
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
 
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..ee7bf47 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -14,12 +14,6 @@
 #include <asm/smp_scu.h>
 #include <asm/cacheflush.h>
 
-#define SCU_CTRL		0x00
-#define SCU_CONFIG		0x04
-#define SCU_CPU_STATUS		0x08
-#define SCU_INVALIDATE		0x0c
-#define SCU_FPGA_REVISION	0x10
-
 /*
  * Get the number of CPU cores from the SCU configuration
  */
-- 
1.6.0.4

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 02:21:18PM +0530, Santosh Shilimkar wrote:
> This patch moves SCU register defines from smp_scu.c to smp_scu.h
> so that its available for platforms to use
> 
> The SCU CPU power status registers is used for power management
> on OMAP4.

Could we instead have an API inside smp_scu.c rather than having each
SoC implement its own SCU PM stuff?

Maybe this.  Note that scu_power_mode() needs to be called from a non-
preemptible context, which is sane as we don't want to be rescheduled
onto a different CPU between scu_power_mode() and the wfi/wfe required
to enter the selected mode.

 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   11 +++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..ec11a92 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,7 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define	SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(unsigned int);
 
 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..bbb9b1c 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -50,3 +50,14 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+int scu_power_mode(unsigned int mode)
+{
+	int cpu = smp_processor_id();
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	__raw_writel(mode << (8 * cpu), scu_base + SCU_CPU_STATUS);
+	return 0;
+}

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 02:21:18PM +0530, Santosh Shilimkar wrote:
> This patch moves SCU register defines from smp_scu.c to smp_scu.h
> so that its available for platforms to use
> 
> The SCU CPU power status registers is used for power management
> on OMAP4.

Could we instead have an API inside smp_scu.c rather than having each
SoC implement its own SCU PM stuff?

Maybe this.  Note that scu_power_mode() needs to be called from a non-
preemptible context, which is sane as we don't want to be rescheduled
onto a different CPU between scu_power_mode() and the wfi/wfe required
to enter the selected mode.

 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   11 +++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..ec11a92 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,7 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define	SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(unsigned int);
 
 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..bbb9b1c 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -50,3 +50,14 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+int scu_power_mode(unsigned int mode)
+{
+	int cpu = smp_processor_id();
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	__raw_writel(mode << (8 * cpu), scu_base + SCU_CPU_STATUS);
+	return 0;
+}

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:17 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel@lists.infradead.org; catalin.marinas@arm.com;
> ccross@android.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Mon, Jan 24, 2011 at 02:21:18PM +0530, Santosh Shilimkar wrote:
> > This patch moves SCU register defines from smp_scu.c to smp_scu.h
> > so that its available for platforms to use
> >
> > The SCU CPU power status registers is used for power management
> > on OMAP4.
>
> Could we instead have an API inside smp_scu.c rather than having
> each
> SoC implement its own SCU PM stuff?
>
I have similar patch implemented but what it does is just
prepares the value to be programmed and stores it to a scratchpad.
I see your point but below patch may now work well. The reason is the SCU
register needs to accessed at the lowest level. May be even when
stack is not available. Also this register needs to be cleared in cases
where the attempted power state is failed for some reason.

Also note that this register can be blocked using trust-zone which
again makes it platform dependent because direct write won't be
allowed in that case.

> Maybe this.  Note that scu_power_mode() needs to be called from a
> non-
> preemptible context, which is sane as we don't want to be
> rescheduled
> onto a different CPU between scu_power_mode() and the wfi/wfe
> required
> to enter the selected mode.
>
I would prefer the header export if there is no major
objection since there is already a possibility of custom
implementation with trust zone.

On OMAP4, on ES1.0 we need to use a secure API to achieve
this where as on ES2.0, it can be directly accessed.

Regards,
Santosh

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:17 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; catalin.marinas at arm.com;
> ccross at android.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Mon, Jan 24, 2011 at 02:21:18PM +0530, Santosh Shilimkar wrote:
> > This patch moves SCU register defines from smp_scu.c to smp_scu.h
> > so that its available for platforms to use
> >
> > The SCU CPU power status registers is used for power management
> > on OMAP4.
>
> Could we instead have an API inside smp_scu.c rather than having
> each
> SoC implement its own SCU PM stuff?
>
I have similar patch implemented but what it does is just
prepares the value to be programmed and stores it to a scratchpad.
I see your point but below patch may now work well. The reason is the SCU
register needs to accessed at the lowest level. May be even when
stack is not available. Also this register needs to be cleared in cases
where the attempted power state is failed for some reason.

Also note that this register can be blocked using trust-zone which
again makes it platform dependent because direct write won't be
allowed in that case.

> Maybe this.  Note that scu_power_mode() needs to be called from a
> non-
> preemptible context, which is sane as we don't want to be
> rescheduled
> onto a different CPU between scu_power_mode() and the wfi/wfe
> required
> to enter the selected mode.
>
I would prefer the header export if there is no major
objection since there is already a possibility of custom
implementation with trust zone.

On OMAP4, on ES1.0 we need to use a secure API to achieve
this where as on ES2.0, it can be directly accessed.

Regards,
Santosh

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 05:32:57PM +0530, Santosh Shilimkar wrote:
> I have similar patch implemented but what it does is just
> prepares the value to be programmed and stores it to a scratchpad.
> I see your point but below patch may now work well. The reason is the SCU
> register needs to accessed at the lowest level. May be even when
> stack is not available. Also this register needs to be cleared in cases
> where the attempted power state is failed for some reason.

>From the documentation I have, the power control register has no effect
until the CPU enters WFI mode - which means that provided we can
guarantee no one issues a WFI instruction between setting the power mode,
and executing that instruction, being woken up (or failing) and resetting
the power mode back... that shouldn't require the power mode to be
programmed from assembly code.

In any case, we actually need the help of spinlocks to deal with
concurrent access to the SCU power control register - something you
can't do in assembly code.

On the way down to a WFI low power mode, we can call scu_power_mode(),
do the rest of the cleanup (which must not schedule) and issue WFI.  On
the way back up, do whatever needs to be done and call scu_power_mode()
to reset back to 'normal' mode.

> Also note that this register can be blocked using trust-zone which
> again makes it platform dependent because direct write won't be
> allowed in that case.

Yes, I did notice.  What's the OMAP SMC interface for that?

> I would prefer the header export if there is no major
> objection since there is already a possibility of custom
> implementation with trust zone.
> 
> On OMAP4, on ES1.0 we need to use a secure API to achieve
> this where as on ES2.0, it can be directly accessed.

As I say, I'd rather not have each SoC implementing access to this as
someone's bound to forget the spinlock if they're dealing with more than
one CPU, which'll make stuff unreliable (just like I did on my initial
version.)

We could have a callback to SoC code which does the appropriate SMC call,
but first I'll need to know what's required for the SMC call.

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 05:32:57PM +0530, Santosh Shilimkar wrote:
> I have similar patch implemented but what it does is just
> prepares the value to be programmed and stores it to a scratchpad.
> I see your point but below patch may now work well. The reason is the SCU
> register needs to accessed at the lowest level. May be even when
> stack is not available. Also this register needs to be cleared in cases
> where the attempted power state is failed for some reason.

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 12:16:56PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 05:32:57PM +0530, Santosh Shilimkar wrote:
> > I have similar patch implemented but what it does is just
> > prepares the value to be programmed and stores it to a scratchpad.
> > I see your point but below patch may now work well. The reason is the SCU
> > register needs to accessed at the lowest level. May be even when
> > stack is not available. Also this register needs to be cleared in cases
> > where the attempted power state is failed for some reason.
> 
> >From the documentation I have, the power control register has no effect
> until the CPU enters WFI mode - which means that provided we can
> guarantee no one issues a WFI instruction between setting the power mode,
> and executing that instruction, being woken up (or failing) and resetting
> the power mode back... that shouldn't require the power mode to be
> programmed from assembly code.
> 
> In any case, we actually need the help of spinlocks to deal with
> concurrent access to the SCU power control register - something you
> can't do in assembly code.
> 
> On the way down to a WFI low power mode, we can call scu_power_mode(),
> do the rest of the cleanup (which must not schedule) and issue WFI.  On
> the way back up, do whatever needs to be done and call scu_power_mode()
> to reset back to 'normal' mode.
> 
> > Also note that this register can be blocked using trust-zone which
> > again makes it platform dependent because direct write won't be
> > allowed in that case.
> 
> Yes, I did notice.  What's the OMAP SMC interface for that?
> 
> > I would prefer the header export if there is no major
> > objection since there is already a possibility of custom
> > implementation with trust zone.
> > 
> > On OMAP4, on ES1.0 we need to use a secure API to achieve
> > this where as on ES2.0, it can be directly accessed.
> 
> As I say, I'd rather not have each SoC implementing access to this as
> someone's bound to forget the spinlock if they're dealing with more than
> one CPU, which'll make stuff unreliable (just like I did on my initial
> version.)
> 
> We could have a callback to SoC code which does the appropriate SMC call,
> but first I'll need to know what's required for the SMC call.

I did mean to include the updated patch:

 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..108f31d 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,7 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(unsigned int);
 
 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..4c4c90d 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -10,6 +10,7 @@
  */
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
 
 #include <asm/smp_scu.h>
 #include <asm/cacheflush.h>
@@ -20,6 +21,8 @@
 #define SCU_INVALIDATE		0x0c
 #define SCU_FPGA_REVISION	0x10
 
+static DEFINE_SPINLOCK(scu_power_lock);
+
 /*
  * Get the number of CPU cores from the SCU configuration
  */
@@ -50,3 +53,23 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+int scu_power_mode(unsigned int mode)
+{
+	int cpu = smp_processor_id();
+	int shift;
+	u32 val;
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	shift = cpu * 8;
+
+	spin_lock(&scu_power_lock);
+	val = raw_readl(scu_base + SCU_CPU_STATUS) & ~(0x03 << shift);
+	val |= mode << shift;
+	__raw_writel(val, scu_base + SCU_CPU_STATUS);
+	spin_unlock(&scu_power_lock);
+
+	return 0;
+}

If we need a SMC call, that should happen within the spinlock'd region via
a function pointer which the platform can set.  I can't code that into this
patch until I know what the SMC call requires as arguments (cpu number,
power state, or new register value, or mask and bits to set?)

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 12:16:56PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 05:32:57PM +0530, Santosh Shilimkar wrote:
> > I have similar patch implemented but what it does is just
> > prepares the value to be programmed and stores it to a scratchpad.
> > I see your point but below patch may now work well. The reason is the SCU
> > register needs to accessed at the lowest level. May be even when
> > stack is not available. Also this register needs to be cleared in cases
> > where the attempted power state is failed for some reason.
> 
> >From the documentation I have, the power control register has no effect
> until the CPU enters WFI mode - which means that provided we can
> guarantee no one issues a WFI instruction between setting the power mode,
> and executing that instruction, being woken up (or failing) and resetting
> the power mode back... that shouldn't require the power mode to be
> programmed from assembly code.
> 
> In any case, we actually need the help of spinlocks to deal with
> concurrent access to the SCU power control register - something you
> can't do in assembly code.
> 
> On the way down to a WFI low power mode, we can call scu_power_mode(),
> do the rest of the cleanup (which must not schedule) and issue WFI.  On
> the way back up, do whatever needs to be done and call scu_power_mode()
> to reset back to 'normal' mode.
> 
> > Also note that this register can be blocked using trust-zone which
> > again makes it platform dependent because direct write won't be
> > allowed in that case.
> 
> Yes, I did notice.  What's the OMAP SMC interface for that?
> 
> > I would prefer the header export if there is no major
> > objection since there is already a possibility of custom
> > implementation with trust zone.
> > 
> > On OMAP4, on ES1.0 we need to use a secure API to achieve
> > this where as on ES2.0, it can be directly accessed.
> 
> As I say, I'd rather not have each SoC implementing access to this as
> someone's bound to forget the spinlock if they're dealing with more than
> one CPU, which'll make stuff unreliable (just like I did on my initial
> version.)
> 
> We could have a callback to SoC code which does the appropriate SMC call,
> but first I'll need to know what's required for the SMC call.

I did mean to include the updated patch:

 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..108f31d 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,7 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(unsigned int);
 
 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..4c4c90d 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -10,6 +10,7 @@
  */
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
 
 #include <asm/smp_scu.h>
 #include <asm/cacheflush.h>
@@ -20,6 +21,8 @@
 #define SCU_INVALIDATE		0x0c
 #define SCU_FPGA_REVISION	0x10
 
+static DEFINE_SPINLOCK(scu_power_lock);
+
 /*
  * Get the number of CPU cores from the SCU configuration
  */
@@ -50,3 +53,23 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+int scu_power_mode(unsigned int mode)
+{
+	int cpu = smp_processor_id();
+	int shift;
+	u32 val;
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	shift = cpu * 8;
+
+	spin_lock(&scu_power_lock);
+	val = raw_readl(scu_base + SCU_CPU_STATUS) & ~(0x03 << shift);
+	val |= mode << shift;
+	__raw_writel(val, scu_base + SCU_CPU_STATUS);
+	spin_unlock(&scu_power_lock);
+
+	return 0;
+}

If we need a SMC call, that should happen within the spinlock'd region via
a function pointer which the platform can set.  I can't code that into this
patch until I know what the SMC call requires as arguments (cpu number,
power state, or new register value, or mask and bits to set?)

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:59 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> ccross@android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> I did mean to include the updated patch:

As said in other email will work towards this.

>
>  arch/arm/include/asm/smp_scu.h |    5 +++++
>  arch/arm/kernel/smp_scu.c      |   23 +++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 0 deletions(-)
>
[....]

>
> If we need a SMC call, that should happen within the spinlock'd
> region via
> a function pointer which the platform can set.  I can't code that
> into this
> patch until I know what the SMC call requires as arguments (cpu
> number,
> power state, or new register value, or mask and bits to set?)

No problem. Will update SMC stuff if needed.

Regards,
Santosh

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:59 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> ccross at android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> I did mean to include the updated patch:

As said in other email will work towards this.

>
>  arch/arm/include/asm/smp_scu.h |    5 +++++
>  arch/arm/kernel/smp_scu.c      |   23 +++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 0 deletions(-)
>
[....]

>
> If we need a SMC call, that should happen within the spinlock'd
> region via
> a function pointer which the platform can set.  I can't code that
> into this
> patch until I know what the SMC call requires as arguments (cpu
> number,
> power state, or new register value, or mask and bits to set?)

No problem. Will update SMC stuff if needed.

Regards,
Santosh

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:47 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel@lists.infradead.org; catalin.marinas@arm.com;
> ccross@android.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 05:32:57PM +0530, Santosh Shilimkar wrote:
> > I have similar patch implemented but what it does is just
> > prepares the value to be programmed and stores it to a scratchpad.
> > I see your point but below patch may now work well. The reason is
> the SCU
> > register needs to accessed at the lowest level. May be even when
> > stack is not available. Also this register needs to be cleared in
> cases
> > where the attempted power state is failed for some reason.
>
> From the documentation I have, the power control register has no
> effect
> until the CPU enters WFI mode - which means that provided we can
> guarantee no one issues a WFI instruction between setting the power
> mode,
> and executing that instruction, being woken up (or failing) and
> resetting
> the power mode back... that shouldn't require the power mode to be
> programmed from assembly code.
>
> In any case, we actually need the help of spinlocks to deal with
> concurrent access to the SCU power control register - something you
> can't do in assembly code.
>
> On the way down to a WFI low power mode, we can call
> scu_power_mode(),
> do the rest of the cleanup (which must not schedule) and issue WFI.
> On
> the way back up, do whatever needs to be done and call
> scu_power_mode()
> to reset back to 'normal' mode.
>
Ok. I missed some information my last email.
The SCU power status programming is used to take CPU in/out
of coherency as an alternative to SMP bit. We don't
have an access to SMP bit on OMAP4. ARM has already
confirmed SCU programming is same as SMP bit enable/disable.

I don't know how safe is to use spin lock when one CPU is
goes out of coherency after programming the power state. The
spin lock release may not even be visible to other CPU.
The programming happens from IDLE or suspend where the
>
> > Also note that this register can be blocked using trust-zone which
> > again makes it platform dependent because direct write won't be
> > allowed in that case.
>
> Yes, I did notice.  What's the OMAP SMC interface for that?
>
> > I would prefer the header export if there is no major
> > objection since there is already a possibility of custom
> > implementation with trust zone.
> >
> > On OMAP4, on ES1.0 we need to use a secure API to achieve
> > this where as on ES2.0, it can be directly accessed.
>
> As I say, I'd rather not have each SoC implementing access to this
> as
> someone's bound to forget the spinlock if they're dealing with more
> than
> one CPU, which'll make stuff unreliable (just like I did on my
> initial
> version.)
> We could have a callback to SoC code which does the appropriate SMC
> call,
> but first I'll need to know what's required for the SMC call.

Ok. Will try to see if things can be worked out as pet your
suggestion.

Regards,
Santosh

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 5:47 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; catalin.marinas at arm.com;
> ccross at android.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 05:32:57PM +0530, Santosh Shilimkar wrote:
> > I have similar patch implemented but what it does is just
> > prepares the value to be programmed and stores it to a scratchpad.
> > I see your point but below patch may now work well. The reason is
> the SCU
> > register needs to accessed at the lowest level. May be even when
> > stack is not available. Also this register needs to be cleared in
> cases
> > where the attempted power state is failed for some reason.
>
> From the documentation I have, the power control register has no
> effect
> until the CPU enters WFI mode - which means that provided we can
> guarantee no one issues a WFI instruction between setting the power
> mode,
> and executing that instruction, being woken up (or failing) and
> resetting
> the power mode back... that shouldn't require the power mode to be
> programmed from assembly code.
>
> In any case, we actually need the help of spinlocks to deal with
> concurrent access to the SCU power control register - something you
> can't do in assembly code.
>
> On the way down to a WFI low power mode, we can call
> scu_power_mode(),
> do the rest of the cleanup (which must not schedule) and issue WFI.
> On
> the way back up, do whatever needs to be done and call
> scu_power_mode()
> to reset back to 'normal' mode.
>
Ok. I missed some information my last email.
The SCU power status programming is used to take CPU in/out
of coherency as an alternative to SMP bit. We don't
have an access to SMP bit on OMAP4. ARM has already
confirmed SCU programming is same as SMP bit enable/disable.

I don't know how safe is to use spin lock when one CPU is
goes out of coherency after programming the power state. The
spin lock release may not even be visible to other CPU.
The programming happens from IDLE or suspend where the
>
> > Also note that this register can be blocked using trust-zone which
> > again makes it platform dependent because direct write won't be
> > allowed in that case.
>
> Yes, I did notice.  What's the OMAP SMC interface for that?
>
> > I would prefer the header export if there is no major
> > objection since there is already a possibility of custom
> > implementation with trust zone.
> >
> > On OMAP4, on ES1.0 we need to use a secure API to achieve
> > this where as on ES2.0, it can be directly accessed.
>
> As I say, I'd rather not have each SoC implementing access to this
> as
> someone's bound to forget the spinlock if they're dealing with more
> than
> one CPU, which'll make stuff unreliable (just like I did on my
> initial
> version.)
> We could have a callback to SoC code which does the appropriate SMC
> call,
> but first I'll need to know what's required for the SMC call.

Ok. Will try to see if things can be worked out as pet your
suggestion.

Regards,
Santosh

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> Ok. I missed some information my last email.
> The SCU power status programming is used to take CPU in/out
> of coherency as an alternative to SMP bit. We don't
> have an access to SMP bit on OMAP4. ARM has already
> confirmed SCU programming is same as SMP bit enable/disable.
> 
> I don't know how safe is to use spin lock when one CPU is
> goes out of coherency after programming the power state. The
> spin lock release may not even be visible to other CPU.

Erm, I do hope that's not the case, as that means it is unsafe for CPUs in
a SMP system to write to this register without them potentially trampling
over each other.

If it is the case, then the solutions are either:
1. Fix the hardware so that coherency requests only yet turned off
   when entering the WFI state.
2. Fix the hardware such that each CPU has a separate register.

I can't see a software solution to this as we can't use ldrex/strex anything
but memory regions, and memory regions without coherency won't work.
Maybe ARM Support can help by suggesting how a 4-CPU system is supposed
to safely read/modify/write the SCU power control register...

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> Ok. I missed some information my last email.
> The SCU power status programming is used to take CPU in/out
> of coherency as an alternative to SMP bit. We don't
> have an access to SMP bit on OMAP4. ARM has already
> confirmed SCU programming is same as SMP bit enable/disable.
> 
> I don't know how safe is to use spin lock when one CPU is
> goes out of coherency after programming the power state. The
> spin lock release may not even be visible to other CPU.

Erm, I do hope that's not the case, as that means it is unsafe for CPUs in
a SMP system to write to this register without them potentially trampling
over each other.

If it is the case, then the solutions are either:
1. Fix the hardware so that coherency requests only yet turned off
   when entering the WFI state.
2. Fix the hardware such that each CPU has a separate register.

I can't see a software solution to this as we can't use ldrex/strex anything
but memory regions, and memory regions without coherency won't work.
Maybe ARM Support can help by suggesting how a 4-CPU system is supposed
to safely read/modify/write the SCU power control register...

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 12:56:56PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> > Ok. I missed some information my last email.
> > The SCU power status programming is used to take CPU in/out
> > of coherency as an alternative to SMP bit. We don't
> > have an access to SMP bit on OMAP4. ARM has already
> > confirmed SCU programming is same as SMP bit enable/disable.
> > 
> > I don't know how safe is to use spin lock when one CPU is
> > goes out of coherency after programming the power state. The
> > spin lock release may not even be visible to other CPU.
> 
> Erm, I do hope that's not the case, as that means it is unsafe for CPUs in
> a SMP system to write to this register without them potentially trampling
> over each other.
> 
> If it is the case, then the solutions are either:
> 1. Fix the hardware so that coherency requests only yet turned off
>    when entering the WFI state.
> 2. Fix the hardware such that each CPU has a separate register.

Actually, we can do this safely - byte stores are permitted to SCU
registers probably for this very reason.

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 12:56:56PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> > Ok. I missed some information my last email.
> > The SCU power status programming is used to take CPU in/out
> > of coherency as an alternative to SMP bit. We don't
> > have an access to SMP bit on OMAP4. ARM has already
> > confirmed SCU programming is same as SMP bit enable/disable.
> > 
> > I don't know how safe is to use spin lock when one CPU is
> > goes out of coherency after programming the power state. The
> > spin lock release may not even be visible to other CPU.
> 
> Erm, I do hope that's not the case, as that means it is unsafe for CPUs in
> a SMP system to write to this register without them potentially trampling
> over each other.
> 
> If it is the case, then the solutions are either:
> 1. Fix the hardware so that coherency requests only yet turned off
>    when entering the WFI state.
> 2. Fix the hardware such that each CPU has a separate register.

Actually, we can do this safely - byte stores are permitted to SCU
registers probably for this very reason.

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 01:04:22PM +0000, Russell King - ARM Linux wrote:
> Actually, we can do this safely - byte stores are permitted to SCU
> registers probably for this very reason.

3rd revision of the patch:

 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   16 ++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..108f31d 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,7 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(unsigned int);
 
 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..c956984 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -50,3 +50,19 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+int scu_power_mode(unsigned int mode)
+{
+	unsigned int val;
+	int cpu = smp_processor_id();
+	int shift;
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	val = __raw_readb(scu_base + SCU_CPU_STATUS + cpu) & ~0x03;
+	val |= mode;
+	__raw_writeb(val, scu_base + SCU_CPU_STATUS + cpu);
+
+	return 0;
+}

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 01:04:22PM +0000, Russell King - ARM Linux wrote:
> Actually, we can do this safely - byte stores are permitted to SCU
> registers probably for this very reason.

3rd revision of the patch:

 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   16 ++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..108f31d 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,7 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(unsigned int);
 
 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..c956984 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -50,3 +50,19 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+int scu_power_mode(unsigned int mode)
+{
+	unsigned int val;
+	int cpu = smp_processor_id();
+	int shift;
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	val = __raw_readb(scu_base + SCU_CPU_STATUS + cpu) & ~0x03;
+	val |= mode;
+	__raw_writeb(val, scu_base + SCU_CPU_STATUS + cpu);
+
+	return 0;
+}

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

[-- Attachment #1: Type: text/plain, Size: 2770 bytes --]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 6:36 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> ccross@android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 01:04:22PM +0000, Russell King - ARM Linux
> wrote:
> > Actually, we can do this safely - byte stores are permitted to SCU
> > registers probably for this very reason.
>
> 3rd revision of the patch:

After fixing the 3rd version for base address break, I was able to
use this patch and test it. Seems to work. SMC related stuff can
be ignored because OMAP4 ES1.0 doesn't have functional PM hardware
support.

Here is the updated patch which will be 4th revision...

4th revision
---
 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   23 +++++++++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h
b/arch/arm/include/asm/smp_scu.h
index b6f42c9..676bc43 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -7,7 +7,12 @@
 #define SCU_INVALIDATE		0x0c
 #define SCU_FPGA_REVISION	0x10

+#define SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(unsigned int);

 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index ee7bf47..aec7c5d 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -14,6 +14,8 @@
 #include <asm/smp_scu.h>
 #include <asm/cacheflush.h>

+static void __iomem *base;
+
 /*
  * Get the number of CPU cores from the SCU configuration
  */
@@ -29,14 +31,15 @@ unsigned int __init scu_get_core_count(void __iomem
*scu_base)
 void __init scu_enable(void __iomem *scu_base)
 {
 	u32 scu_ctrl;
+	base = scu_base;

-	scu_ctrl = __raw_readl(scu_base + SCU_CTRL);
+	scu_ctrl = __raw_readl(base + SCU_CTRL);
 	/* already enabled? */
 	if (scu_ctrl & 1)
 		return;

 	scu_ctrl |= 1;
-	__raw_writel(scu_ctrl, scu_base + SCU_CTRL);
+	__raw_writel(scu_ctrl, base + SCU_CTRL);

 	/*
 	 * Ensure that the data accessed by CPU0 before the SCU was
@@ -44,3 +47,19 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+int scu_power_mode(unsigned int mode)
+{
+	unsigned int val;
+	int cpu = smp_processor_id();
+	int shift;
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	val = __raw_readb(base + SCU_CPU_STATUS + cpu) & ~0x03;
+	val |= mode;
+	__raw_writeb(val, base + SCU_CPU_STATUS + cpu);
+
+	return 0;
+}
-- 
1.6.0.4

[-- Attachment #2: 0001-4th-revision.patch --]
[-- Type: application/octet-stream, Size: 1899 bytes --]

4th revision

---
 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   23 +++++++++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index b6f42c9..676bc43 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -7,7 +7,12 @@
 #define SCU_INVALIDATE		0x0c
 #define SCU_FPGA_REVISION	0x10
 
+#define SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(unsigned int);
 
 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index ee7bf47..aec7c5d 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -14,6 +14,8 @@
 #include <asm/smp_scu.h>
 #include <asm/cacheflush.h>
 
+static void __iomem *base;
+
 /*
  * Get the number of CPU cores from the SCU configuration
  */
@@ -29,14 +31,15 @@ unsigned int __init scu_get_core_count(void __iomem *scu_base)
 void __init scu_enable(void __iomem *scu_base)
 {
 	u32 scu_ctrl;
+	base = scu_base;
 
-	scu_ctrl = __raw_readl(scu_base + SCU_CTRL);
+	scu_ctrl = __raw_readl(base + SCU_CTRL);
 	/* already enabled? */
 	if (scu_ctrl & 1)
 		return;
 
 	scu_ctrl |= 1;
-	__raw_writel(scu_ctrl, scu_base + SCU_CTRL);
+	__raw_writel(scu_ctrl, base + SCU_CTRL);
 
 	/*
 	 * Ensure that the data accessed by CPU0 before the SCU was
@@ -44,3 +47,19 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+int scu_power_mode(unsigned int mode)
+{
+	unsigned int val;
+	int cpu = smp_processor_id();
+	int shift;
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	val = __raw_readb(base + SCU_CPU_STATUS + cpu) & ~0x03;
+	val |= mode;
+	__raw_writeb(val, base + SCU_CPU_STATUS + cpu);
+
+	return 0;
+}
-- 
1.6.0.4

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 6:36 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> ccross at android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 01:04:22PM +0000, Russell King - ARM Linux
> wrote:
> > Actually, we can do this safely - byte stores are permitted to SCU
> > registers probably for this very reason.
>
> 3rd revision of the patch:

After fixing the 3rd version for base address break, I was able to
use this patch and test it. Seems to work. SMC related stuff can
be ignored because OMAP4 ES1.0 doesn't have functional PM hardware
support.

Here is the updated patch which will be 4th revision...

4th revision
---
 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   23 +++++++++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h
b/arch/arm/include/asm/smp_scu.h
index b6f42c9..676bc43 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -7,7 +7,12 @@
 #define SCU_INVALIDATE		0x0c
 #define SCU_FPGA_REVISION	0x10

+#define SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(unsigned int);

 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index ee7bf47..aec7c5d 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -14,6 +14,8 @@
 #include <asm/smp_scu.h>
 #include <asm/cacheflush.h>

+static void __iomem *base;
+
 /*
  * Get the number of CPU cores from the SCU configuration
  */
@@ -29,14 +31,15 @@ unsigned int __init scu_get_core_count(void __iomem
*scu_base)
 void __init scu_enable(void __iomem *scu_base)
 {
 	u32 scu_ctrl;
+	base = scu_base;

-	scu_ctrl = __raw_readl(scu_base + SCU_CTRL);
+	scu_ctrl = __raw_readl(base + SCU_CTRL);
 	/* already enabled? */
 	if (scu_ctrl & 1)
 		return;

 	scu_ctrl |= 1;
-	__raw_writel(scu_ctrl, scu_base + SCU_CTRL);
+	__raw_writel(scu_ctrl, base + SCU_CTRL);

 	/*
 	 * Ensure that the data accessed by CPU0 before the SCU was
@@ -44,3 +47,19 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+int scu_power_mode(unsigned int mode)
+{
+	unsigned int val;
+	int cpu = smp_processor_id();
+	int shift;
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	val = __raw_readb(base + SCU_CPU_STATUS + cpu) & ~0x03;
+	val |= mode;
+	__raw_writeb(val, base + SCU_CPU_STATUS + cpu);
+
+	return 0;
+}
-- 
1.6.0.4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-4th-revision.patch
Type: application/octet-stream
Size: 1898 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110125/21404ae7/attachment.obj>

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 11:53:35PM +0530, Santosh Shilimkar wrote:
> After fixing the 3rd version for base address break, I was able to
> use this patch and test it. Seems to work. SMC related stuff can
> be ignored because OMAP4 ES1.0 doesn't have functional PM hardware
> support.

I think I'd prefer to do as the other functions do, and pass in the
scu base address from the platform code.  It's potentially more
efficient for platforms which have a fixed SCU base address.

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 11:53:35PM +0530, Santosh Shilimkar wrote:
> After fixing the 3rd version for base address break, I was able to
> use this patch and test it. Seems to work. SMC related stuff can
> be ignored because OMAP4 ES1.0 doesn't have functional PM hardware
> support.

I think I'd prefer to do as the other functions do, and pass in the
scu base address from the platform code.  It's potentially more
efficient for platforms which have a fixed SCU base address.

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Friday, February 04, 2011 4:11 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> ccross@android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 11:53:35PM +0530, Santosh Shilimkar wrote:
> > After fixing the 3rd version for base address break, I was able to
> > use this patch and test it. Seems to work. SMC related stuff can
> > be ignored because OMAP4 ES1.0 doesn't have functional PM hardware
> > support.
>
> I think I'd prefer to do as the other functions do, and pass in the
> scu base address from the platform code.  It's potentially more
> efficient for platforms which have a fixed SCU base address.
Ok. I can fix that

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Friday, February 04, 2011 4:11 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> ccross at android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 11:53:35PM +0530, Santosh Shilimkar wrote:
> > After fixing the 3rd version for base address break, I was able to
> > use this patch and test it. Seems to work. SMC related stuff can
> > be ignored because OMAP4 ES1.0 doesn't have functional PM hardware
> > support.
>
> I think I'd prefer to do as the other functions do, and pass in the
> scu base address from the platform code.  It's potentially more
> efficient for platforms which have a fixed SCU base address.
Ok. I can fix that

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Fri, Feb 04, 2011 at 04:16:07PM +0530, Santosh Shilimkar wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> > Sent: Friday, February 04, 2011 4:11 PM
> > To: Santosh Shilimkar
> > Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> > omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > ccross@android.com
> > Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> > file
> >
> > On Tue, Jan 25, 2011 at 11:53:35PM +0530, Santosh Shilimkar wrote:
> > > After fixing the 3rd version for base address break, I was able to
> > > use this patch and test it. Seems to work. SMC related stuff can
> > > be ignored because OMAP4 ES1.0 doesn't have functional PM hardware
> > > support.
> >
> > I think I'd prefer to do as the other functions do, and pass in the
> > scu base address from the platform code.  It's potentially more
> > efficient for platforms which have a fixed SCU base address.
> Ok. I can fix that

8<------
Subject: [PATCH] ARM: smp: add function to set WFI low-power mode for SMP CPUs

Add a function to set the SCU low-power mode for SMP CPUs.  This
centralizes this functionality rather than having to expose the
SCU register definitions to each platform.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..800860d 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,7 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(void __iomem *, unsigned int);
 
 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..0ba329a 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -50,3 +50,27 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+/*
+ * Set the executing CPUs power mode as defined.  This will be in
+ * preparation for it executing a WFI instruction.
+ *
+ * This function must be called with preemption disabled, and as it
+ * has the side effect of disabling coherency, caches must have been
+ * flushed.  Interrupts must also have been disabled.
+ */
+int scu_power_mode(void __iomem *scu_base, unsigned int mode)
+{
+	unsigned int val;
+	int cpu = smp_processor_id();
+	int shift;
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	val = __raw_readb(scu_base + SCU_CPU_STATUS + cpu) & ~0x03;
+	val |= mode;
+	__raw_writeb(val, scu_base + SCU_CPU_STATUS + cpu);
+
+	return 0;
+}
-- 
1.6.2.5

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Fri, Feb 04, 2011 at 04:16:07PM +0530, Santosh Shilimkar wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > Sent: Friday, February 04, 2011 4:11 PM
> > To: Santosh Shilimkar
> > Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> > omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > ccross at android.com
> > Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> > file
> >
> > On Tue, Jan 25, 2011 at 11:53:35PM +0530, Santosh Shilimkar wrote:
> > > After fixing the 3rd version for base address break, I was able to
> > > use this patch and test it. Seems to work. SMC related stuff can
> > > be ignored because OMAP4 ES1.0 doesn't have functional PM hardware
> > > support.
> >
> > I think I'd prefer to do as the other functions do, and pass in the
> > scu base address from the platform code.  It's potentially more
> > efficient for platforms which have a fixed SCU base address.
> Ok. I can fix that

8<------
Subject: [PATCH] ARM: smp: add function to set WFI low-power mode for SMP CPUs

Add a function to set the SCU low-power mode for SMP CPUs.  This
centralizes this functionality rather than having to expose the
SCU register definitions to each platform.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/smp_scu.h |    5 +++++
 arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 2376835..800860d 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -1,7 +1,12 @@
 #ifndef __ASMARM_ARCH_SCU_H
 #define __ASMARM_ARCH_SCU_H
 
+#define SCU_PM_NORMAL	0
+#define SCU_PM_DORMANT	2
+#define SCU_PM_POWEROFF	3
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
+int scu_power_mode(void __iomem *, unsigned int);
 
 #endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 9ab4149..0ba329a 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -50,3 +50,27 @@ void __init scu_enable(void __iomem *scu_base)
 	 */
 	flush_cache_all();
 }
+
+/*
+ * Set the executing CPUs power mode as defined.  This will be in
+ * preparation for it executing a WFI instruction.
+ *
+ * This function must be called with preemption disabled, and as it
+ * has the side effect of disabling coherency, caches must have been
+ * flushed.  Interrupts must also have been disabled.
+ */
+int scu_power_mode(void __iomem *scu_base, unsigned int mode)
+{
+	unsigned int val;
+	int cpu = smp_processor_id();
+	int shift;
+
+	if (mode > 3 || mode == 1 || cpu > 3)
+		return -EINVAL;
+
+	val = __raw_readb(scu_base + SCU_CPU_STATUS + cpu) & ~0x03;
+	val |= mode;
+	__raw_writeb(val, scu_base + SCU_CPU_STATUS + cpu);
+
+	return 0;
+}
-- 
1.6.2.5

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Friday, February 04, 2011 5:01 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> ccross@android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Fri, Feb 04, 2011 at 04:16:07PM +0530, Santosh Shilimkar wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> > > Sent: Friday, February 04, 2011 4:11 PM
> > > To: Santosh Shilimkar
> > > Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> > > omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > ccross@android.com
> > > Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to
> header
> > > file
> > >
> > > On Tue, Jan 25, 2011 at 11:53:35PM +0530, Santosh Shilimkar
> wrote:
> > > > After fixing the 3rd version for base address break, I was
> able to
> > > > use this patch and test it. Seems to work. SMC related stuff
> can
> > > > be ignored because OMAP4 ES1.0 doesn't have functional PM
> hardware
> > > > support.
> > >
> > > I think I'd prefer to do as the other functions do, and pass in
> the
> > > scu base address from the platform code.  It's potentially more
> > > efficient for platforms which have a fixed SCU base address.
> > Ok. I can fix that
>
Thanks. Will use this version then.

> 8<------
> Subject: [PATCH] ARM: smp: add function to set WFI low-power mode
> for SMP CPUs
>
> Add a function to set the SCU low-power mode for SMP CPUs.  This
> centralizes this functionality rather than having to expose the
> SCU register definitions to each platform.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/include/asm/smp_scu.h |    5 +++++
>  arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/include/asm/smp_scu.h
> b/arch/arm/include/asm/smp_scu.h
> index 2376835..800860d 100644
> --- a/arch/arm/include/asm/smp_scu.h
> +++ b/arch/arm/include/asm/smp_scu.h
> @@ -1,7 +1,12 @@
>  #ifndef __ASMARM_ARCH_SCU_H
>  #define __ASMARM_ARCH_SCU_H
>
> +#define SCU_PM_NORMAL	0
> +#define SCU_PM_DORMANT	2
> +#define SCU_PM_POWEROFF	3
> +
>  unsigned int scu_get_core_count(void __iomem *);
>  void scu_enable(void __iomem *);
> +int scu_power_mode(void __iomem *, unsigned int);
>
>  #endif
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 9ab4149..0ba329a 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -50,3 +50,27 @@ void __init scu_enable(void __iomem *scu_base)
>  	 */
>  	flush_cache_all();
>  }
> +
> +/*
> + * Set the executing CPUs power mode as defined.  This will be in
> + * preparation for it executing a WFI instruction.
> + *
> + * This function must be called with preemption disabled, and as it
> + * has the side effect of disabling coherency, caches must have
> been
> + * flushed.  Interrupts must also have been disabled.
> + */
> +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> +{
> +	unsigned int val;
> +	int cpu = smp_processor_id();
> +	int shift;
> +
> +	if (mode > 3 || mode == 1 || cpu > 3)
> +		return -EINVAL;
> +
> +	val = __raw_readb(scu_base + SCU_CPU_STATUS + cpu) & ~0x03;
> +	val |= mode;
> +	__raw_writeb(val, scu_base + SCU_CPU_STATUS + cpu);
> +
> +	return 0;
> +}
> --
> 1.6.2.5

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Friday, February 04, 2011 5:01 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> ccross at android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Fri, Feb 04, 2011 at 04:16:07PM +0530, Santosh Shilimkar wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > > Sent: Friday, February 04, 2011 4:11 PM
> > > To: Santosh Shilimkar
> > > Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> > > omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > > ccross at android.com
> > > Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to
> header
> > > file
> > >
> > > On Tue, Jan 25, 2011 at 11:53:35PM +0530, Santosh Shilimkar
> wrote:
> > > > After fixing the 3rd version for base address break, I was
> able to
> > > > use this patch and test it. Seems to work. SMC related stuff
> can
> > > > be ignored because OMAP4 ES1.0 doesn't have functional PM
> hardware
> > > > support.
> > >
> > > I think I'd prefer to do as the other functions do, and pass in
> the
> > > scu base address from the platform code.  It's potentially more
> > > efficient for platforms which have a fixed SCU base address.
> > Ok. I can fix that
>
Thanks. Will use this version then.

> 8<------
> Subject: [PATCH] ARM: smp: add function to set WFI low-power mode
> for SMP CPUs
>
> Add a function to set the SCU low-power mode for SMP CPUs.  This
> centralizes this functionality rather than having to expose the
> SCU register definitions to each platform.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/include/asm/smp_scu.h |    5 +++++
>  arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/include/asm/smp_scu.h
> b/arch/arm/include/asm/smp_scu.h
> index 2376835..800860d 100644
> --- a/arch/arm/include/asm/smp_scu.h
> +++ b/arch/arm/include/asm/smp_scu.h
> @@ -1,7 +1,12 @@
>  #ifndef __ASMARM_ARCH_SCU_H
>  #define __ASMARM_ARCH_SCU_H
>
> +#define SCU_PM_NORMAL	0
> +#define SCU_PM_DORMANT	2
> +#define SCU_PM_POWEROFF	3
> +
>  unsigned int scu_get_core_count(void __iomem *);
>  void scu_enable(void __iomem *);
> +int scu_power_mode(void __iomem *, unsigned int);
>
>  #endif
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 9ab4149..0ba329a 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -50,3 +50,27 @@ void __init scu_enable(void __iomem *scu_base)
>  	 */
>  	flush_cache_all();
>  }
> +
> +/*
> + * Set the executing CPUs power mode as defined.  This will be in
> + * preparation for it executing a WFI instruction.
> + *
> + * This function must be called with preemption disabled, and as it
> + * has the side effect of disabling coherency, caches must have
> been
> + * flushed.  Interrupts must also have been disabled.
> + */
> +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> +{
> +	unsigned int val;
> +	int cpu = smp_processor_id();
> +	int shift;
> +
> +	if (mode > 3 || mode == 1 || cpu > 3)
> +		return -EINVAL;
> +
> +	val = __raw_readb(scu_base + SCU_CPU_STATUS + cpu) & ~0x03;
> +	val |= mode;
> +	__raw_writeb(val, scu_base + SCU_CPU_STATUS + cpu);
> +
> +	return 0;
> +}
> --
> 1.6.2.5

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Friday, February 04, 2011 5:01 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> ccross@android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
[.....]

> 8<------
> Subject: [PATCH] ARM: smp: add function to set WFI low-power mode
> for SMP CPUs
>
> Add a function to set the SCU low-power mode for SMP CPUs.  This
> centralizes this functionality rather than having to expose the
> SCU register definitions to each platform.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/include/asm/smp_scu.h |    5 +++++
>  arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
>

[....]

> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 9ab4149..0ba329a 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -50,3 +50,27 @@ void __init scu_enable(void __iomem *scu_base)
>  	 */
>  	flush_cache_all();
>  }
> +
> +/*
> + * Set the executing CPUs power mode as defined.  This will be in
> + * preparation for it executing a WFI instruction.
> + *
> + * This function must be called with preemption disabled, and as it
> + * has the side effect of disabling coherency, caches must have
> been
> + * flushed.  Interrupts must also have been disabled.
> + */
> +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> +{
> +	unsigned int val;
> +	int cpu = smp_processor_id();
> +	int shift;
shift is unused with this version now so I am removing it.

> +
> +	if (mode > 3 || mode == 1 || cpu > 3)
> +		return -EINVAL;
> +
> +	val = __raw_readb(scu_base + SCU_CPU_STATUS + cpu) & ~0x03;
> +	val |= mode;
> +	__raw_writeb(val, scu_base + SCU_CPU_STATUS + cpu);
> +
> +	return 0;
> +}
> --
> 1.6.2.5

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Friday, February 04, 2011 5:01 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> ccross at android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
[.....]

> 8<------
> Subject: [PATCH] ARM: smp: add function to set WFI low-power mode
> for SMP CPUs
>
> Add a function to set the SCU low-power mode for SMP CPUs.  This
> centralizes this functionality rather than having to expose the
> SCU register definitions to each platform.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/include/asm/smp_scu.h |    5 +++++
>  arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
>

[....]

> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 9ab4149..0ba329a 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -50,3 +50,27 @@ void __init scu_enable(void __iomem *scu_base)
>  	 */
>  	flush_cache_all();
>  }
> +
> +/*
> + * Set the executing CPUs power mode as defined.  This will be in
> + * preparation for it executing a WFI instruction.
> + *
> + * This function must be called with preemption disabled, and as it
> + * has the side effect of disabling coherency, caches must have
> been
> + * flushed.  Interrupts must also have been disabled.
> + */
> +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> +{
> +	unsigned int val;
> +	int cpu = smp_processor_id();
> +	int shift;
shift is unused with this version now so I am removing it.

> +
> +	if (mode > 3 || mode == 1 || cpu > 3)
> +		return -EINVAL;
> +
> +	val = __raw_readb(scu_base + SCU_CPU_STATUS + cpu) & ~0x03;
> +	val |= mode;
> +	__raw_writeb(val, scu_base + SCU_CPU_STATUS + cpu);
> +
> +	return 0;
> +}
> --
> 1.6.2.5

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Mon, Feb 07, 2011 at 03:21:44PM +0530, Santosh Shilimkar wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> > Sent: Friday, February 04, 2011 5:01 PM
> > To: Santosh Shilimkar
> > Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> > omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > ccross@android.com
> > Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> > file
> >
> [.....]
> 
> > 8<------
> > Subject: [PATCH] ARM: smp: add function to set WFI low-power mode
> > for SMP CPUs
> >
> > Add a function to set the SCU low-power mode for SMP CPUs.  This
> > centralizes this functionality rather than having to expose the
> > SCU register definitions to each platform.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  arch/arm/include/asm/smp_scu.h |    5 +++++
> >  arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
> >  2 files changed, 29 insertions(+), 0 deletions(-)
> >
> 
> [....]
> 
> > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > index 9ab4149..0ba329a 100644
> > --- a/arch/arm/kernel/smp_scu.c
> > +++ b/arch/arm/kernel/smp_scu.c
> > @@ -50,3 +50,27 @@ void __init scu_enable(void __iomem *scu_base)
> >  	 */
> >  	flush_cache_all();
> >  }
> > +
> > +/*
> > + * Set the executing CPUs power mode as defined.  This will be in
> > + * preparation for it executing a WFI instruction.
> > + *
> > + * This function must be called with preemption disabled, and as it
> > + * has the side effect of disabling coherency, caches must have
> > been
> > + * flushed.  Interrupts must also have been disabled.
> > + */
> > +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > +{
> > +	unsigned int val;
> > +	int cpu = smp_processor_id();
> > +	int shift;
> shift is unused with this version now so I am removing it.

Yes, I noticed that - it's gone in the version I merged into my tree.

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Mon, Feb 07, 2011 at 03:21:44PM +0530, Santosh Shilimkar wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > Sent: Friday, February 04, 2011 5:01 PM
> > To: Santosh Shilimkar
> > Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> > omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > ccross at android.com
> > Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> > file
> >
> [.....]
> 
> > 8<------
> > Subject: [PATCH] ARM: smp: add function to set WFI low-power mode
> > for SMP CPUs
> >
> > Add a function to set the SCU low-power mode for SMP CPUs.  This
> > centralizes this functionality rather than having to expose the
> > SCU register definitions to each platform.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  arch/arm/include/asm/smp_scu.h |    5 +++++
> >  arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
> >  2 files changed, 29 insertions(+), 0 deletions(-)
> >
> 
> [....]
> 
> > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > index 9ab4149..0ba329a 100644
> > --- a/arch/arm/kernel/smp_scu.c
> > +++ b/arch/arm/kernel/smp_scu.c
> > @@ -50,3 +50,27 @@ void __init scu_enable(void __iomem *scu_base)
> >  	 */
> >  	flush_cache_all();
> >  }
> > +
> > +/*
> > + * Set the executing CPUs power mode as defined.  This will be in
> > + * preparation for it executing a WFI instruction.
> > + *
> > + * This function must be called with preemption disabled, and as it
> > + * has the side effect of disabling coherency, caches must have
> > been
> > + * flushed.  Interrupts must also have been disabled.
> > + */
> > +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > +{
> > +	unsigned int val;
> > +	int cpu = smp_processor_id();
> > +	int shift;
> shift is unused with this version now so I am removing it.

Yes, I noticed that - it's gone in the version I merged into my tree.

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Monday, February 07, 2011 3:48 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> ccross@android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
[....]

> > > +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > > +{
> > > +	unsigned int val;
> > > +	int cpu = smp_processor_id();
> > > +	int shift;
> > shift is unused with this version now so I am removing it.
>
> Yes, I noticed that - it's gone in the version I merged into my
> tree.
Ok.
Did you also merged twd patch of yours ?

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Monday, February 07, 2011 3:48 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> ccross at android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
[....]

> > > +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > > +{
> > > +	unsigned int val;
> > > +	int cpu = smp_processor_id();
> > > +	int shift;
> > shift is unused with this version now so I am removing it.
>
> Yes, I noticed that - it's gone in the version I merged into my
> tree.
Ok.
Did you also merged twd patch of yours ?

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Mon, Feb 07, 2011 at 03:51:04PM +0530, Santosh Shilimkar wrote:
> > > > +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > > > +{
> > > > +	unsigned int val;
> > > > +	int cpu = smp_processor_id();
> > > > +	int shift;
> > > shift is unused with this version now so I am removing it.
> >
> > Yes, I noticed that - it's gone in the version I merged into my
> > tree.
> Ok.
> Did you also merged twd patch of yours ?

Both:

    ARM: twd: ensure timer reload is reprogrammed on entry to periodic mode
    ARM: twd: fix display of twd frequency

are in mainline.  They both qualify as bug fixes, so go in during -rc
periods.

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Mon, Feb 07, 2011 at 03:51:04PM +0530, Santosh Shilimkar wrote:
> > > > +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > > > +{
> > > > +	unsigned int val;
> > > > +	int cpu = smp_processor_id();
> > > > +	int shift;
> > > shift is unused with this version now so I am removing it.
> >
> > Yes, I noticed that - it's gone in the version I merged into my
> > tree.
> Ok.
> Did you also merged twd patch of yours ?

Both:

    ARM: twd: ensure timer reload is reprogrammed on entry to periodic mode
    ARM: twd: fix display of twd frequency

are in mainline.  They both qualify as bug fixes, so go in during -rc
periods.

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Monday, February 07, 2011 3:54 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> ccross@android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Mon, Feb 07, 2011 at 03:51:04PM +0530, Santosh Shilimkar wrote:
> > > > > +int scu_power_mode(void __iomem *scu_base, unsigned int
> mode)
> > > > > +{
> > > > > +	unsigned int val;
> > > > > +	int cpu = smp_processor_id();
> > > > > +	int shift;
> > > > shift is unused with this version now so I am removing it.
> > >
> > > Yes, I noticed that - it's gone in the version I merged into my
> > > tree.
> > Ok.
> > Did you also merged twd patch of yours ?
>
> Both:
>
>     ARM: twd: ensure timer reload is reprogrammed on entry to
> periodic mode
>     ARM: twd: fix display of twd frequency
>
> are in mainline.  They both qualify as bug fixes, so go in during -
> rc periods.

Nice.
Thanks for pushing it early.

Regards,
Santosh

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Monday, February 07, 2011 3:54 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> ccross at android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Mon, Feb 07, 2011 at 03:51:04PM +0530, Santosh Shilimkar wrote:
> > > > > +int scu_power_mode(void __iomem *scu_base, unsigned int
> mode)
> > > > > +{
> > > > > +	unsigned int val;
> > > > > +	int cpu = smp_processor_id();
> > > > > +	int shift;
> > > > shift is unused with this version now so I am removing it.
> > >
> > > Yes, I noticed that - it's gone in the version I merged into my
> > > tree.
> > Ok.
> > Did you also merged twd patch of yours ?
>
> Both:
>
>     ARM: twd: ensure timer reload is reprogrammed on entry to
> periodic mode
>     ARM: twd: fix display of twd frequency
>
> are in mainline.  They both qualify as bug fixes, so go in during -
> rc periods.

Nice.
Thanks for pushing it early.

Regards,
Santosh

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Monday, February 07, 2011 3:48 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> ccross@android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
[....]

> > > 8<------
> > > Subject: [PATCH] ARM: smp: add function to set WFI low-power
> mode
> > > for SMP CPUs
> > >
> > > Add a function to set the SCU low-power mode for SMP CPUs.  This
> > > centralizes this functionality rather than having to expose the
> > > SCU register definitions to each platform.
> > >
> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > ---
> > >  arch/arm/include/asm/smp_scu.h |    5 +++++
> > >  arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
> > >  2 files changed, 29 insertions(+), 0 deletions(-)
> > >
[...]

> > > +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > > +{
> > > +	unsigned int val;
> > > +	int cpu = smp_processor_id();
> > > +	int shift;
> > shift is unused with this version now so I am removing it.
>
> Yes, I noticed that - it's gone in the version I merged into my
> tree.

This patch will need below update so that the smp_scu.h
header can be included from assembly files. Will you
fold this into your patch or you want me to
send below as separate patch ?

diff --git a/arch/arm/include/asm/smp_scu.h
b/arch/arm/include/asm/smp_scu.h
index 800860d..8895c6c 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -5,8 +5,11 @@
 #define SCU_PM_DORMANT	2
 #define SCU_PM_POWEROFF	3

+#ifndef __ASSEMBLER__
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
 int scu_power_mode(void __iomem *, unsigned int);

 #endif
+#endif

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Monday, February 07, 2011 3:48 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> ccross at android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
[....]

> > > 8<------
> > > Subject: [PATCH] ARM: smp: add function to set WFI low-power
> mode
> > > for SMP CPUs
> > >
> > > Add a function to set the SCU low-power mode for SMP CPUs.  This
> > > centralizes this functionality rather than having to expose the
> > > SCU register definitions to each platform.
> > >
> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > ---
> > >  arch/arm/include/asm/smp_scu.h |    5 +++++
> > >  arch/arm/kernel/smp_scu.c      |   24 ++++++++++++++++++++++++
> > >  2 files changed, 29 insertions(+), 0 deletions(-)
> > >
[...]

> > > +int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > > +{
> > > +	unsigned int val;
> > > +	int cpu = smp_processor_id();
> > > +	int shift;
> > shift is unused with this version now so I am removing it.
>
> Yes, I noticed that - it's gone in the version I merged into my
> tree.

This patch will need below update so that the smp_scu.h
header can be included from assembly files. Will you
fold this into your patch or you want me to
send below as separate patch ?

diff --git a/arch/arm/include/asm/smp_scu.h
b/arch/arm/include/asm/smp_scu.h
index 800860d..8895c6c 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -5,8 +5,11 @@
 #define SCU_PM_DORMANT	2
 #define SCU_PM_POWEROFF	3

+#ifndef __ASSEMBLER__
+
 unsigned int scu_get_core_count(void __iomem *);
 void scu_enable(void __iomem *);
 int scu_power_mode(void __iomem *, unsigned int);

 #endif
+#endif

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Thu, Feb 10, 2011 at 08:19:19PM +0530, Santosh Shilimkar wrote:
> This patch will need below update so that the smp_scu.h
> header can be included from assembly files. Will you
> fold this into your patch or you want me to
> send below as separate patch ?

Folded.

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Thu, Feb 10, 2011 at 08:19:19PM +0530, Santosh Shilimkar wrote:
> This patch will need below update so that the smp_scu.h
> header can be included from assembly files. Will you
> fold this into your patch or you want me to
> send below as separate patch ?

Folded.

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Thursday, February 10, 2011 9:44 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> ccross@android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Thu, Feb 10, 2011 at 08:19:19PM +0530, Santosh Shilimkar wrote:
> > This patch will need below update so that the smp_scu.h
> > header can be included from assembly files. Will you
> > fold this into your patch or you want me to
> > send below as separate patch ?
>
> Folded.

Thanks.

Regards,
Santosh

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Thursday, February 10, 2011 9:44 PM
> To: Santosh Shilimkar
> Cc: catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> ccross at android.com
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Thu, Feb 10, 2011 at 08:19:19PM +0530, Santosh Shilimkar wrote:
> > This patch will need below update so that the smp_scu.h
> > header can be included from assembly files. Will you
> > fold this into your patch or you want me to
> > send below as separate patch ?
>
> Folded.

Thanks.

Regards,
Santosh

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 6:27 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel@lists.infradead.org; catalin.marinas@arm.com;
> ccross@android.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> > Ok. I missed some information my last email.
> > The SCU power status programming is used to take CPU in/out
> > of coherency as an alternative to SMP bit. We don't
> > have an access to SMP bit on OMAP4. ARM has already
> > confirmed SCU programming is same as SMP bit enable/disable.
> >
> > I don't know how safe is to use spin lock when one CPU is
> > goes out of coherency after programming the power state. The
> > spin lock release may not even be visible to other CPU.
>
> Erm, I do hope that's not the case, as that means it is unsafe for
> CPUs in
> a SMP system to write to this register without them potentially
> trampling
> over each other.
>
> If it is the case, then the solutions are either:
> 1. Fix the hardware so that coherency requests only yet turned off
>    when entering the WFI state.
> 2. Fix the hardware such that each CPU has a separate register.
>
> I can't see a software solution to this as we can't use ldrex/strex
> anything
> but memory regions, and memory regions without coherency won't work.
> Maybe ARM Support can help by suggesting how a 4-CPU system is
> supposed
> to safely read/modify/write the SCU power control register...

On system wide suspend scenario's this is already handled because
CPU's are taken down always sequentially.

In CPU ILDE because of some other hardware restrictions we have it
sequenced in 2 CPU system. CPU1 needs to be in OFF mode before any
power management can be done on system along with master core.
And that's how it is working without any races on OMAP

Regards,
Santosh

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 6:27 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; catalin.marinas at arm.com;
> ccross at android.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> > Ok. I missed some information my last email.
> > The SCU power status programming is used to take CPU in/out
> > of coherency as an alternative to SMP bit. We don't
> > have an access to SMP bit on OMAP4. ARM has already
> > confirmed SCU programming is same as SMP bit enable/disable.
> >
> > I don't know how safe is to use spin lock when one CPU is
> > goes out of coherency after programming the power state. The
> > spin lock release may not even be visible to other CPU.
>
> Erm, I do hope that's not the case, as that means it is unsafe for
> CPUs in
> a SMP system to write to this register without them potentially
> trampling
> over each other.
>
> If it is the case, then the solutions are either:
> 1. Fix the hardware so that coherency requests only yet turned off
>    when entering the WFI state.
> 2. Fix the hardware such that each CPU has a separate register.
>
> I can't see a software solution to this as we can't use ldrex/strex
> anything
> but memory regions, and memory regions without coherency won't work.
> Maybe ARM Support can help by suggesting how a 4-CPU system is
> supposed
> to safely read/modify/write the SCU power control register...

On system wide suspend scenario's this is already handled because
CPU's are taken down always sequentially.

In CPU ILDE because of some other hardware restrictions we have it
sequenced in 2 CPU system. CPU1 needs to be in OFF mode before any
power management can be done on system along with master core.
And that's how it is working without any races on OMAP

Regards,
Santosh

Re: [PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> Ok. I missed some information my last email.
> The SCU power status programming is used to take CPU in/out
> of coherency as an alternative to SMP bit. We don't
> have an access to SMP bit on OMAP4. ARM has already
> confirmed SCU programming is same as SMP bit enable/disable.
> 
> I don't know how safe is to use spin lock when one CPU is
> goes out of coherency after programming the power state. The
> spin lock release may not even be visible to other CPU.
> The programming happens from IDLE or suspend where the

BTW, presumably we should be flushing the caches/tlbs before setting
the CPU power register to a non-normal setting?

I'll wait until there's more information available (such as an example
implementation.)

[PATCH 4/5] ARM: scu: Move register defines to header file

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> Ok. I missed some information my last email.
> The SCU power status programming is used to take CPU in/out
> of coherency as an alternative to SMP bit. We don't
> have an access to SMP bit on OMAP4. ARM has already
> confirmed SCU programming is same as SMP bit enable/disable.
> 
> I don't know how safe is to use spin lock when one CPU is
> goes out of coherency after programming the power state. The
> spin lock release may not even be visible to other CPU.
> The programming happens from IDLE or suspend where the

BTW, presumably we should be flushing the caches/tlbs before setting
the CPU power register to a non-normal setting?

I'll wait until there's more information available (such as an example
implementation.)

RE: [PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 7:11 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel@lists.infradead.org; catalin.marinas@arm.com;
> ccross@android.com; linus.ml.walleij@gmail.com; linux-
> omap@vger.kernel.org
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> > Ok. I missed some information my last email.
> > The SCU power status programming is used to take CPU in/out
> > of coherency as an alternative to SMP bit. We don't
> > have an access to SMP bit on OMAP4. ARM has already
> > confirmed SCU programming is same as SMP bit enable/disable.
> >
> > I don't know how safe is to use spin lock when one CPU is
> > goes out of coherency after programming the power state. The
> > spin lock release may not even be visible to other CPU.
> > The programming happens from IDLE or suspend where the
>
> BTW, presumably we should be flushing the caches/tlbs before setting
> the CPU power register to a non-normal setting?
>
Yes but only till inner shareability domain. i.e L1
L2 will be flushed after this. This is one more issue
where the current L2 cache current APIs(use locks) becomes
not so suitable.

> I'll wait until there's more information available (such as an
> example
> implementation.)

[PATCH 4/5] ARM: scu: Move register defines to header file

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 25, 2011 7:11 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; catalin.marinas at arm.com;
> ccross at android.com; linus.ml.walleij at gmail.com; linux-
> omap at vger.kernel.org
> Subject: Re: [PATCH 4/5] ARM: scu: Move register defines to header
> file
>
> On Tue, Jan 25, 2011 at 06:06:27PM +0530, Santosh Shilimkar wrote:
> > Ok. I missed some information my last email.
> > The SCU power status programming is used to take CPU in/out
> > of coherency as an alternative to SMP bit. We don't
> > have an access to SMP bit on OMAP4. ARM has already
> > confirmed SCU programming is same as SMP bit enable/disable.
> >
> > I don't know how safe is to use spin lock when one CPU is
> > goes out of coherency after programming the power state. The
> > spin lock release may not even be visible to other CPU.
> > The programming happens from IDLE or suspend where the
>
> BTW, presumably we should be flushing the caches/tlbs before setting
> the CPU power register to a non-normal setting?
>
Yes but only till inner shareability domain. i.e L1
L2 will be flushed after this. This is one more issue
where the current L2 cache current APIs(use locks) becomes
not so suitable.

> I'll wait until there's more information available (such as an
> example
> implementation.)

[PATCH 5/5] ARM: smp: Skip secondary cpu calibration to speed-up boot

[Santosh Shilimkar]

On some architectures, secondary cores shares clock with primiary
core and hence scale together. Hence secondary core lpj calibration
is not necessary and can be skipped to save considerable time.

This can speed up the secondary cpu boot and hotplug cpu online
paths.

The patch is still under discussion and relevant thread is
here.
	http://www.mail-archive.com/linux-omap@vger.kernel.org/msg42902.html

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/smp.h     |    8 ++++++++
 arch/arm/kernel/smp.c          |   35 ++++++++++++++++++++++++++---------
 arch/arm/mach-omap2/omap-smp.c |    3 +++
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index 96ed521..150d202 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -69,6 +69,14 @@ asmlinkage void secondary_start_kernel(void);
 extern void platform_secondary_init(unsigned int cpu);
 
 /*
+ * Skip the secondary calibration on architectures sharing clock
+ * with primary cpu. Needs to be called for archs inside
+ * platform_secondary_init()
+ */
+extern void secondary_skip_calibrate(void);
+
+
+/*
  * Initialize cpu_possible map, and enable coherency
  */
 extern void platform_smp_prepare_cpus(unsigned int);
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 4539ebc..6aa99bc 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -55,6 +55,8 @@ enum ipi_msg_type {
 	IPI_CPU_STOP,
 };
 
+static unsigned int skip_secondary_calibrate;
+
 int __cpuinit __cpu_up(unsigned int cpu)
 {
 	struct cpuinfo_arm *ci = &per_cpu(cpu_data, cpu);
@@ -270,6 +272,16 @@ static void __cpuinit smp_store_cpu_info(unsigned int cpuid)
 }
 
 /*
+ * Skip the secondary calibration on architectures sharing clock
+ * with primary cpu. Needs to be called for archs from
+ * platform_secondary_init()
+ */
+void secondary_skip_calibrate(void)
+{
+	skip_secondary_calibrate = 1;
+}
+
+/*
  * This is the secondary CPU boot entry.  We're using this CPUs
  * idle thread stack, but a set of temporary page tables.
  */
@@ -312,7 +324,8 @@ asmlinkage void __cpuinit secondary_start_kernel(void)
 	 */
 	percpu_timer_setup();
 
-	calibrate_delay();
+	if (!skip_secondary_calibrate)
+		calibrate_delay();
 
 	smp_store_cpu_info(cpu);
 
@@ -330,16 +343,20 @@ asmlinkage void __cpuinit secondary_start_kernel(void)
 void __init smp_cpus_done(unsigned int max_cpus)
 {
 	int cpu;
-	unsigned long bogosum = 0;
+	char bogosum[32];
+	unsigned long bogosums = 0;
+
+	if (!skip_secondary_calibrate) {
+		for_each_online_cpu(cpu)
+			bogosums += per_cpu(cpu_data, cpu).loops_per_jiffy;
 
-	for_each_online_cpu(cpu)
-		bogosum += per_cpu(cpu_data, cpu).loops_per_jiffy;
+		snprintf(bogosum, sizeof(bogosums), " (%lu.%02lu BogoMIPS).\n",
+			bogosums / (500000/HZ), (bogosums / (5000/HZ)) % 100);
+	} else
+		bogosum[0] = '\0';
 
-	printk(KERN_INFO "SMP: Total of %d processors activated "
-	       "(%lu.%02lu BogoMIPS).\n",
-	       num_online_cpus(),
-	       bogosum / (500000/HZ),
-	       (bogosum / (5000/HZ)) % 100);
+	pr_info("SMP: Total of %d processors activated%s.\n",
+		num_online_cpus(), bogosum);
 }
 
 void __init smp_prepare_boot_cpu(void)
diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
index b66cfe8..7342cd5 100644
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -39,6 +39,9 @@ void __cpuinit platform_secondary_init(unsigned int cpu)
 	 */
 	gic_secondary_init(0);
 
+	/* Allow to skip secondary CPU calibration */
+	secondary_skip_calibrate();
+
 	/*
 	 * Synchronise with the boot thread.
 	 */
-- 
1.6.0.4

[PATCH 5/5] ARM: smp: Skip secondary cpu calibration to speed-up boot

[Santosh Shilimkar]

On some architectures, secondary cores shares clock with primiary
core and hence scale together. Hence secondary core lpj calibration
is not necessary and can be skipped to save considerable time.

This can speed up the secondary cpu boot and hotplug cpu online
paths.

The patch is still under discussion and relevant thread is
here.
	http://www.mail-archive.com/linux-omap at vger.kernel.org/msg42902.html

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/smp.h     |    8 ++++++++
 arch/arm/kernel/smp.c          |   35 ++++++++++++++++++++++++++---------
 arch/arm/mach-omap2/omap-smp.c |    3 +++
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index 96ed521..150d202 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -69,6 +69,14 @@ asmlinkage void secondary_start_kernel(void);
 extern void platform_secondary_init(unsigned int cpu);
 
 /*
+ * Skip the secondary calibration on architectures sharing clock
+ * with primary cpu. Needs to be called for archs inside
+ * platform_secondary_init()
+ */
+extern void secondary_skip_calibrate(void);
+
+
+/*
  * Initialize cpu_possible map, and enable coherency
  */
 extern void platform_smp_prepare_cpus(unsigned int);
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 4539ebc..6aa99bc 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -55,6 +55,8 @@ enum ipi_msg_type {
 	IPI_CPU_STOP,
 };
 
+static unsigned int skip_secondary_calibrate;
+
 int __cpuinit __cpu_up(unsigned int cpu)
 {
 	struct cpuinfo_arm *ci = &per_cpu(cpu_data, cpu);
@@ -270,6 +272,16 @@ static void __cpuinit smp_store_cpu_info(unsigned int cpuid)
 }
 
 /*
+ * Skip the secondary calibration on architectures sharing clock
+ * with primary cpu. Needs to be called for archs from
+ * platform_secondary_init()
+ */
+void secondary_skip_calibrate(void)
+{
+	skip_secondary_calibrate = 1;
+}
+
+/*
  * This is the secondary CPU boot entry.  We're using this CPUs
  * idle thread stack, but a set of temporary page tables.
  */
@@ -312,7 +324,8 @@ asmlinkage void __cpuinit secondary_start_kernel(void)
 	 */
 	percpu_timer_setup();
 
-	calibrate_delay();
+	if (!skip_secondary_calibrate)
+		calibrate_delay();
 
 	smp_store_cpu_info(cpu);
 
@@ -330,16 +343,20 @@ asmlinkage void __cpuinit secondary_start_kernel(void)
 void __init smp_cpus_done(unsigned int max_cpus)
 {
 	int cpu;
-	unsigned long bogosum = 0;
+	char bogosum[32];
+	unsigned long bogosums = 0;
+
+	if (!skip_secondary_calibrate) {
+		for_each_online_cpu(cpu)
+			bogosums += per_cpu(cpu_data, cpu).loops_per_jiffy;
 
-	for_each_online_cpu(cpu)
-		bogosum += per_cpu(cpu_data, cpu).loops_per_jiffy;
+		snprintf(bogosum, sizeof(bogosums), " (%lu.%02lu BogoMIPS).\n",
+			bogosums / (500000/HZ), (bogosums / (5000/HZ)) % 100);
+	} else
+		bogosum[0] = '\0';
 
-	printk(KERN_INFO "SMP: Total of %d processors activated "
-	       "(%lu.%02lu BogoMIPS).\n",
-	       num_online_cpus(),
-	       bogosum / (500000/HZ),
-	       (bogosum / (5000/HZ)) % 100);
+	pr_info("SMP: Total of %d processors activated%s.\n",
+		num_online_cpus(), bogosum);
 }
 
 void __init smp_prepare_boot_cpu(void)
diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
index b66cfe8..7342cd5 100644
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -39,6 +39,9 @@ void __cpuinit platform_secondary_init(unsigned int cpu)
 	 */
 	gic_secondary_init(0);
 
+	/* Allow to skip secondary CPU calibration */
+	secondary_skip_calibrate();
+
 	/*
 	 * Synchronise with the boot thread.
 	 */
-- 
1.6.0.4

Re: [PATCH 5/5] ARM: smp: Skip secondary cpu calibration to speed-up boot

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 02:21:19PM +0530, Santosh Shilimkar wrote:
> On some architectures, secondary cores shares clock with primiary
> core and hence scale together. Hence secondary core lpj calibration
> is not necessary and can be skipped to save considerable time.
> 
> This can speed up the secondary cpu boot and hotplug cpu online
> paths.

I still point out that this will be a user visible change in /proc/cpuinfo.

[PATCH 5/5] ARM: smp: Skip secondary cpu calibration to speed-up boot

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 02:21:19PM +0530, Santosh Shilimkar wrote:
> On some architectures, secondary cores shares clock with primiary
> core and hence scale together. Hence secondary core lpj calibration
> is not necessary and can be skipped to save considerable time.
> 
> This can speed up the secondary cpu boot and hotplug cpu online
> paths.

I still point out that this will be a user visible change in /proc/cpuinfo.

[PATCH 5/5] ARM: smp: Skip secondary cpu calibration to speed-up boot

[Santosh Shilimkar]

 	if (!skip_secondary_calibrate) {
 		for_each_online_cpu(cpu)
-			bogosum += per_cpu(cpu_data, cpu).loops_per_jiffy;
+			bogosums += per_cpu(cpu_data, cpu).loops_per_jiffy;
 
-		snprintf(bogosum, sizeof(bogosum), " (%lu.%02lu BogoMIPS).\n",
-			bogosum / (500000/HZ), (bogosum / (5000/HZ)) % 100);
+		snprintf(bogosum, sizeof(bogosums), " (%lu.%02lu BogoMIPS).\n",
+			bogosums / (500000/HZ), (bogosums / (5000/HZ)) % 100);
 	} else
 		bogosum[0] = '\0';
 

[PATCH 5/5] ARM: smp: Skip secondary cpu calibration to speed-up boot

[Santosh Shilimkar]

 	if (!skip_secondary_calibrate) {
 		for_each_online_cpu(cpu)
-			bogosum += per_cpu(cpu_data, cpu).loops_per_jiffy;
+			bogosums += per_cpu(cpu_data, cpu).loops_per_jiffy;
 
-		snprintf(bogosum, sizeof(bogosum), " (%lu.%02lu BogoMIPS).\n",
-			bogosum / (500000/HZ), (bogosum / (5000/HZ)) % 100);
+		snprintf(bogosum, sizeof(bogosums), " (%lu.%02lu BogoMIPS).\n",
+			bogosums / (500000/HZ), (bogosums / (5000/HZ)) % 100);
 	} else
 		bogosum[0] = '\0';
 

RE: [PATCH 5/5] ARM: smp: Skip secondary cpu calibration to speed-up boot

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Santosh Shilimkar
> Sent: Monday, January 24, 2011 2:21 PM
> To: linux-arm-kernel@lists.infradead.org
> Cc: linux-omap@vger.kernel.org; ccross@android.com;
> catalin.marinas@arm.com; linux@arm.linux.org.uk;
> linus.ml.walleij@gmail.com
> Subject: [PATCH 5/5] ARM: smp: Skip secondary cpu calibration to
> speed-up boot
>
>  	if (!skip_secondary_calibrate) {
>  		for_each_online_cpu(cpu)
> -			bogosum += per_cpu(cpu_data, cpu).loops_per_jiffy;
> +			bogosums += per_cpu(cpu_data,
> cpu).loops_per_jiffy;
>
> -		snprintf(bogosum, sizeof(bogosum), " (%lu.%02lu
> BogoMIPS).\n",
> -			bogosum / (500000/HZ), (bogosum / (5000/HZ)) %
> 100);
> +		snprintf(bogosum, sizeof(bogosums), " (%lu.%02lu
> BogoMIPS).\n",
> +			bogosums / (500000/HZ), (bogosums / (5000/HZ)) %
> 100);
>  	} else
>  		bogosum[0] = '\0';
>
Ignore this change please.

[PATCH 5/5] ARM: smp: Skip secondary cpu calibration to speed-up boot

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-
> owner at vger.kernel.org] On Behalf Of Santosh Shilimkar
> Sent: Monday, January 24, 2011 2:21 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: linux-omap at vger.kernel.org; ccross at android.com;
> catalin.marinas at arm.com; linux at arm.linux.org.uk;
> linus.ml.walleij at gmail.com
> Subject: [PATCH 5/5] ARM: smp: Skip secondary cpu calibration to
> speed-up boot
>
>  	if (!skip_secondary_calibrate) {
>  		for_each_online_cpu(cpu)
> -			bogosum += per_cpu(cpu_data, cpu).loops_per_jiffy;
> +			bogosums += per_cpu(cpu_data,
> cpu).loops_per_jiffy;
>
> -		snprintf(bogosum, sizeof(bogosum), " (%lu.%02lu
> BogoMIPS).\n",
> -			bogosum / (500000/HZ), (bogosum / (5000/HZ)) %
> 100);
> +		snprintf(bogosum, sizeof(bogosums), " (%lu.%02lu
> BogoMIPS).\n",
> +			bogosums / (500000/HZ), (bogosums / (5000/HZ)) %
> 100);
>  	} else
>  		bogosum[0] = '\0';
>
Ignore this change please.

RE: [PATCH 0/5] ARM: Few patches for PM enablement.

[Santosh Shilimkar]

Russell,
> -----Original Message-----
> From: Santosh Shilimkar [mailto:santosh.shilimkar@ti.com]
> Sent: Monday, January 24, 2011 2:21 PM
> To: linux-arm-kernel@lists.infradead.org
> Cc: linux-omap@vger.kernel.org; ccross@android.com;
> catalin.marinas@arm.com; linux@arm.linux.org.uk;
> linus.ml.walleij@gmail.com; Santosh Shilimkar
> Subject: [PATCH 0/5] ARM: Few patches for PM enablement.
>
> The series consist of few patches to address some common PM issues
> on Cortex-A9 based ARM SoCs. The skip calibration on secondary cores
> is still under discussion.
> On OMAP, the GIC save restore is done differently and hence that
> part isn't included. Collin already have a patch for the same which
> should address other SOCs.
>
> The following changes since commit
> 1bae4ce27c9c90344f23c65ea6966c50ffeae2f5:
>   Linus Torvalds (1):
>         Linux 2.6.38-rc2
>
I want to repost this series with update as disussed on the
list.

>       ARM: gic: Add hooks for architecture specific extensions
I am adding set_type and ack as an additional hooks reuested
by Collin.

>       ARM: gic: Add distributor and interface enable/disable
> accessory api
No change

>       ARM: twd: Add context save restore support
Will drop this patch and add your patch instead.

>       ARM: scu: Move register defines to header file
Same for this too. Your version instead of this patch.

>       ARM: smp: Skip secondary cpu calibration to speed-up boot
Your last comment on this one was
"I still point out that this will be a user visible change in
/proc/cpuinfo."
Are you ok with this patch if platforms like OMAP are
ok with this visible change. This patch currently affecting
only OMAP.

Do you have any more points on this series ?

Thanks for review.

Regards,
Santosh

[PATCH 0/5] ARM: Few patches for PM enablement.

[Santosh Shilimkar]

Russell,
> -----Original Message-----
> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
> Sent: Monday, January 24, 2011 2:21 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: linux-omap at vger.kernel.org; ccross at android.com;
> catalin.marinas at arm.com; linux at arm.linux.org.uk;
> linus.ml.walleij at gmail.com; Santosh Shilimkar
> Subject: [PATCH 0/5] ARM: Few patches for PM enablement.
>
> The series consist of few patches to address some common PM issues
> on Cortex-A9 based ARM SoCs. The skip calibration on secondary cores
> is still under discussion.
> On OMAP, the GIC save restore is done differently and hence that
> part isn't included. Collin already have a patch for the same which
> should address other SOCs.
>
> The following changes since commit
> 1bae4ce27c9c90344f23c65ea6966c50ffeae2f5:
>   Linus Torvalds (1):
>         Linux 2.6.38-rc2
>
I want to repost this series with update as disussed on the
list.

>       ARM: gic: Add hooks for architecture specific extensions
I am adding set_type and ack as an additional hooks reuested
by Collin.

>       ARM: gic: Add distributor and interface enable/disable
> accessory api
No change

>       ARM: twd: Add context save restore support
Will drop this patch and add your patch instead.

>       ARM: scu: Move register defines to header file
Same for this too. Your version instead of this patch.

>       ARM: smp: Skip secondary cpu calibration to speed-up boot
Your last comment on this one was
"I still point out that this will be a user visible change in
/proc/cpuinfo."
Are you ok with this patch if platforms like OMAP are
ok with this visible change. This patch currently affecting
only OMAP.

Do you have any more points on this series ?

Thanks for review.

Regards,
Santosh

RE: [PATCH 0/5] ARM: Few patches for PM enablement.

[Santosh Shilimkar]

Russell,
> -----Original Message-----
> From: Santosh Shilimkar [mailto:santosh.shilimkar@ti.com]
> Sent: Friday, February 04, 2011 3:49 PM
> To: Russell King - ARM Linux
> Cc: linux-omap@vger.kernel.org; ccross@android.com;
> catalin.marinas@arm.com; linus.ml.walleij@gmail.com; linux-arm-
> kernel@lists.infradead.org
> Subject: RE: [PATCH 0/5] ARM: Few patches for PM enablement.
>

> ARM: gic: Add hooks for architecture specific extensions
> ARM: gic: Add distributor and interface enable/disable accessory api
I plan to submit below to patches to your patch system.

> ARM: twd: Add context save restore support
> ARM: scu: Move register defines to header file
Alternate for these two are already in you tree, so I drop them.

> ARM: smp: Skip secondary cpu calibration to speed-up boot
This one we still haven't agree. So I postpone this one till you
find any other better way to handle this though I really hate that
~ 200 ms in this path.

Regards,
Santosh

[PATCH 0/5] ARM: Few patches for PM enablement.

[Santosh Shilimkar]

Russell,
> -----Original Message-----
> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
> Sent: Friday, February 04, 2011 3:49 PM
> To: Russell King - ARM Linux
> Cc: linux-omap at vger.kernel.org; ccross at android.com;
> catalin.marinas at arm.com; linus.ml.walleij at gmail.com; linux-arm-
> kernel at lists.infradead.org
> Subject: RE: [PATCH 0/5] ARM: Few patches for PM enablement.
>

> ARM: gic: Add hooks for architecture specific extensions
> ARM: gic: Add distributor and interface enable/disable accessory api
I plan to submit below to patches to your patch system.

> ARM: twd: Add context save restore support
> ARM: scu: Move register defines to header file
Alternate for these two are already in you tree, so I drop them.

> ARM: smp: Skip secondary cpu calibration to speed-up boot
This one we still haven't agree. So I postpone this one till you
find any other better way to handle this though I really hate that
~ 200 ms in this path.

Regards,
Santosh