The CMT device can be used as both a clocksource and a clockevent
provider. The driver tries to be smart and power itself on and off, as
well as enabling and disabling its clock when it's not in operation.
This behavior is slightly altered if the CMT is used as an early
platform device in which case the device is left powered on after probe,
but the clock is still enabled and disabled at runtime.

This has worked for a long time, but recent improvements in PREEMPT_RT
and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
as a clockevent provider, clockevents_register_device(), it needs to use
raw spinlocks internally as this is the context of which the clockevent
framework interacts with the CMT driver. However in the context of
holding a raw spinlock the CMT driver can't really manage its power
state or clock with calls to pm_runtime_*() and clk_*() as these calls
end up in other platform drivers using regular spinlocks to control
power and clocks.

This mix of spinlock contexts trips a lockdep warning.

    =============================
    [ 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

For non-PREEMPT_RT builds this is not really an issue, but for
PREEMPT_RT builds where normal spinlocks can sleep this might be an
issue. Be cautious and always leave the power and clock running after
probe.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
* Changes since RFC
- Simplify a ret = -ETIMEDOUT; return ret; to return -ETIMEDOUT;.
- Remove { } for single if-statement bodies created by this patch.
- Update spelling in commit message.
---
 drivers/clocksource/sh_cmt.c | 36 +++---------------------------------
 1 file changed, 3 insertions(+), 33 deletions(-)

diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index 385eb94bbe7c..791b298c995b 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -355,14 +355,6 @@ static int sh_cmt_enable(struct sh_cmt_channel *ch)
 
 	dev_pm_syscore_device(&ch->cmt->pdev->dev, true);
 
-	/* enable clock */
-	ret = clk_enable(ch->cmt->clk);
-	if (ret) {
-		dev_err(&ch->cmt->pdev->dev, "ch%u: cannot enable clock\n",
-			ch->index);
-		goto err0;
-	}
-
 	/* make sure channel is disabled */
 	sh_cmt_start_stop_ch(ch, 0);
 
@@ -384,19 +376,12 @@ static int sh_cmt_enable(struct sh_cmt_channel *ch)
 	if (ret || sh_cmt_read_cmcnt(ch)) {
 		dev_err(&ch->cmt->pdev->dev, "ch%u: cannot clear CMCNT\n",
 			ch->index);
-		ret = -ETIMEDOUT;
-		goto err1;
+		return -ETIMEDOUT;
 	}
 
 	/* enable channel */
 	sh_cmt_start_stop_ch(ch, 1);
 	return 0;
- err1:
-	/* stop clock */
-	clk_disable(ch->cmt->clk);
-
- err0:
-	return ret;
 }
 
 static void sh_cmt_disable(struct sh_cmt_channel *ch)
@@ -407,9 +392,6 @@ static void sh_cmt_disable(struct sh_cmt_channel *ch)
 	/* disable interrupts in CMT block */
 	sh_cmt_write_cmcsr(ch, 0);
 
-	/* stop clock */
-	clk_disable(ch->cmt->clk);
-
 	dev_pm_syscore_device(&ch->cmt->pdev->dev, false);
 }
 
@@ -583,8 +565,6 @@ static int sh_cmt_start_clocksource(struct sh_cmt_channel *ch)
 	int ret = 0;
 	unsigned long flags;
 
-	pm_runtime_get_sync(&ch->cmt->pdev->dev);
-
 	raw_spin_lock_irqsave(&ch->lock, flags);
 
 	if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE)))
@@ -619,8 +599,6 @@ static void sh_cmt_stop_clocksource(struct sh_cmt_channel *ch)
 		sh_cmt_disable(ch);
 
 	raw_spin_unlock_irqrestore(&ch->lock, flags);
-
-	pm_runtime_put(&ch->cmt->pdev->dev);
 }
 
 static int sh_cmt_start_clockevent(struct sh_cmt_channel *ch)
@@ -630,10 +608,8 @@ static int sh_cmt_start_clockevent(struct sh_cmt_channel *ch)
 
 	raw_spin_lock_irqsave(&ch->lock, flags);
 
