[v4,03/11] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device

Commit Message

Dave Gerlach July 11, 2014, 2:55 a.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]>
---
v3->v4:
	Only use for am33xx soc now.

 arch/arm/mach-omap2/timer.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Tony Lindgren July 14, 2014, 11:15 a.m. UTC | #1

* Dave Gerlach <[e-mail]> [140710 19:59]:
> 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]>
> ---
> v3->v4:
> 	Only use for am33xx soc now.
> 
>  arch/arm/mach-omap2/timer.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index 43d03fb..6fc1748 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -128,6 +128,29 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>  	}
>  }
>  
> +static void omap_clkevt_suspend(struct clock_event_device *unused)
> +{
> +	struct omap_hwmod *oh;
> +
> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
> +	if (!oh)
> +		return;
> +
> +	omap_hwmod_idle(oh);
> +}
> +
> +static void omap_clkevt_resume(struct clock_event_device *unused)
> +{
> +	struct omap_hwmod *oh;
> +
> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
> +	if (!oh)
> +		return;
> +
> +	omap_hwmod_enable(oh);
> +	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
> +}
> +

This is going to make moving the timer code into drivers one step
tougher to do. And you don't need to look up the hwmod entry every
time, just initialize it during the init.

> +	if (soc_is_am33xx()) {
> +		clockevent_gpt.suspend = omap_clkevt_suspend;
> +		clockevent_gpt.resume = omap_clkevt_resume;
> +	}
> +

Maybe try to set up things so we initialize the SoC specific
timer suspend and resume functions in mach-omap2/timer.c
in a way where eventually the device driver can easily use
them?

Regards,

Tony
--
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 July 14, 2014, 2:37 p.m. UTC | #2

On Monday 14 July 2014 07:15 AM, Tony Lindgren wrote:
> * Dave Gerlach <[e-mail]> [140710 19:59]:
>> 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]>
>> ---
>> v3->v4:
>> 	Only use for am33xx soc now.
>>
>>  arch/arm/mach-omap2/timer.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>> index 43d03fb..6fc1748 100644
>> --- a/arch/arm/mach-omap2/timer.c
>> +++ b/arch/arm/mach-omap2/timer.c
>> @@ -128,6 +128,29 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>>  	}
>>  }
>>  
>> +static void omap_clkevt_suspend(struct clock_event_device *unused)
>> +{
>> +	struct omap_hwmod *oh;
>> +
>> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
>> +	if (!oh)
>> +		return;
>> +
>> +	omap_hwmod_idle(oh);
>> +}
>> +
>> +static void omap_clkevt_resume(struct clock_event_device *unused)
>> +{
>> +	struct omap_hwmod *oh;
>> +
>> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
>> +	if (!oh)
>> +		return;
>> +
>> +	omap_hwmod_enable(oh);
>> +	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
>> +}
>> +
> 
> This is going to make moving the timer code into drivers one step
> tougher to do. And you don't need to look up the hwmod entry every
> time, just initialize it during the init.
> 
>> +	if (soc_is_am33xx()) {
>> +		clockevent_gpt.suspend = omap_clkevt_suspend;
>> +		clockevent_gpt.resume = omap_clkevt_resume;
>> +	}
>> +
> 
> Maybe try to set up things so we initialize the SoC specific
> timer suspend and resume functions in mach-omap2/timer.c
> in a way where eventually the device driver can easily use
> them?
> 
+1. I had similar comments on the previous version too.

Regards,
Santosh

--
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

Dave Gerlach July 14, 2014, 5:42 p.m. UTC | #3

Santosh, Tony,

