[PATCH v5 0/5] ARM/ARM64 architected timer updates

[PATCH v5 0/5] ARM/ARM64 architected timer updates

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>

This patch series adds support to configure the rate and enable the
event stream for architected timer. The event streams can be used to
impose a timeout on a wfe, to safeguard against any programming error
in case an expected event is not generated or even to implement
wfe-based timeouts for userspace locking implementations. This feature
can be disabled(enabled by default).

Since the timer control register is reset to zero on warm boot, CPU
PM notifier is added to save and restore the value.

Changes v4->v5:
1. Removed compat_dyn_elf_hwcap and used just compat_elf_hwcap as per
   Catalin's suggestion
2. Instead of resetting event stream in CPU PM callbacks, saving and
   restoring counter control register now.

Changes v3->v4:
1. Added a config option to en/disable the timer event stream feature.
   Due to this the ordering of the patches as well as some cosmetic
   code changes are done.( So dropped previous reviewed-by tags)
2. Removed exporting compat_dyn_elf_hwcap as there are no modules
   requiring it for now.

Changes v2->v3:
1. Moved ARM and ARM64 changes into separate patches
2. Added native hwcaps definations(ARM/ARM64) and compat-specific
   definitions(ARM64) to the users for the event stream feature. 

Changes v1->v2:
1. Saved event stream divider value on cold reset path and re-used it
   in CPU PM notifier instead of calculating every time.
2. Rebased on v3.11-rc2(to avoid conflicts with __cpuinit* deletion)
3. Dropped "ARM/ARM64: arch_timer: remove __cpuinit attribute for
   arch_counter_set_user_access"(already done as part of __cpuinit*
   deletion)

Regards,
Sudeep

Sudeep KarkadaNagesha (4):
  ARM/ARM64: arch_timer: add macros for bits in control register
  ARM: arch_timer: add support to configure and enable event stream
  ARM64: arch_timer: add support to configure and enable event stream
  drivers: clocksource: add CPU PM notifier for ARM architected timer

Will Deacon (1):
  drivers: clocksource: add support for ARM architected timer event
    stream

 arch/arm/include/asm/arch_timer.h    | 38 +++++++++++++++++++++----
 arch/arm/include/uapi/asm/hwcap.h    |  1 +
 arch/arm/kernel/setup.c              |  1 +
 arch/arm64/include/asm/arch_timer.h  | 44 +++++++++++++++++++++++++----
 arch/arm64/include/asm/hwcap.h       |  8 +++++-
 arch/arm64/include/uapi/asm/hwcap.h  |  1 +
 arch/arm64/kernel/setup.c            |  5 ++++
 drivers/clocksource/Kconfig          | 14 +++++++++
 drivers/clocksource/arm_arch_timer.c | 55 ++++++++++++++++++++++++++++++++++++
 include/clocksource/arm_arch_timer.h | 10 +++++++
 10 files changed, 165 insertions(+), 12 deletions(-)

-- 
1.8.1.2

[PATCH v5 1/5] ARM/ARM64: arch_timer: add macros for bits in control register

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>

Add macros to describe the bitfields in the ARM architected timer
control register to make code easy to understand.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Reviewed-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 arch/arm/include/asm/arch_timer.h    |  9 +++++++--
 arch/arm64/include/asm/arch_timer.h  | 12 ++++++++----
 include/clocksource/arm_arch_timer.h |  8 ++++++++
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index e406d57..1a3e967 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -95,8 +95,13 @@ static inline void arch_counter_set_user_access(void)
 
 	asm volatile("mrc p15, 0, %0, c14, c1, 0" : "=r" (cntkctl));
 
-	/* disable user access to everything */
-	cntkctl &= ~((3 << 8) | (7 << 0));
+	/* Disable user access to both physical/virtual counters/timers */
+	/* Also disable virtual event stream */
+	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
+			| ARCH_TIMER_USR_VT_ACCESS_EN
+			| ARCH_TIMER_VIRT_EVT_EN
+			| ARCH_TIMER_USR_VCT_ACCESS_EN
+			| ARCH_TIMER_USR_PCT_ACCESS_EN);
 
 	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
 }
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 98abd47..fa4af76 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -101,12 +101,16 @@ static inline void arch_counter_set_user_access(void)
 {
 	u32 cntkctl;
 
-	/* Disable user access to the timers and the physical counter. */
 	asm volatile("mrs	%0, cntkctl_el1" : "=r" (cntkctl));
-	cntkctl &= ~((3 << 8) | (1 << 0));
 
-	/* Enable user access to the virtual counter and frequency. */
-	cntkctl |= (1 << 1);
+	/* Disable user access to the timers and the physical counter */
+	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
+			| ARCH_TIMER_USR_VT_ACCESS_EN
+			| ARCH_TIMER_USR_PCT_ACCESS_EN);
+
+	/* Enable user access to the virtual counter */
+	cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
+
 	asm volatile("msr	cntkctl_el1, %0" : : "r" (cntkctl));
 }
 
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index c463ce9..551f7e9 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -29,6 +29,14 @@
 #define ARCH_TIMER_PHYS_ACCESS		0
 #define ARCH_TIMER_VIRT_ACCESS		1
 