-	if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
-		pm_runtime_get_sync(&ch->cmt->pdev->dev);
+	if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE)))
 		ret = sh_cmt_enable(ch);
-	}
 
 	if (ret)
 		goto out;
@@ -656,10 +632,8 @@ static void sh_cmt_stop_clockevent(struct sh_cmt_channel *ch)
 
 	ch->flags &= ~FLAG_CLOCKEVENT;
 
-	if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
+	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)
@@ -1134,8 +1108,6 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt, struct platform_device *pdev)
 		mask &= ~(1 << hwidx);
 	}
 
-	clk_disable(cmt->clk);
-
 	platform_set_drvdata(pdev, cmt);
 
 	return 0;
@@ -1183,8 +1155,6 @@ static int sh_cmt_probe(struct platform_device *pdev)
  out:
 	if (cmt->has_clockevent || cmt->has_clocksource)
 		pm_runtime_irq_safe(&pdev->dev);
-	else
-		pm_runtime_idle(&pdev->dev);
 
 	return 0;
 }
-- 
2.51.0

On 10/16/25 20:20, Niklas Söderlund wrote:
> The CMT device can be used as both a clocksource and a clockevent
> provider. The driver tries to be smart and power itself on and off, as
> well as enabling and disabling its clock when it's not in operation.
> This behavior is slightly altered if the CMT is used as an early
> platform device in which case the device is left powered on after probe,
> but the clock is still enabled and disabled at runtime.
> 
> This has worked for a long time, but recent improvements in PREEMPT_RT
> and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
> as a clockevent provider, clockevents_register_device(), it needs to use
> raw spinlocks internally as this is the context of which the clockevent
> framework interacts with the CMT driver. However in the context of
> holding a raw spinlock the CMT driver can't really manage its power
> state or clock with calls to pm_runtime_*() and clk_*() as these calls
> end up in other platform drivers using regular spinlocks to control
> power and clocks.

So the fix is to remove PM management in the driver ?


-- 
<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 2025-11-05 16:36:15 +0100, Daniel Lezcano wrote:
> On 10/16/25 20:20, Niklas Söderlund wrote:
> > The CMT device can be used as both a clocksource and a clockevent
> > provider. The driver tries to be smart and power itself on and off, as
> > well as enabling and disabling its clock when it's not in operation.
> > This behavior is slightly altered if the CMT is used as an early
> > platform device in which case the device is left powered on after probe,
> > but the clock is still enabled and disabled at runtime.
> > 
> > This has worked for a long time, but recent improvements in PREEMPT_RT
> > and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
> > as a clockevent provider, clockevents_register_device(), it needs to use
> > raw spinlocks internally as this is the context of which the clockevent
> > framework interacts with the CMT driver. However in the context of
> > holding a raw spinlock the CMT driver can't really manage its power
> > state or clock with calls to pm_runtime_*() and clk_*() as these calls
> > end up in other platform drivers using regular spinlocks to control
> > power and clocks.
> 
> So the fix is to remove PM management in the driver ?

Yes. As I understand it we can't do runtime pm in these drivers as the 
core calls into the functions with the raw spinlock held. I hope we can 
improve this in future.

> 
> 
> -- 
> <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 11/5/25 17:06, Niklas Söderlund wrote:
> On 2025-11-05 16:36:15 +0100, Daniel Lezcano wrote:
>> On 10/16/25 20:20, Niklas Söderlund wrote:
>>> The CMT device can be used as both a clocksource and a clockevent
>>> provider. The driver tries to be smart and power itself on and off, as
>>> well as enabling and disabling its clock when it's not in operation.
>>> This behavior is slightly altered if the CMT is used as an early
>>> platform device in which case the device is left powered on after probe,
>>> but the clock is still enabled and disabled at runtime.
>>>
>>> This has worked for a long time, but recent improvements in PREEMPT_RT
>>> and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
>>> as a clockevent provider, clockevents_register_device(), it needs to use
>>> raw spinlocks internally as this is the context of which the clockevent
>>> framework interacts with the CMT driver. However in the context of
>>> holding a raw spinlock the CMT driver can't really manage its power
>>> state or clock with calls to pm_runtime_*() and clk_*() as these calls
>>> end up in other platform drivers using regular spinlocks to control
>>> power and clocks.
>>
>> So the fix is to remove PM management in the driver ?
> 
> Yes. As I understand it we can't do runtime pm in these drivers as the
> core calls into the functions with the raw spinlock held. I hope we can
> improve this in future.