On 07/14/2014 09:37 AM, Santosh Shilimkar wrote:
> On Monday 14 July 2014 07:15 AM, Tony Lindgren wrote:
>> * Dave Gerlach <[e-mail]> [140710 19:59]:
>>> 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]>
>>> ---
>>> v3->v4:
>>> 	Only use for am33xx soc now.
>>>
>>>   arch/arm/mach-omap2/timer.c | 28 ++++++++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>>> index 43d03fb..6fc1748 100644
>>> --- a/arch/arm/mach-omap2/timer.c
>>> +++ b/arch/arm/mach-omap2/timer.c
>>> @@ -128,6 +128,29 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>>>   	}
>>>   }
>>>
>>> +static void omap_clkevt_suspend(struct clock_event_device *unused)
>>> +{
>>> +	struct omap_hwmod *oh;
>>> +
>>> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
>>> +	if (!oh)
>>> +		return;
>>> +
>>> +	omap_hwmod_idle(oh);
>>> +}
>>> +
>>> +static void omap_clkevt_resume(struct clock_event_device *unused)
>>> +{
>>> +	struct omap_hwmod *oh;
>>> +
>>> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
>>> +	if (!oh)
>>> +		return;
>>> +
>>> +	omap_hwmod_enable(oh);
>>> +	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
>>> +}
>>> +
>>
>> This is going to make moving the timer code into drivers one step
>> tougher to do. And you don't need to look up the hwmod entry every
>> time, just initialize it during the init.

Yes you are right about looking up only at init I need to change that. I agree 
that this makes moving the timers harder but I'm not sure there's any way around 
this. I attempted to use the omap_device layer here but there is no device at 
all created here, it does not hook into the normal PM layer through omap_device. 
This clock must be shut off as it sits in the peripheral power domain and any 
active clock within the domain will prevent suspend from happening, so we end up 
with a platform specific issue here. It seems that the only way I can get to the 
clock is through the hwmod.

>>
>>> +	if (soc_is_am33xx()) {
>>> +		clockevent_gpt.suspend = omap_clkevt_suspend;
>>> +		clockevent_gpt.resume = omap_clkevt_resume;
>>> +	}
>>> +
>>
>> Maybe try to set up things so we initialize the SoC specific
>> timer suspend and resume functions in mach-omap2/timer.c
>> in a way where eventually the device driver can easily use
>> them?
>>
> +1. I had similar comments on the previous version too.

This was my attempt to make things specific to only am335x based on Santosh's 
previous comments as last time they were populated for every device even when 
unneeded. These are not standard suspend/resume handlers, they are specific to 
clock event. I know there will always need to be at least some code here for the 
early timer init based on previous timer cleanup series I've seen, so perhaps I 
could hook it in there when the time comes?

Regards,
Dave

>
> Regards,
> Santosh
>

--
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

Tony Lindgren July 15, 2014, 6:48 a.m. UTC | #4

* Dave Gerlach <[e-mail]> [140714 10:44]:
> Santosh, Tony,
> 
> On 07/14/2014 09:37 AM, Santosh Shilimkar wrote:
> >On Monday 14 July 2014 07:15 AM, Tony Lindgren wrote:
> >>* Dave Gerlach <[e-mail]> [140710 19:59]:
> >>>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]>
> >>>---
> >>>v3->v4:
> >>>	Only use for am33xx soc now.
> >>>
> >>>  arch/arm/mach-omap2/timer.c | 28 ++++++++++++++++++++++++++++
> >>>  1 file changed, 28 insertions(+)
> >>>
> >>>diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> >>>index 43d03fb..6fc1748 100644
> >>>--- a/arch/arm/mach-omap2/timer.c
> >>>+++ b/arch/arm/mach-omap2/timer.c
> >>>@@ -128,6 +128,29 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
> >>>  	}
> >>>  }
> >>>
> >>>+static void omap_clkevt_suspend(struct clock_event_device *unused)
> >>>+{
> >>>+	struct omap_hwmod *oh;
> >>>+
> >>>+	oh = omap_hwmod_lookup(clockevent_gpt.name);
> >>>+	if (!oh)
> >>>+		return;
> >>>+
> >>>+	omap_hwmod_idle(oh);
> >>>+}
> >>>+
> >>>+static void omap_clkevt_resume(struct clock_event_device *unused)
> >>>+{
> >>>+	struct omap_hwmod *oh;
> >>>+
> >>>+	oh = omap_hwmod_lookup(clockevent_gpt.name);
> >>>+	if (!oh)
> >>>+		return;
> >>>+
> >>>+	omap_hwmod_enable(oh);
> >>>+	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
> >>>+}
> >>>+
> >>
> >>This is going to make moving the timer code into drivers one step
> >>tougher to do. And you don't need to look up the hwmod entry every
> >>time, just initialize it during the init.
> 
> Yes you are right about looking up only at init I need to change that. I
> agree that this makes moving the timers harder but I'm not sure there's any
> way around this. I attempted to use the omap_device layer here but there is
> no device at all created here, it does not hook into the normal PM layer
> through omap_device. This clock must be shut off as it sits in the
> peripheral power domain and any active clock within the domain will prevent
> suspend from happening, so we end up with a platform specific issue here. It
> seems that the only way I can get to the clock is through the hwmod.

