[PATCH v3]OMAP:GPTIMER:1ms tick generation correction

[PATCH v3]OMAP:GPTIMER:1ms tick generation correction

[Tarun Kanti DebBarma]

Generation of 1ms granular GPTIMER events using 32KHz or system
clocks as inputs does not have whole number count value to load
into the register. This inaccurate count value with respect to 1ms
period leads to time drift subsequently. OMAP3 and later silicons
have dedicated registers for GPTIMER1, GPTIMER2 and GPTIMER10,
which can be programmed with computed values to keep this error
controlled within specified limit.

Version 2:
(i) optimized omap_dm_timer_ms_correction() function and corrected
error in computing the positive and negative increments.
(ii) typo corrections in comment section and warning removal related
to 80-character limit

Version 3:
(i) removed un-necessary initialization of ms_correction variable.
(ii) convert omap_dm_timer_ms_correction(...) to static since it
is used locally ONLY.
(iii) use DIV_ROUND_UP() macro in the positive and negative increment
computation to avoid underflow.
(iv) correct coding violations: space around mathematical operators
like *, +, etc. and further 80-character violations.

Version 4:
[In-progress]
(i) coordination with GPTIMER hwmod changes
(ii) investigate generic clock code for similar functionality/
implementation

Signed-off-by: R Sricharan <r.sricharan@ti.com>
Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
 arch/arm/plat-omap/dmtimer.c |   43 ++++++++++++++++++++++++++++++++++++-----
 1 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index c64875f..03ea90d 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -40,6 +40,7 @@
 #include <mach/hardware.h>
 #include <plat/dmtimer.h>
 #include <mach/irqs.h>
+#include <linux/kernel.h>
 
 /* register offsets */
 #define _OMAP_TIMER_ID_OFFSET		0x00
@@ -160,6 +161,9 @@ struct omap_dm_timer {
 	unsigned reserved:1;
 	unsigned enabled:1;
 	unsigned posted:1;
+#ifdef CONFIG_ARCH_OMAP2PLUS
+	unsigned ms_correction:1;
+#endif
 };
 
 static int dm_timer_count;