IIUC, the changes done for PREEMPT_RT prevent to use pm_runtime by 
functions running in atomic context because the spinlocks are actually 
mutexes.

But if PREEMPT_RT is not set, then everything is running as usual.

This change drops the PM while it should be working for kernel compiled 
without PREEMPT_RT.

I suggest to handle the case with/out PREEMPT_RT.

Hopefully pm_runtime will be fixed with PREEMPT_RT and you won't have to 
reintroduce pm_runtime in this driver but just remove the PREEMPT_RT case.


-- 
<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,

Thanks for your feedback.

On 2025-11-05 17:39:14 +0100, Daniel Lezcano wrote:
> On 11/5/25 17:06, Niklas Söderlund wrote:
> > On 2025-11-05 16:36:15 +0100, Daniel Lezcano wrote:
> > > On 10/16/25 20:20, Niklas Söderlund wrote:
> > > > The CMT device can be used as both a clocksource and a clockevent
> > > > provider. The driver tries to be smart and power itself on and off, as
> > > > well as enabling and disabling its clock when it's not in operation.
> > > > This behavior is slightly altered if the CMT is used as an early
> > > > platform device in which case the device is left powered on after probe,
> > > > but the clock is still enabled and disabled at runtime.
> > > > 
> > > > This has worked for a long time, but recent improvements in PREEMPT_RT
> > > > and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
> > > > as a clockevent provider, clockevents_register_device(), it needs to use
> > > > raw spinlocks internally as this is the context of which the clockevent
> > > > framework interacts with the CMT driver. However in the context of
> > > > holding a raw spinlock the CMT driver can't really manage its power
> > > > state or clock with calls to pm_runtime_*() and clk_*() as these calls
> > > > end up in other platform drivers using regular spinlocks to control
> > > > power and clocks.
> > > 
> > > So the fix is to remove PM management in the driver ?
> > 
> > Yes. As I understand it we can't do runtime pm in these drivers as the
> > core calls into the functions with the raw spinlock held. I hope we can
> > improve this in future.
> 
> 
> IIUC, the changes done for PREEMPT_RT prevent to use pm_runtime by functions
> running in atomic context because the spinlocks are actually mutexes.

My understanding is that the core issue is that the clockevent core uses 
raw spinlocks, so all operations done from the callbacks in drivers need 
to use them too.

The Renesas CMT and TMU (which I intend to fix too once we find a way 
forward for CMT) are the only clockenvet drivers attempting to do 
runtime pm.

    $ git grep -l pm_runtime_get -- drivers/clocksource
    drivers/clocksource/sh_cmt.c
    drivers/clocksource/sh_mtu2.c
    drivers/clocksource/sh_tmu.c
    drivers/clocksource/timer-ti-dm.c

The timer-ti-dm.c driver do not register a clockevent device.

> 
> But if PREEMPT_RT is not set, then everything is running as usual.

I still get LOCKDEP warnings.

> 
> This change drops the PM while it should be working for kernel compiled
> without PREEMPT_RT.
> 
> I suggest to handle the case with/out PREEMPT_RT.
> 
> Hopefully pm_runtime will be fixed with PREEMPT_RT and you won't have to
> reintroduce pm_runtime in this driver but just remove the PREEMPT_RT case.

The problem is not really PREEMPT_RT. The problem is the clockevents 
core require drivers to use raw spinlocks as itself uses them.

I would prefer to get the driver in a state without splats, warnings and 
potential PREEMPT_RT issues. Especially if the cost is to disable 
runtime pm.