+#define ARCH_TIMER_USR_PCT_ACCESS_EN	(1 << 0) /* physical counter */
+#define ARCH_TIMER_USR_VCT_ACCESS_EN	(1 << 1) /* virtual counter */
+#define ARCH_TIMER_VIRT_EVT_EN		(1 << 2)
+#define ARCH_TIMER_EVT_TRIGGER_SHIFT	(4)
+#define ARCH_TIMER_EVT_TRIGGER_MASK	(0xF << ARCH_TIMER_EVT_TRIGGER_SHIFT)
+#define ARCH_TIMER_USR_VT_ACCESS_EN	(1 << 8) /* virtual timer registers */
+#define ARCH_TIMER_USR_PT_ACCESS_EN	(1 << 9) /* physical timer registers */
+
 #ifdef CONFIG_ARM_ARCH_TIMER
 
 extern u32 arch_timer_get_rate(void);
-- 
1.8.1.2

[PATCH v5 2/5] ARM: arch_timer: add support to configure and enable event stream

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>

This patch adds support for configuring the event stream frequency
and enabling it.

It also adds the hwcaps definitions to the user to detect this event
stream feature.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 arch/arm/include/asm/arch_timer.h | 33 ++++++++++++++++++++++++++++-----
 arch/arm/include/uapi/asm/hwcap.h |  1 +
 arch/arm/kernel/setup.c           |  1 +
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 1a3e967..4e38609 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -89,22 +89,45 @@ static inline u64 arch_counter_get_cntvct(void)
 	return cval;
 }
 
-static inline void arch_counter_set_user_access(void)
+static inline u32 arch_timer_get_cntkctl(void)
 {
 	u32 cntkctl;
-
 	asm volatile("mrc p15, 0, %0, c14, c1, 0" : "=r" (cntkctl));
+	return cntkctl;
+}
+
+static inline void arch_timer_set_cntkctl(u32 cntkctl)
+{
+	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+}
+
+static inline void arch_counter_set_user_access(void)
+{
+	u32 cntkctl = arch_timer_get_cntkctl();
 
 	/* Disable user access to both physical/virtual counters/timers */
-	/* Also disable virtual event stream */
 	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
 			| ARCH_TIMER_USR_VT_ACCESS_EN
-			| ARCH_TIMER_VIRT_EVT_EN
 			| ARCH_TIMER_USR_VCT_ACCESS_EN
 			| ARCH_TIMER_USR_PCT_ACCESS_EN);
+	arch_timer_set_cntkctl(cntkctl);
+}
 
-	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+static inline void arch_timer_evtstrm_config(bool enable, int divider)
+{
+	u32 cntkctl = arch_timer_get_cntkctl();
+	if (enable) {
+		cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
+		/* Set the divider and enable virtual event stream */
+		cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
+				| ARCH_TIMER_VIRT_EVT_EN;
+		elf_hwcap |= HWCAP_EVTSTRM;
+	} else {
+		cntkctl &= ~ARCH_TIMER_VIRT_EVT_EN; /* disable event stream */
+	}
+	arch_timer_set_cntkctl(cntkctl);
 }
+
 #endif
 
 #endif
diff --git a/arch/arm/include/uapi/asm/hwcap.h b/arch/arm/include/uapi/asm/hwcap.h
index 6d34d08..7dcc10d 100644
--- a/arch/arm/include/uapi/asm/hwcap.h
+++ b/arch/arm/include/uapi/asm/hwcap.h
@@ -26,5 +26,6 @@
 #define HWCAP_VFPD32	(1 << 19)	/* set if VFP has 32 regs (not 16) */
 #define HWCAP_IDIV	(HWCAP_IDIVA | HWCAP_IDIVT)
 #define HWCAP_LPAE	(1 << 20)
+#define HWCAP_EVTSTRM	(1 << 21)
 
 #endif /* _UAPI__ASMARM_HWCAP_H */
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index afc2489..305ffc9 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -975,6 +975,7 @@ static const char *hwcap_str[] = {
 	"idivt",
 	"vfpd32",
 	"lpae",
+	"evtstrm",
 	NULL
 };
 
-- 
1.8.1.2

[PATCH v5 3/5] ARM64: arch_timer: add support to configure and enable event stream

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>

This patch adds support for configuring the event stream frequency
and enabling it.

It also adds the hwcaps as well as compat-specific definitions to
the user to detect this event stream feature.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 arch/arm64/include/asm/arch_timer.h | 34 +++++++++++++++++++++++++++++++---
 arch/arm64/include/asm/hwcap.h      |  8 +++++++-
 arch/arm64/include/uapi/asm/hwcap.h |  1 +
 arch/arm64/kernel/setup.c           |  5 +++++
 4 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index fa4af76..b27e0a5 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -97,11 +97,21 @@ static inline u32 arch_timer_get_cntfrq(void)
 	return val;
 }
 
