When debugging suspend problems on Exynos 5260, I had a large number
of debugging prints going to the serial port after interrupts
had been disabled but before the timer interrupt was shutdown. This
was long enough for a timer tick to occur, but as interrupts were
disabled the ISR didn't run, and so the interrupt wasn't cleared.
Later when the timer was shutdown the interrupt was left asserted and
so the wfi at the heart of the suspend code didn't wait, causing
the suspend to fail.
Currently the code which stops the timer when it is on one-shot mode
and the interrupt occurs is in exynos4_mct_tick_clear(), meaning if we
called this from the shutdown code exynos4_mct_tick_stop() could be
called twice. So first restructure the existing code, so the check for
one-shot mode and stopping the timer is moved to the ISR, leaving
exynos4_mct_tick_clear() just clearing the interrupt flag.
Once this has been done simply call exynos4_mct_tick_clear() from
set_state_shutdown().
Stuart Menefy (2):
clocksource: exynos_mct: Move one-shot check from tick clear to ISR
clocksource: exynos_mct: Clear timer interrupt when shutdown
drivers/clocksource/exynos_mct.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
--
2.13.6
When a timer tick occurs and the clock is in one-shot mode, the timer
needs to be stopped to prevent it triggering subsequent interrupts.
Currently this code is in exynos4_mct_tick_clear(), but as it is
only needed when an ISR occurs move it into exynos4_mct_tick_isr(),
leaving exynos4_mct_tick_clear() just doing what its name suggests it
should.
Signed-off-by: Stuart Menefy <[email protected]>
---
drivers/clocksource/exynos_mct.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 7a244b681876..1e325f89d408 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -388,6 +388,13 @@ static void exynos4_mct_tick_start(unsigned long cycles,
exynos4_mct_write(tmp, mevt->base + MCT_L_TCON_OFFSET);
}
+static void exynos4_mct_tick_clear(struct mct_clock_event_device *mevt)
+{
+ /* Clear the MCT tick interrupt */
+ if (readl_relaxed(reg_base + mevt->base + MCT_L_INT_CSTAT_OFFSET) & 1)
+ exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
+}
+
static int exynos4_tick_set_next_event(unsigned long cycles,
struct clock_event_device *evt)
{
@@ -420,8 +427,11 @@ static int set_state_periodic(struct clock_event_device *evt)
return 0;
}
-static void exynos4_mct_tick_clear(struct mct_clock_event_device *mevt)
+static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
{
+ struct mct_clock_event_device *mevt = dev_id;
+ struct clock_event_device *evt = &mevt->evt;
+
/*
* This is for supporting oneshot mode.
* Mct would generate interrupt periodically
@@ -430,16 +440,6 @@ static void exynos4_mct_tick_clear(struct mct_clock_event_device *mevt)
if (!clockevent_state_periodic(&mevt->evt))
exynos4_mct_tick_stop(mevt);
- /* Clear the MCT tick interrupt */
- if (readl_relaxed(reg_base + mevt->base + MCT_L_INT_CSTAT_OFFSET) & 1)
- exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
-}
-
-static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
-{
- struct mct_clock_event_device *mevt = dev_id;
- struct clock_event_device *evt = &mevt->evt;
-
exynos4_mct_tick_clear(mevt);
evt->event_handler(evt);
--
2.13.6
When shutting down the timer, ensure that after we have stopped the
timer any pending interrupts are cleared. This fixes a problem when
suspending, as interrupts are disabled before the timer is stopped,
so the timer interrupt may still be asserted, preventing the system
entering a low power state when the wfi is executed.
Signed-off-by: Stuart Menefy <[email protected]>
---
drivers/clocksource/exynos_mct.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 1e325f89d408..d55c30f6981d 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -411,6 +411,7 @@ static int set_state_shutdown(struct clock_event_device *evt)
mevt = container_of(evt, struct mct_clock_event_device, evt);
exynos4_mct_tick_stop(mevt);
+ exynos4_mct_tick_clear(mevt);
return 0;
}
--
2.13.6
Hi Stuart
On 2019-02-10 23:51, Stuart Menefy wrote:
> When debugging suspend problems on Exynos 5260, I had a large number
> of debugging prints going to the serial port after interrupts
> had been disabled but before the timer interrupt was shutdown. This
> was long enough for a timer tick to occur, but as interrupts were
> disabled the ISR didn't run, and so the interrupt wasn't cleared.
> Later when the timer was shutdown the interrupt was left asserted and
> so the wfi at the heart of the suspend code didn't wait, causing
> the suspend to fail.
>
> Currently the code which stops the timer when it is on one-shot mode
> and the interrupt occurs is in exynos4_mct_tick_clear(), meaning if we
> called this from the shutdown code exynos4_mct_tick_stop() could be
> called twice. So first restructure the existing code, so the check for
> one-shot mode and stopping the timer is moved to the ISR, leaving
> exynos4_mct_tick_clear() just clearing the interrupt flag.
>
> Once this has been done simply call exynos4_mct_tick_clear() from
> set_state_shutdown().
This also fixes mysterious suspend failures on Odroid XU3/XU4/HC1 :)
Tested-by: Marek Szyprowski <[email protected]>
> Stuart Menefy (2):
> clocksource: exynos_mct: Move one-shot check from tick clear to ISR
> clocksource: exynos_mct: Clear timer interrupt when shutdown
>
> drivers/clocksource/exynos_mct.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Sun, 10 Feb 2019 at 23:51, Stuart Menefy
<[email protected]> wrote:
>
> When debugging suspend problems on Exynos 5260, I had a large number
> of debugging prints going to the serial port after interrupts
> had been disabled but before the timer interrupt was shutdown. This
> was long enough for a timer tick to occur, but as interrupts were
> disabled the ISR didn't run, and so the interrupt wasn't cleared.
> Later when the timer was shutdown the interrupt was left asserted and
> so the wfi at the heart of the suspend code didn't wait, causing
> the suspend to fail.
>
> Currently the code which stops the timer when it is on one-shot mode
> and the interrupt occurs is in exynos4_mct_tick_clear(), meaning if we
> called this from the shutdown code exynos4_mct_tick_stop() could be
> called twice. So first restructure the existing code, so the check for
> one-shot mode and stopping the timer is moved to the ISR, leaving
> exynos4_mct_tick_clear() just clearing the interrupt flag.
>
> Once this has been done simply call exynos4_mct_tick_clear() from
> set_state_shutdown().
For sending the corrected version. This is second revision of the
patchset so please remember to add v2 to the title and changelog in
cover letter. v2 can be easily added with format-patch (-v2).
Best regards,
Krzysztof
On Sun, 10 Feb 2019 at 23:51, Stuart Menefy
<[email protected]> wrote:
>
> When a timer tick occurs and the clock is in one-shot mode, the timer
> needs to be stopped to prevent it triggering subsequent interrupts.
> Currently this code is in exynos4_mct_tick_clear(), but as it is
> only needed when an ISR occurs move it into exynos4_mct_tick_isr(),
> leaving exynos4_mct_tick_clear() just doing what its name suggests it
> should.
>
> Signed-off-by: Stuart Menefy <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Sun, 10 Feb 2019 at 23:51, Stuart Menefy
<[email protected]> wrote:
>
> When shutting down the timer, ensure that after we have stopped the
> timer any pending interrupts are cleared. This fixes a problem when
> suspending, as interrupts are disabled before the timer is stopped,
> so the timer interrupt may still be asserted, preventing the system
> entering a low power state when the wfi is executed.
>
> Signed-off-by: Stuart Menefy <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 11/02/2019 08:14, Marek Szyprowski wrote:
> Hi Stuart
>
> On 2019-02-10 23:51, Stuart Menefy wrote:
>> When debugging suspend problems on Exynos 5260, I had a large number
>> of debugging prints going to the serial port after interrupts
>> had been disabled but before the timer interrupt was shutdown. This
>> was long enough for a timer tick to occur, but as interrupts were
>> disabled the ISR didn't run, and so the interrupt wasn't cleared.
>> Later when the timer was shutdown the interrupt was left asserted and
>> so the wfi at the heart of the suspend code didn't wait, causing
>> the suspend to fail.
>>
>> Currently the code which stops the timer when it is on one-shot mode
>> and the interrupt occurs is in exynos4_mct_tick_clear(), meaning if we
>> called this from the shutdown code exynos4_mct_tick_stop() could be
>> called twice. So first restructure the existing code, so the check for
>> one-shot mode and stopping the timer is moved to the ISR, leaving
>> exynos4_mct_tick_clear() just clearing the interrupt flag.
>>
>> Once this has been done simply call exynos4_mct_tick_clear() from
>> set_state_shutdown().
>
> This also fixes mysterious suspend failures on Odroid XU3/XU4/HC1 :)
>
> Tested-by: Marek Szyprowski <[email protected]>
Applied. Shall I add the stable@ tag?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Mon, 11 Feb 2019 at 12:23, Daniel Lezcano <[email protected]> wrote:
>
> On 11/02/2019 08:14, Marek Szyprowski wrote:
> > Hi Stuart
> >
> > On 2019-02-10 23:51, Stuart Menefy wrote:
> >> When debugging suspend problems on Exynos 5260, I had a large number
> >> of debugging prints going to the serial port after interrupts
> >> had been disabled but before the timer interrupt was shutdown. This
> >> was long enough for a timer tick to occur, but as interrupts were
> >> disabled the ISR didn't run, and so the interrupt wasn't cleared.
> >> Later when the timer was shutdown the interrupt was left asserted and
> >> so the wfi at the heart of the suspend code didn't wait, causing
> >> the suspend to fail.
> >>
> >> Currently the code which stops the timer when it is on one-shot mode
> >> and the interrupt occurs is in exynos4_mct_tick_clear(), meaning if we
> >> called this from the shutdown code exynos4_mct_tick_stop() could be
> >> called twice. So first restructure the existing code, so the check for
> >> one-shot mode and stopping the timer is moved to the ISR, leaving
> >> exynos4_mct_tick_clear() just clearing the interrupt flag.
> >>
> >> Once this has been done simply call exynos4_mct_tick_clear() from
> >> set_state_shutdown().
> >
> > This also fixes mysterious suspend failures on Odroid XU3/XU4/HC1 :)
> >
> > Tested-by: Marek Szyprowski <[email protected]>
>
> Applied. Shall I add the stable@ tag?
Makes sense - for the second patch, although first is a prerequisite.
It should be backported up to v4.3 (the code differ a lot on older
versions).
Best regards,
Krzysztof