As I understand it most systems where CMT exists, who don't use it, keep 
them disabled in DT. And the devices that use them have 
is_sh_early_platform_device() set so they already disable runtime pm.

Once we have that fixed we can work on enabling it without the quirks.  
In your opinion am I missing something where this approach is a bad idea?

> 
> 
> -- 
> <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 11/5/25 19:32, Niklas Söderlund wrote:
> Hi Daniel,
> 
> Thanks for your feedback.
> 
> On 2025-11-05 17:39:14 +0100, Daniel Lezcano wrote:
>> On 11/5/25 17:06, Niklas Söderlund wrote:
>>> On 2025-11-05 16:36:15 +0100, Daniel Lezcano wrote:
>>>> On 10/16/25 20:20, Niklas Söderlund wrote:
>>>>> The CMT device can be used as both a clocksource and a clockevent
>>>>> provider. The driver tries to be smart and power itself on and off, as
>>>>> well as enabling and disabling its clock when it's not in operation.
>>>>> This behavior is slightly altered if the CMT is used as an early
>>>>> platform device in which case the device is left powered on after probe,
>>>>> but the clock is still enabled and disabled at runtime.
>>>>>
>>>>> This has worked for a long time, but recent improvements in PREEMPT_RT
>>>>> and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
>>>>> as a clockevent provider, clockevents_register_device(), it needs to use
>>>>> raw spinlocks internally as this is the context of which the clockevent
>>>>> framework interacts with the CMT driver. However in the context of
>>>>> holding a raw spinlock the CMT driver can't really manage its power
>>>>> state or clock with calls to pm_runtime_*() and clk_*() as these calls
>>>>> end up in other platform drivers using regular spinlocks to control
>>>>> power and clocks.
>>>>
>>>> So the fix is to remove PM management in the driver ?
>>>
>>> Yes. As I understand it we can't do runtime pm in these drivers as the
>>> core calls into the functions with the raw spinlock held. I hope we can
>>> improve this in future.
>>
>>
>> IIUC, the changes done for PREEMPT_RT prevent to use pm_runtime by functions
>> running in atomic context because the spinlocks are actually mutexes.
> 
> My understanding is that the core issue is that the clockevent core uses
> raw spinlocks, so all operations done from the callbacks in drivers need
> to use them too.
> 
> The Renesas CMT and TMU (which I intend to fix too once we find a way
> forward for CMT) are the only clockenvet drivers attempting to do
> runtime pm.
> 
>      $ git grep -l pm_runtime_get -- drivers/clocksource
>      drivers/clocksource/sh_cmt.c
>      drivers/clocksource/sh_mtu2.c
>      drivers/clocksource/sh_tmu.c
>      drivers/clocksource/timer-ti-dm.c
> 
> The timer-ti-dm.c driver do not register a clockevent device.
> 
>>
>> But if PREEMPT_RT is not set, then everything is running as usual.
> 
> I still get LOCKDEP warnings.

Ah ok, you get the LOCKDEP warning because it identifies the called code 
to be invalid in case the PREEMPT_RT is compiled-in but does not reflect 
a real problem with !PREEMPT_RT.

[ ... ]

> The problem is not really PREEMPT_RT. The problem is the clockevents
> core require drivers to use raw spinlocks as itself uses them.
> 
> I would prefer to get the driver in a state without splats, warnings and
> potential PREEMPT_RT issues. Especially if the cost is to disable
> runtime pm.
> 
> As I understand it most systems where CMT exists, who don't use it, keep
> them disabled in DT. And the devices that use them have
> is_sh_early_platform_device() set so they already disable runtime pm.
> 
> Once we have that fixed we can work on enabling it without the quirks.
> In your opinion am I missing something where this approach is a bad idea?

If you prefer to remove the PM in the driver, I'm fine with that.


