From: Tony Lindgren <tony@atomide.com> To: Dave Gerlach <d-gerlach@ti.com> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>, linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org, Paul Walmsley <paul@pwsan.com>, Kevin Hilman <khilman@linaro.org>, Tero Kristo <t-kristo@ti.com>, Nishanth Menon <nm@ti.com>, Russ Dill <russ.dill@ti.com>, Daniel Mack <daniel@zonque.org>, Suman Anna <s-anna@ti.com>, Benoit Cousson <bcousson@baylibre.com>, Vaibhav Bedia <vaibhav.bedia@ti.com> Subject: Re: [PATCH v4 03/11] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device Date: Mon, 14 Jul 2014 23:48:08 -0700 [thread overview] Message-ID: <20140715064807.GJ20068@atomide.com> (raw) In-Reply-To: <53C4166F.1050003@ti.com> * Dave Gerlach <d-gerlach@ti.com> [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 <d-gerlach@ti.com> [140710 19:59]: > >>>From: Vaibhav Bedia <vaibhav.bedia@ti.com> > >>> > >>>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 <vaibhav.bedia@ti.com> > >>>Signed-off-by: Dave Gerlach <d-gerlach@ti.com> > >>>--- > >>>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
WARNING: multiple messages have this Message-ID (diff)
From: tony@atomide.com (Tony Lindgren) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v4 03/11] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device Date: Mon, 14 Jul 2014 23:48:08 -0700 [thread overview] Message-ID: <20140715064807.GJ20068@atomide.com> (raw) In-Reply-To: <53C4166F.1050003@ti.com> * Dave Gerlach <d-gerlach@ti.com> [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 <d-gerlach@ti.com> [140710 19:59]: > >>>From: Vaibhav Bedia <vaibhav.bedia@ti.com> > >>> > >>>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 <vaibhav.bedia@ti.com> > >>>Signed-off-by: Dave Gerlach <d-gerlach@ti.com> > >>>--- > >>>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
next prev parent reply other threads:[~2014-07-15 6:49 UTC|newest] Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-07-11 2:55 [PATCH v4 00/11] ARM: OMAP2+: AM33XX: Add suspend-resume support Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-11 2:55 ` [PATCH v4 01/11] ARM: omap: edma: add suspend suspend/resume hooks Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-14 11:05 ` Tony Lindgren 2014-07-14 11:05 ` Tony Lindgren 2014-07-14 17:47 ` Dave Gerlach 2014-07-14 17:47 ` Dave Gerlach 2014-07-11 2:55 ` [PATCH v4 02/11] memory: emif: Move EMIF register defines to include/linux/ Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-14 11:12 ` Tony Lindgren 2014-07-14 11:12 ` Tony Lindgren 2014-07-14 17:42 ` Dave Gerlach 2014-07-14 17:42 ` Dave Gerlach 2014-07-15 6:38 ` Tony Lindgren 2014-07-15 6:38 ` Tony Lindgren 2014-07-16 2:44 ` Dave Gerlach 2014-07-16 2:44 ` Dave Gerlach 2014-07-16 8:33 ` Tony Lindgren 2014-07-16 8:33 ` Tony Lindgren 2014-07-11 2:55 ` [PATCH v4 03/11] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-14 11:15 ` Tony Lindgren 2014-07-14 11:15 ` Tony Lindgren 2014-07-14 14:37 ` Santosh Shilimkar 2014-07-14 14:37 ` Santosh Shilimkar 2014-07-14 17:42 ` Dave Gerlach 2014-07-14 17:42 ` Dave Gerlach 2014-07-15 6:48 ` Tony Lindgren [this message] 2014-07-15 6:48 ` Tony Lindgren 2014-07-15 19:10 ` Dave Gerlach 2014-07-15 19:10 ` Dave Gerlach 2014-07-16 20:17 ` Dave Gerlach 2014-07-16 20:17 ` Dave Gerlach 2014-07-17 8:16 ` Tony Lindgren 2014-07-17 8:16 ` Tony Lindgren 2014-07-11 2:55 ` [PATCH v4 04/11] ARM: OMAP2+: Use pdata-quirks for wkup_m3 deassert_hardreset Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-14 18:48 ` Suman Anna 2014-07-14 18:48 ` Suman Anna 2014-07-11 2:55 ` [PATCH v4 05/11] Documentation: dt: add ti,am3353_wkup_m3 bindings Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-14 14:41 ` Santosh Shilimkar 2014-07-14 14:41 ` [PATCH v4 05/11] Documentation: dt: add ti, am3353_wkup_m3 bindings Santosh Shilimkar 2014-07-14 16:33 ` [PATCH v4 05/11] Documentation: dt: add ti,am3353_wkup_m3 bindings Suman Anna 2014-07-14 16:33 ` [PATCH v4 05/11] Documentation: dt: add ti, am3353_wkup_m3 bindings Suman Anna 2014-07-14 17:45 ` [PATCH v4 05/11] Documentation: dt: add ti,am3353_wkup_m3 bindings Dave Gerlach 2014-07-14 17:45 ` [PATCH v4 05/11] Documentation: dt: add ti, am3353_wkup_m3 bindings Dave Gerlach 2014-07-11 2:55 ` [PATCH v4 06/11] remoteproc: wkup_m3: Add wkup_m3 remote proc driver Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-14 14:54 ` Santosh Shilimkar 2014-07-14 14:54 ` Santosh Shilimkar 2014-07-14 17:43 ` Dave Gerlach 2014-07-14 17:43 ` Dave Gerlach 2014-07-14 19:07 ` Suman Anna 2014-07-14 19:07 ` Suman Anna 2014-07-14 21:09 ` Dave Gerlach 2014-07-14 21:09 ` Dave Gerlach 2014-07-11 2:55 ` [PATCH v4 07/11] ARM: dts: am33xx: Update wkup_m3 node Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-11 2:55 ` [PATCH v4 08/11] ARM: OMAP2+: AM33XX: Reserve memory to comply with EMIF spec Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-11 2:55 ` [PATCH v4 09/11] ARM: OMAP2+: AM33XX: Add assembly code for PM operations Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-11 2:55 ` [PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-09-08 22:30 ` Kevin Hilman 2014-09-08 22:30 ` Kevin Hilman 2014-09-09 10:31 ` Ohad Ben-Cohen 2014-09-09 10:31 ` Ohad Ben-Cohen 2014-09-09 19:59 ` Suman Anna 2014-09-09 19:59 ` Suman Anna 2014-09-09 20:28 ` Dave Gerlach 2014-09-09 20:28 ` Dave Gerlach 2014-09-09 21:10 ` Kevin Hilman 2014-09-09 21:10 ` Kevin Hilman 2014-09-10 21:19 ` Dave Gerlach 2014-09-10 21:19 ` Dave Gerlach 2014-09-16 15:08 ` Ohad Ben-Cohen 2014-09-16 15:08 ` Ohad Ben-Cohen 2014-09-16 16:14 ` Suman Anna 2014-09-16 16:14 ` Suman Anna 2014-09-17 13:37 ` Ohad Ben-Cohen 2014-09-17 13:37 ` Ohad Ben-Cohen 2014-09-25 19:42 ` Dave Gerlach 2014-09-25 19:42 ` Dave Gerlach 2014-07-11 2:55 ` [PATCH v4 11/11] ARM: OMAP2+: AM33XX: Hookup AM33XX PM code into OMAP builds Dave Gerlach 2014-07-11 2:55 ` Dave Gerlach 2014-07-14 11:21 ` Tony Lindgren 2014-07-14 11:21 ` Tony Lindgren 2014-07-14 17:46 ` Dave Gerlach 2014-07-14 17:46 ` Dave Gerlach 2014-07-11 7:59 ` [PATCH v4 00/11] ARM: OMAP2+: AM33XX: Add suspend-resume support Daniel Mack 2014-07-11 7:59 ` Daniel Mack 2014-07-11 17:24 ` Dave Gerlach 2014-07-11 17:24 ` Dave Gerlach 2014-07-11 15:30 ` Andre Heider 2014-07-11 15:30 ` Andre Heider 2014-07-11 17:31 ` Dave Gerlach 2014-07-11 17:31 ` Dave Gerlach 2014-07-14 9:37 ` Andre Heider 2014-07-14 9:37 ` Andre Heider 2014-07-14 11:24 ` Tony Lindgren 2014-07-14 11:24 ` Tony Lindgren 2014-07-14 17:47 ` Dave Gerlach 2014-07-14 17:47 ` Dave Gerlach
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140715064807.GJ20068@atomide.com \ --to=tony@atomide.com \ --cc=bcousson@baylibre.com \ --cc=d-gerlach@ti.com \ --cc=daniel@zonque.org \ --cc=khilman@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-omap@vger.kernel.org \ --cc=nm@ti.com \ --cc=paul@pwsan.com \ --cc=russ.dill@ti.com \ --cc=s-anna@ti.com \ --cc=santosh.shilimkar@ti.com \ --cc=t-kristo@ti.com \ --cc=vaibhav.bedia@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.