Message-ID: <DD1IXJDTBQ72.2XIEIIN0YA713@ti.com>
Date: Wed, 24 Sep 2025 21:26:17 -0500
From: Randolph Sapp <rs@...com>
To: Kevin Hilman <khilman@...nel.org>, Michael Walle <mwalle@...nel.org>,
        Frank Binns <frank.binns@...tec.com>,
        Matt Coster <matt.coster@...tec.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard
	<mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie
	<airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        "Conor Dooley"
	<conor+dt@...nel.org>, Nishanth Menon <nm@...com>,
        "Vignesh Raghavendra"
	<vigneshr@...com>,
        Tero Kristo <kristo@...nel.org>,
        "Santosh Shilimkar"
	<ssantosh@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen
 Boyd <sboyd@...nel.org>
CC: Andrew Davis <afd@...com>, <dri-devel@...ts.freedesktop.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-clk@...r.kernel.org>
Subject: Re: [PATCH 2/3] clk: keystone: don't cache clock rate

On Wed Sep 17, 2025 at 10:24 AM CDT, Kevin Hilman wrote:
> Michael Walle <mwalle@...nel.org> writes:
>
>> The TISCI firmware will return 0 if the clock or consumer is not
>> enabled although there is a stored value in the firmware. IOW a call to
>> set rate will work but at get rate will always return 0 if the clock is
>> disabled.
>> The clk framework will try to cache the clock rate when it's requested
>> by a consumer. If the clock or consumer is not enabled at that point,
>> the cached value is 0, which is wrong.
>
> Hmm, it also seems wrong to me that the clock framework would cache a
> clock rate when it's disabled.  On platforms with clocks that may have
> shared management (eg. TISCI or other platforms using SCMI) it's
> entirely possible that when Linux has disabled a clock, some other
> entity may have changed it.
>
> Could another solution here be to have the clk framework only cache when
> clocks are enabled?

So I looked into that. There are still about 34 clock operations that are
functionally uncached, but it does seem more logical than treating everything as
uncached.

Side note, why would someone even want to read the rate of an unprepared clock?
I dumped some debug info for all the clocks tripping this new uncached path.
Seems weird that it's even happening this often. Even weirder that it's
apparently happening 3 times to cpu0's core clock on the board I'm currently
testing.

Some devices it only happens once, others get it two or three times. Most of
them seem to be obvious - someone trying to read a rate before the clock was
prepared as part of a probe sequence. One seemed to be happening directly after
a clk_prepare_enable call that completed successfully. Not sure how that could
happen.

>> Thus, disable the cache altogether.
>>
>> Signed-off-by: Michael Walle <mwalle@...nel.org>
>> ---
>> I guess to make it work correctly with the caching of the linux
>> subsystem a new flag to query the real clock rate is needed. That
>> way, one could also query the default value without having to turn
>> the clock and consumer on first. That can be retrofitted later and
>> the driver could query the firmware capabilities.
>>
>> Regarding a Fixes: tag. I didn't include one because it might have a
>> slight performance impact because the firmware has to be queried
>> every time now and it doesn't have been a problem for now. OTOH I've
>> enabled tracing during boot and there were just a handful
>> clock_{get/set}_rate() calls.
>
> The performance hit is not just about boot time, it's for *every*
> [get|set]_rate call.  Since TISCI is relatively slow (involves RPC,
> mailbox, etc. to remote core), this may have a performance impact
> elsewhere too.  That being said, I'm hoping it's unlikely that
> [get|set]_rate calls are in the fast path.
>
> All of that being said, I think the impacts of this patch are pretty
> minimal, so I don't have any real objections.
>
> Reviewed-by: Kevin Hilman <khilman@...libre.com>
>
>> ---
>>  drivers/clk/keystone/sci-clk.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
>> index c5894fc9395e..d73858b5ca7a 100644
>> --- a/drivers/clk/keystone/sci-clk.c
>> +++ b/drivers/clk/keystone/sci-clk.c
>> @@ -333,6 +333,14 @@ static int _sci_clk_build(struct sci_clk_provider *provider,
>>  
>>  	init.ops = &sci_clk_ops;
>>  	init.num_parents = sci_clk->num_parents;
>> +
>> +	/*
>> +	 * A clock rate query to the SCI firmware will return 0 if either the
>> +	 * clock itself is disabled or the attached device/consumer is disabled.
>> +	 * This makes it inherently unsuitable for the caching of the clk
>> +	 * framework.
>> +	 */
>> +	init.flags = CLK_GET_RATE_NOCACHE;
>>  	sci_clk->hw.init = &init;
>>  
>>  	ret = devm_clk_hw_register(provider->dev, &sci_clk->hw);
>> -- 
>> 2.39.5

Powered by blists - more mailing lists