It's best to register these kind of functions as platform_data
in pdata-quirks.c auxdata. That way when this moves to live
in drivers/clocksource the driver does not need to know anything
about the interconnect specific registers.

Also, please don't use Linux generic names here.. Maybe use
omap_clkevt_idle/unidle? The linux suspend and resume hooks
and runtime PM could all use these functions then.

> >>>+	if (soc_is_am33xx()) {
> >>>+		clockevent_gpt.suspend = omap_clkevt_suspend;
> >>>+		clockevent_gpt.resume = omap_clkevt_resume;
> >>>+	}
> >>>+
> >>
> >>Maybe try to set up things so we initialize the SoC specific
> >>timer suspend and resume functions in mach-omap2/timer.c
> >>in a way where eventually the device driver can easily use
> >>them?
> >>
> >+1. I had similar comments on the previous version too.
> 
> This was my attempt to make things specific to only am335x based on
> Santosh's previous comments as last time they were populated for every
> device even when unneeded. These are not standard suspend/resume handlers,
> they are specific to clock event. I know there will always need to be at
> least some code here for the early timer init based on previous timer
> cleanup series I've seen, so perhaps I could hook it in there when the time
> comes?

Well just adding a minimal include/linux/platform_data/timer-omap.h
to pass those function pointers to the driver should do the trick.

Regards,

Tony
--
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

Dave Gerlach July 15, 2014, 7:10 p.m. UTC | #5

On 07/15/2014 01:48 AM, Tony Lindgren wrote:
> * Dave Gerlach <[e-mail]> [140714 10:44]:
>> Santosh, Tony,
>>
>> On 07/14/2014 09:37 AM, Santosh Shilimkar wrote:
>>> On Monday 14 July 2014 07:15 AM, Tony Lindgren wrote:
>>>> * Dave Gerlach <[e-mail]> [140710 19:59]:
>>>>> 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]>
>>>>> ---
>>>>> v3->v4:
>>>>> 	Only use for am33xx soc now.
>>>>>
>>>>>   arch/arm/mach-omap2/timer.c | 28 ++++++++++++++++++++++++++++
>>>>>   1 file changed, 28 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>>>>> index 43d03fb..6fc1748 100644
>>>>> --- a/arch/arm/mach-omap2/timer.c
>>>>> +++ b/arch/arm/mach-omap2/timer.c
>>>>> @@ -128,6 +128,29 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>>>>>   	}
>>>>>   }
>>>>>
>>>>> +static void omap_clkevt_suspend(struct clock_event_device *unused)
>>>>> +{
>>>>> +	struct omap_hwmod *oh;
>>>>> +
>>>>> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
>>>>> +	if (!oh)
>>>>> +		return;
>>>>> +
>>>>> +	omap_hwmod_idle(oh);
>>>>> +}
>>>>> +
>>>>> +static void omap_clkevt_resume(struct clock_event_device *unused)
>>>>> +{
>>>>> +	struct omap_hwmod *oh;
>>>>> +
>>>>> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
>>>>> +	if (!oh)
>>>>> +		return;
>>>>> +
>>>>> +	omap_hwmod_enable(oh);
>>>>> +	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
>>>>> +}
>>>>> +
>>>>
>>>> This is going to make moving the timer code into drivers one step
>>>> tougher to do. And you don't need to look up the hwmod entry every
>>>> time, just initialize it during the init.
>>
>> Yes you are right about looking up only at init I need to change that. I
>> agree that this makes moving the timers harder but I'm not sure there's any
>> way around this. I attempted to use the omap_device layer here but there is
>> no device at all created here, it does not hook into the normal PM layer
>> through omap_device. This clock must be shut off as it sits in the
>> peripheral power domain and any active clock within the domain will prevent
>> suspend from happening, so we end up with a platform specific issue here. It
>> seems that the only way I can get to the clock is through the hwmod.
>
> It's best to register these kind of functions as platform_data
> in pdata-quirks.c auxdata. That way when this moves to live
> in drivers/clocksource the driver does not need to know anything
> about the interconnect specific registers.
>
> Also, please don't use Linux generic names here.. Maybe use
> omap_clkevt_idle/unidle? The linux suspend and resume hooks
> and runtime PM could all use these functions then.

