From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 412FDCAC5A5 for ; Thu, 25 Sep 2025 02:26:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:References:CC:To :From:Subject:Message-ID:Date:Content-Type:Content-Transfer-Encoding: MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Ls9j3ehev03+/T3e7eCjckf/cKK1mjKSxTbjZU7yHko=; b=xuAKfStXJ7Mnzd/gFn5iVQh1+P NbeTBoJ+8GyxqzMKEAGv6BLIsMQbqCMAgJiu8ICr46RShx3UGieuJ6POMWCWCrE6h2JIjL47fA1yF XNGEQgrce5pe+7Va81FnRdzkdaWkCPU4wMhVDB5cC5YWprXVbE9RdHToaVyYVF7MoDxgB8wbT+HXE VJqdv6Hawkp/3qaRtI6PFuAjs3GP2Rq3T4vn/Qg0XuRoX5lOoZwegI/OSmswYVrnAnCcSg93pkFjj 1b+Iti3yO227bgswCK/SSJE5sWskmOetPm2NKLKcyL4KRCwsB310e8WgcNEJDxfSgVx/2D71fduig uJAtvEUw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1v1bgx-00000005IQy-39XH; Thu, 25 Sep 2025 02:26:43 +0000 Received: from fllvem-ot04.ext.ti.com ([198.47.19.246]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1v1bgv-00000005IPt-27lB for linux-arm-kernel@lists.infradead.org; Thu, 25 Sep 2025 02:26:42 +0000 Received: from fllvem-sh04.itg.ti.com ([10.64.41.54]) by fllvem-ot04.ext.ti.com (8.15.2/8.15.2) with ESMTP id 58P2QJxI1799988; Wed, 24 Sep 2025 21:26:19 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1758767179; bh=Ls9j3ehev03+/T3e7eCjckf/cKK1mjKSxTbjZU7yHko=; h=Date:Subject:From:To:CC:References:In-Reply-To; b=ij/PIj8UccGFJ7MtcytPs4WJyZsF6uzCdNN0Z28H3V55N5fvGL7G4jkSAVtCZ5gD6 e20BhlrEH7snCwah3nHrtT1T2aoL9J9WHvoJtIdWkKWBa0ye2fHIrKnX0Qj2EqHfo8 nEVjIrMff8N08aipOCtJBRA11+PZ2oG+jgM1LWSY= Received: from DLEE109.ent.ti.com (dlee109.ent.ti.com [157.170.170.41]) by fllvem-sh04.itg.ti.com (8.18.1/8.18.1) with ESMTPS id 58P2QIaK4027010 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA256 bits=128 verify=FAIL); Wed, 24 Sep 2025 21:26:18 -0500 Received: from DLEE205.ent.ti.com (157.170.170.85) by DLEE109.ent.ti.com (157.170.170.41) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.55; Wed, 24 Sep 2025 21:26:17 -0500 Received: from lelvem-mr06.itg.ti.com (10.180.75.8) by DLEE205.ent.ti.com (157.170.170.85) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.2562.20 via Frontend Transport; Wed, 24 Sep 2025 21:26:17 -0500 Received: from localhost (rs-desk.dhcp.ti.com [128.247.81.144]) by lelvem-mr06.itg.ti.com (8.18.1/8.18.1) with ESMTP id 58P2QHDi2737277; Wed, 24 Sep 2025 21:26:17 -0500 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Date: Wed, 24 Sep 2025 21:26:17 -0500 Message-ID: Subject: Re: [PATCH 2/3] clk: keystone: don't cache clock rate From: Randolph Sapp To: Kevin Hilman , Michael Walle , Frank Binns , Matt Coster , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Rob Herring , Krzysztof Kozlowski , "Conor Dooley" , Nishanth Menon , "Vignesh Raghavendra" , Tero Kristo , "Santosh Shilimkar" , Michael Turquette , Stephen Boyd CC: Andrew Davis , , , , , X-Mailer: aerc 0.21.0-0-g5549850facc2 References: <20250915143440.2362812-1-mwalle@kernel.org> <20250915143440.2362812-3-mwalle@kernel.org> <7hv7lhp0e8.fsf@baylibre.com> In-Reply-To: <7hv7lhp0e8.fsf@baylibre.com> X-C2ProcessedOrg: 333ef613-75bf-4e12-a4b1-8e3623f5dcea X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250924_192641_627771_D08AEBCF X-CRM114-Status: GOOD ( 34.80 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed Sep 17, 2025 at 10:24 AM CDT, Kevin Hilman wrote: > Michael Walle 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 everythi= ng as uncached. Side note, why would someone even want to read the rate of an unprepared cl= ock? 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 currentl= y testing. Some devices it only happens once, others get it two or three times. Most o= f them seem to be obvious - someone trying to read a rate before the clock wa= s prepared as part of a probe sequence. One seemed to be happening directly a= fter a clk_prepare_enable call that completed successfully. Not sure how that co= uld happen. >> Thus, disable the cache altogether. >> >> Signed-off-by: Michael Walle >> --- >> 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 > >> --- >> 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-c= lk.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, >> =20 >> init.ops =3D &sci_clk_ops; >> init.num_parents =3D 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 disable= d. >> + * This makes it inherently unsuitable for the caching of the clk >> + * framework. >> + */ >> + init.flags =3D CLK_GET_RATE_NOCACHE; >> sci_clk->hw.init =3D &init; >> =20 >> ret =3D devm_clk_hw_register(provider->dev, &sci_clk->hw); >> --=20 >> 2.39.5