[PATCHv3,6/9] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device

Commit Message

Dave Gerlach Aug. 6, 2013, 5:49 p.m. UTC

From: Vaibhav Bedia <[e-mail]>

OMAP timer code registers two timers - one as clocksource
and one as clockevent. Since AM33XX has only one usable timer
in the WKUP domain one of the timers needs suspend-resume
support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume
of clock event devices) introduced .suspend and .resume
callbacks for clock event devices. Leverages these
callbacks to have AM33XX clockevent timer which is
in not in WKUP domain to behave properly across system
suspend.

Signed-off-by: Vaibhav Bedia <[e-mail]>
Signed-off-by: Dave Gerlach <[e-mail]>
---
 arch/arm/mach-omap2/timer.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Russ Dill Aug. 8, 2013, 7:03 a.m. UTC | #1

On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach <[e-mail]> wrote:
> From: Vaibhav Bedia <[e-mail]>
>
> OMAP timer code registers two timers - one as clocksource
> and one as clockevent. Since AM33XX has only one usable timer
> in the WKUP domain one of the timers needs suspend-resume
> support to restore the configuration to pre-suspend state.
>
> commit adc78e6 (timekeeping: Add suspend and resume
> of clock event devices) introduced .suspend and .resume
> callbacks for clock event devices. Leverages these
> callbacks to have AM33XX clockevent timer which is
> in not in WKUP domain to behave properly across system
> suspend.
>
> Signed-off-by: Vaibhav Bedia <[e-mail]>
> Signed-off-by: Dave Gerlach <[e-mail]>

Reviewed-by: Russ Dill <[e-mail]>

