Nishant,
> -----Original Message-----
> From: Nishanth Menon [mailto:[email protected]]
> Sent: Monday, June 21, 2010 4:15 PM
> To: DebBarma, Tarun Kanti
> Cc: [email protected]; 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<[email protected]>
> > Signed-off-by: Tarun Kanti DebBarma<[email protected]>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