Ok to both of the above, I definitely like your suggestion better.

Regards,
Dave

>
>>>>> +	if (soc_is_am33xx()) {
>>>>> +		clockevent_gpt.suspend = omap_clkevt_suspend;
>>>>> +		clockevent_gpt.resume = omap_clkevt_resume;
>>>>> +	}
>>>>> +
>>>>
>>>> Maybe try to set up things so we initialize the SoC specific
>>>> timer suspend and resume functions in mach-omap2/timer.c
>>>> in a way where eventually the device driver can easily use
>>>> them?
>>>>
>>> +1. I had similar comments on the previous version too.
>>
>> This was my attempt to make things specific to only am335x based on
>> Santosh's previous comments as last time they were populated for every
>> device even when unneeded. These are not standard suspend/resume handlers,
>> they are specific to clock event. I know there will always need to be at
>> least some code here for the early timer init based on previous timer
>> cleanup series I've seen, so perhaps I could hook it in there when the time
>> comes?
>
> Well just adding a minimal include/linux/platform_data/timer-omap.h
> to pass those function pointers to the driver should do the trick.
>
> Regards,
>
> Tony
>

--
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

Dave Gerlach July 16, 2014, 8:17 p.m. UTC | #6

On 07/15/2014 01:48 AM, Tony Lindgren wrote:
> * Dave Gerlach <[e-mail]> [140714 10:44]:
>> Santosh, Tony,
>>
>> On 07/14/2014 09:37 AM, Santosh Shilimkar wrote:
>>> On Monday 14 July 2014 07:15 AM, Tony Lindgren wrote:
>>>> * Dave Gerlach <[e-mail]> [140710 19:59]:
>>>>> 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]>
>>>>> ---
>>>>> v3->v4:
>>>>> 	Only use for am33xx soc now.
>>>>>
>>>>>  arch/arm/mach-omap2/timer.c | 28 ++++++++++++++++++++++++++++
>>>>>  1 file changed, 28 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>>>>> index 43d03fb..6fc1748 100644
>>>>> --- a/arch/arm/mach-omap2/timer.c
>>>>> +++ b/arch/arm/mach-omap2/timer.c
>>>>> @@ -128,6 +128,29 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
>>>>>  	}
>>>>>  }
>>>>>
>>>>> +static void omap_clkevt_suspend(struct clock_event_device *unused)
>>>>> +{
>>>>> +	struct omap_hwmod *oh;
>>>>> +
>>>>> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
>>>>> +	if (!oh)
>>>>> +		return;
>>>>> +
>>>>> +	omap_hwmod_idle(oh);
>>>>> +}
>>>>> +
>>>>> +static void omap_clkevt_resume(struct clock_event_device *unused)
>>>>> +{
>>>>> +	struct omap_hwmod *oh;
>>>>> +
>>>>> +	oh = omap_hwmod_lookup(clockevent_gpt.name);
>>>>> +	if (!oh)
>>>>> +		return;
>>>>> +
>>>>> +	omap_hwmod_enable(oh);
>>>>> +	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
>>>>> +}
>>>>> +
>>>>
>>>> This is going to make moving the timer code into drivers one step
>>>> tougher to do. And you don't need to look up the hwmod entry every
>>>> time, just initialize it during the init.
>>
>> Yes you are right about looking up only at init I need to change that. I
>> agree that this makes moving the timers harder but I'm not sure there's any
>> way around this. I attempted to use the omap_device layer here but there is
>> no device at all created here, it does not hook into the normal PM layer
>> through omap_device. This clock must be shut off as it sits in the
>> peripheral power domain and any active clock within the domain will prevent
>> suspend from happening, so we end up with a platform specific issue here. It
>> seems that the only way I can get to the clock is through the hwmod.
> 
> It's best to register these kind of functions as platform_data
> in pdata-quirks.c auxdata. That way when this moves to live
> in drivers/clocksource the driver does not need to know anything
> about the interconnect specific registers.