> ---
>  arch/arm/mach-omap2/timer.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index b37e1fc..cce5d39 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>         }
>  }
>
> +static void omap_clkevt_suspend(struct clock_event_device *unused)
> +{
> +       char name[10];
> +       struct omap_hwmod *oh;
> +
> +       sprintf(name, "timer%d", clkev.id);
> +       oh = omap_hwmod_lookup(name);
> +       if (!oh)
> +               return;
> +
> +       __omap_dm_timer_stop(&clkev, 1, clkev.rate);
> +       omap_hwmod_idle(oh);
> +}
> +
> +static void omap_clkevt_resume(struct clock_event_device *unused)
> +{
> +       char name[10];
> +       struct omap_hwmod *oh;
> +
> +       sprintf(name, "timer%d", clkev.id);
> +       oh = omap_hwmod_lookup(name);
> +       if (!oh)
> +               return;
> +
> +       omap_hwmod_enable(oh);
> +       __omap_dm_timer_load_start(&clkev,
> +                       OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
> +       __omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
> +}
> +
>  static struct clock_event_device clockevent_gpt = {
>         .features       = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
>         .rating         = 300,
>         .set_next_event = omap2_gp_timer_set_next_event,
>         .set_mode       = omap2_gp_timer_set_mode,
> +       .suspend        = omap_clkevt_suspend,
> +       .resume         = omap_clkevt_resume,
>  };
>
>  static struct property device_disabled = {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Santosh Shilimkar Aug. 8, 2013, 2:23 p.m. UTC | #2

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia <[e-mail]>
> 
> OMAP timer code registers two timers - one as clocksource
> and one as clockevent. Since AM33XX has only one usable timer
> in the WKUP domain one of the timers needs suspend-resume
> support to restore the configuration to pre-suspend state.
> 
> commit adc78e6 (timekeeping: Add suspend and resume
> of clock event devices) introduced .suspend and .resume
> callbacks for clock event devices. Leverages these
> callbacks to have AM33XX clockevent timer which is
> in not in WKUP domain to behave properly across system
> suspend.
> 
> Signed-off-by: Vaibhav Bedia <[e-mail]>
> Signed-off-by: Dave Gerlach <[e-mail]>
> ---
NAK

This patch doesn't addressed previous comments.
- The issue is specific to AM33XX and hence you
need to take care of that. These callbacks will happen on
all OMAP machines where the problem doesn't exist.

- Don't use hwmod APIs directly. At least abstract it
at omap_device layer and use that one instead.

>  arch/arm/mach-omap2/timer.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index b37e1fc..cce5d39 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>  	}
>  }
>  
> +static void omap_clkevt_suspend(struct clock_event_device *unused)
> +{
> +	char name[10];
> +	struct omap_hwmod *oh;
> +
> +	sprintf(name, "timer%d", clkev.id);
> +	oh = omap_hwmod_lookup(name);
> +	if (!oh)
> +		return;
> +
> +	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
> +	omap_hwmod_idle(oh);
> +}
> +
> +static void omap_clkevt_resume(struct clock_event_device *unused)
> +{
> +	char name[10];
> +	struct omap_hwmod *oh;
> +
> +	sprintf(name, "timer%d", clkev.id);
> +	oh = omap_hwmod_lookup(name);
> +	if (!oh)
> +		return;
> +
> +	omap_hwmod_enable(oh);
> +	__omap_dm_timer_load_start(&clkev,
> +			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
> +	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
> +}
> +
>  static struct clock_event_device clockevent_gpt = {
>  	.features       = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
>  	.rating		= 300,
>  	.set_next_event	= omap2_gp_timer_set_next_event,
>  	.set_mode	= omap2_gp_timer_set_mode,
> +	.suspend	= omap_clkevt_suspend,
> +	.resume		= omap_clkevt_resume,
>  };
>  
>  static struct property device_disabled = {
>

Dave Gerlach Aug. 8, 2013, 4:09 p.m. UTC | #3

On 08/08/2013 09:23 AM, Santosh Shilimkar wrote:
> On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
>> From: Vaibhav Bedia <[e-mail]>
>>
>> OMAP timer code registers two timers - one as clocksource
>> and one as clockevent. Since AM33XX has only one usable timer
>> in the WKUP domain one of the timers needs suspend-resume
>> support to restore the configuration to pre-suspend state.
>>
>> commit adc78e6 (timekeeping: Add suspend and resume
>> of clock event devices) introduced .suspend and .resume
>> callbacks for clock event devices. Leverages these
>> callbacks to have AM33XX clockevent timer which is
>> in not in WKUP domain to behave properly across system
>> suspend.
>>
>> Signed-off-by: Vaibhav Bedia <[e-mail]>
>> Signed-off-by: Dave Gerlach <[e-mail]>
>> ---
> NAK
>
> This patch doesn't addressed previous comments.
> - The issue is specific to AM33XX and hence you
> need to take care of that. These callbacks will happen on
> all OMAP machines where the problem doesn't exist.
>
> - Don't use hwmod APIs directly. At least abstract it
> at omap_device layer and use that one instead.
>

Ok I will fix this, seems I missed those.

>>   arch/arm/mach-omap2/timer.c |   32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>> index b37e1fc..cce5d39 100644
>> --- a/arch/arm/mach-omap2/timer.c
>> +++ b/arch/arm/mach-omap2/timer.c
>> @@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>>   	}
>>   }
>>
>> +static void omap_clkevt_suspend(struct clock_event_device *unused)
>> +{
>> +	char name[10];
>> +	struct omap_hwmod *oh;
>> +
>> +	sprintf(name, "timer%d", clkev.id);
>> +	oh = omap_hwmod_lookup(name);
>> +	if (!oh)
>> +		return;
>> +
>> +	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
>> +	omap_hwmod_idle(oh);
>> +}
>> +
>> +static void omap_clkevt_resume(struct clock_event_device *unused)
>> +{
>> +	char name[10];
>> +	struct omap_hwmod *oh;
>> +
>> +	sprintf(name, "timer%d", clkev.id);
>> +	oh = omap_hwmod_lookup(name);
>> +	if (!oh)
>> +		return;
>> +
>> +	omap_hwmod_enable(oh);
>> +	__omap_dm_timer_load_start(&clkev,
>> +			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
>> +	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
>> +}
>> +
>>   static struct clock_event_device clockevent_gpt = {
>>   	.features       = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
>>   	.rating		= 300,
>>   	.set_next_event	= omap2_gp_timer_set_next_event,
>>   	.set_mode	= omap2_gp_timer_set_mode,
>> +	.suspend	= omap_clkevt_suspend,
>> +	.resume		= omap_clkevt_resume,
>>   };
>>
>>   static struct property device_disabled = {
>>
>

Kevin Hilman Aug. 8, 2013, 6:25 p.m. UTC | #4

Dave Gerlach <[e-mail]> writes:

> From: Vaibhav Bedia <[e-mail]>
>
> OMAP timer code registers two timers - one as clocksource
> and one as clockevent. Since AM33XX has only one usable timer
> in the WKUP domain one of the timers needs suspend-resume
> support to restore the configuration to pre-suspend state.
>
> commit adc78e6 (timekeeping: Add suspend and resume
> of clock event devices) introduced .suspend and .resume
> callbacks for clock event devices. Leverages these
> callbacks to have AM33XX clockevent timer which is
> in not in WKUP domain to behave properly across system
> suspend.
>
> Signed-off-by: Vaibhav Bedia <[e-mail]>
> Signed-off-by: Dave Gerlach <[e-mail]>
> ---
>  arch/arm/mach-omap2/timer.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index b37e1fc..cce5d39 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>  	}
>  }
>  
> +static void omap_clkevt_suspend(struct clock_event_device *unused)
> +{
> +	char name[10];
> +	struct omap_hwmod *oh;
> +
> +	sprintf(name, "timer%d", clkev.id);
> +	oh = omap_hwmod_lookup(name);

/me stops reviewing here.  This should be a one-time thing.

Seeing things *again* in patches that I've already reviewed (multiple
times) is very frustrating.  It also increases the likelihood of future
patches to be "filtered."  (in this case, you get a pass since you seem
to have inherited Vaibhav's code, but please take care to address all
reviewer comments, or at least explain why you didn'.)

Kevin

Dave Gerlach Aug. 8, 2013, 7:49 p.m. UTC | #5

On 08/08/2013 01:25 PM, Kevin Hilman wrote:
> Dave Gerlach <[e-mail]> writes:
>
>> From: Vaibhav Bedia <[e-mail]>
>>
>> OMAP timer code registers two timers - one as clocksource
>> and one as clockevent. Since AM33XX has only one usable timer
>> in the WKUP domain one of the timers needs suspend-resume
>> support to restore the configuration to pre-suspend state.
>>
>> commit adc78e6 (timekeeping: Add suspend and resume
>> of clock event devices) introduced .suspend and .resume
>> callbacks for clock event devices. Leverages these
>> callbacks to have AM33XX clockevent timer which is
>> in not in WKUP domain to behave properly across system
>> suspend.
>>
>> Signed-off-by: Vaibhav Bedia <[e-mail]>
>> Signed-off-by: Dave Gerlach <[e-mail]>
>> ---
>>   arch/arm/mach-omap2/timer.c |   32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>> index b37e1fc..cce5d39 100644
>> --- a/arch/arm/mach-omap2/timer.c
>> +++ b/arch/arm/mach-omap2/timer.c
>> @@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>>   	}
>>   }
>>
>> +static void omap_clkevt_suspend(struct clock_event_device *unused)
>> +{
>> +	char name[10];
>> +	struct omap_hwmod *oh;
>> +
>> +	sprintf(name, "timer%d", clkev.id);
>> +	oh = omap_hwmod_lookup(name);
>
> /me stops reviewing here.  This should be a one-time thing.
>
> Seeing things *again* in patches that I've already reviewed (multiple
> times) is very frustrating.  It also increases the likelihood of future
> patches to be "filtered."  (in this case, you get a pass since you seem
> to have inherited Vaibhav's code, but please take care to address all
> reviewer comments, or at least explain why you didn'.)
>
> Kevin
>

Sorry for missing this. Seems I missed this patch completely when taking 
over. I'll make sure it's addressed in the next version.

Regards,
Dave

Patch

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index b37e1fc..cce5d39 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -118,11 +118,43 @@  static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
 	}
 }
 
+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	omap_hwmod_enable(oh);
+	__omap_dm_timer_load_start(&clkev,
+			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
+	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
+}
+
 static struct clock_event_device clockevent_gpt = {
 	.features       = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
 	.rating		= 300,
 	.set_next_event	= omap2_gp_timer_set_next_event,
 	.set_mode	= omap2_gp_timer_set_mode,
+	.suspend	= omap_clkevt_suspend,
+	.resume		= omap_clkevt_resume,
 };
 
 static struct property device_disabled = {