-- 
<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-11-07 10:53:41 +0100, Daniel Lezcano wrote:
> On 11/5/25 19:32, Niklas Söderlund wrote:
> > Hi Daniel,
> > 
> > Thanks for your feedback.
> > 
> > On 2025-11-05 17:39:14 +0100, Daniel Lezcano wrote:
> > > On 11/5/25 17:06, Niklas Söderlund wrote:
> > > > On 2025-11-05 16:36:15 +0100, Daniel Lezcano wrote:
> > > > > On 10/16/25 20:20, Niklas Söderlund wrote:
> > > > > > The CMT device can be used as both a clocksource and a clockevent
> > > > > > provider. The driver tries to be smart and power itself on and off, as
> > > > > > well as enabling and disabling its clock when it's not in operation.
> > > > > > This behavior is slightly altered if the CMT is used as an early
> > > > > > platform device in which case the device is left powered on after probe,
> > > > > > but the clock is still enabled and disabled at runtime.
> > > > > > 
> > > > > > This has worked for a long time, but recent improvements in PREEMPT_RT
> > > > > > and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
> > > > > > as a clockevent provider, clockevents_register_device(), it needs to use
> > > > > > raw spinlocks internally as this is the context of which the clockevent
> > > > > > framework interacts with the CMT driver. However in the context of
> > > > > > holding a raw spinlock the CMT driver can't really manage its power
> > > > > > state or clock with calls to pm_runtime_*() and clk_*() as these calls
> > > > > > end up in other platform drivers using regular spinlocks to control
> > > > > > power and clocks.
> > > > > 
> > > > > So the fix is to remove PM management in the driver ?
> > > > 
> > > > Yes. As I understand it we can't do runtime pm in these drivers as the
> > > > core calls into the functions with the raw spinlock held. I hope we can
> > > > improve this in future.
> > > 
> > > 
> > > IIUC, the changes done for PREEMPT_RT prevent to use pm_runtime by functions
> > > running in atomic context because the spinlocks are actually mutexes.
> > 
> > My understanding is that the core issue is that the clockevent core uses
> > raw spinlocks, so all operations done from the callbacks in drivers need
> > to use them too.
> > 
> > The Renesas CMT and TMU (which I intend to fix too once we find a way
> > forward for CMT) are the only clockenvet drivers attempting to do
> > runtime pm.
> > 
> >      $ git grep -l pm_runtime_get -- drivers/clocksource
> >      drivers/clocksource/sh_cmt.c
> >      drivers/clocksource/sh_mtu2.c
> >      drivers/clocksource/sh_tmu.c
> >      drivers/clocksource/timer-ti-dm.c
> > 
> > The timer-ti-dm.c driver do not register a clockevent device.
> > 
> > > 
> > > But if PREEMPT_RT is not set, then everything is running as usual.
> > 
> > I still get LOCKDEP warnings.
> 
> Ah ok, you get the LOCKDEP warning because it identifies the called code to
> be invalid in case the PREEMPT_RT is compiled-in but does not reflect a real
> problem with !PREEMPT_RT.
> 
> [ ... ]
> 
> > The problem is not really PREEMPT_RT. The problem is the clockevents
> > core require drivers to use raw spinlocks as itself uses them.
> > 
> > I would prefer to get the driver in a state without splats, warnings and
> > potential PREEMPT_RT issues. Especially if the cost is to disable
> > runtime pm.
> > 
> > As I understand it most systems where CMT exists, who don't use it, keep
> > them disabled in DT. And the devices that use them have
> > is_sh_early_platform_device() set so they already disable runtime pm.
> > 
> > Once we have that fixed we can work on enabling it without the quirks.
> > In your opinion am I missing something where this approach is a bad idea?
> 
> If you prefer to remove the PM in the driver, I'm fine with that.

I'm not super fond of removing it, but for now I think it is the best 
way to ensure correct operation in all cases. If anybody have ideas I'm 
all ears. Specially as I will need to do a similar fix for the TMU 
driver once we have found a acceptable way for CMT.

Once we I'm out of the woods and not worried about whats currently in 
tree can have issues I think it's time to start thinking, if and how we 
can improve tings in the core so we can enable some or all of the PM CMT 
and TMU.

