* Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 20:23 [PATCH v2] OMAP:GPTIMER:1ms tick generation correction Tarun Kanti DebBarma
@ 2010-06-21 10:44 ` Nishanth Menon
2010-06-21 12:51 ` DebBarma, Tarun Kanti
2010-06-21 14:21 ` David Brownell
2010-06-21 15:54 ` Kevin Hilman
1 sibling, 2 replies; 11+ messages in thread
From: Nishanth Menon @ 2010-06-21 10:44 UTC (permalink / raw)
To: Tarun Kanti DebBarma; +Cc: linux-omap, R Sricharan
NAK - my prev comments are not fixed here either.
On 06/21/2010 03:23 PM, Tarun Kanti DebBarma wrote:
> 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:
move this to diffstat section please.
> (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
>
> Tested on Zoom3 using Linus tree.
>
> 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 | 131 +++++++++++++++++++++--------
> arch/arm/plat-omap/include/plat/dmtimer.h | 1 +
> 2 files changed, 96 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index c64875f..42344a7
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -160,6 +160,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;
> @@ -185,18 +188,30 @@ static const int omap1_dm_timer_count = ARRAY_SIZE(omap1_dm_timers);
>
> #ifdef CONFIG_ARCH_OMAP2
> static struct omap_dm_timer omap2_dm_timers[] = {
> - { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1 },
> - { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2 },
> - { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3 },
> - { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4 },
> - { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5 },
> - { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6 },
> - { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7 },
> - { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8 },
> - { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9 },
> - { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10 },
> - { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11 },
> - { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12 },
> + { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1,
> + .ms_correction = 0 },
> + { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2,
> + .ms_correction = 0 },
> + { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3,
> + .ms_correction = 0 },
> + { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4,
> + .ms_correction = 0 },
> + { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5,
> + .ms_correction = 0 },
> + { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6,
> + .ms_correction = 0 },
> + { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7,
> + .ms_correction = 0 },
> + { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8,
> + .ms_correction = 0 },
> + { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9,
> + .ms_correction = 0 },
> + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
> + .ms_correction = 0 },
> + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
> + .ms_correction = 0 },
> + { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12,
> + .ms_correction = 0 },
ignore of previous comments on .ms_correction initialization.
try the output of the following code:
struct b {
unsigned char z:1;
unsigned char a:1;
};
static struct b b1[2]={
{.z =1},
{.a = 1},
};
unsigned int main()
{
int i;
for (i=0; i<2; i++)
printf("%d: %d %d\n", i, b1[i].z, b1[i].a);
}
> };
>
> static const char *omap2_dm_source_names[] __initdata = {
> @@ -218,18 +233,30 @@ 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 = 0x49034000, .irq = INT_24XX_GPTIMER3 },
> - { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4 },
> - { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5 },
> - { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6 },
> - { .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 = 0x48088000, .irq = INT_24XX_GPTIMER11 },
> - { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ },
> + { .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,
> + .ms_correction = 0 },
> + { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4,
> + .ms_correction = 0 },
> + { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5,
> + .ms_correction = 0 },
> + { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6,
> + .ms_correction = 0 },
> + { .phys_base = 0x4903C000, .irq = INT_24XX_GPTIMER7,
> + .ms_correction = 0 },
> + { .phys_base = 0x4903E001, .irq = INT_24XX_GPTIMER8,
> + .ms_correction = 0 },
> + { .phys_base = 0x49040000, .irq = INT_24XX_GPTIMER9,
> + .ms_correction = 0 },
> + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
> + .ms_correction = 1 },
> + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
> + .ms_correction = 0 },
> + { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ,
> +
NAK
.ms_correction = 0 },
> };
>
> static const char *omap3_dm_source_names[] __initdata = {
> @@ -250,18 +277,30 @@ 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 = 0x48034000, .irq = OMAP44XX_IRQ_GPT3 },
> - { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4 },
> - { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5 },
> - { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6 },
> - { .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 = 0x48088000, .irq = OMAP44XX_IRQ_GPT11 },
> - { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12 },
> + { .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,
> + .ms_correction = 0 },
> + { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4,
> + .ms_correction = 0 },
> + { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5,
> + .ms_correction = 0 },
> + { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6,
> + .ms_correction = 0 },
> + { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT7,
> + .ms_correction = 0 },
> + { .phys_base = 0x4013e000, .irq = OMAP44XX_IRQ_GPT8,
> + .ms_correction = 0 },
> + { .phys_base = 0x4803e000, .irq = OMAP44XX_IRQ_GPT9,
> + .ms_correction = 0 },
> + { .phys_base = 0x48086000, .irq = OMAP44XX_IRQ_GPT10,
> + .ms_correction = 1 },
> + { .phys_base = 0x48088000, .irq = OMAP44XX_IRQ_GPT11,
> + .ms_correction = 0 },
> + { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12,
> +
.ms_correction = 0 },
NAK
> };
> static const char *omap4_dm_source_names[] __initdata = {
> "sys_clkin_ck",
> @@ -612,6 +651,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;
> @@ -733,6 +776,22 @@ int omap_dm_timers_active(void)
> }
> EXPORT_SYMBOL_GPL(omap_dm_timers_active);
>
> +/**
> + * 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
> + */
> +void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32 load)
does this function need to be exposed to the world? why?
omap_dm_timer_set_load_start seems to automatically use it. and
omap_dm_timer_set_load_start is private within dmtimer.c
> +{
> + int pos_increment, neg_increment;
> + unsigned int count = (0xFFFFFFFF - load)*1024;
> +
> + pos_increment = (((count/1000)+1)*1000000) - (count*1000);
> + neg_increment = ((count/1000)*1000000) - (count*1000);
NAK. comments from last time not fixed! - fix check patch AND use
kernel.h ROUND functions.
> + 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);
> +}
> +
> int __init omap_dm_timer_init(void)
> {
> struct omap_dm_timer *timer;
> diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
> index 20f1054..ad37bcc 100644
> --- a/arch/arm/plat-omap/include/plat/dmtimer.h
> +++ b/arch/arm/plat-omap/include/plat/dmtimer.h
> @@ -80,5 +80,6 @@ void omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value
>
> int omap_dm_timers_active(void);
>
> +void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32 load);
as mentioned offline: why does this function need to be exposed to the
world?
>
> #endif /* __ASM_ARCH_DMTIMER_H */
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 10:44 ` Nishanth Menon
@ 2010-06-21 12:51 ` DebBarma, Tarun Kanti
2010-06-21 12:56 ` Nishanth Menon
2010-06-21 14:21 ` David Brownell
1 sibling, 1 reply; 11+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-06-21 12:51 UTC (permalink / raw)
To: menon.nishant@gmail.com; +Cc: linux-omap@vger.kernel.org, R, Sricharan
Nishant,
> -----Original Message-----
> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
> Sent: Monday, June 21, 2010 4:15 PM
> To: DebBarma, Tarun Kanti
> Cc: linux-omap@vger.kernel.org; R, Sricharan
> Subject: Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
>
> NAK - my prev comments are not fixed here either.
>
> On 06/21/2010 03:23 PM, Tarun Kanti DebBarma wrote:
> > 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:
> move this to diffstat section please.
> > (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
> >
> > Tested on Zoom3 using Linus tree.
> >
> > 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 | 131
> +++++++++++++++++++++--------
> > arch/arm/plat-omap/include/plat/dmtimer.h | 1 +
> > 2 files changed, 96 insertions(+), 36 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> > index c64875f..42344a7
> > --- a/arch/arm/plat-omap/dmtimer.c
> > +++ b/arch/arm/plat-omap/dmtimer.c
> > @@ -160,6 +160,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;
> > @@ -185,18 +188,30 @@ static const int omap1_dm_timer_count =
> ARRAY_SIZE(omap1_dm_timers);
> >
> > #ifdef CONFIG_ARCH_OMAP2
> > static struct omap_dm_timer omap2_dm_timers[] = {
> > - { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1 },
> > - { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2 },
> > - { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3 },
> > - { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4 },
> > - { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5 },
> > - { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6 },
> > - { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7 },
> > - { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8 },
> > - { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9 },
> > - { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10 },
> > - { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11 },
> > - { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12 },
> > + { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12,
> > + .ms_correction = 0 },
>
> ignore of previous comments on .ms_correction initialization.
> try the output of the following code:
> struct b {
> unsigned char z:1;
> unsigned char a:1;
> };
>
> static struct b b1[2]={
> {.z =1},
> {.a = 1},
> };
>
> unsigned int main()
> {
> int i;
> for (i=0; i<2; i++)
> printf("%d: %d %d\n", i, b1[i].z, b1[i].a);
> }
This comment is not clear to me.
Are you suggesting to make the initialization of ms_correction variable
in a separate function instead of doing the present way?
>
> > };
> >
> > static const char *omap2_dm_source_names[] __initdata = {
> > @@ -218,18 +233,30 @@ 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 = 0x49034000, .irq = INT_24XX_GPTIMER3 },
> > - { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4 },
> > - { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5 },
> > - { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6 },
> > - { .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 = 0x48088000, .irq = INT_24XX_GPTIMER11 },
> > - { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ },
> > + { .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,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4903C000, .irq = INT_24XX_GPTIMER7,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4903E001, .irq = INT_24XX_GPTIMER8,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x49040000, .irq = INT_24XX_GPTIMER9,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
> > + .ms_correction = 1 },
> > + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ,
> > +
>
> NAK
Will be taken care.
> .ms_correction = 0 },
> > };
> >
> > static const char *omap3_dm_source_names[] __initdata = {
> > @@ -250,18 +277,30 @@ 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 = 0x48034000, .irq = OMAP44XX_IRQ_GPT3 },
> > - { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4 },
> > - { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5 },
> > - { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6 },
> > - { .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 = 0x48088000, .irq = OMAP44XX_IRQ_GPT11 },
> > - { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12 },
> > + { .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,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT7,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4013e000, .irq = OMAP44XX_IRQ_GPT8,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4803e000, .irq = OMAP44XX_IRQ_GPT9,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x48086000, .irq = OMAP44XX_IRQ_GPT10,
> > + .ms_correction = 1 },
> > + { .phys_base = 0x48088000, .irq = OMAP44XX_IRQ_GPT11,
> > + .ms_correction = 0 },
> > + { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12,
> > +
> .ms_correction = 0 },
> NAK
Will be taken care.
>
> > };
> > static const char *omap4_dm_source_names[] __initdata = {
> > "sys_clkin_ck",
> > @@ -612,6 +651,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;
> > @@ -733,6 +776,22 @@ int omap_dm_timers_active(void)
> > }
> > EXPORT_SYMBOL_GPL(omap_dm_timers_active);
> >
> > +/**
> > + * 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
> > + */
> > +void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32 load)
> does this function need to be exposed to the world? why?
> omap_dm_timer_set_load_start seems to automatically use it. and
> omap_dm_timer_set_load_start is private within dmtimer.c
This will be changed to static void omap_dm_timer_ms_correction (...)
>
> > +{
> > + int pos_increment, neg_increment;
> > + unsigned int count = (0xFFFFFFFF - load)*1024;
> > +
> > + pos_increment = (((count/1000)+1)*1000000) - (count*1000);
> > + neg_increment = ((count/1000)*1000000) - (count*1000);
>
> NAK. comments from last time not fixed! - fix check patch AND use
> kernel.h ROUND functions.
I did not quite understand the comment.
With regard to using round() macro in kernel.h is there any advantage?
>
> > + 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);
> > +}
> > +
> > int __init omap_dm_timer_init(void)
> > {
> > struct omap_dm_timer *timer;
> > diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-
> omap/include/plat/dmtimer.h
> > index 20f1054..ad37bcc 100644
> > --- a/arch/arm/plat-omap/include/plat/dmtimer.h
> > +++ b/arch/arm/plat-omap/include/plat/dmtimer.h
> > @@ -80,5 +80,6 @@ void omap_dm_timer_write_counter(struct omap_dm_timer
> *timer, unsigned int value
> >
> > int omap_dm_timers_active(void);
> >
> > +void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32
> load);
> as mentioned offline: why does this function need to be exposed to the
> world?
This will be removed.
>
> >
> > #endif /* __ASM_ARCH_DMTIMER_H */
>
>
> Regards,
> Nishanth Menon
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 12:51 ` DebBarma, Tarun Kanti
@ 2010-06-21 12:56 ` Nishanth Menon
2010-06-21 13:19 ` DebBarma, Tarun Kanti
0 siblings, 1 reply; 11+ messages in thread
From: Nishanth Menon @ 2010-06-21 12:56 UTC (permalink / raw)
To: DebBarma, Tarun Kanti; +Cc: linux-omap@vger.kernel.org, R, Sricharan
DebBarma, Tarun Kanti had written, on 06/21/2010 07:51 AM, the following:
> Nishant,
>
>> -----Original Message-----
>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
>> Sent: Monday, June 21, 2010 4:15 PM
>> To: DebBarma, Tarun Kanti
>> Cc: linux-omap@vger.kernel.org; R, Sricharan
>> Subject: Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
>>
>> NAK - my prev comments are not fixed here either.
>>
>> On 06/21/2010 03:23 PM, Tarun Kanti DebBarma wrote:
>>> 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:
>> move this to diffstat section please.
>>> (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
>>>
>>> Tested on Zoom3 using Linus tree.
>>>
>>> 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 | 131
>> +++++++++++++++++++++--------
>>> arch/arm/plat-omap/include/plat/dmtimer.h | 1 +
>>> 2 files changed, 96 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
>>> index c64875f..42344a7
>>> --- a/arch/arm/plat-omap/dmtimer.c
>>> +++ b/arch/arm/plat-omap/dmtimer.c
>>> @@ -160,6 +160,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;
>>> @@ -185,18 +188,30 @@ static const int omap1_dm_timer_count =
>> ARRAY_SIZE(omap1_dm_timers);
>>> #ifdef CONFIG_ARCH_OMAP2
>>> static struct omap_dm_timer omap2_dm_timers[] = {
>>> - { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1 },
>>> - { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2 },
>>> - { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3 },
>>> - { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4 },
>>> - { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5 },
>>> - { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6 },
>>> - { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7 },
>>> - { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8 },
>>> - { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9 },
>>> - { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10 },
>>> - { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11 },
>>> - { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12 },
>>> + { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12,
>>> + .ms_correction = 0 },
>> ignore of previous comments on .ms_correction initialization.
>> try the output of the following code:
>> struct b {
>> unsigned char z:1;
>> unsigned char a:1;
>> };
>>
>> static struct b b1[2]={
>> {.z =1},
>> {.a = 1},
>> };
>>
>> unsigned int main()
>> {
>> int i;
>> for (i=0; i<2; i++)
>> printf("%d: %d %d\n", i, b1[i].z, b1[i].a);
>> }
> This comment is not clear to me.
> Are you suggesting to make the initialization of ms_correction variable
> in a separate function instead of doing the present way?
no. my suggestion is this:
you dont need to do .ms_correction = 0
when the variable is static - by C standards static variables are
initialized to 0. so, .ms_correction = 1 alone is needed in the specific
GPTimer cases where this is needed.
the example i posted shows that it works :).
>
>>> };
>>>
>>> static const char *omap2_dm_source_names[] __initdata = {
>>> @@ -218,18 +233,30 @@ 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 = 0x49034000, .irq = INT_24XX_GPTIMER3 },
>>> - { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4 },
>>> - { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5 },
>>> - { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6 },
>>> - { .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 = 0x48088000, .irq = INT_24XX_GPTIMER11 },
>>> - { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ },
>>> + { .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,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4903C000, .irq = INT_24XX_GPTIMER7,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4903E001, .irq = INT_24XX_GPTIMER8,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x49040000, .irq = INT_24XX_GPTIMER9,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
>>> + .ms_correction = 1 },
>>> + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ,
>>> +
>> NAK
> Will be taken care.
>> .ms_correction = 0 },
>>> };
>>>
>>> static const char *omap3_dm_source_names[] __initdata = {
>>> @@ -250,18 +277,30 @@ 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 = 0x48034000, .irq = OMAP44XX_IRQ_GPT3 },
>>> - { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4 },
>>> - { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5 },
>>> - { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6 },
>>> - { .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 = 0x48088000, .irq = OMAP44XX_IRQ_GPT11 },
>>> - { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12 },
>>> + { .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,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT7,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4013e000, .irq = OMAP44XX_IRQ_GPT8,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4803e000, .irq = OMAP44XX_IRQ_GPT9,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x48086000, .irq = OMAP44XX_IRQ_GPT10,
>>> + .ms_correction = 1 },
>>> + { .phys_base = 0x48088000, .irq = OMAP44XX_IRQ_GPT11,
>>> + .ms_correction = 0 },
>>> + { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12,
>>> +
>> .ms_correction = 0 },
>> NAK
> Will be taken care.
>>> };
>>> static const char *omap4_dm_source_names[] __initdata = {
>>> "sys_clkin_ck",
>>> @@ -612,6 +651,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;
>>> @@ -733,6 +776,22 @@ int omap_dm_timers_active(void)
>>> }
>>> EXPORT_SYMBOL_GPL(omap_dm_timers_active);
>>>
>>> +/**
>>> + * 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
>>> + */
>>> +void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32 load)
>> does this function need to be exposed to the world? why?
>> omap_dm_timer_set_load_start seems to automatically use it. and
>> omap_dm_timer_set_load_start is private within dmtimer.c
> This will be changed to static void omap_dm_timer_ms_correction (...)
>
>>> +{
>>> + int pos_increment, neg_increment;
>>> + unsigned int count = (0xFFFFFFFF - load)*1024;
>>> +
>>> + pos_increment = (((count/1000)+1)*1000000) - (count*1000);
>>> + neg_increment = ((count/1000)*1000000) - (count*1000);
>> NAK. comments from last time not fixed! - fix check patch AND use
>> kernel.h ROUND functions.
> I did not quite understand the comment.
> With regard to using round() macro in kernel.h is there any advantage?
yes, ROUND macros are safe from a perspective of overflow and truncation
- further this is a standard mechanism used elsewhere in the kernel. so
why define your own macros?
Also scripts/checkpatch.pl --strict has not been run on this patch, you
definitely need space between operators such as / and * ..
>
>>> + 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);
>>> +}
>>> +
>>> int __init omap_dm_timer_init(void)
>>> {
>>> struct omap_dm_timer *timer;
>>> diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-
>> omap/include/plat/dmtimer.h
>>> index 20f1054..ad37bcc 100644
>>> --- a/arch/arm/plat-omap/include/plat/dmtimer.h
>>> +++ b/arch/arm/plat-omap/include/plat/dmtimer.h
>>> @@ -80,5 +80,6 @@ void omap_dm_timer_write_counter(struct omap_dm_timer
>> *timer, unsigned int value
>>> int omap_dm_timers_active(void);
>>>
>>> +void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32
>> load);
>> as mentioned offline: why does this function need to be exposed to the
>> world?
> This will be removed.
>>> #endif /* __ASM_ARCH_DMTIMER_H */
>>
>> Regards,
>> Nishanth Menon
> --
> 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
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 12:56 ` Nishanth Menon
@ 2010-06-21 13:19 ` DebBarma, Tarun Kanti
2010-06-21 13:26 ` Nishanth Menon
0 siblings, 1 reply; 11+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-06-21 13:19 UTC (permalink / raw)
To: Menon, Nishanth; +Cc: linux-omap@vger.kernel.org, R, Sricharan
> -----Original Message-----
> From: Menon, Nishanth
> Sent: Monday, June 21, 2010 6:27 PM
> To: DebBarma, Tarun Kanti
> Cc: linux-omap@vger.kernel.org; R, Sricharan
> Subject: Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
>
> DebBarma, Tarun Kanti had written, on 06/21/2010 07:51 AM, the following:
> > Nishant,
> >
> >> -----Original Message-----
> >> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
> >> Sent: Monday, June 21, 2010 4:15 PM
> >> To: DebBarma, Tarun Kanti
> >> Cc: linux-omap@vger.kernel.org; R, Sricharan
> >> Subject: Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
> >>
> >> NAK - my prev comments are not fixed here either.
> >>
> >> On 06/21/2010 03:23 PM, Tarun Kanti DebBarma wrote:
> >>> 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:
> >> move this to diffstat section please.
> >>> (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
> >>>
> >>> Tested on Zoom3 using Linus tree.
> >>>
> >>> 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 | 131
> >> +++++++++++++++++++++--------
> >>> arch/arm/plat-omap/include/plat/dmtimer.h | 1 +
> >>> 2 files changed, 96 insertions(+), 36 deletions(-)
> >>>
> >>> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-
> omap/dmtimer.c
> >>> index c64875f..42344a7
> >>> --- a/arch/arm/plat-omap/dmtimer.c
> >>> +++ b/arch/arm/plat-omap/dmtimer.c
> >>> @@ -160,6 +160,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;
> >>> @@ -185,18 +188,30 @@ static const int omap1_dm_timer_count =
> >> ARRAY_SIZE(omap1_dm_timers);
> >>> #ifdef CONFIG_ARCH_OMAP2
> >>> static struct omap_dm_timer omap2_dm_timers[] = {
> >>> - { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1 },
> >>> - { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2 },
> >>> - { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3 },
> >>> - { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4 },
> >>> - { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5 },
> >>> - { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6 },
> >>> - { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7 },
> >>> - { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8 },
> >>> - { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9 },
> >>> - { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10 },
> >>> - { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11 },
> >>> - { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12 },
> >>> + { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12,
> >>> + .ms_correction = 0 },
> >> ignore of previous comments on .ms_correction initialization.
> >> try the output of the following code:
> >> struct b {
> >> unsigned char z:1;
> >> unsigned char a:1;
> >> };
> >>
> >> static struct b b1[2]={
> >> {.z =1},
> >> {.a = 1},
> >> };
> >>
> >> unsigned int main()
> >> {
> >> int i;
> >> for (i=0; i<2; i++)
> >> printf("%d: %d %d\n", i, b1[i].z, b1[i].a);
> >> }
> > This comment is not clear to me.
> > Are you suggesting to make the initialization of ms_correction variable
> > in a separate function instead of doing the present way?
>
> no. my suggestion is this:
> you dont need to do .ms_correction = 0
> when the variable is static - by C standards static variables are
> initialized to 0. so, .ms_correction = 1 alone is needed in the specific
> GPTimer cases where this is needed.
>
> the example i posted shows that it works :).
>
OK.
But in this case I have to re-design / re-structure the other bit fields
Viz. reserved, enabled, posted. Then we have to decide what to name the structure, etc. If you feel this is the right direction then I can go ahead.
> >
> >>> };
> >>>
> >>> static const char *omap2_dm_source_names[] __initdata = {
> >>> @@ -218,18 +233,30 @@ 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 = 0x49034000, .irq = INT_24XX_GPTIMER3 },
> >>> - { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4 },
> >>> - { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5 },
> >>> - { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6 },
> >>> - { .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 = 0x48088000, .irq = INT_24XX_GPTIMER11 },
> >>> - { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ },
> >>> + { .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,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4903C000, .irq = INT_24XX_GPTIMER7,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4903E001, .irq = INT_24XX_GPTIMER8,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x49040000, .irq = INT_24XX_GPTIMER9,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
> >>> + .ms_correction = 1 },
> >>> + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ,
> >>> +
> >> NAK
> > Will be taken care.
> >> .ms_correction = 0 },
> >>> };
> >>>
> >>> static const char *omap3_dm_source_names[] __initdata = {
> >>> @@ -250,18 +277,30 @@ 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 = 0x48034000, .irq = OMAP44XX_IRQ_GPT3 },
> >>> - { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4 },
> >>> - { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5 },
> >>> - { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6 },
> >>> - { .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 = 0x48088000, .irq = OMAP44XX_IRQ_GPT11 },
> >>> - { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12 },
> >>> + { .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,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT7,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4013e000, .irq = OMAP44XX_IRQ_GPT8,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4803e000, .irq = OMAP44XX_IRQ_GPT9,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x48086000, .irq = OMAP44XX_IRQ_GPT10,
> >>> + .ms_correction = 1 },
> >>> + { .phys_base = 0x48088000, .irq = OMAP44XX_IRQ_GPT11,
> >>> + .ms_correction = 0 },
> >>> + { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12,
> >>> +
> >> .ms_correction = 0 },
> >> NAK
> > Will be taken care.
> >>> };
> >>> static const char *omap4_dm_source_names[] __initdata = {
> >>> "sys_clkin_ck",
> >>> @@ -612,6 +651,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;
> >>> @@ -733,6 +776,22 @@ int omap_dm_timers_active(void)
> >>> }
> >>> EXPORT_SYMBOL_GPL(omap_dm_timers_active);
> >>>
> >>> +/**
> >>> + * 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
> >>> + */
> >>> +void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32
> load)
> >> does this function need to be exposed to the world? why?
> >> omap_dm_timer_set_load_start seems to automatically use it. and
> >> omap_dm_timer_set_load_start is private within dmtimer.c
> > This will be changed to static void omap_dm_timer_ms_correction (...)
> >
> >>> +{
> >>> + int pos_increment, neg_increment;
> >>> + unsigned int count = (0xFFFFFFFF - load)*1024;
> >>> +
> >>> + pos_increment = (((count/1000)+1)*1000000) - (count*1000);
> >>> + neg_increment = ((count/1000)*1000000) - (count*1000);
> >> NAK. comments from last time not fixed! - fix check patch AND use
> >> kernel.h ROUND functions.
> > I did not quite understand the comment.
> > With regard to using round() macro in kernel.h is there any advantage?
>
> yes, ROUND macros are safe from a perspective of overflow and truncation
> - further this is a standard mechanism used elsewhere in the kernel. so
> why define your own macros?
>
> Also scripts/checkpatch.pl --strict has not been run on this patch, you
> definitely need space between operators such as / and * ..
>
Unfortunately "scripts/checkpatch.pl --strict" did not give me this error/warning. Anyways, I will incorporate this.
> >
> >>> + 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);
> >>> +}
> >>> +
> >>> int __init omap_dm_timer_init(void)
> >>> {
> >>> struct omap_dm_timer *timer;
> >>> diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h
> b/arch/arm/plat-
> >> omap/include/plat/dmtimer.h
> >>> index 20f1054..ad37bcc 100644
> >>> --- a/arch/arm/plat-omap/include/plat/dmtimer.h
> >>> +++ b/arch/arm/plat-omap/include/plat/dmtimer.h
> >>> @@ -80,5 +80,6 @@ void omap_dm_timer_write_counter(struct
> omap_dm_timer
> >> *timer, unsigned int value
> >>> int omap_dm_timers_active(void);
> >>>
> >>> +void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32
> >> load);
> >> as mentioned offline: why does this function need to be exposed to the
> >> world?
> > This will be removed.
> >>> #endif /* __ASM_ARCH_DMTIMER_H */
> >>
> >> Regards,
> >> Nishanth Menon
> > --
> > 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
>
>
> --
> Regards,
> Nishanth Menon
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 13:19 ` DebBarma, Tarun Kanti
@ 2010-06-21 13:26 ` Nishanth Menon
2010-06-21 13:33 ` DebBarma, Tarun Kanti
0 siblings, 1 reply; 11+ messages in thread
From: Nishanth Menon @ 2010-06-21 13:26 UTC (permalink / raw)
To: DebBarma, Tarun Kanti; +Cc: linux-omap@vger.kernel.org, R, Sricharan
DebBarma, Tarun Kanti had written, on 06/21/2010 08:19 AM, the following:
[...]
>>>>> ---
>>>>> arch/arm/plat-omap/dmtimer.c | 131
>>>> +++++++++++++++++++++--------
>>>>> arch/arm/plat-omap/include/plat/dmtimer.h | 1 +
>>>>> 2 files changed, 96 insertions(+), 36 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-
>> omap/dmtimer.c
>>>>> index c64875f..42344a7
>>>>> --- a/arch/arm/plat-omap/dmtimer.c
>>>>> +++ b/arch/arm/plat-omap/dmtimer.c
>>>>> @@ -160,6 +160,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;
>>>>> @@ -185,18 +188,30 @@ static const int omap1_dm_timer_count =
>>>> ARRAY_SIZE(omap1_dm_timers);
>>>>> #ifdef CONFIG_ARCH_OMAP2
>>>>> static struct omap_dm_timer omap2_dm_timers[] = {
>>>>> - { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1 },
>>>>> - { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2 },
>>>>> - { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3 },
>>>>> - { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4 },
>>>>> - { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5 },
>>>>> - { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6 },
>>>>> - { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7 },
>>>>> - { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8 },
>>>>> - { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9 },
>>>>> - { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10 },
>>>>> - { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11 },
>>>>> - { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12 },
>>>>> + { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
>>>>> + .ms_correction = 0 },
>>>>> + { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12,
>>>>> + .ms_correction = 0 },
>>>> ignore of previous comments on .ms_correction initialization.
>>>> try the output of the following code:
>>>> struct b {
>>>> unsigned char z:1;
>>>> unsigned char a:1;
>>>> };
>>>>
>>>> static struct b b1[2]={
>>>> {.z =1},
>>>> {.a = 1},
>>>> };
>>>>
>>>> unsigned int main()
>>>> {
>>>> int i;
>>>> for (i=0; i<2; i++)
>>>> printf("%d: %d %d\n", i, b1[i].z, b1[i].a);
>>>> }
>>> This comment is not clear to me.
>>> Are you suggesting to make the initialization of ms_correction variable
>>> in a separate function instead of doing the present way?
>> no. my suggestion is this:
>> you dont need to do .ms_correction = 0
>> when the variable is static - by C standards static variables are
>> initialized to 0. so, .ms_correction = 1 alone is needed in the specific
>> GPTimer cases where this is needed.
>>
>> the example i posted shows that it works :).
>>
> OK.
> But in this case I have to re-design / re-structure the other bit fields
> Viz. reserved, enabled, posted. Then we have to decide what to name the structure, etc. If you feel this is the right direction then I can go ahead.
My only complaint is with the unwanted initialization.
[...]
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 13:26 ` Nishanth Menon
@ 2010-06-21 13:33 ` DebBarma, Tarun Kanti
0 siblings, 0 replies; 11+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-06-21 13:33 UTC (permalink / raw)
To: Menon, Nishanth; +Cc: linux-omap@vger.kernel.org, R, Sricharan
> -----Original Message-----
> From: Menon, Nishanth
> Sent: Monday, June 21, 2010 6:56 PM
> To: DebBarma, Tarun Kanti
> Cc: linux-omap@vger.kernel.org; R, Sricharan
> Subject: Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
>
> DebBarma, Tarun Kanti had written, on 06/21/2010 08:19 AM, the following:
> [...]
> >>>>> ---
> >>>>> arch/arm/plat-omap/dmtimer.c | 131
> >>>> +++++++++++++++++++++--------
> >>>>> arch/arm/plat-omap/include/plat/dmtimer.h | 1 +
> >>>>> 2 files changed, 96 insertions(+), 36 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-
> >> omap/dmtimer.c
> >>>>> index c64875f..42344a7
> >>>>> --- a/arch/arm/plat-omap/dmtimer.c
> >>>>> +++ b/arch/arm/plat-omap/dmtimer.c
> >>>>> @@ -160,6 +160,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;
> >>>>> @@ -185,18 +188,30 @@ static const int omap1_dm_timer_count =
> >>>> ARRAY_SIZE(omap1_dm_timers);
> >>>>> #ifdef CONFIG_ARCH_OMAP2
> >>>>> static struct omap_dm_timer omap2_dm_timers[] = {
> >>>>> - { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1 },
> >>>>> - { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2 },
> >>>>> - { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3 },
> >>>>> - { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4 },
> >>>>> - { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5 },
> >>>>> - { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6 },
> >>>>> - { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7 },
> >>>>> - { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8 },
> >>>>> - { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9 },
> >>>>> - { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10 },
> >>>>> - { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11 },
> >>>>> - { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12 },
> >>>>> + { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
> >>>>> + .ms_correction = 0 },
> >>>>> + { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12,
> >>>>> + .ms_correction = 0 },
> >>>> ignore of previous comments on .ms_correction initialization.
> >>>> try the output of the following code:
> >>>> struct b {
> >>>> unsigned char z:1;
> >>>> unsigned char a:1;
> >>>> };
> >>>>
> >>>> static struct b b1[2]={
> >>>> {.z =1},
> >>>> {.a = 1},
> >>>> };
> >>>>
> >>>> unsigned int main()
> >>>> {
> >>>> int i;
> >>>> for (i=0; i<2; i++)
> >>>> printf("%d: %d %d\n", i, b1[i].z, b1[i].a);
> >>>> }
> >>> This comment is not clear to me.
> >>> Are you suggesting to make the initialization of ms_correction
> variable
> >>> in a separate function instead of doing the present way?
> >> no. my suggestion is this:
> >> you dont need to do .ms_correction = 0
> >> when the variable is static - by C standards static variables are
> >> initialized to 0. so, .ms_correction = 1 alone is needed in the
> specific
> >> GPTimer cases where this is needed.
> >>
> >> the example i posted shows that it works :).
> >>
> > OK.
> > But in this case I have to re-design / re-structure the other bit fields
> > Viz. reserved, enabled, posted. Then we have to decide what to name the
> structure, etc. If you feel this is the right direction then I can go
> ahead.
> My only complaint is with the unwanted initialization.
>
> [...]
OK, I understand now!
>
> --
> Regards,
> Nishanth Menon
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 10:44 ` Nishanth Menon
2010-06-21 12:51 ` DebBarma, Tarun Kanti
@ 2010-06-21 14:21 ` David Brownell
2010-06-21 14:30 ` DebBarma, Tarun Kanti
1 sibling, 1 reply; 11+ messages in thread
From: David Brownell @ 2010-06-21 14:21 UTC (permalink / raw)
To: Tarun Kanti DebBarma, Nishanth Menon; +Cc: linux-omap, R Sricharan
Doesn't the generic clock code have logic
to handle such rounding issues? I'm pretty
sure I remember dealing with that issue for
32K timers on AT91 processors. If so that
means the setup is done wrong, and is fixable.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 14:21 ` David Brownell
@ 2010-06-21 14:30 ` DebBarma, Tarun Kanti
0 siblings, 0 replies; 11+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-06-21 14:30 UTC (permalink / raw)
To: David Brownell, Nishanth Menon; +Cc: linux-omap@vger.kernel.org, R, Sricharan
David,
> -----Original Message-----
> From: David Brownell [mailto:david-b@pacbell.net]
> Sent: Monday, June 21, 2010 7:51 PM
> To: DebBarma, Tarun Kanti; Nishanth Menon
> Cc: linux-omap@vger.kernel.org; R, Sricharan
> Subject: Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
>
> Doesn't the generic clock code have logic
> to handle such rounding issues? I'm pretty
> sure I remember dealing with that issue for
> 32K timers on AT91 processors. If so that
> means the setup is done wrong, and is fixable.
>
I am not sure. But let me have look again.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 20:23 [PATCH v2] OMAP:GPTIMER:1ms tick generation correction Tarun Kanti DebBarma
2010-06-21 10:44 ` Nishanth Menon
@ 2010-06-21 15:54 ` Kevin Hilman
2010-06-22 3:38 ` DebBarma, Tarun Kanti
1 sibling, 1 reply; 11+ messages in thread
From: Kevin Hilman @ 2010-06-21 15:54 UTC (permalink / raw)
To: Tarun Kanti DebBarma; +Cc: linux-omap, R Sricharan
Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
> 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
>
> Tested on Zoom3 using Linus tree.
>
> Signed-off-by: R Sricharan <r.sricharan@ti.com>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
Please coordinate this change with the GPTIMER hwmod changes. The timers
that have/need this feature should be flagged at the hwmod level (probably by
a dev_attr flag.)
Kevin
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
@ 2010-06-21 20:23 Tarun Kanti DebBarma
2010-06-21 10:44 ` Nishanth Menon
2010-06-21 15:54 ` Kevin Hilman
0 siblings, 2 replies; 11+ messages in thread
From: Tarun Kanti DebBarma @ 2010-06-21 20:23 UTC (permalink / raw)
To: linux-omap; +Cc: Tarun Kanti DebBarma, R Sricharan
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
Tested on Zoom3 using Linus tree.
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 | 131 +++++++++++++++++++++--------
arch/arm/plat-omap/include/plat/dmtimer.h | 1 +
2 files changed, 96 insertions(+), 36 deletions(-)
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index c64875f..42344a7
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -160,6 +160,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;
@@ -185,18 +188,30 @@ static const int omap1_dm_timer_count = ARRAY_SIZE(omap1_dm_timers);
#ifdef CONFIG_ARCH_OMAP2
static struct omap_dm_timer omap2_dm_timers[] = {
- { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1 },
- { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2 },
- { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3 },
- { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4 },
- { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5 },
- { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6 },
- { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7 },
- { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8 },
- { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9 },
- { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10 },
- { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11 },
- { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12 },
+ { .phys_base = 0x48028000, .irq = INT_24XX_GPTIMER1,
+ .ms_correction = 0 },
+ { .phys_base = 0x4802a000, .irq = INT_24XX_GPTIMER2,
+ .ms_correction = 0 },
+ { .phys_base = 0x48078000, .irq = INT_24XX_GPTIMER3,
+ .ms_correction = 0 },
+ { .phys_base = 0x4807a000, .irq = INT_24XX_GPTIMER4,
+ .ms_correction = 0 },
+ { .phys_base = 0x4807c000, .irq = INT_24XX_GPTIMER5,
+ .ms_correction = 0 },
+ { .phys_base = 0x4807e000, .irq = INT_24XX_GPTIMER6,
+ .ms_correction = 0 },
+ { .phys_base = 0x48080000, .irq = INT_24XX_GPTIMER7,
+ .ms_correction = 0 },
+ { .phys_base = 0x48082000, .irq = INT_24XX_GPTIMER8,
+ .ms_correction = 0 },
+ { .phys_base = 0x48084000, .irq = INT_24XX_GPTIMER9,
+ .ms_correction = 0 },
+ { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
+ .ms_correction = 0 },
+ { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
+ .ms_correction = 0 },
+ { .phys_base = 0x4808a000, .irq = INT_24XX_GPTIMER12,
+ .ms_correction = 0 },
};
static const char *omap2_dm_source_names[] __initdata = {
@@ -218,18 +233,30 @@ 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 = 0x49034000, .irq = INT_24XX_GPTIMER3 },
- { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4 },
- { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5 },
- { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6 },
- { .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 = 0x48088000, .irq = INT_24XX_GPTIMER11 },
- { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ },
+ { .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,
+ .ms_correction = 0 },
+ { .phys_base = 0x49036000, .irq = INT_24XX_GPTIMER4,
+ .ms_correction = 0 },
+ { .phys_base = 0x49038000, .irq = INT_24XX_GPTIMER5,
+ .ms_correction = 0 },
+ { .phys_base = 0x4903A000, .irq = INT_24XX_GPTIMER6,
+ .ms_correction = 0 },
+ { .phys_base = 0x4903C000, .irq = INT_24XX_GPTIMER7,
+ .ms_correction = 0 },
+ { .phys_base = 0x4903E001, .irq = INT_24XX_GPTIMER8,
+ .ms_correction = 0 },
+ { .phys_base = 0x49040000, .irq = INT_24XX_GPTIMER9,
+ .ms_correction = 0 },
+ { .phys_base = 0x48086000, .irq = INT_24XX_GPTIMER10,
+ .ms_correction = 1 },
+ { .phys_base = 0x48088000, .irq = INT_24XX_GPTIMER11,
+ .ms_correction = 0 },
+ { .phys_base = 0x48304000, .irq = INT_34XX_GPT12_IRQ,
+ .ms_correction = 0 },
};
static const char *omap3_dm_source_names[] __initdata = {
@@ -250,18 +277,30 @@ 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 = 0x48034000, .irq = OMAP44XX_IRQ_GPT3 },
- { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4 },
- { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5 },
- { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6 },
- { .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 = 0x48088000, .irq = OMAP44XX_IRQ_GPT11 },
- { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12 },
+ { .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,
+ .ms_correction = 0 },
+ { .phys_base = 0x48036000, .irq = OMAP44XX_IRQ_GPT4,
+ .ms_correction = 0 },
+ { .phys_base = 0x40138000, .irq = OMAP44XX_IRQ_GPT5,
+ .ms_correction = 0 },
+ { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT6,
+ .ms_correction = 0 },
+ { .phys_base = 0x4013a000, .irq = OMAP44XX_IRQ_GPT7,
+ .ms_correction = 0 },
+ { .phys_base = 0x4013e000, .irq = OMAP44XX_IRQ_GPT8,
+ .ms_correction = 0 },
+ { .phys_base = 0x4803e000, .irq = OMAP44XX_IRQ_GPT9,
+ .ms_correction = 0 },
+ { .phys_base = 0x48086000, .irq = OMAP44XX_IRQ_GPT10,
+ .ms_correction = 1 },
+ { .phys_base = 0x48088000, .irq = OMAP44XX_IRQ_GPT11,
+ .ms_correction = 0 },
+ { .phys_base = 0x4a320000, .irq = OMAP44XX_IRQ_GPT12,
+ .ms_correction = 0 },
};
static const char *omap4_dm_source_names[] __initdata = {
"sys_clkin_ck",
@@ -612,6 +651,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;
@@ -733,6 +776,22 @@ int omap_dm_timers_active(void)
}
EXPORT_SYMBOL_GPL(omap_dm_timers_active);
+/**
+ * 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
+ */
+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 = (((count/1000)+1)*1000000) - (count*1000);
+ neg_increment = ((count/1000)*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);
+}
+
int __init omap_dm_timer_init(void)
{
struct omap_dm_timer *timer;
diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
index 20f1054..ad37bcc 100644
--- a/arch/arm/plat-omap/include/plat/dmtimer.h
+++ b/arch/arm/plat-omap/include/plat/dmtimer.h
@@ -80,5 +80,6 @@ void omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value
int omap_dm_timers_active(void);
+void omap_dm_timer_ms_correction(struct omap_dm_timer *timer, u32 load);
#endif /* __ASM_ARCH_DMTIMER_H */
--
1.6.0.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
2010-06-21 15:54 ` Kevin Hilman
@ 2010-06-22 3:38 ` DebBarma, Tarun Kanti
0 siblings, 0 replies; 11+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-06-22 3:38 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap@vger.kernel.org, R, Sricharan
Kevin,
> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Monday, June 21, 2010 9:24 PM
> To: DebBarma, Tarun Kanti
> Cc: linux-omap@vger.kernel.org; R, Sricharan
> Subject: Re: [PATCH v2] OMAP:GPTIMER:1ms tick generation correction
>
> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>
> > 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
> >
> > Tested on Zoom3 using Linus tree.
> >
> > Signed-off-by: R Sricharan <r.sricharan@ti.com>
> > Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>
> Please coordinate this change with the GPTIMER hwmod changes. The timers
> that have/need this feature should be flagged at the hwmod level (probably
> by
> a dev_attr flag.)
OK!
>
> Kevin
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-06-22 3:38 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-21 20:23 [PATCH v2] OMAP:GPTIMER:1ms tick generation correction Tarun Kanti DebBarma
2010-06-21 10:44 ` Nishanth Menon
2010-06-21 12:51 ` DebBarma, Tarun Kanti
2010-06-21 12:56 ` Nishanth Menon
2010-06-21 13:19 ` DebBarma, Tarun Kanti
2010-06-21 13:26 ` Nishanth Menon
2010-06-21 13:33 ` DebBarma, Tarun Kanti
2010-06-21 14:21 ` David Brownell
2010-06-21 14:30 ` DebBarma, Tarun Kanti
2010-06-21 15:54 ` Kevin Hilman
2010-06-22 3:38 ` DebBarma, Tarun Kanti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox