The CMT do runtime PM and call clk_enable()/clk_disable() when a new
clock event is register and the CMT is not already started. This is not
always possible as a spinlock is also needed to sync the internals of
the CMT. Running with PROVE_LOCKING uncovers one such issue.
=============================
[ BUG: Invalid wait context ]
6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 Not tainted
-----------------------------
swapper/1/0 is trying to lock:
ffff00000898d180 (&dev->power.lock){-...}-{3:3}, at: __pm_runtime_resume+0x38/0x88
ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version 0xAF400001/0xDCC63000, Driver version 5.0
other info that might help us debug this:
ccree e6601000.crypto: ARM ccree device initialized
context-{5:5}
2 locks held by swapper/1/0:
#0: ffff80008173c298 (tick_broadcast_lock){-...}-{2:2}, at: __tick_broadcast_oneshot_control+0xa4/0x3a8
#1: ffff0000089a5858 (&ch->lock){....}-{2:2}
usbcore: registered new interface driver usbhid
, at: sh_cmt_start+0x30/0x364
stack backtrace:
CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Not tainted 6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 PREEMPT
Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
Call trace:
show_stack+0x14/0x1c (C)
dump_stack_lvl+0x6c/0x90
dump_stack+0x14/0x1c
__lock_acquire+0x904/0x1584
lock_acquire+0x220/0x34c
_raw_spin_lock_irqsave+0x58/0x80
__pm_runtime_resume+0x38/0x88
sh_cmt_start+0x54/0x364
sh_cmt_clock_event_set_oneshot+0x64/0xb8
clockevents_switch_state+0xfc/0x13c
tick_broadcast_set_event+0x30/0xa4
__tick_broadcast_oneshot_control+0x1e0/0x3a8
tick_broadcast_oneshot_control+0x30/0x40
cpuidle_enter_state+0x40c/0x680
cpuidle_enter+0x30/0x40
do_idle+0x1f4/0x26c
cpu_startup_entry+0x34/0x40
secondary_start_kernel+0x11c/0x13c
__secondary_switched+0x74/0x78
Fix this by unconditionally powering on and enabling the needed clocks
for all CMT channels which are used for clock events. Do this before
registering any clock source or event to avid having to take the
spin lock at probe time.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Move the unconditional power on case before registering any clock
source or event to avoid having to use the spinlock to synchronize the
powerup sequence in probe.
---
drivers/clocksource/sh_cmt.c | 91 +++++++++++++++++-------------------
1 file changed, 42 insertions(+), 49 deletions(-)
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index 385eb94bbe7c..1d91730ee3cb 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -5,6 +5,7 @@
* Copyright (C) 2008 Magnus Damm
*/
+#include <linux/cleanup.h>
#include <linux/clk.h>
#include <linux/clockchips.h>
#include <linux/clocksource.h>
@@ -623,51 +624,6 @@ static void sh_cmt_stop_clocksource(struct sh_cmt_channel *ch)
pm_runtime_put(&ch->cmt->pdev->dev);
}
-static int sh_cmt_start_clockevent(struct sh_cmt_channel *ch)
-{
- int ret = 0;
- unsigned long flags;
-
- raw_spin_lock_irqsave(&ch->lock, flags);
-
- if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
- pm_runtime_get_sync(&ch->cmt->pdev->dev);
- ret = sh_cmt_enable(ch);
- }
-
- if (ret)
- goto out;
-
- ch->flags |= FLAG_CLOCKEVENT;
- out:
- raw_spin_unlock_irqrestore(&ch->lock, flags);
-
- return ret;
-}
-
-static void sh_cmt_stop_clockevent(struct sh_cmt_channel *ch)
-{
- unsigned long flags;
- unsigned long f;
-
- raw_spin_lock_irqsave(&ch->lock, flags);
-
- f = ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE);
-
- ch->flags &= ~FLAG_CLOCKEVENT;
-
- if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
- sh_cmt_disable(ch);
- pm_runtime_put(&ch->cmt->pdev->dev);
- }
-
- /* adjust the timeout to maximum if only clocksource left */
- if (ch->flags & FLAG_CLOCKSOURCE)
- __sh_cmt_set_next(ch, ch->max_match_value);
-
- raw_spin_unlock_irqrestore(&ch->lock, flags);
-}
-
static struct sh_cmt_channel *cs_to_sh_cmt(struct clocksource *cs)
{
return container_of(cs, struct sh_cmt_channel, cs);
@@ -774,19 +730,30 @@ static struct sh_cmt_channel *ced_to_sh_cmt(struct clock_event_device *ced)
static void sh_cmt_clock_event_start(struct sh_cmt_channel *ch, int periodic)
{
- sh_cmt_start_clockevent(ch);
-
if (periodic)
sh_cmt_set_next(ch, ((ch->cmt->rate + HZ/2) / HZ) - 1);
else
sh_cmt_set_next(ch, ch->max_match_value);
}
+static void sh_cmt_clock_event_stop(struct sh_cmt_channel *ch)
+{
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&ch->lock, flags);
+
+ /* adjust the timeout to maximum if only clocksource left */
+ if (ch->flags & FLAG_CLOCKSOURCE)
+ __sh_cmt_set_next(ch, ch->max_match_value);
+
+ raw_spin_unlock_irqrestore(&ch->lock, flags);
+}
+
static int sh_cmt_clock_event_shutdown(struct clock_event_device *ced)
{
struct sh_cmt_channel *ch = ced_to_sh_cmt(ced);
- sh_cmt_stop_clockevent(ch);
+ sh_cmt_clock_event_stop(ch);
return 0;
}
@@ -797,7 +764,7 @@ static int sh_cmt_clock_event_set_state(struct clock_event_device *ced,
/* deal with old setting first */
if (clockevent_state_oneshot(ced) || clockevent_state_periodic(ced))
- sh_cmt_stop_clockevent(ch);
+ sh_cmt_clock_event_stop(ch);
dev_info(&ch->cmt->pdev->dev, "ch%u: used for %s clock events\n",
ch->index, periodic ? "periodic" : "oneshot");
@@ -968,6 +935,32 @@ static int sh_cmt_setup_channel(struct sh_cmt_channel *ch, unsigned int index,
ch->match_value = ch->max_match_value;
raw_spin_lock_init(&ch->lock);
+ if (clockevent) {
+ /*
+ * To support clock events the CMT must always be ready to
+ * accept new events, start it and leave no way for it to be
+ * turned off.
+ *
+ * This is OK as we can't never unregister a CMT device. If this
+ * is fixed in future an equal unconditional shutdown is needed.
+ *
+ * We don't need to hold the channel lock here as we have not
+ * yet register any clock source or event so there is no
+ * possible race. And holding the spinlock at probe time
+ * produces lockdep warnings.
+ */
+ pm_runtime_get_sync(&ch->cmt->pdev->dev);
+ ret = sh_cmt_enable(ch);
+ if (ret)
+ return ret;
+
+ /*
+ * Flag that the channel is used as a clock event, it's not
+ * allowed to be powered off!
+ */
+ ch->flags |= FLAG_CLOCKEVENT;
+ }
+
ret = sh_cmt_register(ch, dev_name(&cmt->pdev->dev),
clockevent, clocksource);
if (ret) {
--
2.51.0
Hi Niklas,
On Wed, 10 Sept 2025 at 16:27, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The CMT do runtime PM and call clk_enable()/clk_disable() when a new
> clock event is register and the CMT is not already started. This is not
> always possible as a spinlock is also needed to sync the internals of
> the CMT. Running with PROVE_LOCKING uncovers one such issue.
>
> =============================
> [ BUG: Invalid wait context ]
> 6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 Not tainted
> -----------------------------
> swapper/1/0 is trying to lock:
> ffff00000898d180 (&dev->power.lock){-...}-{3:3}, at: __pm_runtime_resume+0x38/0x88
> ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version 0xAF400001/0xDCC63000, Driver version 5.0
> other info that might help us debug this:
> ccree e6601000.crypto: ARM ccree device initialized
> context-{5:5}
> 2 locks held by swapper/1/0:
> #0: ffff80008173c298 (tick_broadcast_lock){-...}-{2:2}, at: __tick_broadcast_oneshot_control+0xa4/0x3a8
> #1: ffff0000089a5858 (&ch->lock){....}-{2:2}
> usbcore: registered new interface driver usbhid
> , at: sh_cmt_start+0x30/0x364
> stack backtrace:
> CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Not tainted 6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 PREEMPT
> Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
> Call trace:
> show_stack+0x14/0x1c (C)
> dump_stack_lvl+0x6c/0x90
> dump_stack+0x14/0x1c
> __lock_acquire+0x904/0x1584
> lock_acquire+0x220/0x34c
> _raw_spin_lock_irqsave+0x58/0x80
> __pm_runtime_resume+0x38/0x88
> sh_cmt_start+0x54/0x364
> sh_cmt_clock_event_set_oneshot+0x64/0xb8
> clockevents_switch_state+0xfc/0x13c
> tick_broadcast_set_event+0x30/0xa4
> __tick_broadcast_oneshot_control+0x1e0/0x3a8
> tick_broadcast_oneshot_control+0x30/0x40
> cpuidle_enter_state+0x40c/0x680
> cpuidle_enter+0x30/0x40
> do_idle+0x1f4/0x26c
> cpu_startup_entry+0x34/0x40
> secondary_start_kernel+0x11c/0x13c
> __secondary_switched+0x74/0x78
>
> Fix this by unconditionally powering on and enabling the needed clocks
> for all CMT channels which are used for clock events. Do this before
> registering any clock source or event to avid having to take the
> spin lock at probe time.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Move the unconditional power on case before registering any clock
> source or event to avoid having to use the spinlock to synchronize the
> powerup sequence in probe.
Thanks for your patch, which is now commit cfbc0f1d24030ff9
("clocksource/drivers/sh_cmt: Do not power down channels used for
events") in clockevents/timers/drivers/next.
Unfortunately this commit introduces an s2ram regression on e.g.
Atmark Techo Armadillo-800EVA with R-Mobile A1: the system wakes
up immediately. There is no evidence of a wake-up event shown in
/sys/kernel/debug/wakeup_sources. This happens with or without
console_suspend enabled.
Reverting this commit fixes the issue. I suspect the system wakes up
because the periodic clock event fires, and causes an interrupt.
> --- a/drivers/clocksource/sh_cmt.c
> +++ b/drivers/clocksource/sh_cmt.c
> @@ -5,6 +5,7 @@
> * Copyright (C) 2008 Magnus Damm
> */
>
> +#include <linux/cleanup.h>
> #include <linux/clk.h>
> #include <linux/clockchips.h>
> #include <linux/clocksource.h>
> @@ -623,51 +624,6 @@ static void sh_cmt_stop_clocksource(struct sh_cmt_channel *ch)
> pm_runtime_put(&ch->cmt->pdev->dev);
> }
>
> -static int sh_cmt_start_clockevent(struct sh_cmt_channel *ch)
> -{
> - int ret = 0;
> - unsigned long flags;
> -
> - raw_spin_lock_irqsave(&ch->lock, flags);
> -
> - if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
> - pm_runtime_get_sync(&ch->cmt->pdev->dev);
> - ret = sh_cmt_enable(ch);
> - }
> -
> - if (ret)
> - goto out;
> -
> - ch->flags |= FLAG_CLOCKEVENT;
> - out:
> - raw_spin_unlock_irqrestore(&ch->lock, flags);
> -
> - return ret;
> -}
> -
> -static void sh_cmt_stop_clockevent(struct sh_cmt_channel *ch)
> -{
> - unsigned long flags;
> - unsigned long f;
> -
> - raw_spin_lock_irqsave(&ch->lock, flags);
> -
> - f = ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE);
> -
> - ch->flags &= ~FLAG_CLOCKEVENT;
> -
> - if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
> - sh_cmt_disable(ch);
> - pm_runtime_put(&ch->cmt->pdev->dev);
> - }
> -
> - /* adjust the timeout to maximum if only clocksource left */
> - if (ch->flags & FLAG_CLOCKSOURCE)
> - __sh_cmt_set_next(ch, ch->max_match_value);
> -
> - raw_spin_unlock_irqrestore(&ch->lock, flags);
> -}
> -
> static struct sh_cmt_channel *cs_to_sh_cmt(struct clocksource *cs)
> {
> return container_of(cs, struct sh_cmt_channel, cs);
> @@ -774,19 +730,30 @@ static struct sh_cmt_channel *ced_to_sh_cmt(struct clock_event_device *ced)
>
> static void sh_cmt_clock_event_start(struct sh_cmt_channel *ch, int periodic)
> {
> - sh_cmt_start_clockevent(ch);
> -
> if (periodic)
> sh_cmt_set_next(ch, ((ch->cmt->rate + HZ/2) / HZ) - 1);
> else
> sh_cmt_set_next(ch, ch->max_match_value);
> }
>
> +static void sh_cmt_clock_event_stop(struct sh_cmt_channel *ch)
> +{
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&ch->lock, flags);
> +
> + /* adjust the timeout to maximum if only clocksource left */
> + if (ch->flags & FLAG_CLOCKSOURCE)
> + __sh_cmt_set_next(ch, ch->max_match_value);
> +
> + raw_spin_unlock_irqrestore(&ch->lock, flags);
> +}
> +
> static int sh_cmt_clock_event_shutdown(struct clock_event_device *ced)
> {
> struct sh_cmt_channel *ch = ced_to_sh_cmt(ced);
>
> - sh_cmt_stop_clockevent(ch);
> + sh_cmt_clock_event_stop(ch);
> return 0;
> }
>
> @@ -797,7 +764,7 @@ static int sh_cmt_clock_event_set_state(struct clock_event_device *ced,
>
> /* deal with old setting first */
> if (clockevent_state_oneshot(ced) || clockevent_state_periodic(ced))
> - sh_cmt_stop_clockevent(ch);
> + sh_cmt_clock_event_stop(ch);
>
> dev_info(&ch->cmt->pdev->dev, "ch%u: used for %s clock events\n",
> ch->index, periodic ? "periodic" : "oneshot");
> @@ -968,6 +935,32 @@ static int sh_cmt_setup_channel(struct sh_cmt_channel *ch, unsigned int index,
> ch->match_value = ch->max_match_value;
> raw_spin_lock_init(&ch->lock);
>
> + if (clockevent) {
> + /*
> + * To support clock events the CMT must always be ready to
> + * accept new events, start it and leave no way for it to be
> + * turned off.
> + *
> + * This is OK as we can't never unregister a CMT device. If this
> + * is fixed in future an equal unconditional shutdown is needed.
> + *
> + * We don't need to hold the channel lock here as we have not
> + * yet register any clock source or event so there is no
> + * possible race. And holding the spinlock at probe time
> + * produces lockdep warnings.
> + */
> + pm_runtime_get_sync(&ch->cmt->pdev->dev);
> + ret = sh_cmt_enable(ch);
> + if (ret)
> + return ret;
> +
> + /*
> + * Flag that the channel is used as a clock event, it's not
> + * allowed to be powered off!
> + */
> + ch->flags |= FLAG_CLOCKEVENT;
> + }
> +
> ret = sh_cmt_register(ch, dev_name(&cmt->pdev->dev),
> clockevent, clocksource);
> if (ret) {
> --
> 2.51.0
>
--
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On 23/09/2025 16:56, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Wed, 10 Sept 2025 at 16:27, Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
>> The CMT do runtime PM and call clk_enable()/clk_disable() when a new
>> clock event is register and the CMT is not already started. This is not
>> always possible as a spinlock is also needed to sync the internals of
>> the CMT. Running with PROVE_LOCKING uncovers one such issue.
>>
>> =============================
>> [ BUG: Invalid wait context ]
>> 6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 Not tainted
>> -----------------------------
>> swapper/1/0 is trying to lock:
>> ffff00000898d180 (&dev->power.lock){-...}-{3:3}, at: __pm_runtime_resume+0x38/0x88
>> ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version 0xAF400001/0xDCC63000, Driver version 5.0
>> other info that might help us debug this:
>> ccree e6601000.crypto: ARM ccree device initialized
>> context-{5:5}
>> 2 locks held by swapper/1/0:
>> #0: ffff80008173c298 (tick_broadcast_lock){-...}-{2:2}, at: __tick_broadcast_oneshot_control+0xa4/0x3a8
>> #1: ffff0000089a5858 (&ch->lock){....}-{2:2}
>> usbcore: registered new interface driver usbhid
>> , at: sh_cmt_start+0x30/0x364
>> stack backtrace:
>> CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Not tainted 6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 PREEMPT
>> Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
>> Call trace:
>> show_stack+0x14/0x1c (C)
>> dump_stack_lvl+0x6c/0x90
>> dump_stack+0x14/0x1c
>> __lock_acquire+0x904/0x1584
>> lock_acquire+0x220/0x34c
>> _raw_spin_lock_irqsave+0x58/0x80
>> __pm_runtime_resume+0x38/0x88
>> sh_cmt_start+0x54/0x364
>> sh_cmt_clock_event_set_oneshot+0x64/0xb8
>> clockevents_switch_state+0xfc/0x13c
>> tick_broadcast_set_event+0x30/0xa4
>> __tick_broadcast_oneshot_control+0x1e0/0x3a8
>> tick_broadcast_oneshot_control+0x30/0x40
>> cpuidle_enter_state+0x40c/0x680
>> cpuidle_enter+0x30/0x40
>> do_idle+0x1f4/0x26c
>> cpu_startup_entry+0x34/0x40
>> secondary_start_kernel+0x11c/0x13c
>> __secondary_switched+0x74/0x78
>>
>> Fix this by unconditionally powering on and enabling the needed clocks
>> for all CMT channels which are used for clock events. Do this before
>> registering any clock source or event to avid having to take the
>> spin lock at probe time.
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> ---
>> * Changes since v1
>> - Move the unconditional power on case before registering any clock
>> source or event to avoid having to use the spinlock to synchronize the
>> powerup sequence in probe.
>
> Thanks for your patch, which is now commit cfbc0f1d24030ff9
> ("clocksource/drivers/sh_cmt: Do not power down channels used for
> events") in clockevents/timers/drivers/next.
>
> Unfortunately this commit introduces an s2ram regression on e.g.
> Atmark Techo Armadillo-800EVA with R-Mobile A1: the system wakes
> up immediately. There is no evidence of a wake-up event shown in
> /sys/kernel/debug/wakeup_sources. This happens with or without
> console_suspend enabled.
>
> Reverting this commit fixes the issue. I suspect the system wakes up
> because the periodic clock event fires, and causes an interrupt.
I'm about to send a PR.
Shall I drop this change which fixes a lock issue or keep it ?
What has the higher priority ?
--
<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
Hi Daniel,
On 2025-09-24 11:19:21 +0200, Daniel Lezcano wrote:
>
> Hi Geert,
>
> On 23/09/2025 16:56, Geert Uytterhoeven wrote:
> > Hi Niklas,
> >
> > On Wed, 10 Sept 2025 at 16:27, Niklas Söderlund
> > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > The CMT do runtime PM and call clk_enable()/clk_disable() when a new
> > > clock event is register and the CMT is not already started. This is not
> > > always possible as a spinlock is also needed to sync the internals of
> > > the CMT. Running with PROVE_LOCKING uncovers one such issue.
> > >
> > > =============================
> > > [ BUG: Invalid wait context ]
> > > 6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 Not tainted
> > > -----------------------------
> > > swapper/1/0 is trying to lock:
> > > ffff00000898d180 (&dev->power.lock){-...}-{3:3}, at: __pm_runtime_resume+0x38/0x88
> > > ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version 0xAF400001/0xDCC63000, Driver version 5.0
> > > other info that might help us debug this:
> > > ccree e6601000.crypto: ARM ccree device initialized
> > > context-{5:5}
> > > 2 locks held by swapper/1/0:
> > > #0: ffff80008173c298 (tick_broadcast_lock){-...}-{2:2}, at: __tick_broadcast_oneshot_control+0xa4/0x3a8
> > > #1: ffff0000089a5858 (&ch->lock){....}-{2:2}
> > > usbcore: registered new interface driver usbhid
> > > , at: sh_cmt_start+0x30/0x364
> > > stack backtrace:
> > > CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Not tainted 6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 PREEMPT
> > > Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
> > > Call trace:
> > > show_stack+0x14/0x1c (C)
> > > dump_stack_lvl+0x6c/0x90
> > > dump_stack+0x14/0x1c
> > > __lock_acquire+0x904/0x1584
> > > lock_acquire+0x220/0x34c
> > > _raw_spin_lock_irqsave+0x58/0x80
> > > __pm_runtime_resume+0x38/0x88
> > > sh_cmt_start+0x54/0x364
> > > sh_cmt_clock_event_set_oneshot+0x64/0xb8
> > > clockevents_switch_state+0xfc/0x13c
> > > tick_broadcast_set_event+0x30/0xa4
> > > __tick_broadcast_oneshot_control+0x1e0/0x3a8
> > > tick_broadcast_oneshot_control+0x30/0x40
> > > cpuidle_enter_state+0x40c/0x680
> > > cpuidle_enter+0x30/0x40
> > > do_idle+0x1f4/0x26c
> > > cpu_startup_entry+0x34/0x40
> > > secondary_start_kernel+0x11c/0x13c
> > > __secondary_switched+0x74/0x78
> > >
> > > Fix this by unconditionally powering on and enabling the needed clocks
> > > for all CMT channels which are used for clock events. Do this before
> > > registering any clock source or event to avid having to take the
> > > spin lock at probe time.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > > * Changes since v1
> > > - Move the unconditional power on case before registering any clock
> > > source or event to avoid having to use the spinlock to synchronize the
> > > powerup sequence in probe.
> >
> > Thanks for your patch, which is now commit cfbc0f1d24030ff9
> > ("clocksource/drivers/sh_cmt: Do not power down channels used for
> > events") in clockevents/timers/drivers/next.
> >
> > Unfortunately this commit introduces an s2ram regression on e.g.
> > Atmark Techo Armadillo-800EVA with R-Mobile A1: the system wakes
> > up immediately. There is no evidence of a wake-up event shown in
> > /sys/kernel/debug/wakeup_sources. This happens with or without
> > console_suspend enabled.
> >
> > Reverting this commit fixes the issue. I suspect the system wakes up
> > because the periodic clock event fires, and causes an interrupt.
>
> I'm about to send a PR.
>
> Shall I drop this change which fixes a lock issue or keep it ?
>
> What has the higher priority ?
If it's not too late I think we should drop it. The issue this work
tries to solve is a lockdep blurb which highlights a design issue in the
driver. But the driver have function properly in the past. So I think
it's better I work on solving the blurb without any regressions.
Thanks for checking.
>
>
> --
> <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
--
Kind Regards,
Niklas Söderlund
On 24/09/2025 11:31, Niklas Söderlund wrote: [ ... ] > If it's not too late I think we should drop it. The issue this work > tries to solve is a lockdep blurb which highlights a design issue in the > driver. But the driver have function properly in the past. So I think > it's better I work on solving the blurb without any regressions. Ok, dropping 2/2 but keeping 1/2 is fine, right ? -- <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
Hi Daniel,
On Wed, 24 Sept 2025 at 15:43, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 24/09/2025 11:31, Niklas Söderlund wrote:
> > If it's not too late I think we should drop it. The issue this work
> > tries to solve is a lockdep blurb which highlights a design issue in the
> > driver. But the driver have function properly in the past. So I think
> > it's better I work on solving the blurb without any regressions.
>
> Ok, dropping 2/2 but keeping 1/2 is fine, right ?
1/2 is a refactoring without any functional impact.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
© 2016 - 2026 Red Hat, Inc.