-static inline void arch_counter_set_user_access(void)
+static inline u32 arch_timer_get_cntkctl(void)
 {
 	u32 cntkctl;
-
 	asm volatile("mrs	%0, cntkctl_el1" : "=r" (cntkctl));
+	return cntkctl;
+}
+
+static inline void arch_timer_set_cntkctl(u32 cntkctl)
+{
+	asm volatile("msr	cntkctl_el1, %0" : : "r" (cntkctl));
+}
+
+static inline void arch_counter_set_user_access(void)
+{
+	u32 cntkctl = arch_timer_get_cntkctl();
 
 	/* Disable user access to the timers and the physical counter */
 	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
@@ -111,7 +121,25 @@ static inline void arch_counter_set_user_access(void)
 	/* Enable user access to the virtual counter */
 	cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
 
-	asm volatile("msr	cntkctl_el1, %0" : : "r" (cntkctl));
+	arch_timer_set_cntkctl(cntkctl);
+}
+
+static inline void arch_timer_evtstrm_config(bool enable, int divider)
+{
+	u32 cntkctl = arch_timer_get_cntkctl();
+	if (enable) {
+		cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
+		/* Set the divider and enable virtual event stream */
+		cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
+				| ARCH_TIMER_VIRT_EVT_EN;
+		elf_hwcap |= HWCAP_EVTSTRM;
+#ifdef CONFIG_COMPAT
+		compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
+#endif
+	} else {
+		cntkctl &= ~ARCH_TIMER_VIRT_EVT_EN; /* disable event stream */
+	}
+	arch_timer_set_cntkctl(cntkctl);
 }
 
 static inline u64 arch_counter_get_cntvct(void)
diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
index 6d4482f..530fa879 100644
--- a/arch/arm64/include/asm/hwcap.h
+++ b/arch/arm64/include/asm/hwcap.h
@@ -30,6 +30,7 @@
 #define COMPAT_HWCAP_IDIVA	(1 << 17)
 #define COMPAT_HWCAP_IDIVT	(1 << 18)
 #define COMPAT_HWCAP_IDIV	(COMPAT_HWCAP_IDIVA|COMPAT_HWCAP_IDIVT)
+#define COMPAT_HWCAP_EVTSTRM	(1 << 21)
 
 #ifndef __ASSEMBLY__
 /*
@@ -37,11 +38,16 @@
  * instruction set this cpu supports.
  */
 #define ELF_HWCAP		(elf_hwcap)
-#define COMPAT_ELF_HWCAP	(COMPAT_HWCAP_HALF|COMPAT_HWCAP_THUMB|\
+#ifdef CONFIG_COMPAT
+#define COMPAT_ELF_HWCAP	(compat_elf_hwcap)
+#define COMPAT_ELF_HWCAP_DEFAULT	\
+				(COMPAT_HWCAP_HALF|COMPAT_HWCAP_THUMB|\
 				 COMPAT_HWCAP_FAST_MULT|COMPAT_HWCAP_EDSP|\
 				 COMPAT_HWCAP_TLS|COMPAT_HWCAP_VFP|\
 				 COMPAT_HWCAP_VFPv3|COMPAT_HWCAP_VFPv4|\
 				 COMPAT_HWCAP_NEON|COMPAT_HWCAP_IDIV)
+extern unsigned int compat_elf_hwcap;
+#endif
 
 extern unsigned int elf_hwcap;
 #endif
diff --git a/arch/arm64/include/uapi/asm/hwcap.h b/arch/arm64/include/uapi/asm/hwcap.h
index eea4975..9b12476 100644
--- a/arch/arm64/include/uapi/asm/hwcap.h
+++ b/arch/arm64/include/uapi/asm/hwcap.h
@@ -21,6 +21,7 @@
  */
 #define HWCAP_FP		(1 << 0)
 #define HWCAP_ASIMD		(1 << 1)
+#define HWCAP_EVTSTRM		(1 << 2)
 
 
 #endif /* _UAPI__ASM_HWCAP_H */
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index add6ea6..e5c1725 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -60,6 +60,10 @@ EXPORT_SYMBOL(processor_id);
 unsigned int elf_hwcap __read_mostly;
 EXPORT_SYMBOL_GPL(elf_hwcap);
 
+#ifdef CONFIG_COMPAT
+unsigned int compat_elf_hwcap __read_mostly = COMPAT_ELF_HWCAP_DEFAULT;
+#endif
+
 static const char *cpu_name;
 static const char *machine_name;
 phys_addr_t __fdt_pointer __initdata;
@@ -309,6 +313,7 @@ subsys_initcall(topology_init);
 static const char *hwcap_str[] = {
 	"fp",
 	"asimd",
+	"evtstrm",
 	NULL
 };
 
-- 
1.8.1.2

[PATCH v5 4/5] drivers: clocksource: add support for ARM architected timer event stream

