* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 16:25 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 16:25 UTC (permalink / raw) To: linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Grygorii Strashko, Arnd Bergmann, John Stultz, Felipe Balbi, Tony Lindgren Now the System stall is observed on TI AM437x based board (am437x-gp-evm) during resuming from System suspend when ARM Global timer is selected as clocksource device - SysRq are working, but nothing else. The reason of stall is that ARM Global timer loses its contexts. The reason of stall is that ARM Global timer loses its contexts during System suspend: GT_CONTROL.TIMER_ENABLE = 0 (unbanked) GT_COUNTERx = 0 Hence, update ARM Global timer driver to reflect above behaviour - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; - ensure clocksource and clockevent devices have coresponding flags (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set depending on presence of "always-on" DT property. CC: Arnd Bergmann <arnd@arndb.de> Cc: John Stultz <john.stultz@linaro.org> Cc: Felipe Balbi <balbi@ti.com> Cc: Tony Lindgren <tony@atomide.com> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index a2cb6fa..1bbaf64 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -51,6 +51,8 @@ static void __iomem *gt_base; static unsigned long gt_clk_rate; static int gt_ppi; static struct clock_event_device __percpu *gt_evt; +static bool gt_always_on; +static u32 gt_control; /* * To get the value from the Global Timer Counter register proceed as follows: @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) { int cpu = smp_processor_id(); + if (!gt_always_on) + clk->features |= CLOCK_EVT_FEAT_C3STOP; + clk->name = "arm_global_timer"; clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERCPU; @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) return gt_counter_read(); } +static void gt_suspend(struct clocksource *cs) +{ + gt_control = readl(gt_base + GT_CONTROL); +} + +static void gt_resume(struct clocksource *cs) +{ + /* enables timer on all the cores */ + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); +} + static struct clocksource gt_clocksource = { .name = "arm_global_timer", .rating = 300, .read = gt_clocksource_read, .mask = CLOCKSOURCE_MASK(64), .flags = CLOCK_SOURCE_IS_CONTINUOUS, + .suspend = gt_suspend, + .resume = gt_resume, }; #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK @@ -218,6 +236,9 @@ static void __init gt_clocksource_init(void) /* enables timer on all the cores */ writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); + if (gt_always_on) + gt_clocksource.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP; + #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate); #endif @@ -289,6 +310,8 @@ static void __init global_timer_of_register(struct device_node *np) goto out_clk; } + gt_always_on = of_property_read_bool(np, "always-on"); + err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt, "gt", gt_evt); if (err) { -- 2.6.3 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 16:25 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 16:25 UTC (permalink / raw) To: linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Grygorii Strashko, Arnd Bergmann, John Stultz, Felipe Balbi, Tony Lindgren Now the System stall is observed on TI AM437x based board (am437x-gp-evm) during resuming from System suspend when ARM Global timer is selected as clocksource device - SysRq are working, but nothing else. The reason of stall is that ARM Global timer loses its contexts. The reason of stall is that ARM Global timer loses its contexts during System suspend: GT_CONTROL.TIMER_ENABLE = 0 (unbanked) GT_COUNTERx = 0 Hence, update ARM Global timer driver to reflect above behaviour - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; - ensure clocksource and clockevent devices have coresponding flags (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set depending on presence of "always-on" DT property. CC: Arnd Bergmann <arnd@arndb.de> Cc: John Stultz <john.stultz@linaro.org> Cc: Felipe Balbi <balbi@ti.com> Cc: Tony Lindgren <tony@atomide.com> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index a2cb6fa..1bbaf64 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -51,6 +51,8 @@ static void __iomem *gt_base; static unsigned long gt_clk_rate; static int gt_ppi; static struct clock_event_device __percpu *gt_evt; +static bool gt_always_on; +static u32 gt_control; /* * To get the value from the Global Timer Counter register proceed as follows: @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) { int cpu = smp_processor_id(); + if (!gt_always_on) + clk->features |= CLOCK_EVT_FEAT_C3STOP; + clk->name = "arm_global_timer"; clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERCPU; @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) return gt_counter_read(); } +static void gt_suspend(struct clocksource *cs) +{ + gt_control = readl(gt_base + GT_CONTROL); +} + +static void gt_resume(struct clocksource *cs) +{ + /* enables timer on all the cores */ + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); +} + static struct clocksource gt_clocksource = { .name = "arm_global_timer", .rating = 300, .read = gt_clocksource_read, .mask = CLOCKSOURCE_MASK(64), .flags = CLOCK_SOURCE_IS_CONTINUOUS, + .suspend = gt_suspend, + .resume = gt_resume, }; #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK @@ -218,6 +236,9 @@ static void __init gt_clocksource_init(void) /* enables timer on all the cores */ writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); + if (gt_always_on) + gt_clocksource.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP; + #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate); #endif @@ -289,6 +310,8 @@ static void __init global_timer_of_register(struct device_node *np) goto out_clk; } + gt_always_on = of_property_read_bool(np, "always-on"); + err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt, "gt", gt_evt); if (err) { -- 2.6.3 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 16:25 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 16:25 UTC (permalink / raw) To: linux-arm-kernel Now the System stall is observed on TI AM437x based board (am437x-gp-evm) during resuming from System suspend when ARM Global timer is selected as clocksource device - SysRq are working, but nothing else. The reason of stall is that ARM Global timer loses its contexts. The reason of stall is that ARM Global timer loses its contexts during System suspend: GT_CONTROL.TIMER_ENABLE = 0 (unbanked) GT_COUNTERx = 0 Hence, update ARM Global timer driver to reflect above behaviour - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; - ensure clocksource and clockevent devices have coresponding flags (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set depending on presence of "always-on" DT property. CC: Arnd Bergmann <arnd@arndb.de> Cc: John Stultz <john.stultz@linaro.org> Cc: Felipe Balbi <balbi@ti.com> Cc: Tony Lindgren <tony@atomide.com> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index a2cb6fa..1bbaf64 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -51,6 +51,8 @@ static void __iomem *gt_base; static unsigned long gt_clk_rate; static int gt_ppi; static struct clock_event_device __percpu *gt_evt; +static bool gt_always_on; +static u32 gt_control; /* * To get the value from the Global Timer Counter register proceed as follows: @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) { int cpu = smp_processor_id(); + if (!gt_always_on) + clk->features |= CLOCK_EVT_FEAT_C3STOP; + clk->name = "arm_global_timer"; clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERCPU; @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) return gt_counter_read(); } +static void gt_suspend(struct clocksource *cs) +{ + gt_control = readl(gt_base + GT_CONTROL); +} + +static void gt_resume(struct clocksource *cs) +{ + /* enables timer on all the cores */ + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); +} + static struct clocksource gt_clocksource = { .name = "arm_global_timer", .rating = 300, .read = gt_clocksource_read, .mask = CLOCKSOURCE_MASK(64), .flags = CLOCK_SOURCE_IS_CONTINUOUS, + .suspend = gt_suspend, + .resume = gt_resume, }; #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK @@ -218,6 +236,9 @@ static void __init gt_clocksource_init(void) /* enables timer on all the cores */ writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); + if (gt_always_on) + gt_clocksource.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP; + #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate); #endif @@ -289,6 +310,8 @@ static void __init global_timer_of_register(struct device_node *np) goto out_clk; } + gt_always_on = of_property_read_bool(np, "always-on"); + err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt, "gt", gt_evt); if (err) { -- 2.6.3 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 16:25 ` Grygorii Strashko (?) @ 2015-11-13 16:43 ` Felipe Balbi -1 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 16:43 UTC (permalink / raw) To: linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Grygorii Strashko, Arnd Bergmann, John Stultz, Tony Lindgren [-- Attachment #1: Type: text/plain, Size: 2712 bytes --] Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > Now the System stall is observed on TI AM437x based board > (am437x-gp-evm) during resuming from System suspend when ARM Global > timer is selected as clocksource device - SysRq are working, but > nothing else. The reason of stall is that ARM Global timer loses its > contexts. > > The reason of stall is that ARM Global timer loses its contexts during > System suspend: > GT_CONTROL.TIMER_ENABLE = 0 (unbanked) > GT_COUNTERx = 0 > > Hence, update ARM Global timer driver to reflect above behaviour > - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; > - ensure clocksource and clockevent devices have coresponding flags > (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set > depending on presence of "always-on" DT property. > > CC: Arnd Bergmann <arnd@arndb.de> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Felipe Balbi <balbi@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > index a2cb6fa..1bbaf64 100644 > --- a/drivers/clocksource/arm_global_timer.c > +++ b/drivers/clocksource/arm_global_timer.c > @@ -51,6 +51,8 @@ static void __iomem *gt_base; > static unsigned long gt_clk_rate; > static int gt_ppi; > static struct clock_event_device __percpu *gt_evt; > +static bool gt_always_on; > +static u32 gt_control; > > /* > * To get the value from the Global Timer Counter register proceed as follows: > @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) > { > int cpu = smp_processor_id(); > > + if (!gt_always_on) > + clk->features |= CLOCK_EVT_FEAT_C3STOP; > + > clk->name = "arm_global_timer"; > clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > CLOCK_EVT_FEAT_PERCPU; > @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) > return gt_counter_read(); > } > > +static void gt_suspend(struct clocksource *cs) > +{ > + gt_control = readl(gt_base + GT_CONTROL); > +} > + > +static void gt_resume(struct clocksource *cs) > +{ > + /* enables timer on all the cores */ > + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); do you really need to save context if all you restore is TIMER_ENABLE bit ? seems like you could skip gt_suspend altogether. Is there really a situation where this driver is running and GT isn't enabled ? -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 16:43 ` Felipe Balbi 0 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 16:43 UTC (permalink / raw) To: Grygorii Strashko, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Grygorii Strashko, Arnd Bergmann, John Stultz, Tony Lindgren [-- Attachment #1: Type: text/plain, Size: 2712 bytes --] Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > Now the System stall is observed on TI AM437x based board > (am437x-gp-evm) during resuming from System suspend when ARM Global > timer is selected as clocksource device - SysRq are working, but > nothing else. The reason of stall is that ARM Global timer loses its > contexts. > > The reason of stall is that ARM Global timer loses its contexts during > System suspend: > GT_CONTROL.TIMER_ENABLE = 0 (unbanked) > GT_COUNTERx = 0 > > Hence, update ARM Global timer driver to reflect above behaviour > - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; > - ensure clocksource and clockevent devices have coresponding flags > (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set > depending on presence of "always-on" DT property. > > CC: Arnd Bergmann <arnd@arndb.de> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Felipe Balbi <balbi@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > index a2cb6fa..1bbaf64 100644 > --- a/drivers/clocksource/arm_global_timer.c > +++ b/drivers/clocksource/arm_global_timer.c > @@ -51,6 +51,8 @@ static void __iomem *gt_base; > static unsigned long gt_clk_rate; > static int gt_ppi; > static struct clock_event_device __percpu *gt_evt; > +static bool gt_always_on; > +static u32 gt_control; > > /* > * To get the value from the Global Timer Counter register proceed as follows: > @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) > { > int cpu = smp_processor_id(); > > + if (!gt_always_on) > + clk->features |= CLOCK_EVT_FEAT_C3STOP; > + > clk->name = "arm_global_timer"; > clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > CLOCK_EVT_FEAT_PERCPU; > @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) > return gt_counter_read(); > } > > +static void gt_suspend(struct clocksource *cs) > +{ > + gt_control = readl(gt_base + GT_CONTROL); > +} > + > +static void gt_resume(struct clocksource *cs) > +{ > + /* enables timer on all the cores */ > + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); do you really need to save context if all you restore is TIMER_ENABLE bit ? seems like you could skip gt_suspend altogether. Is there really a situation where this driver is running and GT isn't enabled ? -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 16:43 ` Felipe Balbi 0 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 16:43 UTC (permalink / raw) To: linux-arm-kernel Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > Now the System stall is observed on TI AM437x based board > (am437x-gp-evm) during resuming from System suspend when ARM Global > timer is selected as clocksource device - SysRq are working, but > nothing else. The reason of stall is that ARM Global timer loses its > contexts. > > The reason of stall is that ARM Global timer loses its contexts during > System suspend: > GT_CONTROL.TIMER_ENABLE = 0 (unbanked) > GT_COUNTERx = 0 > > Hence, update ARM Global timer driver to reflect above behaviour > - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; > - ensure clocksource and clockevent devices have coresponding flags > (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set > depending on presence of "always-on" DT property. > > CC: Arnd Bergmann <arnd@arndb.de> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Felipe Balbi <balbi@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > index a2cb6fa..1bbaf64 100644 > --- a/drivers/clocksource/arm_global_timer.c > +++ b/drivers/clocksource/arm_global_timer.c > @@ -51,6 +51,8 @@ static void __iomem *gt_base; > static unsigned long gt_clk_rate; > static int gt_ppi; > static struct clock_event_device __percpu *gt_evt; > +static bool gt_always_on; > +static u32 gt_control; > > /* > * To get the value from the Global Timer Counter register proceed as follows: > @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) > { > int cpu = smp_processor_id(); > > + if (!gt_always_on) > + clk->features |= CLOCK_EVT_FEAT_C3STOP; > + > clk->name = "arm_global_timer"; > clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > CLOCK_EVT_FEAT_PERCPU; > @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) > return gt_counter_read(); > } > > +static void gt_suspend(struct clocksource *cs) > +{ > + gt_control = readl(gt_base + GT_CONTROL); > +} > + > +static void gt_resume(struct clocksource *cs) > +{ > + /* enables timer on all the cores */ > + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); do you really need to save context if all you restore is TIMER_ENABLE bit ? seems like you could skip gt_suspend altogether. Is there really a situation where this driver is running and GT isn't enabled ? -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 818 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151113/26e803ba/attachment.sig> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 16:43 ` Felipe Balbi (?) @ 2015-11-13 16:57 ` Grygorii Strashko -1 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 16:57 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren On 11/13/2015 06:43 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> Now the System stall is observed on TI AM437x based board >> (am437x-gp-evm) during resuming from System suspend when ARM Global >> timer is selected as clocksource device - SysRq are working, but >> nothing else. The reason of stall is that ARM Global timer loses its >> contexts. >> >> The reason of stall is that ARM Global timer loses its contexts during >> System suspend: >> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >> GT_COUNTERx = 0 >> >> Hence, update ARM Global timer driver to reflect above behaviour >> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >> - ensure clocksource and clockevent devices have coresponding flags >> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >> depending on presence of "always-on" DT property. >> >> CC: Arnd Bergmann <arnd@arndb.de> >> Cc: John Stultz <john.stultz@linaro.org> >> Cc: Felipe Balbi <balbi@ti.com> >> Cc: Tony Lindgren <tony@atomide.com> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >> index a2cb6fa..1bbaf64 100644 >> --- a/drivers/clocksource/arm_global_timer.c >> +++ b/drivers/clocksource/arm_global_timer.c >> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >> static unsigned long gt_clk_rate; >> static int gt_ppi; >> static struct clock_event_device __percpu *gt_evt; >> +static bool gt_always_on; >> +static u32 gt_control; >> >> /* >> * To get the value from the Global Timer Counter register proceed as follows: >> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >> { >> int cpu = smp_processor_id(); >> >> + if (!gt_always_on) >> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >> + >> clk->name = "arm_global_timer"; >> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >> CLOCK_EVT_FEAT_PERCPU; >> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >> return gt_counter_read(); >> } >> >> +static void gt_suspend(struct clocksource *cs) >> +{ >> + gt_control = readl(gt_base + GT_CONTROL); >> +} >> + >> +static void gt_resume(struct clocksource *cs) >> +{ >> + /* enables timer on all the cores */ >> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); > > do you really need to save context if all you restore is TIMER_ENABLE > bit ? seems like you could skip gt_suspend altogether. Is there really a > situation where this driver is running and GT isn't enabled ? Now It's not. It's always enabled. I did it because .suspend() is called for all registered clock sources regardless of their usage. So, potentially in the future, at the moment when .suspend() is called it might be disabled (for example, .enable/disable() callbacks can be added and, if ARM Global timer will not be registered as sched_clock, it will be possible to keep it disabled if not used now). But It's not essentially now - I can update it and drop save restore. Pls, confirm. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 16:57 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 16:57 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren On 11/13/2015 06:43 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> Now the System stall is observed on TI AM437x based board >> (am437x-gp-evm) during resuming from System suspend when ARM Global >> timer is selected as clocksource device - SysRq are working, but >> nothing else. The reason of stall is that ARM Global timer loses its >> contexts. >> >> The reason of stall is that ARM Global timer loses its contexts during >> System suspend: >> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >> GT_COUNTERx = 0 >> >> Hence, update ARM Global timer driver to reflect above behaviour >> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >> - ensure clocksource and clockevent devices have coresponding flags >> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >> depending on presence of "always-on" DT property. >> >> CC: Arnd Bergmann <arnd@arndb.de> >> Cc: John Stultz <john.stultz@linaro.org> >> Cc: Felipe Balbi <balbi@ti.com> >> Cc: Tony Lindgren <tony@atomide.com> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >> index a2cb6fa..1bbaf64 100644 >> --- a/drivers/clocksource/arm_global_timer.c >> +++ b/drivers/clocksource/arm_global_timer.c >> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >> static unsigned long gt_clk_rate; >> static int gt_ppi; >> static struct clock_event_device __percpu *gt_evt; >> +static bool gt_always_on; >> +static u32 gt_control; >> >> /* >> * To get the value from the Global Timer Counter register proceed as follows: >> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >> { >> int cpu = smp_processor_id(); >> >> + if (!gt_always_on) >> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >> + >> clk->name = "arm_global_timer"; >> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >> CLOCK_EVT_FEAT_PERCPU; >> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >> return gt_counter_read(); >> } >> >> +static void gt_suspend(struct clocksource *cs) >> +{ >> + gt_control = readl(gt_base + GT_CONTROL); >> +} >> + >> +static void gt_resume(struct clocksource *cs) >> +{ >> + /* enables timer on all the cores */ >> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); > > do you really need to save context if all you restore is TIMER_ENABLE > bit ? seems like you could skip gt_suspend altogether. Is there really a > situation where this driver is running and GT isn't enabled ? Now It's not. It's always enabled. I did it because .suspend() is called for all registered clock sources regardless of their usage. So, potentially in the future, at the moment when .suspend() is called it might be disabled (for example, .enable/disable() callbacks can be added and, if ARM Global timer will not be registered as sched_clock, it will be possible to keep it disabled if not used now). But It's not essentially now - I can update it and drop save restore. Pls, confirm. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 16:57 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 16:57 UTC (permalink / raw) To: linux-arm-kernel On 11/13/2015 06:43 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> Now the System stall is observed on TI AM437x based board >> (am437x-gp-evm) during resuming from System suspend when ARM Global >> timer is selected as clocksource device - SysRq are working, but >> nothing else. The reason of stall is that ARM Global timer loses its >> contexts. >> >> The reason of stall is that ARM Global timer loses its contexts during >> System suspend: >> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >> GT_COUNTERx = 0 >> >> Hence, update ARM Global timer driver to reflect above behaviour >> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >> - ensure clocksource and clockevent devices have coresponding flags >> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >> depending on presence of "always-on" DT property. >> >> CC: Arnd Bergmann <arnd@arndb.de> >> Cc: John Stultz <john.stultz@linaro.org> >> Cc: Felipe Balbi <balbi@ti.com> >> Cc: Tony Lindgren <tony@atomide.com> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >> index a2cb6fa..1bbaf64 100644 >> --- a/drivers/clocksource/arm_global_timer.c >> +++ b/drivers/clocksource/arm_global_timer.c >> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >> static unsigned long gt_clk_rate; >> static int gt_ppi; >> static struct clock_event_device __percpu *gt_evt; >> +static bool gt_always_on; >> +static u32 gt_control; >> >> /* >> * To get the value from the Global Timer Counter register proceed as follows: >> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >> { >> int cpu = smp_processor_id(); >> >> + if (!gt_always_on) >> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >> + >> clk->name = "arm_global_timer"; >> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >> CLOCK_EVT_FEAT_PERCPU; >> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >> return gt_counter_read(); >> } >> >> +static void gt_suspend(struct clocksource *cs) >> +{ >> + gt_control = readl(gt_base + GT_CONTROL); >> +} >> + >> +static void gt_resume(struct clocksource *cs) >> +{ >> + /* enables timer on all the cores */ >> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); > > do you really need to save context if all you restore is TIMER_ENABLE > bit ? seems like you could skip gt_suspend altogether. Is there really a > situation where this driver is running and GT isn't enabled ? Now It's not. It's always enabled. I did it because .suspend() is called for all registered clock sources regardless of their usage. So, potentially in the future, at the moment when .suspend() is called it might be disabled (for example, .enable/disable() callbacks can be added and, if ARM Global timer will not be registered as sched_clock, it will be possible to keep it disabled if not used now). But It's not essentially now - I can update it and drop save restore. Pls, confirm. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 16:57 ` Grygorii Strashko (?) @ 2015-11-13 17:40 ` Felipe Balbi -1 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 17:40 UTC (permalink / raw) To: Grygorii Strashko, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren [-- Attachment #1: Type: text/plain, Size: 3652 bytes --] Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/13/2015 06:43 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> Now the System stall is observed on TI AM437x based board >>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>> timer is selected as clocksource device - SysRq are working, but >>> nothing else. The reason of stall is that ARM Global timer loses its >>> contexts. >>> >>> The reason of stall is that ARM Global timer loses its contexts during >>> System suspend: >>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>> GT_COUNTERx = 0 >>> >>> Hence, update ARM Global timer driver to reflect above behaviour >>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>> - ensure clocksource and clockevent devices have coresponding flags >>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>> depending on presence of "always-on" DT property. >>> >>> CC: Arnd Bergmann <arnd@arndb.de> >>> Cc: John Stultz <john.stultz@linaro.org> >>> Cc: Felipe Balbi <balbi@ti.com> >>> Cc: Tony Lindgren <tony@atomide.com> >>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>> --- >>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >>> >>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>> index a2cb6fa..1bbaf64 100644 >>> --- a/drivers/clocksource/arm_global_timer.c >>> +++ b/drivers/clocksource/arm_global_timer.c >>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>> static unsigned long gt_clk_rate; >>> static int gt_ppi; >>> static struct clock_event_device __percpu *gt_evt; >>> +static bool gt_always_on; >>> +static u32 gt_control; >>> >>> /* >>> * To get the value from the Global Timer Counter register proceed as follows: >>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>> { >>> int cpu = smp_processor_id(); >>> >>> + if (!gt_always_on) >>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>> + >>> clk->name = "arm_global_timer"; >>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>> CLOCK_EVT_FEAT_PERCPU; >>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>> return gt_counter_read(); >>> } >>> >>> +static void gt_suspend(struct clocksource *cs) >>> +{ >>> + gt_control = readl(gt_base + GT_CONTROL); >>> +} >>> + >>> +static void gt_resume(struct clocksource *cs) >>> +{ >>> + /* enables timer on all the cores */ >>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >> >> do you really need to save context if all you restore is TIMER_ENABLE >> bit ? seems like you could skip gt_suspend altogether. Is there really a >> situation where this driver is running and GT isn't enabled ? > > Now It's not. It's always enabled. I did it because .suspend() is called for > all registered clock sources regardless of their usage. So, potentially > in the future, at the moment when .suspend() is called it might be disabled > (for example, .enable/disable() callbacks can be added and, if ARM Global timer > will not be registered as sched_clock, it will be possible to keep it disabled > if not used now). > > But It's not essentially now - I can update it and drop save restore. > Pls, confirm. I think it's best to skip suspend completely. You're not restoring anything you saved during suspend, unless you meant | where you used &. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 17:40 ` Felipe Balbi 0 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 17:40 UTC (permalink / raw) To: Grygorii Strashko, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren [-- Attachment #1: Type: text/plain, Size: 3652 bytes --] Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/13/2015 06:43 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> Now the System stall is observed on TI AM437x based board >>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>> timer is selected as clocksource device - SysRq are working, but >>> nothing else. The reason of stall is that ARM Global timer loses its >>> contexts. >>> >>> The reason of stall is that ARM Global timer loses its contexts during >>> System suspend: >>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>> GT_COUNTERx = 0 >>> >>> Hence, update ARM Global timer driver to reflect above behaviour >>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>> - ensure clocksource and clockevent devices have coresponding flags >>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>> depending on presence of "always-on" DT property. >>> >>> CC: Arnd Bergmann <arnd@arndb.de> >>> Cc: John Stultz <john.stultz@linaro.org> >>> Cc: Felipe Balbi <balbi@ti.com> >>> Cc: Tony Lindgren <tony@atomide.com> >>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>> --- >>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >>> >>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>> index a2cb6fa..1bbaf64 100644 >>> --- a/drivers/clocksource/arm_global_timer.c >>> +++ b/drivers/clocksource/arm_global_timer.c >>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>> static unsigned long gt_clk_rate; >>> static int gt_ppi; >>> static struct clock_event_device __percpu *gt_evt; >>> +static bool gt_always_on; >>> +static u32 gt_control; >>> >>> /* >>> * To get the value from the Global Timer Counter register proceed as follows: >>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>> { >>> int cpu = smp_processor_id(); >>> >>> + if (!gt_always_on) >>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>> + >>> clk->name = "arm_global_timer"; >>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>> CLOCK_EVT_FEAT_PERCPU; >>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>> return gt_counter_read(); >>> } >>> >>> +static void gt_suspend(struct clocksource *cs) >>> +{ >>> + gt_control = readl(gt_base + GT_CONTROL); >>> +} >>> + >>> +static void gt_resume(struct clocksource *cs) >>> +{ >>> + /* enables timer on all the cores */ >>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >> >> do you really need to save context if all you restore is TIMER_ENABLE >> bit ? seems like you could skip gt_suspend altogether. Is there really a >> situation where this driver is running and GT isn't enabled ? > > Now It's not. It's always enabled. I did it because .suspend() is called for > all registered clock sources regardless of their usage. So, potentially > in the future, at the moment when .suspend() is called it might be disabled > (for example, .enable/disable() callbacks can be added and, if ARM Global timer > will not be registered as sched_clock, it will be possible to keep it disabled > if not used now). > > But It's not essentially now - I can update it and drop save restore. > Pls, confirm. I think it's best to skip suspend completely. You're not restoring anything you saved during suspend, unless you meant | where you used &. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 17:40 ` Felipe Balbi 0 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 17:40 UTC (permalink / raw) To: linux-arm-kernel Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/13/2015 06:43 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> Now the System stall is observed on TI AM437x based board >>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>> timer is selected as clocksource device - SysRq are working, but >>> nothing else. The reason of stall is that ARM Global timer loses its >>> contexts. >>> >>> The reason of stall is that ARM Global timer loses its contexts during >>> System suspend: >>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>> GT_COUNTERx = 0 >>> >>> Hence, update ARM Global timer driver to reflect above behaviour >>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>> - ensure clocksource and clockevent devices have coresponding flags >>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>> depending on presence of "always-on" DT property. >>> >>> CC: Arnd Bergmann <arnd@arndb.de> >>> Cc: John Stultz <john.stultz@linaro.org> >>> Cc: Felipe Balbi <balbi@ti.com> >>> Cc: Tony Lindgren <tony@atomide.com> >>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>> --- >>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >>> >>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>> index a2cb6fa..1bbaf64 100644 >>> --- a/drivers/clocksource/arm_global_timer.c >>> +++ b/drivers/clocksource/arm_global_timer.c >>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>> static unsigned long gt_clk_rate; >>> static int gt_ppi; >>> static struct clock_event_device __percpu *gt_evt; >>> +static bool gt_always_on; >>> +static u32 gt_control; >>> >>> /* >>> * To get the value from the Global Timer Counter register proceed as follows: >>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>> { >>> int cpu = smp_processor_id(); >>> >>> + if (!gt_always_on) >>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>> + >>> clk->name = "arm_global_timer"; >>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>> CLOCK_EVT_FEAT_PERCPU; >>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>> return gt_counter_read(); >>> } >>> >>> +static void gt_suspend(struct clocksource *cs) >>> +{ >>> + gt_control = readl(gt_base + GT_CONTROL); >>> +} >>> + >>> +static void gt_resume(struct clocksource *cs) >>> +{ >>> + /* enables timer on all the cores */ >>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >> >> do you really need to save context if all you restore is TIMER_ENABLE >> bit ? seems like you could skip gt_suspend altogether. Is there really a >> situation where this driver is running and GT isn't enabled ? > > Now It's not. It's always enabled. I did it because .suspend() is called for > all registered clock sources regardless of their usage. So, potentially > in the future, at the moment when .suspend() is called it might be disabled > (for example, .enable/disable() callbacks can be added and, if ARM Global timer > will not be registered as sched_clock, it will be possible to keep it disabled > if not used now). > > But It's not essentially now - I can update it and drop save restore. > Pls, confirm. I think it's best to skip suspend completely. You're not restoring anything you saved during suspend, unless you meant | where you used &. -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 818 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151113/55244c0a/attachment-0001.sig> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 17:40 ` Felipe Balbi (?) @ 2015-11-13 18:09 ` Grygorii Strashko -1 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 18:09 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren On 11/13/2015 07:40 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> Now the System stall is observed on TI AM437x based board >>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>> timer is selected as clocksource device - SysRq are working, but >>>> nothing else. The reason of stall is that ARM Global timer loses its >>>> contexts. >>>> >>>> The reason of stall is that ARM Global timer loses its contexts during >>>> System suspend: >>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>> GT_COUNTERx = 0 >>>> >>>> Hence, update ARM Global timer driver to reflect above behaviour >>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>> - ensure clocksource and clockevent devices have coresponding flags >>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>> depending on presence of "always-on" DT property. >>>> >>>> CC: Arnd Bergmann <arnd@arndb.de> >>>> Cc: John Stultz <john.stultz@linaro.org> >>>> Cc: Felipe Balbi <balbi@ti.com> >>>> Cc: Tony Lindgren <tony@atomide.com> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> --- >>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>>> >>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>> index a2cb6fa..1bbaf64 100644 >>>> --- a/drivers/clocksource/arm_global_timer.c >>>> +++ b/drivers/clocksource/arm_global_timer.c >>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>> static unsigned long gt_clk_rate; >>>> static int gt_ppi; >>>> static struct clock_event_device __percpu *gt_evt; >>>> +static bool gt_always_on; >>>> +static u32 gt_control; >>>> >>>> /* >>>> * To get the value from the Global Timer Counter register proceed as follows: >>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>> { >>>> int cpu = smp_processor_id(); >>>> >>>> + if (!gt_always_on) >>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>> + >>>> clk->name = "arm_global_timer"; >>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>> CLOCK_EVT_FEAT_PERCPU; >>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>> return gt_counter_read(); >>>> } >>>> >>>> +static void gt_suspend(struct clocksource *cs) >>>> +{ >>>> + gt_control = readl(gt_base + GT_CONTROL); >>>> +} >>>> + >>>> +static void gt_resume(struct clocksource *cs) >>>> +{ >>>> + /* enables timer on all the cores */ >>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>> >>> do you really need to save context if all you restore is TIMER_ENABLE >>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>> situation where this driver is running and GT isn't enabled ? >> >> Now It's not. It's always enabled. I did it because .suspend() is called for >> all registered clock sources regardless of their usage. So, potentially >> in the future, at the moment when .suspend() is called it might be disabled >> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >> will not be registered as sched_clock, it will be possible to keep it disabled >> if not used now). >> >> But It's not essentially now - I can update it and drop save restore. >> Pls, confirm. > > I think it's best to skip suspend completely. You're not restoring > anything you saved during suspend, unless you meant | where you used &. > I didn't get it - I'm restoring one bit(0) only. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:09 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 18:09 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren On 11/13/2015 07:40 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> Now the System stall is observed on TI AM437x based board >>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>> timer is selected as clocksource device - SysRq are working, but >>>> nothing else. The reason of stall is that ARM Global timer loses its >>>> contexts. >>>> >>>> The reason of stall is that ARM Global timer loses its contexts during >>>> System suspend: >>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>> GT_COUNTERx = 0 >>>> >>>> Hence, update ARM Global timer driver to reflect above behaviour >>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>> - ensure clocksource and clockevent devices have coresponding flags >>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>> depending on presence of "always-on" DT property. >>>> >>>> CC: Arnd Bergmann <arnd@arndb.de> >>>> Cc: John Stultz <john.stultz@linaro.org> >>>> Cc: Felipe Balbi <balbi@ti.com> >>>> Cc: Tony Lindgren <tony@atomide.com> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> --- >>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>>> >>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>> index a2cb6fa..1bbaf64 100644 >>>> --- a/drivers/clocksource/arm_global_timer.c >>>> +++ b/drivers/clocksource/arm_global_timer.c >>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>> static unsigned long gt_clk_rate; >>>> static int gt_ppi; >>>> static struct clock_event_device __percpu *gt_evt; >>>> +static bool gt_always_on; >>>> +static u32 gt_control; >>>> >>>> /* >>>> * To get the value from the Global Timer Counter register proceed as follows: >>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>> { >>>> int cpu = smp_processor_id(); >>>> >>>> + if (!gt_always_on) >>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>> + >>>> clk->name = "arm_global_timer"; >>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>> CLOCK_EVT_FEAT_PERCPU; >>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>> return gt_counter_read(); >>>> } >>>> >>>> +static void gt_suspend(struct clocksource *cs) >>>> +{ >>>> + gt_control = readl(gt_base + GT_CONTROL); >>>> +} >>>> + >>>> +static void gt_resume(struct clocksource *cs) >>>> +{ >>>> + /* enables timer on all the cores */ >>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>> >>> do you really need to save context if all you restore is TIMER_ENABLE >>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>> situation where this driver is running and GT isn't enabled ? >> >> Now It's not. It's always enabled. I did it because .suspend() is called for >> all registered clock sources regardless of their usage. So, potentially >> in the future, at the moment when .suspend() is called it might be disabled >> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >> will not be registered as sched_clock, it will be possible to keep it disabled >> if not used now). >> >> But It's not essentially now - I can update it and drop save restore. >> Pls, confirm. > > I think it's best to skip suspend completely. You're not restoring > anything you saved during suspend, unless you meant | where you used &. > I didn't get it - I'm restoring one bit(0) only. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:09 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 18:09 UTC (permalink / raw) To: linux-arm-kernel On 11/13/2015 07:40 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> Now the System stall is observed on TI AM437x based board >>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>> timer is selected as clocksource device - SysRq are working, but >>>> nothing else. The reason of stall is that ARM Global timer loses its >>>> contexts. >>>> >>>> The reason of stall is that ARM Global timer loses its contexts during >>>> System suspend: >>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>> GT_COUNTERx = 0 >>>> >>>> Hence, update ARM Global timer driver to reflect above behaviour >>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>> - ensure clocksource and clockevent devices have coresponding flags >>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>> depending on presence of "always-on" DT property. >>>> >>>> CC: Arnd Bergmann <arnd@arndb.de> >>>> Cc: John Stultz <john.stultz@linaro.org> >>>> Cc: Felipe Balbi <balbi@ti.com> >>>> Cc: Tony Lindgren <tony@atomide.com> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> --- >>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>>> >>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>> index a2cb6fa..1bbaf64 100644 >>>> --- a/drivers/clocksource/arm_global_timer.c >>>> +++ b/drivers/clocksource/arm_global_timer.c >>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>> static unsigned long gt_clk_rate; >>>> static int gt_ppi; >>>> static struct clock_event_device __percpu *gt_evt; >>>> +static bool gt_always_on; >>>> +static u32 gt_control; >>>> >>>> /* >>>> * To get the value from the Global Timer Counter register proceed as follows: >>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>> { >>>> int cpu = smp_processor_id(); >>>> >>>> + if (!gt_always_on) >>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>> + >>>> clk->name = "arm_global_timer"; >>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>> CLOCK_EVT_FEAT_PERCPU; >>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>> return gt_counter_read(); >>>> } >>>> >>>> +static void gt_suspend(struct clocksource *cs) >>>> +{ >>>> + gt_control = readl(gt_base + GT_CONTROL); >>>> +} >>>> + >>>> +static void gt_resume(struct clocksource *cs) >>>> +{ >>>> + /* enables timer on all the cores */ >>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>> >>> do you really need to save context if all you restore is TIMER_ENABLE >>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>> situation where this driver is running and GT isn't enabled ? >> >> Now It's not. It's always enabled. I did it because .suspend() is called for >> all registered clock sources regardless of their usage. So, potentially >> in the future, at the moment when .suspend() is called it might be disabled >> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >> will not be registered as sched_clock, it will be possible to keep it disabled >> if not used now). >> >> But It's not essentially now - I can update it and drop save restore. >> Pls, confirm. > > I think it's best to skip suspend completely. You're not restoring > anything you saved during suspend, unless you meant | where you used &. > I didn't get it - I'm restoring one bit(0) only. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 18:09 ` Grygorii Strashko (?) @ 2015-11-13 18:15 ` Felipe Balbi -1 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 18:15 UTC (permalink / raw) To: Grygorii Strashko, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: kernel, Arnd Bergmann, Tony Lindgren, linux-kernel, John Stultz, linux-omap, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 4098 bytes --] Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/13/2015 07:40 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>> >>>> Hi, >>>> >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>> Now the System stall is observed on TI AM437x based board >>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>> timer is selected as clocksource device - SysRq are working, but >>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>> contexts. >>>>> >>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>> System suspend: >>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>> GT_COUNTERx = 0 >>>>> >>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>> depending on presence of "always-on" DT property. >>>>> >>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>> --- >>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>> 1 file changed, 23 insertions(+) >>>>> >>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>> index a2cb6fa..1bbaf64 100644 >>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>> static unsigned long gt_clk_rate; >>>>> static int gt_ppi; >>>>> static struct clock_event_device __percpu *gt_evt; >>>>> +static bool gt_always_on; >>>>> +static u32 gt_control; >>>>> >>>>> /* >>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>> { >>>>> int cpu = smp_processor_id(); >>>>> >>>>> + if (!gt_always_on) >>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>> + >>>>> clk->name = "arm_global_timer"; >>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>> CLOCK_EVT_FEAT_PERCPU; >>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>> return gt_counter_read(); >>>>> } >>>>> >>>>> +static void gt_suspend(struct clocksource *cs) >>>>> +{ >>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>> +} >>>>> + >>>>> +static void gt_resume(struct clocksource *cs) >>>>> +{ >>>>> + /* enables timer on all the cores */ >>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>> >>>> do you really need to save context if all you restore is TIMER_ENABLE >>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>> situation where this driver is running and GT isn't enabled ? >>> >>> Now It's not. It's always enabled. I did it because .suspend() is called for >>> all registered clock sources regardless of their usage. So, potentially >>> in the future, at the moment when .suspend() is called it might be disabled >>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>> will not be registered as sched_clock, it will be possible to keep it disabled >>> if not used now). >>> >>> But It's not essentially now - I can update it and drop save restore. >>> Pls, confirm. >> >> I think it's best to skip suspend completely. You're not restoring >> anything you saved during suspend, unless you meant | where you used &. >> > > I didn't get it - I'm restoring one bit(0) only. that's the point, if you know you're restoring only that bit. Why save anything at all ? -- balbi [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:15 ` Felipe Balbi 0 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 18:15 UTC (permalink / raw) To: Grygorii Strashko, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren [-- Attachment #1: Type: text/plain, Size: 4098 bytes --] Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/13/2015 07:40 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>> >>>> Hi, >>>> >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>> Now the System stall is observed on TI AM437x based board >>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>> timer is selected as clocksource device - SysRq are working, but >>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>> contexts. >>>>> >>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>> System suspend: >>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>> GT_COUNTERx = 0 >>>>> >>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>> depending on presence of "always-on" DT property. >>>>> >>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>> --- >>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>> 1 file changed, 23 insertions(+) >>>>> >>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>> index a2cb6fa..1bbaf64 100644 >>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>> static unsigned long gt_clk_rate; >>>>> static int gt_ppi; >>>>> static struct clock_event_device __percpu *gt_evt; >>>>> +static bool gt_always_on; >>>>> +static u32 gt_control; >>>>> >>>>> /* >>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>> { >>>>> int cpu = smp_processor_id(); >>>>> >>>>> + if (!gt_always_on) >>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>> + >>>>> clk->name = "arm_global_timer"; >>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>> CLOCK_EVT_FEAT_PERCPU; >>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>> return gt_counter_read(); >>>>> } >>>>> >>>>> +static void gt_suspend(struct clocksource *cs) >>>>> +{ >>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>> +} >>>>> + >>>>> +static void gt_resume(struct clocksource *cs) >>>>> +{ >>>>> + /* enables timer on all the cores */ >>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>> >>>> do you really need to save context if all you restore is TIMER_ENABLE >>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>> situation where this driver is running and GT isn't enabled ? >>> >>> Now It's not. It's always enabled. I did it because .suspend() is called for >>> all registered clock sources regardless of their usage. So, potentially >>> in the future, at the moment when .suspend() is called it might be disabled >>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>> will not be registered as sched_clock, it will be possible to keep it disabled >>> if not used now). >>> >>> But It's not essentially now - I can update it and drop save restore. >>> Pls, confirm. >> >> I think it's best to skip suspend completely. You're not restoring >> anything you saved during suspend, unless you meant | where you used &. >> > > I didn't get it - I'm restoring one bit(0) only. that's the point, if you know you're restoring only that bit. Why save anything at all ? -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:15 ` Felipe Balbi 0 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 18:15 UTC (permalink / raw) To: linux-arm-kernel Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/13/2015 07:40 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>> >>>> Hi, >>>> >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>> Now the System stall is observed on TI AM437x based board >>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>> timer is selected as clocksource device - SysRq are working, but >>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>> contexts. >>>>> >>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>> System suspend: >>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>> GT_COUNTERx = 0 >>>>> >>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>> depending on presence of "always-on" DT property. >>>>> >>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>> --- >>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>> 1 file changed, 23 insertions(+) >>>>> >>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>> index a2cb6fa..1bbaf64 100644 >>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>> static unsigned long gt_clk_rate; >>>>> static int gt_ppi; >>>>> static struct clock_event_device __percpu *gt_evt; >>>>> +static bool gt_always_on; >>>>> +static u32 gt_control; >>>>> >>>>> /* >>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>> { >>>>> int cpu = smp_processor_id(); >>>>> >>>>> + if (!gt_always_on) >>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>> + >>>>> clk->name = "arm_global_timer"; >>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>> CLOCK_EVT_FEAT_PERCPU; >>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>> return gt_counter_read(); >>>>> } >>>>> >>>>> +static void gt_suspend(struct clocksource *cs) >>>>> +{ >>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>> +} >>>>> + >>>>> +static void gt_resume(struct clocksource *cs) >>>>> +{ >>>>> + /* enables timer on all the cores */ >>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>> >>>> do you really need to save context if all you restore is TIMER_ENABLE >>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>> situation where this driver is running and GT isn't enabled ? >>> >>> Now It's not. It's always enabled. I did it because .suspend() is called for >>> all registered clock sources regardless of their usage. So, potentially >>> in the future, at the moment when .suspend() is called it might be disabled >>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>> will not be registered as sched_clock, it will be possible to keep it disabled >>> if not used now). >>> >>> But It's not essentially now - I can update it and drop save restore. >>> Pls, confirm. >> >> I think it's best to skip suspend completely. You're not restoring >> anything you saved during suspend, unless you meant | where you used &. >> > > I didn't get it - I'm restoring one bit(0) only. that's the point, if you know you're restoring only that bit. Why save anything at all ? -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 818 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151113/cbd5df97/attachment.sig> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 18:15 ` Felipe Balbi (?) @ 2015-11-13 18:29 ` Grygorii Strashko -1 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 18:29 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren On 11/13/2015 08:15 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>> >>>>> Hi, >>>>> >>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>> Now the System stall is observed on TI AM437x based board >>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>> contexts. >>>>>> >>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>> System suspend: >>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>> GT_COUNTERx = 0 >>>>>> >>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>> depending on presence of "always-on" DT property. >>>>>> >>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>> --- >>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>> 1 file changed, 23 insertions(+) >>>>>> >>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>> index a2cb6fa..1bbaf64 100644 >>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>> static unsigned long gt_clk_rate; >>>>>> static int gt_ppi; >>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>> +static bool gt_always_on; >>>>>> +static u32 gt_control; >>>>>> >>>>>> /* >>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>> { >>>>>> int cpu = smp_processor_id(); >>>>>> >>>>>> + if (!gt_always_on) >>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>> + >>>>>> clk->name = "arm_global_timer"; >>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>> return gt_counter_read(); >>>>>> } >>>>>> >>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>> +{ >>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>> +} >>>>>> + >>>>>> +static void gt_resume(struct clocksource *cs) >>>>>> +{ >>>>>> + /* enables timer on all the cores */ >>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>> >>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>> situation where this driver is running and GT isn't enabled ? >>>> >>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>> all registered clock sources regardless of their usage. So, potentially >>>> in the future, at the moment when .suspend() is called it might be disabled >>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>> if not used now). >>>> >>>> But It's not essentially now - I can update it and drop save restore. >>>> Pls, confirm. >>> >>> I think it's best to skip suspend completely. You're not restoring >>> anything you saved during suspend, unless you meant | where you used &. >>> >> >> I didn't get it - I'm restoring one bit(0) only. > > that's the point, if you know you're restoring only that bit. Why save > anything at all ? > i think there are difference between "restoring" and "re-enabling". "restoring" - assume saving smth.. then restore saving value. I'm saving & restoring one bit here. But I can do just "re-enabling" - writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); and then I don't need to save anything. It will work with current code. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:29 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 18:29 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren On 11/13/2015 08:15 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>> >>>>> Hi, >>>>> >>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>> Now the System stall is observed on TI AM437x based board >>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>> contexts. >>>>>> >>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>> System suspend: >>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>> GT_COUNTERx = 0 >>>>>> >>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>> depending on presence of "always-on" DT property. >>>>>> >>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>> --- >>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>> 1 file changed, 23 insertions(+) >>>>>> >>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>> index a2cb6fa..1bbaf64 100644 >>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>> static unsigned long gt_clk_rate; >>>>>> static int gt_ppi; >>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>> +static bool gt_always_on; >>>>>> +static u32 gt_control; >>>>>> >>>>>> /* >>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>> { >>>>>> int cpu = smp_processor_id(); >>>>>> >>>>>> + if (!gt_always_on) >>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>> + >>>>>> clk->name = "arm_global_timer"; >>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>> return gt_counter_read(); >>>>>> } >>>>>> >>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>> +{ >>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>> +} >>>>>> + >>>>>> +static void gt_resume(struct clocksource *cs) >>>>>> +{ >>>>>> + /* enables timer on all the cores */ >>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>> >>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>> situation where this driver is running and GT isn't enabled ? >>>> >>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>> all registered clock sources regardless of their usage. So, potentially >>>> in the future, at the moment when .suspend() is called it might be disabled >>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>> if not used now). >>>> >>>> But It's not essentially now - I can update it and drop save restore. >>>> Pls, confirm. >>> >>> I think it's best to skip suspend completely. You're not restoring >>> anything you saved during suspend, unless you meant | where you used &. >>> >> >> I didn't get it - I'm restoring one bit(0) only. > > that's the point, if you know you're restoring only that bit. Why save > anything at all ? > i think there are difference between "restoring" and "re-enabling". "restoring" - assume saving smth.. then restore saving value. I'm saving & restoring one bit here. But I can do just "re-enabling" - writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); and then I don't need to save anything. It will work with current code. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:29 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 18:29 UTC (permalink / raw) To: linux-arm-kernel On 11/13/2015 08:15 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>> >>>>> Hi, >>>>> >>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>> Now the System stall is observed on TI AM437x based board >>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>> contexts. >>>>>> >>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>> System suspend: >>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>> GT_COUNTERx = 0 >>>>>> >>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>> depending on presence of "always-on" DT property. >>>>>> >>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>> --- >>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>> 1 file changed, 23 insertions(+) >>>>>> >>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>> index a2cb6fa..1bbaf64 100644 >>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>> static unsigned long gt_clk_rate; >>>>>> static int gt_ppi; >>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>> +static bool gt_always_on; >>>>>> +static u32 gt_control; >>>>>> >>>>>> /* >>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>> { >>>>>> int cpu = smp_processor_id(); >>>>>> >>>>>> + if (!gt_always_on) >>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>> + >>>>>> clk->name = "arm_global_timer"; >>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>> return gt_counter_read(); >>>>>> } >>>>>> >>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>> +{ >>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>> +} >>>>>> + >>>>>> +static void gt_resume(struct clocksource *cs) >>>>>> +{ >>>>>> + /* enables timer on all the cores */ >>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>> >>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>> situation where this driver is running and GT isn't enabled ? >>>> >>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>> all registered clock sources regardless of their usage. So, potentially >>>> in the future, at the moment when .suspend() is called it might be disabled >>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>> if not used now). >>>> >>>> But It's not essentially now - I can update it and drop save restore. >>>> Pls, confirm. >>> >>> I think it's best to skip suspend completely. You're not restoring >>> anything you saved during suspend, unless you meant | where you used &. >>> >> >> I didn't get it - I'm restoring one bit(0) only. > > that's the point, if you know you're restoring only that bit. Why save > anything at all ? > i think there are difference between "restoring" and "re-enabling". "restoring" - assume saving smth.. then restore saving value. I'm saving & restoring one bit here. But I can do just "re-enabling" - writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); and then I don't need to save anything. It will work with current code. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 18:29 ` Grygorii Strashko (?) @ 2015-11-13 18:32 ` Felipe Balbi -1 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 18:32 UTC (permalink / raw) To: Grygorii Strashko, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren [-- Attachment #1: Type: text/plain, Size: 4788 bytes --] Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/13/2015 08:15 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>>> >>>> Hi, >>>> >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>>> Now the System stall is observed on TI AM437x based board >>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>>> contexts. >>>>>>> >>>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>>> System suspend: >>>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>>> GT_COUNTERx = 0 >>>>>>> >>>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>>> depending on presence of "always-on" DT property. >>>>>>> >>>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>>> --- >>>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>>> 1 file changed, 23 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>>> index a2cb6fa..1bbaf64 100644 >>>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>>> static unsigned long gt_clk_rate; >>>>>>> static int gt_ppi; >>>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>>> +static bool gt_always_on; >>>>>>> +static u32 gt_control; >>>>>>> >>>>>>> /* >>>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>>> { >>>>>>> int cpu = smp_processor_id(); >>>>>>> >>>>>>> + if (!gt_always_on) >>>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>>> + >>>>>>> clk->name = "arm_global_timer"; >>>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>>> return gt_counter_read(); >>>>>>> } >>>>>>> >>>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>>> +{ >>>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>>> +} >>>>>>> + >>>>>>> +static void gt_resume(struct clocksource *cs) >>>>>>> +{ >>>>>>> + /* enables timer on all the cores */ >>>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>>> >>>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>>> situation where this driver is running and GT isn't enabled ? >>>>> >>>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>>> all registered clock sources regardless of their usage. So, potentially >>>>> in the future, at the moment when .suspend() is called it might be disabled >>>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>>> if not used now). >>>>> >>>>> But It's not essentially now - I can update it and drop save restore. >>>>> Pls, confirm. >>>> >>>> I think it's best to skip suspend completely. You're not restoring >>>> anything you saved during suspend, unless you meant | where you used &. >>>> >>> >>> I didn't get it - I'm restoring one bit(0) only. >> >> that's the point, if you know you're restoring only that bit. Why save >> anything at all ? >> > > i think there are difference between "restoring" and "re-enabling". > "restoring" - assume saving smth.. then restore saving value. > I'm saving & restoring one bit here. with your current suspend/resume, they are the same thing. You save GT_CONTROL contents, timer goes off and looses context, you set ENABLE bit. No difference what so ever. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:32 ` Felipe Balbi 0 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 18:32 UTC (permalink / raw) To: Grygorii Strashko, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren [-- Attachment #1: Type: text/plain, Size: 4788 bytes --] Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/13/2015 08:15 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>>> >>>> Hi, >>>> >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>>> Now the System stall is observed on TI AM437x based board >>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>>> contexts. >>>>>>> >>>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>>> System suspend: >>>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>>> GT_COUNTERx = 0 >>>>>>> >>>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>>> depending on presence of "always-on" DT property. >>>>>>> >>>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>>> --- >>>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>>> 1 file changed, 23 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>>> index a2cb6fa..1bbaf64 100644 >>>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>>> static unsigned long gt_clk_rate; >>>>>>> static int gt_ppi; >>>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>>> +static bool gt_always_on; >>>>>>> +static u32 gt_control; >>>>>>> >>>>>>> /* >>>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>>> { >>>>>>> int cpu = smp_processor_id(); >>>>>>> >>>>>>> + if (!gt_always_on) >>>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>>> + >>>>>>> clk->name = "arm_global_timer"; >>>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>>> return gt_counter_read(); >>>>>>> } >>>>>>> >>>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>>> +{ >>>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>>> +} >>>>>>> + >>>>>>> +static void gt_resume(struct clocksource *cs) >>>>>>> +{ >>>>>>> + /* enables timer on all the cores */ >>>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>>> >>>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>>> situation where this driver is running and GT isn't enabled ? >>>>> >>>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>>> all registered clock sources regardless of their usage. So, potentially >>>>> in the future, at the moment when .suspend() is called it might be disabled >>>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>>> if not used now). >>>>> >>>>> But It's not essentially now - I can update it and drop save restore. >>>>> Pls, confirm. >>>> >>>> I think it's best to skip suspend completely. You're not restoring >>>> anything you saved during suspend, unless you meant | where you used &. >>>> >>> >>> I didn't get it - I'm restoring one bit(0) only. >> >> that's the point, if you know you're restoring only that bit. Why save >> anything at all ? >> > > i think there are difference between "restoring" and "re-enabling". > "restoring" - assume saving smth.. then restore saving value. > I'm saving & restoring one bit here. with your current suspend/resume, they are the same thing. You save GT_CONTROL contents, timer goes off and looses context, you set ENABLE bit. No difference what so ever. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:32 ` Felipe Balbi 0 siblings, 0 replies; 32+ messages in thread From: Felipe Balbi @ 2015-11-13 18:32 UTC (permalink / raw) To: linux-arm-kernel Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/13/2015 08:15 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>>> >>>> Hi, >>>> >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>>> Now the System stall is observed on TI AM437x based board >>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>>> contexts. >>>>>>> >>>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>>> System suspend: >>>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>>> GT_COUNTERx = 0 >>>>>>> >>>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>>> depending on presence of "always-on" DT property. >>>>>>> >>>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>>> --- >>>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>>> 1 file changed, 23 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>>> index a2cb6fa..1bbaf64 100644 >>>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>>> static unsigned long gt_clk_rate; >>>>>>> static int gt_ppi; >>>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>>> +static bool gt_always_on; >>>>>>> +static u32 gt_control; >>>>>>> >>>>>>> /* >>>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>>> { >>>>>>> int cpu = smp_processor_id(); >>>>>>> >>>>>>> + if (!gt_always_on) >>>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>>> + >>>>>>> clk->name = "arm_global_timer"; >>>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>>> return gt_counter_read(); >>>>>>> } >>>>>>> >>>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>>> +{ >>>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>>> +} >>>>>>> + >>>>>>> +static void gt_resume(struct clocksource *cs) >>>>>>> +{ >>>>>>> + /* enables timer on all the cores */ >>>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>>> >>>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>>> situation where this driver is running and GT isn't enabled ? >>>>> >>>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>>> all registered clock sources regardless of their usage. So, potentially >>>>> in the future, at the moment when .suspend() is called it might be disabled >>>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>>> if not used now). >>>>> >>>>> But It's not essentially now - I can update it and drop save restore. >>>>> Pls, confirm. >>>> >>>> I think it's best to skip suspend completely. You're not restoring >>>> anything you saved during suspend, unless you meant | where you used &. >>>> >>> >>> I didn't get it - I'm restoring one bit(0) only. >> >> that's the point, if you know you're restoring only that bit. Why save >> anything at all ? >> > > i think there are difference between "restoring" and "re-enabling". > "restoring" - assume saving smth.. then restore saving value. > I'm saving & restoring one bit here. with your current suspend/resume, they are the same thing. You save GT_CONTROL contents, timer goes off and looses context, you set ENABLE bit. No difference what so ever. -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 818 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151113/8cd09b78/attachment.sig> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 18:32 ` Felipe Balbi (?) @ 2015-11-13 18:59 ` Grygorii Strashko -1 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 18:59 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren On 11/13/2015 08:32 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/13/2015 08:15 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>>>> >>>>> Hi, >>>>> >>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>>>> Now the System stall is observed on TI AM437x based board >>>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>>>> contexts. >>>>>>>> >>>>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>>>> System suspend: >>>>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>>>> GT_COUNTERx = 0 >>>>>>>> >>>>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>>>> depending on presence of "always-on" DT property. >>>>>>>> >>>>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>>>> --- >>>>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>>>> 1 file changed, 23 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>>>> index a2cb6fa..1bbaf64 100644 >>>>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>>>> static unsigned long gt_clk_rate; >>>>>>>> static int gt_ppi; >>>>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>>>> +static bool gt_always_on; >>>>>>>> +static u32 gt_control; >>>>>>>> >>>>>>>> /* >>>>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>>>> { >>>>>>>> int cpu = smp_processor_id(); >>>>>>>> >>>>>>>> + if (!gt_always_on) >>>>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>>>> + >>>>>>>> clk->name = "arm_global_timer"; >>>>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>>>> return gt_counter_read(); >>>>>>>> } >>>>>>>> >>>>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>>>> +{ >>>>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void gt_resume(struct clocksource *cs) >>>>>>>> +{ >>>>>>>> + /* enables timer on all the cores */ >>>>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>>>> >>>>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>>>> situation where this driver is running and GT isn't enabled ? >>>>>> >>>>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>>>> all registered clock sources regardless of their usage. So, potentially >>>>>> in the future, at the moment when .suspend() is called it might be disabled >>>>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>>>> if not used now). >>>>>> >>>>>> But It's not essentially now - I can update it and drop save restore. >>>>>> Pls, confirm. >>>>> >>>>> I think it's best to skip suspend completely. You're not restoring >>>>> anything you saved during suspend, unless you meant | where you used &. >>>>> >>>> >>>> I didn't get it - I'm restoring one bit(0) only. >>> >>> that's the point, if you know you're restoring only that bit. Why save >>> anything at all ? >>> >> >> i think there are difference between "restoring" and "re-enabling". >> "restoring" - assume saving smth.. then restore saving value. >> I'm saving & restoring one bit here. > > with your current suspend/resume, they are the same thing. You save > GT_CONTROL contents, timer goes off and looses context, you set ENABLE > bit. No difference what so ever. > I'm writing: gt_control & GT_CONTROL_TIMER_ENABLE, where gt_control is old value of GT_CONTROL before entering suspend, so on resume: - if gt_control == 0x0000 0001 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x1 - if gt_control == 0x0000 0000 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x0 What am I missed? -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:59 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 18:59 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren On 11/13/2015 08:32 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/13/2015 08:15 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>>>> >>>>> Hi, >>>>> >>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>>>> Now the System stall is observed on TI AM437x based board >>>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>>>> contexts. >>>>>>>> >>>>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>>>> System suspend: >>>>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>>>> GT_COUNTERx = 0 >>>>>>>> >>>>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>>>> depending on presence of "always-on" DT property. >>>>>>>> >>>>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>>>> --- >>>>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>>>> 1 file changed, 23 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>>>> index a2cb6fa..1bbaf64 100644 >>>>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>>>> static unsigned long gt_clk_rate; >>>>>>>> static int gt_ppi; >>>>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>>>> +static bool gt_always_on; >>>>>>>> +static u32 gt_control; >>>>>>>> >>>>>>>> /* >>>>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>>>> { >>>>>>>> int cpu = smp_processor_id(); >>>>>>>> >>>>>>>> + if (!gt_always_on) >>>>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>>>> + >>>>>>>> clk->name = "arm_global_timer"; >>>>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>>>> return gt_counter_read(); >>>>>>>> } >>>>>>>> >>>>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>>>> +{ >>>>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void gt_resume(struct clocksource *cs) >>>>>>>> +{ >>>>>>>> + /* enables timer on all the cores */ >>>>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>>>> >>>>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>>>> situation where this driver is running and GT isn't enabled ? >>>>>> >>>>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>>>> all registered clock sources regardless of their usage. So, potentially >>>>>> in the future, at the moment when .suspend() is called it might be disabled >>>>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>>>> if not used now). >>>>>> >>>>>> But It's not essentially now - I can update it and drop save restore. >>>>>> Pls, confirm. >>>>> >>>>> I think it's best to skip suspend completely. You're not restoring >>>>> anything you saved during suspend, unless you meant | where you used &. >>>>> >>>> >>>> I didn't get it - I'm restoring one bit(0) only. >>> >>> that's the point, if you know you're restoring only that bit. Why save >>> anything at all ? >>> >> >> i think there are difference between "restoring" and "re-enabling". >> "restoring" - assume saving smth.. then restore saving value. >> I'm saving & restoring one bit here. > > with your current suspend/resume, they are the same thing. You save > GT_CONTROL contents, timer goes off and looses context, you set ENABLE > bit. No difference what so ever. > I'm writing: gt_control & GT_CONTROL_TIMER_ENABLE, where gt_control is old value of GT_CONTROL before entering suspend, so on resume: - if gt_control == 0x0000 0001 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x1 - if gt_control == 0x0000 0000 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x0 What am I missed? -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 18:59 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 18:59 UTC (permalink / raw) To: linux-arm-kernel On 11/13/2015 08:32 PM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/13/2015 08:15 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>>>> >>>>> Hi, >>>>> >>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>>>> Now the System stall is observed on TI AM437x based board >>>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>>>> contexts. >>>>>>>> >>>>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>>>> System suspend: >>>>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>>>> GT_COUNTERx = 0 >>>>>>>> >>>>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>>>> depending on presence of "always-on" DT property. >>>>>>>> >>>>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>>>> --- >>>>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>>>> 1 file changed, 23 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>>>> index a2cb6fa..1bbaf64 100644 >>>>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>>>> static unsigned long gt_clk_rate; >>>>>>>> static int gt_ppi; >>>>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>>>> +static bool gt_always_on; >>>>>>>> +static u32 gt_control; >>>>>>>> >>>>>>>> /* >>>>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>>>> { >>>>>>>> int cpu = smp_processor_id(); >>>>>>>> >>>>>>>> + if (!gt_always_on) >>>>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>>>> + >>>>>>>> clk->name = "arm_global_timer"; >>>>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>>>> return gt_counter_read(); >>>>>>>> } >>>>>>>> >>>>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>>>> +{ >>>>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void gt_resume(struct clocksource *cs) >>>>>>>> +{ >>>>>>>> + /* enables timer on all the cores */ >>>>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>>>> >>>>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>>>> situation where this driver is running and GT isn't enabled ? >>>>>> >>>>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>>>> all registered clock sources regardless of their usage. So, potentially >>>>>> in the future, at the moment when .suspend() is called it might be disabled >>>>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>>>> if not used now). >>>>>> >>>>>> But It's not essentially now - I can update it and drop save restore. >>>>>> Pls, confirm. >>>>> >>>>> I think it's best to skip suspend completely. You're not restoring >>>>> anything you saved during suspend, unless you meant | where you used &. >>>>> >>>> >>>> I didn't get it - I'm restoring one bit(0) only. >>> >>> that's the point, if you know you're restoring only that bit. Why save >>> anything at all ? >>> >> >> i think there are difference between "restoring" and "re-enabling". >> "restoring" - assume saving smth.. then restore saving value. >> I'm saving & restoring one bit here. > > with your current suspend/resume, they are the same thing. You save > GT_CONTROL contents, timer goes off and looses context, you set ENABLE > bit. No difference what so ever. > I'm writing: gt_control & GT_CONTROL_TIMER_ENABLE, where gt_control is old value of GT_CONTROL before entering suspend, so on resume: - if gt_control == 0x0000 0001 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x1 - if gt_control == 0x0000 0000 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x0 What am I missed? -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 18:59 ` Grygorii Strashko (?) @ 2015-11-13 19:14 ` Grygorii Strashko -1 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 19:14 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren Hi Felipe, On 11/13/2015 08:59 PM, Grygorii Strashko wrote: > On 11/13/2015 08:32 PM, Felipe Balbi wrote: >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/13/2015 08:15 PM, Felipe Balbi wrote: >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>>>>> GrygoCONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCKrii Strashko <grygorii.strashko@ti.com> writes: >>>>>>>>> Now the System stall is observed on TI AM437x based board >>>>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>>>>> contexts. >>>>>>>>> >>>>>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>>>>> System suspend: >>>>>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>>>>> GT_COUNTERx = 0 >>>>>>>>> >>>>>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>>>>> depending on presence of "always-on" DT property. >>>>>>>>> >>>>>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>>>>> --- >>>>>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>>>>> 1 file changed, 23 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>>>>> index a2cb6fa..1bbaf64 100644 >>>>>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>>>>> static unsigned long gt_clk_rate; >>>>>>>>> static int gt_ppi; >>>>>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>>>>> +static bool gt_always_on; >>>>>>>>> +static u32 gt_control; >>>>>>>>> >>>>>>>>> /* >>>>>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>>>>> { >>>>>>>>> int cpu = smp_processor_id(); >>>>>>>>> >>>>>>>>> + if (!gt_always_on) >>>>>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>>>>> + >>>>>>>>> clk->name = "arm_global_timer"; >>>>>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>>>>> return gt_counter_read(); >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>>>>> +{ >>>>>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static void gt_resume(struct clocksource *cs) >>>>>>>>> +{ >>>>>>>>> + /* enables timer on all the cores */ >>>>>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>>>>> >>>>>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>>>>> situation where this driver is running and GT isn't enabled ? >>>>>>> >>>>>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>>>>> all registered clock sources regardless of their usage. So, potentially >>>>>>> in the future, at the moment when .suspend() is called it might be disabled >>>>>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>>>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>>>>> if not used now). >>>>>>> >>>>>>> But It's not essentially now - I can update it and drop save restore. >>>>>>> Pls, confirm. >>>>>> >>>>>> I think it's best to skip suspend completely. You're not restoring >>>>>> anything you saved during suspend, unless you meant | where you used &. >>>>>> >>>>> >>>>> I didn't get it - I'm restoring one bit(0) only. >>>> >>>> that's the point, if you know you're restoring only that bit. Why save >>>> anything at all ? >>>> >>> >>> i think there are difference between "restoring" and "re-enabling". >>> "restoring" - assume saving smth.. then restore saving value. >>> I'm saving & restoring one bit here. >> >> with your current suspend/resume, they are the same thing. You save >> GT_CONTROL contents, timer goes off and looses context, you set ENABLE >> bit. No difference what so ever. >> > > I'm writing: > gt_control & GT_CONTROL_TIMER_ENABLE, where gt_control is old value of GT_CONTROL > before entering suspend, so on resume: > - if gt_control == 0x0000 0001 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x1 > - if gt_control == 0x0000 0000 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x0 > > What am I missed? > Any way, I'll wait for other comments a bit, replace save/restore with just pure re-enabling and re-send. Thanks. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 19:14 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 19:14 UTC (permalink / raw) To: Felipe Balbi, linux, Daniel Lezcano, Thomas Gleixner, Srinivas Kandagatla, Maxime Coquelin Cc: linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren Hi Felipe, On 11/13/2015 08:59 PM, Grygorii Strashko wrote: > On 11/13/2015 08:32 PM, Felipe Balbi wrote: >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/13/2015 08:15 PM, Felipe Balbi wrote: >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>>>>> GrygoCONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCKrii Strashko <grygorii.strashko@ti.com> writes: >>>>>>>>> Now the System stall is observed on TI AM437x based board >>>>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>>>>> contexts. >>>>>>>>> >>>>>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>>>>> System suspend: >>>>>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>>>>> GT_COUNTERx = 0 >>>>>>>>> >>>>>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>>>>> depending on presence of "always-on" DT property. >>>>>>>>> >>>>>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>>>>> --- >>>>>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>>>>> 1 file changed, 23 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>>>>> index a2cb6fa..1bbaf64 100644 >>>>>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>>>>> static unsigned long gt_clk_rate; >>>>>>>>> static int gt_ppi; >>>>>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>>>>> +static bool gt_always_on; >>>>>>>>> +static u32 gt_control; >>>>>>>>> >>>>>>>>> /* >>>>>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>>>>> { >>>>>>>>> int cpu = smp_processor_id(); >>>>>>>>> >>>>>>>>> + if (!gt_always_on) >>>>>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>>>>> + >>>>>>>>> clk->name = "arm_global_timer"; >>>>>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>>>>> return gt_counter_read(); >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>>>>> +{ >>>>>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static void gt_resume(struct clocksource *cs) >>>>>>>>> +{ >>>>>>>>> + /* enables timer on all the cores */ >>>>>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>>>>> >>>>>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>>>>> situation where this driver is running and GT isn't enabled ? >>>>>>> >>>>>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>>>>> all registered clock sources regardless of their usage. So, potentially >>>>>>> in the future, at the moment when .suspend() is called it might be disabled >>>>>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>>>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>>>>> if not used now). >>>>>>> >>>>>>> But It's not essentially now - I can update it and drop save restore. >>>>>>> Pls, confirm. >>>>>> >>>>>> I think it's best to skip suspend completely. You're not restoring >>>>>> anything you saved during suspend, unless you meant | where you used &. >>>>>> >>>>> >>>>> I didn't get it - I'm restoring one bit(0) only. >>>> >>>> that's the point, if you know you're restoring only that bit. Why save >>>> anything at all ? >>>> >>> >>> i think there are difference between "restoring" and "re-enabling". >>> "restoring" - assume saving smth.. then restore saving value. >>> I'm saving & restoring one bit here. >> >> with your current suspend/resume, they are the same thing. You save >> GT_CONTROL contents, timer goes off and looses context, you set ENABLE >> bit. No difference what so ever. >> > > I'm writing: > gt_control & GT_CONTROL_TIMER_ENABLE, where gt_control is old value of GT_CONTROL > before entering suspend, so on resume: > - if gt_control == 0x0000 0001 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x1 > - if gt_control == 0x0000 0000 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x0 > > What am I missed? > Any way, I'll wait for other comments a bit, replace save/restore with just pure re-enabling and re-send. Thanks. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 19:14 ` Grygorii Strashko 0 siblings, 0 replies; 32+ messages in thread From: Grygorii Strashko @ 2015-11-13 19:14 UTC (permalink / raw) To: linux-arm-kernel Hi Felipe, On 11/13/2015 08:59 PM, Grygorii Strashko wrote: > On 11/13/2015 08:32 PM, Felipe Balbi wrote: >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/13/2015 08:15 PM, Felipe Balbi wrote: >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>> On 11/13/2015 07:40 PM, Felipe Balbi wrote: >>>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>>>>> On 11/13/2015 06:43 PM, Felipe Balbi wrote: >>>>>>>> GrygoCONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCKrii Strashko <grygorii.strashko@ti.com> writes: >>>>>>>>> Now the System stall is observed on TI AM437x based board >>>>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global >>>>>>>>> timer is selected as clocksource device - SysRq are working, but >>>>>>>>> nothing else. The reason of stall is that ARM Global timer loses its >>>>>>>>> contexts. >>>>>>>>> >>>>>>>>> The reason of stall is that ARM Global timer loses its contexts during >>>>>>>>> System suspend: >>>>>>>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked) >>>>>>>>> GT_COUNTERx = 0 >>>>>>>>> >>>>>>>>> Hence, update ARM Global timer driver to reflect above behaviour >>>>>>>>> - save GT_CONTROL.TIMER_ENABLE during suspend and restore on resume; >>>>>>>>> - ensure clocksource and clockevent devices have coresponding flags >>>>>>>>> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set >>>>>>>>> depending on presence of "always-on" DT property. >>>>>>>>> >>>>>>>>> CC: Arnd Bergmann <arnd@arndb.de> >>>>>>>>> Cc: John Stultz <john.stultz@linaro.org> >>>>>>>>> Cc: Felipe Balbi <balbi@ti.com> >>>>>>>>> Cc: Tony Lindgren <tony@atomide.com> >>>>>>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>>>>>>> --- >>>>>>>>> drivers/clocksource/arm_global_timer.c | 23 +++++++++++++++++++++++ >>>>>>>>> 1 file changed, 23 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >>>>>>>>> index a2cb6fa..1bbaf64 100644 >>>>>>>>> --- a/drivers/clocksource/arm_global_timer.c >>>>>>>>> +++ b/drivers/clocksource/arm_global_timer.c >>>>>>>>> @@ -51,6 +51,8 @@ static void __iomem *gt_base; >>>>>>>>> static unsigned long gt_clk_rate; >>>>>>>>> static int gt_ppi; >>>>>>>>> static struct clock_event_device __percpu *gt_evt; >>>>>>>>> +static bool gt_always_on; >>>>>>>>> +static u32 gt_control; >>>>>>>>> >>>>>>>>> /* >>>>>>>>> * To get the value from the Global Timer Counter register proceed as follows: >>>>>>>>> @@ -168,6 +170,9 @@ static int gt_clockevents_init(struct clock_event_device *clk) >>>>>>>>> { >>>>>>>>> int cpu = smp_processor_id(); >>>>>>>>> >>>>>>>>> + if (!gt_always_on) >>>>>>>>> + clk->features |= CLOCK_EVT_FEAT_C3STOP; >>>>>>>>> + >>>>>>>>> clk->name = "arm_global_timer"; >>>>>>>>> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >>>>>>>>> CLOCK_EVT_FEAT_PERCPU; >>>>>>>>> @@ -195,12 +200,25 @@ static cycle_t gt_clocksource_read(struct clocksource *cs) >>>>>>>>> return gt_counter_read(); >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static void gt_suspend(struct clocksource *cs) >>>>>>>>> +{ >>>>>>>>> + gt_control = readl(gt_base + GT_CONTROL); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static void gt_resume(struct clocksource *cs) >>>>>>>>> +{ >>>>>>>>> + /* enables timer on all the cores */ >>>>>>>>> + writel(gt_control & GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >>>>>>>> >>>>>>>> do you really need to save context if all you restore is TIMER_ENABLE >>>>>>>> bit ? seems like you could skip gt_suspend altogether. Is there really a >>>>>>>> situation where this driver is running and GT isn't enabled ? >>>>>>> >>>>>>> Now It's not. It's always enabled. I did it because .suspend() is called for >>>>>>> all registered clock sources regardless of their usage. So, potentially >>>>>>> in the future, at the moment when .suspend() is called it might be disabled >>>>>>> (for example, .enable/disable() callbacks can be added and, if ARM Global timer >>>>>>> will not be registered as sched_clock, it will be possible to keep it disabled >>>>>>> if not used now). >>>>>>> >>>>>>> But It's not essentially now - I can update it and drop save restore. >>>>>>> Pls, confirm. >>>>>> >>>>>> I think it's best to skip suspend completely. You're not restoring >>>>>> anything you saved during suspend, unless you meant | where you used &. >>>>>> >>>>> >>>>> I didn't get it - I'm restoring one bit(0) only. >>>> >>>> that's the point, if you know you're restoring only that bit. Why save >>>> anything at all ? >>>> >>> >>> i think there are difference between "restoring" and "re-enabling". >>> "restoring" - assume saving smth.. then restore saving value. >>> I'm saving & restoring one bit here. >> >> with your current suspend/resume, they are the same thing. You save >> GT_CONTROL contents, timer goes off and looses context, you set ENABLE >> bit. No difference what so ever. >> > > I'm writing: > gt_control & GT_CONTROL_TIMER_ENABLE, where gt_control is old value of GT_CONTROL > before entering suspend, so on resume: > - if gt_control == 0x0000 0001 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x1 > - if gt_control == 0x0000 0000 ==> gt_control & GT_CONTROL_TIMER_ENABLE(0x1) == 0x0 > > What am I missed? > Any way, I'll wait for other comments a bit, replace save/restore with just pure re-enabling and re-send. Thanks. -- regards, -grygorii ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] clocksource: arm_global_timer: fix suspend resume 2015-11-13 19:14 ` Grygorii Strashko @ 2015-11-13 19:34 ` Thomas Gleixner -1 siblings, 0 replies; 32+ messages in thread From: Thomas Gleixner @ 2015-11-13 19:34 UTC (permalink / raw) To: Grygorii Strashko Cc: Felipe Balbi, linux, Daniel Lezcano, Srinivas Kandagatla, Maxime Coquelin, linux-kernel, linux-arm-kernel, kernel, linux-omap, Arnd Bergmann, John Stultz, Tony Lindgren On Fri, 13 Nov 2015, Grygorii Strashko wrote: <Delete 100 lines of useless information> > Any way, I'll wait for other comments a bit, replace save/restore with > just pure re-enabling and re-send. Please folks, trim your emails! I'm really tired of going through hundreds of quoted (quoting level here was 10!!!) useless lines to find a single line of useful information. Thanks, tglx ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] clocksource: arm_global_timer: fix suspend resume @ 2015-11-13 19:34 ` Thomas Gleixner 0 siblings, 0 replies; 32+ messages in thread From: Thomas Gleixner @ 2015-11-13 19:34 UTC (permalink / raw) To: linux-arm-kernel On Fri, 13 Nov 2015, Grygorii Strashko wrote: <Delete 100 lines of useless information> > Any way, I'll wait for other comments a bit, replace save/restore with > just pure re-enabling and re-send. Please folks, trim your emails! I'm really tired of going through hundreds of quoted (quoting level here was 10!!!) useless lines to find a single line of useful information. Thanks, tglx ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2015-11-13 19:34 UTC | newest] Thread overview: 32+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-13 16:25 [PATCH] clocksource: arm_global_timer: fix suspend resume Grygorii Strashko 2015-11-13 16:25 ` Grygorii Strashko 2015-11-13 16:25 ` Grygorii Strashko 2015-11-13 16:43 ` Felipe Balbi 2015-11-13 16:43 ` Felipe Balbi 2015-11-13 16:43 ` Felipe Balbi 2015-11-13 16:57 ` Grygorii Strashko 2015-11-13 16:57 ` Grygorii Strashko 2015-11-13 16:57 ` Grygorii Strashko 2015-11-13 17:40 ` Felipe Balbi 2015-11-13 17:40 ` Felipe Balbi 2015-11-13 17:40 ` Felipe Balbi 2015-11-13 18:09 ` Grygorii Strashko 2015-11-13 18:09 ` Grygorii Strashko 2015-11-13 18:09 ` Grygorii Strashko 2015-11-13 18:15 ` Felipe Balbi 2015-11-13 18:15 ` Felipe Balbi 2015-11-13 18:15 ` Felipe Balbi 2015-11-13 18:29 ` Grygorii Strashko 2015-11-13 18:29 ` Grygorii Strashko 2015-11-13 18:29 ` Grygorii Strashko 2015-11-13 18:32 ` Felipe Balbi 2015-11-13 18:32 ` Felipe Balbi 2015-11-13 18:32 ` Felipe Balbi 2015-11-13 18:59 ` Grygorii Strashko 2015-11-13 18:59 ` Grygorii Strashko 2015-11-13 18:59 ` Grygorii Strashko 2015-11-13 19:14 ` Grygorii Strashko 2015-11-13 19:14 ` Grygorii Strashko 2015-11-13 19:14 ` Grygorii Strashko 2015-11-13 19:34 ` Thomas Gleixner 2015-11-13 19:34 ` Thomas Gleixner
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.