@@ -218,8 +222,10 @@ static const int omap2_dm_timer_count = ARRAY_SIZE(omap2_dm_timers);
 
 #ifdef CONFIG_ARCH_OMAP3
 static struct omap_dm_timer omap3_dm_timers[] = {
-	{ .phys_base = 0x48318000, .irq = INT_24XX_GPTIMER1 },
-	{ .phys_base = 0x49032000, .irq = INT_24XX_GPTIMER2 },
+	{ .phys_base = 0x48318000, .irq = INT_24XX_GPTIMER1,
+					.ms_correction = 1 },
+	{ .phys_base = 0x49032000, .irq = INT_24XX_GPTIMER2,
+					.ms_correction = 1 },
 	{ .phys_base = 0x49034000, .irq = INT_24XX_GPTIMER3 },
 	{ .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4 },
 	{ .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5 },
@@ -227,7 +233,8 @@ static struct omap_dm_timer omap3_dm_timers[] = {
 	{ .phys_base = 0x4903C000, .irq = INT_24XX_GPTIMER7 },
 	{ .phys_base = 0x4903E000, .irq = INT_24XX_GPTIMER8 },
 	{ .phys_base = 0x49040000, .irq = INT_24XX_GPTIMER9 },
-	{ .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10 },
+	{ .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
+					.ms_correction = 1 },
 	{ .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11 },
 	{ .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ },
 };
@@ -250,8 +257,10 @@ static const int omap3_dm_timer_count = ARRAY_SIZE(omap3_dm_timers);
 
 #ifdef CONFIG_ARCH_OMAP4
 static struct omap_dm_timer omap4_dm_timers[] = {
-	{ .phys_base = 0x4a318000, .irq = OMAP44XX_IRQ_GPT1 },
-	{ .phys_base = 0x48032000, .irq = OMAP44XX_IRQ_GPT2 },
+	{ .phys_base = 0x4a318000, .irq = OMAP44XX_IRQ_GPT1,
+					.ms_correction = 1 },
+	{ .phys_base = 0x48032000, .irq = OMAP44XX_IRQ_GPT2,
+					.ms_correction = 1 },
 	{ .phys_base = 0x48034000, .irq = OMAP44XX_IRQ_GPT3 },
 	{ .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4 },
 	{ .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5 },
@@ -259,7 +268,8 @@ static struct omap_dm_timer omap4_dm_timers[] = {
 	{ .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT7 },
 	{ .phys_base = 0x4013e000, .irq = OMAP44XX_IRQ_GPT8 },
 	{ .phys_base = 0x4803e000, .irq = OMAP44XX_IRQ_GPT9 },
-	{ .phys_base = 0x48086000, .irq = OMAP44XX_IRQ_GPT10 },
+	{ .phys_base = 0x48086000, .irq = OMAP44XX_IRQ_GPT10,
+					.ms_correction = 1 },
 	{ .phys_base = 0x48088000, .irq = OMAP44XX_IRQ_GPT11 },
 	{ .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12 },
 };
@@ -360,6 +370,23 @@ static void omap_dm_timer_prepare(struct omap_dm_timer *timer)
 	omap_dm_timer_enable(timer);
 	omap_dm_timer_reset(timer);
 }
+/**
+ * omap_dm_timer_ms_correction - hardware correction for millisecond timer
+ * @timer: GPTIMER on which the correction support is to be applied
+ * @load: timer load value, used here to extract the expiry count
+ */
+static void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32 load)
+{
+	int pos_increment, neg_increment;
+	unsigned int count = (0xFFFFFFFF - load) * 1024;
+
+	pos_increment = (DIV_ROUND_UP(count, 1000) * 1000000) \
+			- (count * 1000);
+	neg_increment = ((DIV_ROUND_UP(count, 1000) - 1) * 1000000) \
+			- (count * 1000);
+	omap_dm_timer_write_reg(timer, OMAP_TIMER_TICK_POS_REG, pos_increment);
+	omap_dm_timer_write_reg(timer, OMAP_TIMER_TICK_NEG_REG, neg_increment);
+}
 
 struct omap_dm_timer *omap_dm_timer_request(void)
 {
@@ -612,6 +639,10 @@ void omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
 {
 	u32 l;
 
+#ifdef CONFIG_ARCH_OMAP2PLUS
+	if (timer->ms_correction)
+		omap_dm_timer_ms_correction(timer, load);
+#endif
 	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
 	if (autoreload) {
 		l |= OMAP_TIMER_CTRL_AR;
-- 
1.6.0.4

Re: [PATCH v3]OMAP:GPTIMER:1ms tick generation correction

[Tony Lindgren]

* Tarun Kanti DebBarma <tarun.kanti@ti.com> [100622 12:35]:
> +/**
> + * omap_dm_timer_ms_correction - hardware correction for millisecond timer
> + * @timer: GPTIMER on which the correction support is to be applied
> + * @load: timer load value, used here to extract the expiry count
> + */
> +static void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32 load)
> +{
> +	int pos_increment, neg_increment;
> +	unsigned int count = (0xFFFFFFFF - load) * 1024;
> +
> +	pos_increment = (DIV_ROUND_UP(count, 1000) * 1000000) \
> +			- (count * 1000);
> +	neg_increment = ((DIV_ROUND_UP(count, 1000) - 1) * 1000000) \
> +			- (count * 1000);
> +	omap_dm_timer_write_reg(timer, OMAP_TIMER_TICK_POS_REG, pos_increment);
> +	omap_dm_timer_write_reg(timer, OMAP_TIMER_TICK_NEG_REG, neg_increment);
> +}
>  
>  struct omap_dm_timer *omap_dm_timer_request(void)
>  {
> @@ -612,6 +639,10 @@ void omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
>  {
>  	u32 l;
>  
> +#ifdef CONFIG_ARCH_OMAP2PLUS
> +	if (timer->ms_correction)
> +		omap_dm_timer_ms_correction(timer, load);
> +#endif
>  	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
>  	if (autoreload) {
>  		l |= OMAP_TIMER_CTRL_AR;

How do you know that the timer is configured to use the 32KiHZ source?

Also, since omap2_gp_timer_set_next_event calls all the time,
we don't want to do this calculation for every tick.. So we should
make it a separate optional function.

Or can we somehow calculate this drift once during init?

Regards,

Tony

Re: [PATCH v3]OMAP:GPTIMER:1ms tick generation correction

[David Brownell]

--- On Wed, 7/7/10, Tony Lindgren <tony@atomide.com> wrote:


> Or can we somehow calculate this drift once during init?

If it's set up in the gentime framework, yes; very
standard.  AT91 is one model (they all have
32K clocks used to generate the system tick).

Re: [PATCH v3]OMAP:GPTIMER:1ms tick generation correction

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [100707 15:26]:
> 
> --- On Wed, 7/7/10, Tony Lindgren <tony@atomide.com> wrote:
> 
> 
> > Or can we somehow calculate this drift once during init?
> 
> If it's set up in the gentime framework, yes; very
> standard.  AT91 is one model (they all have
> 32K clocks used to generate the system tick).

OK thats good to hear.

Tony 

RE: [PATCH v3]OMAP:GPTIMER:1ms tick generation correction

[DebBarma, Tarun Kanti]

Tony,
Thanks for the comments! Please see my response.

> -----Original Message-----
> From: Tony Lindgren [mailto:tony@atomide.com]
> Sent: Wednesday, July 07, 2010 5:30 PM
> To: DebBarma, Tarun Kanti
> Cc: linux-omap@vger.kernel.org; R, Sricharan
> Subject: Re: [PATCH v3]OMAP:GPTIMER:1ms tick generation correction
> 
> * Tarun Kanti DebBarma <tarun.kanti@ti.com> [100622 12:35]:
> > +/**
> > + * omap_dm_timer_ms_correction - hardware correction for millisecond
> timer
> > + * @timer: GPTIMER on which the correction support is to be applied
> > + * @load: timer load value, used here to extract the expiry count
> > + */
> > +static void omap_dm_timer_ms_correction(struct omap_dm_timer *timer,
> u32 load)
> > +{
> > +	int pos_increment, neg_increment;
> > +	unsigned int count = (0xFFFFFFFF - load) * 1024;
> > +
> > +	pos_increment = (DIV_ROUND_UP(count, 1000) * 1000000) \
> > +			- (count * 1000);
> > +	neg_increment = ((DIV_ROUND_UP(count, 1000) - 1) * 1000000) \
> > +			- (count * 1000);
> > +	omap_dm_timer_write_reg(timer, OMAP_TIMER_TICK_POS_REG,
> pos_increment);
> > +	omap_dm_timer_write_reg(timer, OMAP_TIMER_TICK_NEG_REG,
> neg_increment);
> > +}
> >
> >  struct omap_dm_timer *omap_dm_timer_request(void)
> >  {
> > @@ -612,6 +639,10 @@ void omap_dm_timer_set_load_start(struct
> omap_dm_timer *timer, int autoreload,
> >  {
> >  	u32 l;
> >
> > +#ifdef CONFIG_ARCH_OMAP2PLUS
> > +	if (timer->ms_correction)
> > +		omap_dm_timer_ms_correction(timer, load);
> > +#endif
> >  	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
> >  	if (autoreload) {
> >  		l |= OMAP_TIMER_CTRL_AR;
> 
> How do you know that the timer is configured to use the 32KiHZ source?
> 

[Tarun]
At this point we do not know. The point is we need not know.
The caller function calculates the load based upon the source clock frequency. Viz. sys_clk or 32Khz. Please note that correction is needed both for sys_clk and 32Khz clock.

> Also, since omap2_gp_timer_set_next_event calls all the time,
> we don't want to do this calculation for every tick.. So we should
> make it a separate optional function.

[Tarun]
Agreed, but need some clarification here. 
At first my intention was to make the call to omap_dm_timer_ms_correction() just once since the tick period is supposed to be constant. However, during testing it was found that the caller of omap2_gp_timer_set_next_event(unsigned long cycles,...) passes different values for cycles and so different load value. This gave me the impression that caller framework is programming for counts related to different number ticks with the necessity to apply omap_dm_timer_ms_correction() every time.

I would be glad to get feedback on my observation.

> 
> Or can we somehow calculate this drift once during init?
> 
As described, for a constant tick we would need to compute and program just once, during initialization.


> Regards,
> 
> Tony