[Sudeep KarkadaNagesha]

From: Will Deacon <will.deacon@arm.com>

The ARM architected timer can generate events (used for waking up
CPUs executing the wfe instruction) at a frequency represented as a
power-of-2 divisor of the clock rate.

An event stream might be used:
- To impose a time-out on a wfe polling loop.
- To safeguard against any programming error in case an expected event
  is not generated.
- To implement wfe-based timeouts for userspace locking implementations.

This patch computes the event stream frequency aiming for a period
of 100us between events. It uses ARM/ARM64 specific backends to configure
and enable the event stream.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
[sudeep: moving ARM/ARM64 changes into separate patches
         and adding Kconfig option]
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 drivers/clocksource/Kconfig          | 14 ++++++++++++++
 drivers/clocksource/arm_arch_timer.c | 21 +++++++++++++++++++++
 include/clocksource/arm_arch_timer.h |  2 ++
 3 files changed, 37 insertions(+)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index b7b9b04..74946ca 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -74,6 +74,20 @@ config ARM_ARCH_TIMER
 	bool
 	select CLKSRC_OF if OF
 
+config ARM_ARCH_TIMER_EVTSTREAM
+	bool "Support for ARM architected timer event stream generation"
+	default y if ARM_ARCH_TIMER
+	help
+	  This option enables support for event stream generation based on
+	  the ARM architected timer. It is used for waking up CPUs executing
+	  the wfe instruction at a frequency represented as a power-of-2
+	  divisor of the clock rate. An event stream might be useful for
+	  imposing timeout on a wfe, safeguarding against any programming
+	  errors in case an expected event is not generated or even to
+	  implement wfe-based timeouts for userspace locking implementations.
+	  This can be disabled for hardware validation purposes to detect any
+	  hardware anomalies of missing events.
+
 config ARM_GLOBAL_TIMER
 	bool
 	select CLKSRC_OF if OF
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index ffadd83..f42bef4 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -123,6 +123,26 @@ static int arch_timer_set_next_event_phys(unsigned long evt,
 	return 0;
 }
 
+#ifdef CONFIG_ARM_ARCH_TIMER_EVTSTREAM
+static void arch_timer_setup_evtstream(void)
+{
+	int evt_stream_div, pos;
+
+	/* Find the closest power of two to the divisor */
+	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
+	pos = fls(evt_stream_div);
+	if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
+		pos--;
+	/* enable event stream */
+	arch_timer_evtstrm_config(true, min(pos, 15));
+}
+#else
+static void arch_timer_setup_evtstream(void)
+{
+	arch_timer_evtstrm_config(false, 0);
+}
+#endif
+
 static int arch_timer_setup(struct clock_event_device *clk)
 {
 	clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP;
@@ -154,6 +174,7 @@ static int arch_timer_setup(struct clock_event_device *clk)
 	}
 
 	arch_counter_set_user_access();
+	arch_timer_setup_evtstream();
 
 	return 0;
 }
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 551f7e9..c4d0fc4 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -37,6 +37,8 @@
 #define ARCH_TIMER_USR_VT_ACCESS_EN	(1 << 8) /* virtual timer registers */
 #define ARCH_TIMER_USR_PT_ACCESS_EN	(1 << 9) /* physical timer registers */
 
+#define ARCH_TIMER_EVT_STREAM_FREQ	10000	/* 100us */
+
 #ifdef CONFIG_ARM_ARCH_TIMER
 
 extern u32 arch_timer_get_rate(void);
-- 
1.8.1.2

[PATCH v5 5/5] drivers: clocksource: add CPU PM notifier for ARM architected timer

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>

Few control settings done in architected timer as part of initialisation
can be lost when CPU enters deeper power states. They need to be
re-initialised when the CPU is (warm)reset again.

This patch adds CPU PM notifiers to do the timer initialisation on warm
resets. It also save the event stream divider value calculated during
cold reset and uses the same in warm reset path.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Reviewed-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 drivers/clocksource/arm_arch_timer.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index f42bef4..ffb0bfb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -13,6 +13,7 @@
 #include <linux/device.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
+#include <linux/cpu_pm.h>
 #include <linux/clockchips.h>
 #include <linux/interrupt.h>
 #include <linux/of_irq.h>
@@ -281,6 +282,33 @@ static struct notifier_block arch_timer_cpu_nb = {
 	.notifier_call = arch_timer_cpu_notify,
 };
 
