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 280DAC6FD20 for ; Fri, 24 Mar 2023 11:20:26 +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:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=jnhC/DvfTlyZcOnZuUe9LuHIarcRp2uTbbPObwOvLtk=; b=DnWjUrpLMeHdtJl6z3h/Bp6GOX D34oN1Lp+ShBz10es/VAKSQHN1RoIlOYDIoRXTxIgT7bhQKpUx4SEgruCnosA27LwsHHjKw9by2pW aXdRxirbdIvEKGATBPbR1jGUEbMzlM9Jnd3FVzuUzDK7BKa+Odhu4YI48l6YG5Dn+md86CNCT24MW wqt8FFW+TMF4uNk6ajziAKDi7Cgk1TqBnq/YsiUl7a5PytSJJAWNVtfrQEe9GdDu7c84YeJ1elQBo NLEU91GtHYZ98EoncZ/fknPHDQPBQkH9P2AACrU6gI0Cwb7ZQc78qF4fKfmr9m7o9Rb9EL4Zk3R7H +GIDmk7w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pffSy-004EZg-1b; Fri, 24 Mar 2023 11:20:16 +0000 Received: from new2-smtp.messagingengine.com ([66.111.4.224]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pffSt-004EXn-0R; Fri, 24 Mar 2023 11:20:13 +0000 Received: from compute6.internal (compute6.nyi.internal [10.202.2.47]) by mailnew.nyi.internal (Postfix) with ESMTP id 02E8758211D; Fri, 24 Mar 2023 07:20:06 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute6.internal (MEProxy); Fri, 24 Mar 2023 07:20:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc :cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:sender :subject:subject:to:to; s=fm3; t=1679656805; x=1679664005; bh=jn hC/DvfTlyZcOnZuUe9LuHIarcRp2uTbbPObwOvLtk=; b=RINbDJrDavr91qApxS 4BIapnsTSMpJsQcMuiekOJ4buZfgO1V5bxZUX92x96X+70RG5pM597H6X6A8ze2Q J3K3HTIk9bh/ZbHNJcL4cY9CXoNlp9BFd+TTAQzaGhbL4Q/xS5FNx1TElQW6xvG7 0pWgXxxHWJn2xo8oKO6WZBtJ8/apSTxWaFgzT4XANpTX/yFywGqPfreFSFA+SxYy DVWfFOmgiUFFAwcBCuUS5l1GOHRlbwP6IKA226oMHwgNZfpxX+vE/kqqplzGKxN6 3CA6w8YfKEc956VkNbvyMcVk4fjXMyE6jZ/K83ddG+CzpHvsr07jRCCCdY737Omz uzeA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; t=1679656805; x=1679664005; bh=jnhC/DvfTlyZc OnZuUe9LuHIarcRp2uTbbPObwOvLtk=; b=G4KWdAq5u3yOdbhwSK/77/yGH7IQB 2xysJ+4DHASMDHD4DpUNSh0qiUQ7PoZ0HeB31FPCqa3ypIcHnjDyiqx4+OASl4vt YII+0RiTBeUGCTuHiRp/2zrlkY7JsRYyob4hyT9jfvFspEvX+TWDxi6HeqVtkkkI EIEz+43mJMF3TccMFqhY9/s2d2q0Bnzb5506eHVsiiRibRjc0JM/OyGd5+joNcoL b2y+9ja4sW5wabUkTzlsQiAxcTUWRGjZRPmuIIWmJFWEdsTq0+X56oaR8GkrkVqB dX4i5J533jmuMxkiG1iqzEHijtLWz0ghQXTKvlKaafFgHLlEw3jiDRZ4g== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrvdegiedgvdejucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvfevuffkfhggtggujgesghdtreertddtvdenucfhrhhomhepofgrgihi mhgvucftihhprghrugcuoehmrgigihhmvgestggvrhhnohdrthgvtghhqeenucggtffrrg htthgvrhhnpeetfefffefgkedtfefgledugfdtjeefjedvtddtkeetieffjedvgfehheff hfevudenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpe hmrgigihhmvgestggvrhhnohdrthgvtghh X-ME-Proxy: Feedback-ID: i8771445c:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 24 Mar 2023 07:20:01 -0400 (EDT) Date: Fri, 24 Mar 2023 12:19:59 +0100 From: Maxime Ripard To: Aidan MacDonald Cc: Stephen Boyd , Paul Cercueil , Maxime Coquelin , Chen-Yu Tsai , Daniel Vetter , Nicolas Ferre , Thierry Reding , Jaroslav Kysela , Shawn Guo , Fabio Estevam , Ulf Hansson , Claudiu Beznea , Michael Turquette , Dinh Nguyen , Chunyan Zhang , Manivannan Sadhasivam , Andreas =?utf-8?Q?F=C3=A4rber?= , Jonathan Hunter , Abel Vesa , Charles Keepax , Alessandro Zummo , Peter De Schrijver , Orson Zhai , Alexandre Torgue , Prashant Gaikwad , Liam Girdwood , Alexandre Belloni , Samuel Holland , Matthias Brugger , Richard Fitzgerald , Vinod Koul , NXP Linux Team , Sekhar Nori , Kishon Vijay Abraham I , Linus Walleij , Takashi Iwai , David Airlie , Luca Ceresoli , Jernej Skrabec , Pengutronix Kernel Team , Baolin Wang , David Lechner , Sascha Hauer , Mark Brown , Max Filippov , Geert Uytterhoeven , linux-stm32@st-md-mailman.stormreply.com, alsa-devel@alsa-project.org, linux-mediatek@lists.infradead.org, linux-phy@lists.infradead.org, linux-mips@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-actions@lists.infradead.org, linux-clk@vger.kernel.org, AngeloGioacchino Del Regno , patches@opensource.cirrus.com, linux-tegra@vger.kernel.org, linux-rtc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate Message-ID: <20230324111959.frjf4neopbs67ugd@houat> References: <20221018-clk-range-checks-fixes-v2-0-f6736dec138e@cerno.tech> <20221018-clk-range-checks-fixes-v2-56-f6736dec138e@cerno.tech> <80VTKR.CE8RVN8M3ZYK3@crapouillou.net> <20221104145946.orsyrhiqvypisl5j@houat> <20221107085417.xrsh6xy3ouwdkp4z@houat> <20221109110045.j24vwkaq3s4yzoy3@houat> <06a293adc75990ed3e297b076fc38d8a.sboyd@kernel.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="ygvujwxxgbycerm7" Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230324_042012_024103_BAFB5FB5 X-CRM114-Status: GOOD ( 46.57 ) X-BeenThere: linux-mediatek@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-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org --ygvujwxxgbycerm7 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Thu, Mar 23, 2023 at 03:35:30PM +0000, Aidan MacDonald wrote: >=20 > Stephen Boyd writes: >=20 > > Quoting Maxime Ripard (2022-11-09 03:00:45) > >> On Mon, Nov 07, 2022 at 08:57:22PM +0000, Aidan MacDonald wrote: > >> > > >> > Maxime Ripard writes: > >> > > >> > > Hi, > >> > > > >> > > On Fri, Nov 04, 2022 at 05:35:29PM +0000, Aidan MacDonald wrote: > >> > > >> > Assigning the parent clock in the DT works once, at boot, but going = off > >> > what you wrote in the commit message, if the clock driver has a > >> > .determine_rate() implementation that *can* reparent clocks then it > >> > probably *will* reparent them, and the DT assignment will be lost. > >> > >> Yes, indeed, but assigned-clock-parents never provided any sort of > >> guarantee on whether or not the clock was allowed to reparent or not. > >> It's just a one-off thing, right before probe, and a clk_set_parent() > >> call at probe will override that just fine. > >> > >> Just like assigned-clock-rates isn't permanent. > >> > >> > What I'm suggesting is a runtime constraint that the clock subsystem > >> > would enforce, and actively prevent drivers from changing the parent. > >> > Either explicitly with clk_set_parent() or due to .determine_rate(). > >> > > >> > That way you could write a .determine_rate() implementation that *ca= n* > >> > select a better parent, but if the DT applies a constraint to fix the > >> > clock to a particular parent, the clock subsystem will force that pa= rent > >> > to be used so you can be sure the clock is never reparented by accid= ent. > >> > >> Yeah, that sounds like a good idea, and CLK_SET_RATE_NO_REPARENT isn't > >> too far off from this, it's just ignored by clk_set_parent() for now. I > >> guess we could rename CLK_SET_RATE_NO_REPARENT to CLK_NO_REPARENT, make > >> clk_set_parent handle it, and set that flag whenever > >> assigned-clock-parents is set on a clock. > >> > >> It's out of scope for this series though, and I certainly don't want to > >> deal with all the regressions it might create :) > >> > > > > This sounds like a new dt binding that says the assigned parent should > > never change. It sounds sort of like gpio hogs. A clock-hogs binding? >=20 > Ideally we want the clock driver to be able to reparent clocks freely > to get the best rate. But we also need some control over that to stop > consumers from being reparented in undesired ways. Eg. you might want > to make sure the GPU gets its own PLL so it can be reclocked easily, > and putting another device on the GPU's PLL could prevent that. >=20 > The only way to achieve this today is (1) never do any reparenting in > the clock driver; and (2) use assigned-clock-parents in the DT to set > up the entire clock tree manually. >=20 > Maxime said that (2) is basically wrong -- if assigned-clock-parents > provides no guarantee on what the OS does "after boot" then the OS is > pretty much free to ignore it. I didn't really say it's wrong, just that it never provided the guarantee you expect it to provide. I can't really say whether it's an issue or not on your platform. It's mostly unrelated to this series though, none of these patches affect that behavior in one way or the other. > My suggestion: add a per-clock bitmap to keep track of which parents > are allowed. Any operation that would select a parent clock not on the > whitelist should fail. Automatic reparenting should only select from > clocks on the whitelist. And we need new DT bindings for controlling > the whitelist, for example: >=20 > clock-parents-0 =3D <&clk1>, <&pll_c>; > clock-parents-1 =3D <&clk2>, <&pll_a>, <&pll_b>; >=20 > This means that clk1 can only have pll_c as a parent, while clk2 can > have pll_a or pll_b as parents. By default every clock will be able > to use any parent, so a list is only needed if the machine needs a > more restrictive policy. >=20 > assigned-clock-parents should disable automatic reparenting, but allow > explicit clk_set_parent(). This will allow clock drivers to start doing > reparenting without breaking old DTs. I'm generally not a fan of putting all these policies in the device tree. Do you have an example where it wouldn't be possible to do exactly this from the driver itself? Maxime --ygvujwxxgbycerm7 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCZB2HXwAKCRDj7w1vZxhR xSbnAQCJvmVlpJgunPtELVTvf4BU6vbdciJ5jecqJV2UslBqNAEA3GtvUaTD5e0p e0nSvm2EbCQGLtQFj+xVrIWIaKTMYAc= =GgBk -----END PGP SIGNATURE----- --ygvujwxxgbycerm7--