[PATCH 02/10] ARM: OMAP: Fix timer posted mode support

[Hiremath, Vaibhav]

On Thu, Sep 06, 2012 at 21:31:21, Hunter, Jon wrote:
> 
> On 09/06/2012 09:20 AM, Jon Hunter wrote:
> > 
> > On 09/06/2012 07:57 AM, Vaibhav Hiremath wrote:
> >>
> >>
> >> On 9/6/2012 12:34 AM, Jon Hunter wrote:
> >>> Currently the dmtimer posted mode is being enabled when the function
> >>> __omap_dm_timer_reset() is called. This function is only being called for
> >>> OMAP1 timers and OMAP2+ timers that are being used as system timers. Hence,
> >>> for OMAP2+ timers that are NOT being used as a system timer, posted mode is
> >>> not enabled but the "timer->posted" variable is still set (incorrectly) in
> >>> the omap_dm_timer_prepare() function.
> >>>
> >>> This is a regression introduced by commit 3392cdd3 (ARM: OMAP: dmtimer:
> >>> switch-over to platform device driver) which changed the code to only call
> >>> omap_dm_timer_reset() for OMAP1 devices. Although this is a regression from
> >>> the original code it only impacts performance and so is not needed for stable.
> >>>
> >>> Signed-off-by: Jon Hunter <jon-hunter at ti.com>
> >>> ---
> >>>  arch/arm/mach-omap2/timer.c               |    3 +--
> >>>  arch/arm/plat-omap/dmtimer.c              |   14 +++++---------
> >>>  arch/arm/plat-omap/include/plat/dmtimer.h |    9 ++++++++-
> >>>  3 files changed, 14 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> >>> index 5471706..e24ee0f 100644
> >>> --- a/arch/arm/mach-omap2/timer.c
> >>> +++ b/arch/arm/mach-omap2/timer.c
> >>> @@ -194,10 +194,9 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
> >>>  	}
> >>>  	__omap_dm_timer_init_regs(timer);
> >>>  	__omap_dm_timer_reset(timer, 1, 1);
> >>> -	timer->posted = 1;
> >>> +	__omap_dm_timer_enable_posted(timer);
> >>>  
> >>>  	timer->rate = clk_get_rate(timer->fclk);
> >>> -
> >>>  	timer->reserved = 1;
> >>>  
> >>>  	return res;
> >>> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> >>> index c34f55b..22790ea 100644
> >>> --- a/arch/arm/plat-omap/dmtimer.c
> >>> +++ b/arch/arm/plat-omap/dmtimer.c
> >>> @@ -122,21 +122,15 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
> >>>  
> >>>  static void omap_dm_timer_reset(struct omap_dm_timer *timer)
> >>>  {
> >>> -	omap_dm_timer_enable(timer);
> >>>  	if (timer->pdev->id != 1) {
> >>>  		omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
> >>>  		omap_dm_timer_wait_for_reset(timer);
> >>>  	}
> >>> -
> >>>  	__omap_dm_timer_reset(timer, 0, 0);
> >>> -	omap_dm_timer_disable(timer);
> >>> -	timer->posted = 1;
> >>>  }
> >>>  
> >>>  int omap_dm_timer_prepare(struct omap_dm_timer *timer)
> >>>  {
> >>> -	int ret;
> >>> -
> >>>  	/*
> >>>  	 * FIXME: OMAP1 devices do not use the clock framework for dmtimers so
> >>>  	 * do not call clk_get() for these devices.
> >>> @@ -150,13 +144,15 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
> >>>  		}
> >>>  	}
> >>>  
> >>> +	omap_dm_timer_enable(timer);
> >>> +
> >>>  	if (timer->capability & OMAP_TIMER_NEEDS_RESET)
> >>>  		omap_dm_timer_reset(timer);
> >>>  
> >>> -	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> >>> +	__omap_dm_timer_enable_posted(timer);
> >>> +	omap_dm_timer_disable(timer);
> >>>  
> >>> -	timer->posted = 1;
> >>> -	return ret;
> >>> +	return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> >>
> >> May be I am speculating here and I know this is tested and supposed to
> >> work, but Isn't it safe to set parent keeping module enables.
> > 
> > So you would rather change the functional clock while the timer is
> > enabled/active? So although that we are doing this today, that does not
> > sound like a good idea to me :-)
> 
> Actually, we are not doing this today. If you look at the current code
> we are only enabling the timer while doing the soft-reset for omap1
> devices. Hence, even in the current code we set the parent while the
> timer is not enabled. So there is no actual change here in the sequence.
> 

Yes, you are absolutely right here. As such there is no change in the 
sequence.

Thanks,
Vaibhav