Perhaps I spoke too soon on this, I can't use pdata-quirks right now to pass the
idle/unidle functions. The clockevent source is special and reads the dt node
itself and then marks it as disabled so nothing else can touch it. No device is
created and no auxdata matched. I will gladly fix it up once the driver is ready
to move but right now I can't use any existing functionality to do it.

Regards,
Dave

> 
> Also, please don't use Linux generic names here.. Maybe use
> omap_clkevt_idle/unidle? The linux suspend and resume hooks
> and runtime PM could all use these functions then.
> 
>>>>> +	if (soc_is_am33xx()) {
>>>>> +		clockevent_gpt.suspend = omap_clkevt_suspend;
>>>>> +		clockevent_gpt.resume = omap_clkevt_resume;
>>>>> +	}
>>>>> +
>>>>
>>>> Maybe try to set up things so we initialize the SoC specific
>>>> timer suspend and resume functions in mach-omap2/timer.c
>>>> in a way where eventually the device driver can easily use
>>>> them?
>>>>
>>> +1. I had similar comments on the previous version too.
>>
>> This was my attempt to make things specific to only am335x based on
>> Santosh's previous comments as last time they were populated for every
>> device even when unneeded. These are not standard suspend/resume handlers,
>> they are specific to clock event. I know there will always need to be at
>> least some code here for the early timer init based on previous timer
>> cleanup series I've seen, so perhaps I could hook it in there when the time
>> comes?
> 
> Well just adding a minimal include/linux/platform_data/timer-omap.h
> to pass those function pointers to the driver should do the trick.
> 
> Regards,
> 
> Tony
> 

--
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

Tony Lindgren July 17, 2014, 8:16 a.m. UTC | #7

* Dave Gerlach <[e-mail]> [140716 13:19]:
> On 07/15/2014 01:48 AM, Tony Lindgren wrote:
> > 
> > It's best to register these kind of functions as platform_data
> > in pdata-quirks.c auxdata. That way when this moves to live
> > in drivers/clocksource the driver does not need to know anything
> > about the interconnect specific registers.
> 
> Perhaps I spoke too soon on this, I can't use pdata-quirks right now to pass the
> idle/unidle functions. The clockevent source is special and reads the dt node
> itself and then marks it as disabled so nothing else can touch it. No device is
> created and no auxdata matched. I will gladly fix it up once the driver is ready
> to move but right now I can't use any existing functionality to do it.

OK. So maybe just initialize the function pointers during init and
use appropriate naming so the function pointers will stay usable
later on too once it's a driver.

Regards,

Tony
--
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

Patch

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 43d03fb..6fc1748 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -128,6 +128,29 @@  static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
 	}
 }
 
+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+	struct omap_hwmod *oh;
+
+	oh = omap_hwmod_lookup(clockevent_gpt.name);
+	if (!oh)
+		return;
+
+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	struct omap_hwmod *oh;
+
+	oh = omap_hwmod_lookup(clockevent_gpt.name);
+	if (!oh)
+		return;
+
+	omap_hwmod_enable(oh);
+	__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,
@@ -333,6 +356,11 @@  static void __init omap2_gp_clockevent_init(int gptimer_id,
 	clkev.id = gptimer_id;
 	clkev.errata = omap_dm_timer_get_errata();
 
+	if (soc_is_am33xx()) {
+		clockevent_gpt.suspend = omap_clkevt_suspend;
+		clockevent_gpt.resume = omap_clkevt_resume;
+	}
+
 	/*
 	 * For clock-event timers we never read the timer counter and
 	 * so we are not impacted by errata i103 and i767. Therefore,