+#ifdef CONFIG_CPU_PM
+static unsigned int saved_cntkctl;
+static int arch_timer_cpu_pm_notify(struct notifier_block *self,
+				    unsigned long action, void *hcpu)
+{
+	if (action == CPU_PM_ENTER)
+		saved_cntkctl = arch_timer_get_cntkctl();
+	else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT)
+		arch_timer_set_cntkctl(saved_cntkctl);
+	return NOTIFY_OK;
+}
+
+static struct notifier_block arch_timer_cpu_pm_notifier = {
+	.notifier_call = arch_timer_cpu_pm_notify,
+};
+
+static int __init arch_timer_cpu_pm_init(void)
+{
+	return cpu_pm_register_notifier(&arch_timer_cpu_pm_notifier);
+}
+#else
+static int __init arch_timer_cpu_pm_init(void)
+{
+	return 0;
+}
+#endif
+
 static int __init arch_timer_register(void)
 {
 	int err;
@@ -330,11 +358,17 @@ static int __init arch_timer_register(void)
 	if (err)
 		goto out_free_irq;
 
+	err = arch_timer_cpu_pm_init();
+	if (err)
+		goto out_unreg_notify;
+
 	/* Immediately configure the timer on the boot CPU */
 	arch_timer_setup(this_cpu_ptr(arch_timer_evt));
 
 	return 0;
 
+out_unreg_notify:
+	unregister_cpu_notifier(&arch_timer_cpu_nb);
 out_free_irq:
 	if (arch_timer_use_virtual)
 		free_percpu_irq(arch_timer_ppi[VIRT_PPI], arch_timer_evt);
-- 
1.8.1.2

Re: [PATCH v5 5/5] drivers: clocksource: add CPU PM notifier for ARM architected timer

[Sudeep KarkadaNagesha]

On 28/08/13 11:44, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> 
> Few control settings done in architected timer as part of initialisation
> can be lost when CPU enters deeper power states. They need to be
> re-initialised when the CPU is (warm)reset again.
> 
> This patch adds CPU PM notifiers to do the timer initialisation on warm
> resets. It also save the event stream divider value calculated during
> cold reset and uses the same in warm reset path.
> 
Ah, forgot to update the commit log. It's now updated:

Few control settings done in architected timer as part of initialisation
can be lost when CPU enters deeper power states. They need to be
restored when the CPU is (warm)reset again.

This patch adds CPU PM notifiers to save the counter control register
when entering low power modes and restore it when CPU exits low power.

> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Reviewed-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index f42bef4..ffb0bfb 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -13,6 +13,7 @@
>  #include <linux/device.h>
>  #include <linux/smp.h>
>  #include <linux/cpu.h>
> +#include <linux/cpu_pm.h>
>  #include <linux/clockchips.h>
>  #include <linux/interrupt.h>
>  #include <linux/of_irq.h>
> @@ -281,6 +282,33 @@ static struct notifier_block arch_timer_cpu_nb = {
>  	.notifier_call = arch_timer_cpu_notify,
>  };
>  
> +#ifdef CONFIG_CPU_PM
> +static unsigned int saved_cntkctl;
> +static int arch_timer_cpu_pm_notify(struct notifier_block *self,
> +				    unsigned long action, void *hcpu)
> +{
> +	if (action == CPU_PM_ENTER)
> +		saved_cntkctl = arch_timer_get_cntkctl();
> +	else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT)
> +		arch_timer_set_cntkctl(saved_cntkctl);
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block arch_timer_cpu_pm_notifier = {
> +	.notifier_call = arch_timer_cpu_pm_notify,
> +};
> +
> +static int __init arch_timer_cpu_pm_init(void)
> +{
> +	return cpu_pm_register_notifier(&arch_timer_cpu_pm_notifier);
> +}
> +#else
> +static int __init arch_timer_cpu_pm_init(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>  static int __init arch_timer_register(void)
>  {
>  	int err;
> @@ -330,11 +358,17 @@ static int __init arch_timer_register(void)
>  	if (err)
>  		goto out_free_irq;
>  
> +	err = arch_timer_cpu_pm_init();
> +	if (err)
> +		goto out_unreg_notify;
> +
>  	/* Immediately configure the timer on the boot CPU */
>  	arch_timer_setup(this_cpu_ptr(arch_timer_evt));
>  
>  	return 0;
>  
> +out_unreg_notify:
> +	unregister_cpu_notifier(&arch_timer_cpu_nb);
>  out_free_irq:
>  	if (arch_timer_use_virtual)
>  		free_percpu_irq(arch_timer_ppi[VIRT_PPI], arch_timer_evt);
> 

Re: [PATCH v5 4/5] drivers: clocksource: add support for ARM architected timer event stream

[Catalin Marinas]

On Wed, Aug 28, 2013 at 11:44:48AM +0100, Sudeep KarkadaNagesha wrote:
> From: Will Deacon <will.deacon@arm.com>
> 
> The ARM architected timer can generate events (used for waking up
> CPUs executing the wfe instruction) at a frequency represented as a
> power-of-2 divisor of the clock rate.
> 
> An event stream might be used:
> - To impose a time-out on a wfe polling loop.
> - To safeguard against any programming error in case an expected event
>   is not generated.
> - To implement wfe-based timeouts for userspace locking implementations.
> 
> This patch computes the event stream frequency aiming for a period
> of 100us between events. It uses ARM/ARM64 specific backends to configure
> and enable the event stream.
> 
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> [sudeep: moving ARM/ARM64 changes into separate patches
>          and adding Kconfig option]
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  drivers/clocksource/Kconfig          | 14 ++++++++++++++
>  drivers/clocksource/arm_arch_timer.c | 21 +++++++++++++++++++++
>  include/clocksource/arm_arch_timer.h |  2 ++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index b7b9b04..74946ca 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -74,6 +74,20 @@ config ARM_ARCH_TIMER
>  	bool
>  	select CLKSRC_OF if OF
>  
> +config ARM_ARCH_TIMER_EVTSTREAM
> +	bool "Support for ARM architected timer event stream generation"
> +	default y if ARM_ARCH_TIMER
> +	help
> +	  This option enables support for event stream generation based on
> +	  the ARM architected timer. It is used for waking up CPUs executing
> +	  the wfe instruction at a frequency represented as a power-of-2
> +	  divisor of the clock rate. An event stream might be useful for
> +	  imposing timeout on a wfe, safeguarding against any programming
> +	  errors in case an expected event is not generated or even to
> +	  implement wfe-based timeouts for userspace locking implementations.
> +	  This can be disabled for hardware validation purposes to detect any
> +	  hardware anomalies of missing events.

Maybe we should reorder this a bit and make the wfe-based timeouts as
the main reason.

> +
>  config ARM_GLOBAL_TIMER
>  	bool
>  	select CLKSRC_OF if OF
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index ffadd83..f42bef4 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -123,6 +123,26 @@ static int arch_timer_set_next_event_phys(unsigned long evt,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_ARM_ARCH_TIMER_EVTSTREAM
> +static void arch_timer_setup_evtstream(void)
> +{
> +	int evt_stream_div, pos;
> +
> +	/* Find the closest power of two to the divisor */
> +	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
> +	pos = fls(evt_stream_div);
> +	if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
> +		pos--;
> +	/* enable event stream */
> +	arch_timer_evtstrm_config(true, min(pos, 15));
> +}
> +#else
> +static void arch_timer_setup_evtstream(void)
> +{
> +	arch_timer_evtstrm_config(false, 0);
> +}
> +#endif

I think we only need an arch_timer_evtstrm_enable() function without the
bool argument. We don't need to disable it at run-time.

> +
>  static int arch_timer_setup(struct clock_event_device *clk)
>  {
>  	clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP;
> @@ -154,6 +174,7 @@ static int arch_timer_setup(struct clock_event_device *clk)
>  	}
>  
>  	arch_counter_set_user_access();
> +	arch_timer_setup_evtstream();

and here just:

	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
		arch_timer_evtstrm_enable();

-- 
Catalin

Re: [PATCH v5 3/5] ARM64: arch_timer: add support to configure and enable event stream

[Catalin Marinas]

On Wed, Aug 28, 2013 at 11:44:47AM +0100, Sudeep KarkadaNagesha wrote:
>  static inline u64 arch_counter_get_cntvct(void)
> diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
> index 6d4482f..530fa879 100644
> --- a/arch/arm64/include/asm/hwcap.h
> +++ b/arch/arm64/include/asm/hwcap.h
> @@ -30,6 +30,7 @@
>  #define COMPAT_HWCAP_IDIVA	(1 << 17)
>  #define COMPAT_HWCAP_IDIVT	(1 << 18)
>  #define COMPAT_HWCAP_IDIV	(COMPAT_HWCAP_IDIVA|COMPAT_HWCAP_IDIVT)
> +#define COMPAT_HWCAP_EVTSTRM	(1 << 21)
>  
>  #ifndef __ASSEMBLY__
>  /*
> @@ -37,11 +38,16 @@
>   * instruction set this cpu supports.
>   */
>  #define ELF_HWCAP		(elf_hwcap)
> -#define COMPAT_ELF_HWCAP	(COMPAT_HWCAP_HALF|COMPAT_HWCAP_THUMB|\
> +#ifdef CONFIG_COMPAT
> +#define COMPAT_ELF_HWCAP	(compat_elf_hwcap)
> +#define COMPAT_ELF_HWCAP_DEFAULT	\
> +				(COMPAT_HWCAP_HALF|COMPAT_HWCAP_THUMB|\
>  				 COMPAT_HWCAP_FAST_MULT|COMPAT_HWCAP_EDSP|\
>  				 COMPAT_HWCAP_TLS|COMPAT_HWCAP_VFP|\
>  				 COMPAT_HWCAP_VFPv3|COMPAT_HWCAP_VFPv4|\
>  				 COMPAT_HWCAP_NEON|COMPAT_HWCAP_IDIV)
> +extern unsigned int compat_elf_hwcap;
> +#endif

I think you can move the default to the setup.c file, it's not of any
use apart from initialisation.

-- 
Catalin

Re: [PATCH v5 2/5] ARM: arch_timer: add support to configure and enable event stream

[Catalin Marinas]

On Wed, Aug 28, 2013 at 11:44:46AM +0100, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> 
> This patch adds support for configuring the event stream frequency
> and enabling it.
> 
> It also adds the hwcaps definitions to the user to detect this event
> stream feature.
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  arch/arm/include/asm/arch_timer.h | 33 ++++++++++++++++++++++++++++-----
>  arch/arm/include/uapi/asm/hwcap.h |  1 +
>  arch/arm/kernel/setup.c           |  1 +
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 1a3e967..4e38609 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -89,22 +89,45 @@ static inline u64 arch_counter_get_cntvct(void)
>  	return cval;
>  }
>  
> -static inline void arch_counter_set_user_access(void)
> +static inline u32 arch_timer_get_cntkctl(void)
>  {
>  	u32 cntkctl;
> -
>  	asm volatile("mrc p15, 0, %0, c14, c1, 0" : "=r" (cntkctl));
> +	return cntkctl;
> +}
> +
> +static inline void arch_timer_set_cntkctl(u32 cntkctl)
> +{
> +	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
> +}
> +
> +static inline void arch_counter_set_user_access(void)
> +{
> +	u32 cntkctl = arch_timer_get_cntkctl();
>  
>  	/* Disable user access to both physical/virtual counters/timers */
> -	/* Also disable virtual event stream */
>  	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
>  			| ARCH_TIMER_USR_VT_ACCESS_EN
> -			| ARCH_TIMER_VIRT_EVT_EN
>  			| ARCH_TIMER_USR_VCT_ACCESS_EN
>  			| ARCH_TIMER_USR_PCT_ACCESS_EN);
> +	arch_timer_set_cntkctl(cntkctl);
> +}

You can keep the ARCH_TIMER_VIRT_EVT_EN clearing here, especially if we
drop the bool enable argument to arch_timer_evtstrm_config(). You can
add the same clearing to arm64 in one of the latter patches.

-- 
Catalin

Re: [PATCH v5 5/5] drivers: clocksource: add CPU PM notifier for ARM architected timer

[Venkappa Mala]

Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha <at> arm.com> writes:

> 
> On 28/08/13 11:44, Sudeep KarkadaNagesha wrote:
> > From: Sudeep KarkadaNagesha <sudeep.karkadanagesha <at> arm.com>
> > 
> > Few control settings done in architected timer as part of 
initialisation
> > can be lost when CPU enters deeper power states. They need to be
> > re-initialised when the CPU is (warm)reset again.
> > 
> > This patch adds CPU PM notifiers to do the timer initialisation on 
warm
> > resets. It also save the event stream divider value calculated 
during
> > cold reset and uses the same in warm reset path.
> > 
> Ah, forgot to update the commit log. It's now updated:
> 
> Few control settings done in architected timer as part of 
initialisation
> can be lost when CPU enters deeper power states. They need to be
> restored when the CPU is (warm)reset again.
> 
> This patch adds CPU PM notifiers to save the counter control register
> when entering low power modes and restore it when CPU exits low power.
> 
> > Cc: Catalin Marinas <catalin.marinas <at> arm.com>
> > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi <at> arm.com>
> > Reviewed-by: Will Deacon <will.deacon <at> arm.com>
> > Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha <at> 
arm.com>
> > ---
> >  drivers/clocksource/arm_arch_timer.c | 34 
++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
> > index f42bef4..ffb0bfb 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> >  <at>  <at>  -13,6 +13,7  <at>  <at> 
> >  #include <linux/device.h>
> >  #include <linux/smp.h>
> >  #include <linux/cpu.h>
> > +#include <linux/cpu_pm.h>
> >  #include <linux/clockchips.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/of_irq.h>
> >  <at>  <at>  -281,6 +282,33  <at>  <at>  static struct 
notifier_block arch_timer_cpu_nb = {
> >  	.notifier_call = arch_timer_cpu_notify,
> >  };
> >  
> > +#ifdef CONFIG_CPU_PM
> > +static unsigned int saved_cntkctl;
> > +static int arch_timer_cpu_pm_notify(struct notifier_block *self,
> > +				    unsigned long action, void *hcpu)
> > +{
> > +	if (action == CPU_PM_ENTER)
> > +		saved_cntkctl = arch_timer_get_cntkctl();
> > +	else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT)
> > +		arch_timer_set_cntkctl(saved_cntkctl);
> > +	return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block arch_timer_cpu_pm_notifier = {
> > +	.notifier_call = arch_timer_cpu_pm_notify,
> > +};
> > +
> > +static int __init arch_timer_cpu_pm_init(void)
> > +{
> > +	return cpu_pm_register_notifier(&arch_timer_cpu_pm_notifier);
> > +}
> > +#else
> > +static int __init arch_timer_cpu_pm_init(void)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> >  static int __init arch_timer_register(void)
> >  {
> >  	int err;
> >  <at>  <at>  -330,11 +358,17  <at>  <at>  static int __init 
arch_timer_register(void)
> >  	if (err)
> >  		goto out_free_irq;
> >  
> > +	err = arch_timer_cpu_pm_init();
> > +	if (err)
> > +		goto out_unreg_notify;
> > +
> >  	/* Immediately configure the timer on the boot CPU */
> >  	arch_timer_setup(this_cpu_ptr(arch_timer_evt));
> >  
> >  	return 0;
> >  
> > +out_unreg_notify:
> > +	unregister_cpu_notifier(&arch_timer_cpu_nb);
> >  out_free_irq:
> >  	if (arch_timer_use_virtual)
> >  		free_percpu_irq(arch_timer_ppi[VIRT_PPI], 
arch_timer_evt);
> > 
> 
> 

Hello Catalin,

I have noticed this patch merge in 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git 
at >= linux-3.13.y branches.

As part of ARMv7/ARMv8 vDSO experiments, i have tried to enable arch 
timer in 3.10 branch but it is failed to boot like 
"[1246696886.455361]      swapper/0:    0] [c0] [c0] Calibrating delay 
loop (skipped), value calculated using timer frequency.. 52.00 BogoMIPS 
(lpj=260000)" 
due to cntkctl context lost on both armv7/armv8 cases.However, it works 
fine when I have pulled the patch to 3.10 branch.

Perhaps, it will be good to have this patch in 3.10.y (LTS kernel) 
branch as well.
is there any potential issue if we merge this patch or is it 
intentional?

BR,
Venkappa

Re: [PATCH v5 5/5] drivers: clocksource: add CPU PM notifier for ARM architected timer

[Sudeep Holla]

Hi Venkappa,

First of all, please make sure the relevant mailing list(ALKML in this case)
and maintainers (MarcZ and Catalin as you are addressing him) are cc-ed,
else there's every chance that the mail gets lost.

On Thu, Feb 19, 2015 at 1:03 PM, Venkappa Mala <venkappa.m@samsung.com> wrote:
> Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha <at> arm.com> writes:
>
>>
>> On 28/08/13 11:44, Sudeep KarkadaNagesha wrote:
>> > From: Sudeep KarkadaNagesha <sudeep.karkadanagesha <at> arm.com>
>> >
>> > Few control settings done in architected timer as part of
> initialisation
>> > can be lost when CPU enters deeper power states. They need to be
>> > re-initialised when the CPU is (warm)reset again.
>> >
>> > This patch adds CPU PM notifiers to do the timer initialisation on
> warm
>> > resets. It also save the event stream divider value calculated
> during
>> > cold reset and uses the same in warm reset path.
>> >
>> Ah, forgot to update the commit log. It's now updated:
>>
>> Few control settings done in architected timer as part of
> initialisation
>> can be lost when CPU enters deeper power states. They need to be
>> restored when the CPU is (warm)reset again.
>>
>> This patch adds CPU PM notifiers to save the counter control register
>> when entering low power modes and restore it when CPU exits low power.
>>
>> > Cc: Catalin Marinas <catalin.marinas <at> arm.com>
>> > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi <at> arm.com>
>> > Reviewed-by: Will Deacon <will.deacon <at> arm.com>
>> > Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha <at>
> arm.com>

[...]

> Hello Catalin,
>
> I have noticed this patch merge in
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
> at >= linux-3.13.y branches.
>
> As part of ARMv7/ARMv8 vDSO experiments, i have tried to enable arch
> timer in 3.10 branch but it is failed to boot like
> "[1246696886.455361]      swapper/0:    0] [c0] [c0] Calibrating delay
> loop (skipped), value calculated using timer frequency.. 52.00 BogoMIPS
> (lpj=260000)"
> due to cntkctl context lost on both armv7/armv8 cases.However, it works
> fine when I have pulled the patch to 3.10 branch.
>
> Perhaps, it will be good to have this patch in 3.10.y (LTS kernel)
> branch as well.
> is there any potential issue if we merge this patch or is it
> intentional?
>

Are you targeting this on ARM64 platform ?
If so, I don't understand how is the arch timer context is lost as the cpuidle
driver is not merge to 3.10 stable kernel ?

Regards,
Sudeep

Re: [PATCH v5 5/5] drivers: clocksource: add CPU PM notifier for ARM architected timer

[Venkappa Mala]

Sudeep Holla <sudeep.holla <at> arm.com> writes:

> Are you targeting this on ARM64 platform ?
> If so, I don't understand how is the arch timer context is lost as the 
cpuidle
> driver is not merge to 3.10 stable kernel ?


For time being, you can ignore about ARM64.
let's have a discussion on ARMv7 first. 
I have enabled generic arch arm timer in kernel 3.10 frame work to 
validate vDSO (ARM: vDSO -> still, it did not merge in the vanilla kernel) 
on ARMv7. As part of this, i have noticed that the contex lost is 
happening. After pulling the patch, the issue is resolved, timer/counter 
is up and running. In future, someone wants to enable arch timer in 3.10, 
then I anticipate that the patch needs to be merged in vanilla kernel.
correct me if I am wrong.

BR,
Venkappa