I really appreciate you pushed back on this, I agree removing runtime PM 
support is kind of a last resort here. But I see no other path.
> 
> 
> -- 
> <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

Hi Daniel,

On 2025-11-09 15:37:12 +0100, Niklas Söderlund wrote:
> Hi Daniel,
> 
> On 2025-11-07 10:53:41 +0100, Daniel Lezcano wrote:
> > On 11/5/25 19:32, Niklas Söderlund wrote:
> > > Hi Daniel,
> > > 
> > > Thanks for your feedback.
> > > 
> > > On 2025-11-05 17:39:14 +0100, Daniel Lezcano wrote:
> > > > On 11/5/25 17:06, Niklas Söderlund wrote:
> > > > > On 2025-11-05 16:36:15 +0100, Daniel Lezcano wrote:
> > > > > > On 10/16/25 20:20, Niklas Söderlund wrote:
> > > > > > > The CMT device can be used as both a clocksource and a clockevent
> > > > > > > provider. The driver tries to be smart and power itself on and off, as
> > > > > > > well as enabling and disabling its clock when it's not in operation.
> > > > > > > This behavior is slightly altered if the CMT is used as an early
> > > > > > > platform device in which case the device is left powered on after probe,
> > > > > > > but the clock is still enabled and disabled at runtime.
> > > > > > > 
> > > > > > > This has worked for a long time, but recent improvements in PREEMPT_RT
> > > > > > > and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
> > > > > > > as a clockevent provider, clockevents_register_device(), it needs to use
> > > > > > > raw spinlocks internally as this is the context of which the clockevent
> > > > > > > framework interacts with the CMT driver. However in the context of
> > > > > > > holding a raw spinlock the CMT driver can't really manage its power
> > > > > > > state or clock with calls to pm_runtime_*() and clk_*() as these calls
> > > > > > > end up in other platform drivers using regular spinlocks to control
> > > > > > > power and clocks.
> > > > > > 
> > > > > > So the fix is to remove PM management in the driver ?
> > > > > 
> > > > > Yes. As I understand it we can't do runtime pm in these drivers as the
> > > > > core calls into the functions with the raw spinlock held. I hope we can
> > > > > improve this in future.
> > > > 
> > > > 
> > > > IIUC, the changes done for PREEMPT_RT prevent to use pm_runtime by functions
> > > > running in atomic context because the spinlocks are actually mutexes.
> > > 
> > > My understanding is that the core issue is that the clockevent core uses
> > > raw spinlocks, so all operations done from the callbacks in drivers need
> > > to use them too.
> > > 
> > > The Renesas CMT and TMU (which I intend to fix too once we find a way
> > > forward for CMT) are the only clockenvet drivers attempting to do
> > > runtime pm.
> > > 
> > >      $ git grep -l pm_runtime_get -- drivers/clocksource
> > >      drivers/clocksource/sh_cmt.c
> > >      drivers/clocksource/sh_mtu2.c
> > >      drivers/clocksource/sh_tmu.c
> > >      drivers/clocksource/timer-ti-dm.c
> > > 
> > > The timer-ti-dm.c driver do not register a clockevent device.
> > > 
> > > > 
> > > > But if PREEMPT_RT is not set, then everything is running as usual.
> > > 
> > > I still get LOCKDEP warnings.
> > 
> > Ah ok, you get the LOCKDEP warning because it identifies the called code to
> > be invalid in case the PREEMPT_RT is compiled-in but does not reflect a real
> > problem with !PREEMPT_RT.
> > 
> > [ ... ]
> > 
> > > The problem is not really PREEMPT_RT. The problem is the clockevents
> > > core require drivers to use raw spinlocks as itself uses them.
> > > 
> > > I would prefer to get the driver in a state without splats, warnings and
> > > potential PREEMPT_RT issues. Especially if the cost is to disable
> > > runtime pm.
> > > 
> > > As I understand it most systems where CMT exists, who don't use it, keep
> > > them disabled in DT. And the devices that use them have
> > > is_sh_early_platform_device() set so they already disable runtime pm.
> > > 
> > > Once we have that fixed we can work on enabling it without the quirks.
> > > In your opinion am I missing something where this approach is a bad idea?
> > 
> > If you prefer to remove the PM in the driver, I'm fine with that.
> 
> I'm not super fond of removing it, but for now I think it is the best 
> way to ensure correct operation in all cases. If anybody have ideas I'm 
> all ears. Specially as I will need to do a similar fix for the TMU 
> driver once we have found a acceptable way for CMT.
> 
> Once we I'm out of the woods and not worried about whats currently in 
> tree can have issues I think it's time to start thinking, if and how we 
> can improve tings in the core so we can enable some or all of the PM CMT 
> and TMU.
> 
> I really appreciate you pushed back on this, I agree removing runtime PM 
> support is kind of a last resort here. But I see no other path.

