Re: [PATCH v2] clocksource/drivers/sh_cmt: Always leave device running after probe

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

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