| Message ID | 1356959231-17335-13-git-send-email-vaibhav.bedia@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 31 December 2012 06:37 PM, Vaibhav Bedia wrote: > The current OMAP timer code registers two timers - > one as clocksource and one as clockevent. > AM33XX has only one usable timer in the WKUP domain > so 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> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Benoit Cousson <b-cousson@ti.com> > Cc: Paul Walmsley <paul@pwsan.com> > Cc: Kevin Hilman <khilman@deeprootsystems.com> > Cc: Vaibhav Hiremath <hvaibhav@ti.com> > Cc: Jon Hunter <jon-hunter@ti.com> > --- > v1->v2: > Get rid of harcoded timer id. > Note: since a platform device is not created for these timer > instances and because there's very minimal change needed for > restarting the timer a full blown context save and restore > has been skipped. > > arch/arm/mach-omap2/timer.c | 33 +++++++++++++++++++++++++++++++++ > 1 files changed, 33 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c > index 691aa67..38f9cbc 100644 > --- a/arch/arm/mach-omap2/timer.c > +++ b/arch/arm/mach-omap2/timer.c > @@ -128,6 +128,36 @@ 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); > +} > + Am still bit uncomfortable with direct hwmod usage in the suspend/resmue hooks. Jon, Any alternatives you can think of ? Regards, Santosh
On Tue, Jan 08, 2013 at 20:45:10, Shilimkar, Santosh wrote: > On Monday 31 December 2012 06:37 PM, Vaibhav Bedia wrote: > > The current OMAP timer code registers two timers - > > one as clocksource and one as clockevent. > > AM33XX has only one usable timer in the WKUP domain > > so 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> > > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > > Cc: Benoit Cousson <b-cousson@ti.com> > > Cc: Paul Walmsley <paul@pwsan.com> > > Cc: Kevin Hilman <khilman@deeprootsystems.com> > > Cc: Vaibhav Hiremath <hvaibhav@ti.com> > > Cc: Jon Hunter <jon-hunter@ti.com> > > --- > > v1->v2: > > Get rid of harcoded timer id. > > Note: since a platform device is not created for these timer > > instances and because there's very minimal change needed for > > restarting the timer a full blown context save and restore > > has been skipped. > > > > arch/arm/mach-omap2/timer.c | 33 +++++++++++++++++++++++++++++++++ > > 1 files changed, 33 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c > > index 691aa67..38f9cbc 100644 > > --- a/arch/arm/mach-omap2/timer.c > > +++ b/arch/arm/mach-omap2/timer.c > > @@ -128,6 +128,36 @@ 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); > > +} > > + > Am still bit uncomfortable with direct hwmod usage in the suspend/resmue > hooks. > > Jon, Any alternatives you can think of ? > Jon, Any suggestions here? Regards, Vaibhav
On 12/31/2012 07:07 AM, Vaibhav Bedia wrote: > The current OMAP timer code registers two timers - > one as clocksource and one as clockevent. > AM33XX has only one usable timer in the WKUP domain > so 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> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Benoit Cousson <b-cousson@ti.com> > Cc: Paul Walmsley <paul@pwsan.com> > Cc: Kevin Hilman <khilman@deeprootsystems.com> > Cc: Vaibhav Hiremath <hvaibhav@ti.com> > Cc: Jon Hunter <jon-hunter@ti.com> > --- > v1->v2: > Get rid of harcoded timer id. > Note: since a platform device is not created for these timer > instances and because there's very minimal change needed for > restarting the timer a full blown context save and restore > has been skipped. > > arch/arm/mach-omap2/timer.c | 33 +++++++++++++++++++++++++++++++++ > 1 files changed, 33 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c > index 691aa67..38f9cbc 100644 > --- a/arch/arm/mach-omap2/timer.c > +++ b/arch/arm/mach-omap2/timer.c > @@ -128,6 +128,36 @@ 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); I am not sure you need to call __omap_dm_timer_stop() here. This should have already been called as timekeeping_suspend() will call omap2_gp_timer_set_mode() to shutdown the timer. > + 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); Similarly here, I am not sure these __omap_dm_timer_xxxx calls are needed. > +} > + > static struct clock_event_device clockevent_gpt = { > .name = "gp_timer", > .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, > @@ -135,6 +165,8 @@ static struct clock_event_device clockevent_gpt = { > .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, AFAIK, this is only applicable for AM335x devices and so should not be added for all. > }; > > static struct property device_disabled = { > @@ -323,6 +355,7 @@ static void __init omap2_gp_clockevent_init(int gptimer_id, > int res; > > clkev.errata = omap_dm_timer_get_errata(); > + clkev.id = gptimer_id; We should not use gptimer_id anymore. This will go away once the migration to dev-tree is completed. You may be better off storing the oh_name in the clock_event_device name field and passing to the suspend/resume handlers. Cheers Jon
On 01/10/2013 10:37 PM, Bedia, Vaibhav wrote: > On Tue, Jan 08, 2013 at 20:45:10, Shilimkar, Santosh wrote: >> On Monday 31 December 2012 06:37 PM, Vaibhav Bedia wrote: >>> The current OMAP timer code registers two timers - >>> one as clocksource and one as clockevent. >>> AM33XX has only one usable timer in the WKUP domain >>> so 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> >>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> >>> Cc: Benoit Cousson <b-cousson@ti.com> >>> Cc: Paul Walmsley <paul@pwsan.com> >>> Cc: Kevin Hilman <khilman@deeprootsystems.com> >>> Cc: Vaibhav Hiremath <hvaibhav@ti.com> >>> Cc: Jon Hunter <jon-hunter@ti.com> >>> --- >>> v1->v2: >>> Get rid of harcoded timer id. >>> Note: since a platform device is not created for these timer >>> instances and because there's very minimal change needed for >>> restarting the timer a full blown context save and restore >>> has been skipped. >>> >>> arch/arm/mach-omap2/timer.c | 33 +++++++++++++++++++++++++++++++++ >>> 1 files changed, 33 insertions(+), 0 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c >>> index 691aa67..38f9cbc 100644 >>> --- a/arch/arm/mach-omap2/timer.c >>> +++ b/arch/arm/mach-omap2/timer.c >>> @@ -128,6 +128,36 @@ 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); >>> +} >>> + >> Am still bit uncomfortable with direct hwmod usage in the suspend/resmue >> hooks. >> >> Jon, Any alternatives you can think of ? >> > > Jon, > > Any suggestions here? Sorry completed this missed this! Unfortunately, I don't have any good suggestions here. However, I am wondering if the suspend/resume handlers can just be calls to omap_hwmod_idle/enable and we can remove these __omap_dm_timer_xxxx calls (I don't think they are needed). Furthermore, my understanding is this is only needed for AM335x (per the changelog), and so we should not add suspend/resume handlers for all OMAP2+ based devices. Cheers Jon
On Friday 18 January 2013 12:15 AM, Jon Hunter wrote: > > On 01/10/2013 10:37 PM, Bedia, Vaibhav wrote: >> On Tue, Jan 08, 2013 at 20:45:10, Shilimkar, Santosh wrote: >>> On Monday 31 December 2012 06:37 PM, Vaibhav Bedia wrote: >>>> The current OMAP timer code registers two timers - >>>> one as clocksource and one as clockevent. >>>> AM33XX has only one usable timer in the WKUP domain >>>> so 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> >>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>> Cc: Benoit Cousson <b-cousson@ti.com> >>>> Cc: Paul Walmsley <paul@pwsan.com> >>>> Cc: Kevin Hilman <khilman@deeprootsystems.com> >>>> Cc: Vaibhav Hiremath <hvaibhav@ti.com> >>>> Cc: Jon Hunter <jon-hunter@ti.com> >>>> --- >>>> v1->v2: >>>> Get rid of harcoded timer id. >>>> Note: since a platform device is not created for these timer >>>> instances and because there's very minimal change needed for >>>> restarting the timer a full blown context save and restore >>>> has been skipped. >>>> >>>> arch/arm/mach-omap2/timer.c | 33 +++++++++++++++++++++++++++++++++ >>>> 1 files changed, 33 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c >>>> index 691aa67..38f9cbc 100644 >>>> --- a/arch/arm/mach-omap2/timer.c >>>> +++ b/arch/arm/mach-omap2/timer.c >>>> @@ -128,6 +128,36 @@ 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); >>>> +} >>>> + >>> Am still bit uncomfortable with direct hwmod usage in the suspend/resmue >>> hooks. >>> >>> Jon, Any alternatives you can think of ? >>> >> >> Jon, >> >> Any suggestions here? > > Sorry completed this missed this! > > Unfortunately, I don't have any good suggestions here. However, I am > wondering if the suspend/resume handlers can just be calls to > omap_hwmod_idle/enable and we can remove these __omap_dm_timer_xxxx > calls (I don't think they are needed). Furthermore, my understanding is > this is only needed for AM335x (per the changelog), and so we should not > add suspend/resume handlers for all OMAP2+ based devices. > I agree with the direction. Regards, Santosh
Hi Jon, On Fri, Jan 18, 2013 at 00:10:40, Hunter, Jon wrote: > > On 12/31/2012 07:07 AM, Vaibhav Bedia wrote: > > The current OMAP timer code registers two timers - > > one as clocksource and one as clockevent. > > AM33XX has only one usable timer in the WKUP domain > > so 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> > > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > > Cc: Benoit Cousson <b-cousson@ti.com> > > Cc: Paul Walmsley <paul@pwsan.com> > > Cc: Kevin Hilman <khilman@deeprootsystems.com> > > Cc: Vaibhav Hiremath <hvaibhav@ti.com> > > Cc: Jon Hunter <jon-hunter@ti.com> > > --- > > v1->v2: > > Get rid of harcoded timer id. > > Note: since a platform device is not created for these timer > > instances and because there's very minimal change needed for > > restarting the timer a full blown context save and restore > > has been skipped. > > > > arch/arm/mach-omap2/timer.c | 33 +++++++++++++++++++++++++++++++++ > > 1 files changed, 33 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c > > index 691aa67..38f9cbc 100644 > > --- a/arch/arm/mach-omap2/timer.c > > +++ b/arch/arm/mach-omap2/timer.c > > @@ -128,6 +128,36 @@ 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); > > I am not sure you need to call __omap_dm_timer_stop() here. This should > have already been called as timekeeping_suspend() will call > omap2_gp_timer_set_mode() to shutdown the timer. You are right, i can drop the call to __omap_dm_timer_stop(). > > > + 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); > > Similarly here, I am not sure these __omap_dm_timer_xxxx calls are needed. I went through the code again. __omap_dm_timer_load_start() is invoked from omap2_gp_timer_set_mode() with the auto-reload flag when setting the mode to CLOCK_EVT_MODE_PERIODIC and without the auto-reload flag in omap2_gp_timer_set_next_event(). So looks like this call can be dropped. But I do need the call to __omap_dm_timer_int_enable() to re-enable the interrupts from the timer. > > > +} > > + > > static struct clock_event_device clockevent_gpt = { > > .name = "gp_timer", > > .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, > > @@ -135,6 +165,8 @@ static struct clock_event_device clockevent_gpt = { > > .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, > > AFAIK, this is only applicable for AM335x devices and so should not be > added for all. > Agreed. Will address this in the next version. > > }; > > > > static struct property device_disabled = { > > @@ -323,6 +355,7 @@ static void __init omap2_gp_clockevent_init(int gptimer_id, > > int res; > > > > clkev.errata = omap_dm_timer_get_errata(); > > + clkev.id = gptimer_id; > > We should not use gptimer_id anymore. This will go away once the > migration to dev-tree is completed. You may be better off storing the > oh_name in the clock_event_device name field and passing to the > suspend/resume handlers. > Currently the name field in clock_event_device is set to "gp_timer". Should I set the name in omap2_gp_clockevent_init() based on the gptimer_id? Regards, Vaibhav
On Fri, Jan 18, 2013 at 10:55:43, Shilimkar, Santosh wrote: > On Friday 18 January 2013 12:15 AM, Jon Hunter wrote: > > > > On 01/10/2013 10:37 PM, Bedia, Vaibhav wrote: > >> On Tue, Jan 08, 2013 at 20:45:10, Shilimkar, Santosh wrote: > >>> On Monday 31 December 2012 06:37 PM, Vaibhav Bedia wrote: > >>>> The current OMAP timer code registers two timers - > >>>> one as clocksource and one as clockevent. > >>>> AM33XX has only one usable timer in the WKUP domain > >>>> so 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> > >>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > >>>> Cc: Benoit Cousson <b-cousson@ti.com> > >>>> Cc: Paul Walmsley <paul@pwsan.com> > >>>> Cc: Kevin Hilman <khilman@deeprootsystems.com> > >>>> Cc: Vaibhav Hiremath <hvaibhav@ti.com> > >>>> Cc: Jon Hunter <jon-hunter@ti.com> > >>>> --- > >>>> v1->v2: > >>>> Get rid of harcoded timer id. > >>>> Note: since a platform device is not created for these timer > >>>> instances and because there's very minimal change needed for > >>>> restarting the timer a full blown context save and restore > >>>> has been skipped. > >>>> > >>>> arch/arm/mach-omap2/timer.c | 33 +++++++++++++++++++++++++++++++++ > >>>> 1 files changed, 33 insertions(+), 0 deletions(-) > >>>> > >>>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c > >>>> index 691aa67..38f9cbc 100644 > >>>> --- a/arch/arm/mach-omap2/timer.c > >>>> +++ b/arch/arm/mach-omap2/timer.c > >>>> @@ -128,6 +128,36 @@ 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); > >>>> +} > >>>> + > >>> Am still bit uncomfortable with direct hwmod usage in the suspend/resmue > >>> hooks. > >>> > >>> Jon, Any alternatives you can think of ? > >>> > >> > >> Jon, > >> > >> Any suggestions here? > > > > Sorry completed this missed this! > > > > Unfortunately, I don't have any good suggestions here. However, I am > > wondering if the suspend/resume handlers can just be calls to > > omap_hwmod_idle/enable and we can remove these __omap_dm_timer_xxxx > > calls (I don't think they are needed). Furthermore, my understanding is > > this is only needed for AM335x (per the changelog), and so we should not > > add suspend/resume handlers for all OMAP2+ based devices. > > > I agree with the direction. > I need to retain the call to enable the interrupt but the others can be dropped. Will take care of this and the comment on invoking the suspend/resume handlers only for AM335x in the next version. Regards, Vaibhav
On 01/21/2013 01:22 AM, Bedia, Vaibhav wrote: > On Fri, Jan 18, 2013 at 10:55:43, Shilimkar, Santosh wrote: >> On Friday 18 January 2013 12:15 AM, Jon Hunter wrote: >>> >>> On 01/10/2013 10:37 PM, Bedia, Vaibhav wrote: >>>> On Tue, Jan 08, 2013 at 20:45:10, Shilimkar, Santosh wrote: >>>>> On Monday 31 December 2012 06:37 PM, Vaibhav Bedia wrote: >>>>>> The current OMAP timer code registers two timers - >>>>>> one as clocksource and one as clockevent. >>>>>> AM33XX has only one usable timer in the WKUP domain >>>>>> so 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> >>>>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>>>> Cc: Benoit Cousson <b-cousson@ti.com> >>>>>> Cc: Paul Walmsley <paul@pwsan.com> >>>>>> Cc: Kevin Hilman <khilman@deeprootsystems.com> >>>>>> Cc: Vaibhav Hiremath <hvaibhav@ti.com> >>>>>> Cc: Jon Hunter <jon-hunter@ti.com> >>>>>> --- >>>>>> v1->v2: >>>>>> Get rid of harcoded timer id. >>>>>> Note: since a platform device is not created for these timer >>>>>> instances and because there's very minimal change needed for >>>>>> restarting the timer a full blown context save and restore >>>>>> has been skipped. >>>>>> >>>>>> arch/arm/mach-omap2/timer.c | 33 +++++++++++++++++++++++++++++++++ >>>>>> 1 files changed, 33 insertions(+), 0 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c >>>>>> index 691aa67..38f9cbc 100644 >>>>>> --- a/arch/arm/mach-omap2/timer.c >>>>>> +++ b/arch/arm/mach-omap2/timer.c >>>>>> @@ -128,6 +128,36 @@ 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); >>>>>> +} >>>>>> + >>>>> Am still bit uncomfortable with direct hwmod usage in the suspend/resmue >>>>> hooks. >>>>> >>>>> Jon, Any alternatives you can think of ? >>>>> >>>> >>>> Jon, >>>> >>>> Any suggestions here? >>> >>> Sorry completed this missed this! >>> >>> Unfortunately, I don't have any good suggestions here. However, I am >>> wondering if the suspend/resume handlers can just be calls to >>> omap_hwmod_idle/enable and we can remove these __omap_dm_timer_xxxx >>> calls (I don't think they are needed). Furthermore, my understanding is >>> this is only needed for AM335x (per the changelog), and so we should not >>> add suspend/resume handlers for all OMAP2+ based devices. >>> >> I agree with the direction. >> > > I need to retain the call to enable the interrupt but the others can be dropped. > Will take care of this and the comment on invoking the suspend/resume handlers > only for AM335x in the next version. Ok fair enough. By the way, I posted a patch today [1] that will use the hwmod name as the clockevent timer name. Care to try on top of that patch and then we can eliminate the sprintf. Cheers Jon [1] http://www.spinics.net/lists/arm-kernel/msg221262.html
On Wed, Jan 30, 2013 at 23:16:34, Hunter, Jon wrote: > > Ok fair enough. By the way, I posted a patch today [1] that will use the > hwmod name as the clockevent timer name. Care to try on top of that > patch and then we can eliminate the sprintf. > Thanks. Will try it out later today. Regards, Vaibhav
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index 691aa67..38f9cbc 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -128,6 +128,36 @@ 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 = { .name = "gp_timer", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, @@ -135,6 +165,8 @@ static struct clock_event_device clockevent_gpt = { .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 = { @@ -323,6 +355,7 @@ static void __init omap2_gp_clockevent_init(int gptimer_id, int res; clkev.errata = omap_dm_timer_get_errata(); + clkev.id = gptimer_id; /* * For clock-event timers we never read the timer counter and
The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so 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> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> Cc: Benoit Cousson <b-cousson@ti.com> Cc: Paul Walmsley <paul@pwsan.com> Cc: Kevin Hilman <khilman@deeprootsystems.com> Cc: Vaibhav Hiremath <hvaibhav@ti.com> Cc: Jon Hunter <jon-hunter@ti.com> --- v1->v2: Get rid of harcoded timer id. Note: since a platform device is not created for these timer instances and because there's very minimal change needed for restarting the timer a full blown context save and restore has been skipped. arch/arm/mach-omap2/timer.c | 33 +++++++++++++++++++++++++++++++++ 1 files changed, 33 insertions(+), 0 deletions(-)