Just wanted to check if you plan to take this patch in this cycle, no 
rush. But I don't want to start trying similar fixes for TMU before we 
have agreed on this one.

> > 
> > 
> > -- 
> > <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

-- 
Kind Regards,
Niklas Söderlund

On 11/18/25 21:18, Niklas Söderlund wrote:

[ ... ]

> Just wanted to check if you plan to take this patch in this cycle, no
> rush. But I don't want to start trying similar fixes for TMU before we
> have agreed on this one.


Yes, I picked it up. It should be in linux-next


-- 
<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 2025-11-18 21:26:32 +0100, Daniel Lezcano wrote:
> On 11/18/25 21:18, Niklas Söderlund wrote:
> 
> [ ... ]
> 
> > Just wanted to check if you plan to take this patch in this cycle, no
> > rush. But I don't want to start trying similar fixes for TMU before we
> > have agreed on this one.
> 
> 
> Yes, I picked it up. It should be in linux-next

Yes, I see it now. Thanks and sorry for the noise.

> 
> 
> -- 
> <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 11/18/25 21:29, Niklas Söderlund wrote:
> On 2025-11-18 21:26:32 +0100, Daniel Lezcano wrote:
>> On 11/18/25 21:18, Niklas Söderlund wrote:
>>
>> [ ... ]
>>
>>> Just wanted to check if you plan to take this patch in this cycle, no
>>> rush. But I don't want to start trying similar fixes for TMU before we
>>> have agreed on this one.
>>
>>
>> Yes, I picked it up. It should be in linux-next
> 
> Yes, I see it now. Thanks and sorry for the noise.

No problem


-- 
<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

The following commit has been merged into the timers/clocksource branch of tip:

Commit-ID:     62524f285c11d6e6168ad31b586143755b27b2e5
Gitweb:        https://git.kernel.org/tip/62524f285c11d6e6168ad31b586143755b27b2e5
Author:        Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
AuthorDate:    Thu, 16 Oct 2025 20:20:22 +02:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Wed, 26 Nov 2025 11:24:40 +01:00

clocksource/drivers/sh_cmt: Always leave device running after probe

The CMT device can be used as both a clocksource and a clockevent
provider. The driver tries to be smart and power itself on and off, as
well as enabling and disabling its clock when it's not in operation.
This behavior is slightly altered if the CMT is used as an early
platform device in which case the device is left powered on after probe,
but the clock is still enabled and disabled at runtime.

This has worked for a long time, but recent improvements in PREEMPT_RT
and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
as a clockevent provider, clockevents_register_device(), it needs to use
raw spinlocks internally as this is the context of which the clockevent
framework interacts with the CMT driver. However in the context of
holding a raw spinlock the CMT driver can't really manage its power
state or clock with calls to pm_runtime_*() and clk_*() as these calls
end up in other platform drivers using regular spinlocks to control
power and clocks.

This mix of spinlock contexts trips a lockdep warning.

    =============================
    [ 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

For non-PREEMPT_RT builds this is not really an issue, but for
PREEMPT_RT builds where normal spinlocks can sleep this might be an
issue. Be cautious and always leave the power and clock running after
probe.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Link: https://patch.msgid.link/20251016182022.1837417-1-niklas.soderlund+renesas@ragnatech.se
---
 drivers/clocksource/sh_cmt.c | 36 ++---------------------------------
 1 file changed, 3 insertions(+), 33 deletions(-)

diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index 385eb94..791b298 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -355,14 +355,6 @@ static int sh_cmt_enable(struct sh_cmt_channel *ch)
 
 	dev_pm_syscore_device(&ch->cmt->pdev->dev, true);
 
-	/* enable clock */
-	ret = clk_enable(ch->cmt->clk);
-	if (ret) {
-		dev_err(&ch->cmt->pdev->dev, "ch%u: cannot enable clock\n",
-			ch->index);
-		goto err0;
-	}
-
 	/* make sure channel is disabled */
 	sh_cmt_start_stop_ch(ch, 0);
 
@@ -384,19 +376,12 @@ static int sh_cmt_enable(struct sh_cmt_channel *ch)
 	if (ret || sh_cmt_read_cmcnt(ch)) {
 		dev_err(&ch->cmt->pdev->dev, "ch%u: cannot clear CMCNT\n",
 			ch->index);
-		ret = -ETIMEDOUT;
-		goto err1;
+		return -ETIMEDOUT;
 	}
 
 	/* enable channel */
 	sh_cmt_start_stop_ch(ch, 1);
 	return 0;
- err1:
-	/* stop clock */
-	clk_disable(ch->cmt->clk);
-
- err0:
-	return ret;
 }
 
 static void sh_cmt_disable(struct sh_cmt_channel *ch)
@@ -407,9 +392,6 @@ static void sh_cmt_disable(struct sh_cmt_channel *ch)
 	/* disable interrupts in CMT block */
 	sh_cmt_write_cmcsr(ch, 0);
 
-	/* stop clock */
-	clk_disable(ch->cmt->clk);
-
 	dev_pm_syscore_device(&ch->cmt->pdev->dev, false);
 }
 
@@ -583,8 +565,6 @@ static int sh_cmt_start_clocksource(struct sh_cmt_channel *ch)
 	int ret = 0;
 	unsigned long flags;
 
-	pm_runtime_get_sync(&ch->cmt->pdev->dev);
-
 	raw_spin_lock_irqsave(&ch->lock, flags);
 
 	if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE)))
@@ -619,8 +599,6 @@ static void sh_cmt_stop_clocksource(struct sh_cmt_channel *ch)
 		sh_cmt_disable(ch);
 
 	raw_spin_unlock_irqrestore(&ch->lock, flags);
-
-	pm_runtime_put(&ch->cmt->pdev->dev);
 }
 
 static int sh_cmt_start_clockevent(struct sh_cmt_channel *ch)
@@ -630,10 +608,8 @@ static int sh_cmt_start_clockevent(struct sh_cmt_channel *ch)
 
 	raw_spin_lock_irqsave(&ch->lock, flags);
 
-	if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
-		pm_runtime_get_sync(&ch->cmt->pdev->dev);
+	if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE)))
 		ret = sh_cmt_enable(ch);
-	}
 
 	if (ret)
 		goto out;
@@ -656,10 +632,8 @@ static void sh_cmt_stop_clockevent(struct sh_cmt_channel *ch)
 
 	ch->flags &= ~FLAG_CLOCKEVENT;
 
-	if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
+	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)
@@ -1134,8 +1108,6 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt, struct platform_device *pdev)
 		mask &= ~(1 << hwidx);
 	}
 
-	clk_disable(cmt->clk);
-
 	platform_set_drvdata(pdev, cmt);
 
 	return 0;
@@ -1183,8 +1155,6 @@ static int sh_cmt_probe(struct platform_device *pdev)
  out:
 	if (cmt->has_clockevent || cmt->has_clocksource)
 		pm_runtime_irq_safe(&pdev->dev);
-	else
-		pm_runtime_idle(&pdev->dev);
 
 	return 0;
 }