From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aposti.net (aposti.net [89.234.176.197]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 814872F45 for ; Wed, 9 Nov 2022 11:37:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=crapouillou.net; s=mail; t=1667993821; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3HeCzuAd5DrGQl3yX0PoEWG+18EHx/4Mkssb32++3BI=; b=WwCPTNJPOIDWtdzd5piGmEgZU4J4b1t9NVy+qRbY3+ISBvVQ6apE6t+8jx2p/zEgIxnja5 9nWJEIf1duGHYh5JmCtFyh2CbFVaBUveP5Si2RGhwuWsnzKNfsXmdpkN0q+yUABhtgFuRh Y+C1HQZU871ihv7USL4Dk3I6J/l65gU= Date: Wed, 09 Nov 2022 11:36:35 +0000 From: Paul Cercueil Subject: Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate To: Maxime Ripard Cc: Stephen Boyd , 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 =?iso-8859-1?q?F=E4rber?= , 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 Message-Id: In-Reply-To: <20221109105301.ueus7o3b75j5yeff@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> <20221109105301.ueus7o3b75j5yeff@houat> Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Hi Maxime, Le mer. 9 nov. 2022 =E0 11:53:01 +0100, Maxime Ripard=20 a =E9crit : > Hi Paul, >=20 > On Sat, Nov 05, 2022 at 10:33:54AM +0000, Paul Cercueil wrote: >> Hi Maxime, >>=20 >> Le ven. 4 nov. 2022 =E0 15:59:46 +0100, Maxime Ripard=20 >> a >> =E9crit : >> > Hi Paul, >> > >> > On Fri, Nov 04, 2022 at 02:31:20PM +0000, Paul Cercueil wrote: >> > > Le ven. 4 nov. 2022 =E0 14:18:13 +0100, Maxime Ripard >> > > a >> > > =E9crit : >> > > > The Ingenic CGU clocks implements a mux with a set_parent=20 >> hook, >> > > but >> > > > doesn't provide a determine_rate implementation. >> > > > >> > > > This is a bit odd, since set_parent() is there to, as its=20 >> name >> > > implies, >> > > > change the parent of a clock. However, the most likely=20 >> candidate >> > > to >> > > > trigger that parent change is a call to clk_set_rate(), with >> > > > determine_rate() figuring out which parent is the best=20 >> suited for >> > > a >> > > > given rate. >> > > > >> > > > The other trigger would be a call to clk_set_parent(), but=20 >> it's >> > > far less >> > > > used, and it doesn't look like there's any obvious user for=20 >> that >> > > clock. >> > > > >> > > > So, the set_parent hook is effectively unused, possibly=20 >> because >> > > of an >> > > > oversight. However, it could also be an explicit decision by=20 >> the >> > > > original author to avoid any reparenting but through an=20 >> explicit >> > > call to >> > > > clk_set_parent(). >> > > > >> > > > The driver does implement round_rate() though, which means=20 >> that >> > > we can >> > > > change the rate of the clock, but we will never get to=20 >> change the >> > > > parent. >> > > > >> > > > However, It's hard to tell whether it's been done on purpose=20 >> or >> > > not. >> > > > >> > > > Since we'll start mandating a determine_rate()=20 >> implementation, >> > > let's >> > > > convert the round_rate() implementation to a=20 >> determine_rate(), >> > > which >> > > > will also make the current behavior explicit. And if it was=20 >> an >> > > > oversight, the clock behaviour can be adjusted later on. >> > > >> > > So it's partly on purpose, partly because I didn't know about >> > > .determine_rate. >> > > >> > > There's nothing odd about having a lonely .set_parent=20 >> callback; in >> > > my case >> > > the clocks are parented from the device tree. >> > > >> > > Having the clocks driver trigger a parent change when=20 >> requesting a >> > > rate >> > > change sounds very dangerous, IMHO. My MMC controller can be >> > > parented to the >> > > external 48 MHz oscillator, and if the card requests 50 MHz, it >> > > could switch >> > > to one of the PLLs. That works as long as the PLLs don't change >> > > rate, but if >> > > one is configured as driving the CPU clock, it becomes messy. >> > > The thing is, the clocks driver has no way to know whether or=20 >> not >> > > it is >> > > "safe" to use a designated parent. >> > > >> > > For that reason, in practice, I never actually want to have a=20 >> clock >> > > re-parented - it's almost always a bad idea vs. sticking to the >> > > parent clock >> > > configured in the DTS. >> > >> > Yeah, and this is totally fine. But we need to be explicit about=20 >> it. The >> > determine_rate implementation I did in all the patches is an exact >> > equivalent to the round_rate one if there was one. We will never=20 >> ask to >> > change the parent. >> > >> > Given what you just said, I would suggest to set the >> > CLK_SET_RATE_NO_REPARENT flag as well. >>=20 >> But that would introduce policy into the driver... >=20 > I'm not sure why you're bringing policies into that discussion.=20 > There's > plenty of policy in the driver already, and the current code doesn't=20 > do > something that the old wasn't doing (implicitly). Yes, I was just talking about the CLK_SET_RATE_NO_REPARENT flag adding=20 policy. The fact that there's plenty of policy in the driver already is=20 not an argument for adding some more. > And there's plenty of policies in drivers in general. Whether you=20 > limit > the rate or not, whether you allow reparenting or not, even the > CLK_SET_RATE_NO_REPARENT flag mentioned above is a policy decision set > by drivers. Allowing reparenting and not limiting the rates is not a policy, it's=20 just following what the hardware allows you to do. The absence of=20 policy means that the driver allows you to configure the hardware in=20 any way you might want to. Limiting rates, forbidding reparenting, that's policy, and it doesn't=20 belong in a driver. You can argue that choosing not to reparent on rate change is a policy,=20 and it is. That's why we need a way to enforce these policies outside=20 the driver. >> The fact that I don't want the MMC parented to the PLLs, doesn't=20 >> mean >> that it's an invalid configuration per se. >=20 > Sure, and that's another policy :) A policy that is not enforced by the driver. Going back to the patch itself... I am fine with the change, although=20 the patch description should probably be updated. We have .set_parent=20 callbacks to configure clocks from DT, there's nothing more to it. Cheers, -Paul 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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (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 5BAAFC433FE for ; Wed, 9 Nov 2022 11:37:09 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B427D10E09C; Wed, 9 Nov 2022 11:37:07 +0000 (UTC) Received: from aposti.net (aposti.net [89.234.176.197]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6AF3510E09C for ; Wed, 9 Nov 2022 11:37:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=crapouillou.net; s=mail; t=1667993821; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3HeCzuAd5DrGQl3yX0PoEWG+18EHx/4Mkssb32++3BI=; b=WwCPTNJPOIDWtdzd5piGmEgZU4J4b1t9NVy+qRbY3+ISBvVQ6apE6t+8jx2p/zEgIxnja5 9nWJEIf1duGHYh5JmCtFyh2CbFVaBUveP5Si2RGhwuWsnzKNfsXmdpkN0q+yUABhtgFuRh Y+C1HQZU871ihv7USL4Dk3I6J/l65gU= Date: Wed, 09 Nov 2022 11:36:35 +0000 From: Paul Cercueil Subject: Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate To: Maxime Ripard Message-Id: In-Reply-To: <20221109105301.ueus7o3b75j5yeff@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> <20221109105301.ueus7o3b75j5yeff@houat> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Ulf Hansson , Prashant Gaikwad , Alexandre Belloni , Liam Girdwood , Michael Turquette , Sekhar Nori , Alexandre Torgue , dri-devel@lists.freedesktop.org, Jaroslav Kysela , Max Filippov , Thierry Reding , linux-phy@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, Abel Vesa , Kishon Vijay Abraham I , Geert Uytterhoeven , Samuel Holland , Chunyan Zhang , Takashi Iwai , linux-tegra@vger.kernel.org, Jernej Skrabec , Jonathan Hunter , Chen-Yu Tsai , NXP Linux Team , Orson Zhai , linux-mips@vger.kernel.org, Luca Ceresoli , linux-rtc@vger.kernel.org, linux-clk@vger.kernel.org, Charles Keepax , alsa-devel@alsa-project.org, Manivannan Sadhasivam , linux-kernel@vger.kernel.org, Sascha Hauer , linux-actions@lists.infradead.org, Richard Fitzgerald , Mark Brown , linux-mediatek@lists.infradead.org, Baolin Wang , Matthias Brugger , Pengutronix Kernel Team , linux-arm-kernel@lists.infradead.org, AngeloGioacchino Del Regno , Alessandro Zummo , linux-sunxi@lists.linux.dev, Stephen Boyd , patches@opensource.cirrus.com, Peter De Schrijver , Nicolas Ferre , Andreas =?iso-8859-1?q?F=E4rber?= , linux-renesas-soc@vger.kernel.org, Dinh Nguyen , Vinod Koul , Maxime Coquelin , David Lechner , Shawn Guo , Claudiu Beznea Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi Maxime, Le mer. 9 nov. 2022 =E0 11:53:01 +0100, Maxime Ripard=20 a =E9crit : > Hi Paul, >=20 > On Sat, Nov 05, 2022 at 10:33:54AM +0000, Paul Cercueil wrote: >> Hi Maxime, >>=20 >> Le ven. 4 nov. 2022 =E0 15:59:46 +0100, Maxime Ripard=20 >> a >> =E9crit : >> > Hi Paul, >> > >> > On Fri, Nov 04, 2022 at 02:31:20PM +0000, Paul Cercueil wrote: >> > > Le ven. 4 nov. 2022 =E0 14:18:13 +0100, Maxime Ripard >> > > a >> > > =E9crit : >> > > > The Ingenic CGU clocks implements a mux with a set_parent=20 >> hook, >> > > but >> > > > doesn't provide a determine_rate implementation. >> > > > >> > > > This is a bit odd, since set_parent() is there to, as its=20 >> name >> > > implies, >> > > > change the parent of a clock. However, the most likely=20 >> candidate >> > > to >> > > > trigger that parent change is a call to clk_set_rate(), with >> > > > determine_rate() figuring out which parent is the best=20 >> suited for >> > > a >> > > > given rate. >> > > > >> > > > The other trigger would be a call to clk_set_parent(), but=20 >> it's >> > > far less >> > > > used, and it doesn't look like there's any obvious user for=20 >> that >> > > clock. >> > > > >> > > > So, the set_parent hook is effectively unused, possibly=20 >> because >> > > of an >> > > > oversight. However, it could also be an explicit decision by=20 >> the >> > > > original author to avoid any reparenting but through an=20 >> explicit >> > > call to >> > > > clk_set_parent(). >> > > > >> > > > The driver does implement round_rate() though, which means=20 >> that >> > > we can >> > > > change the rate of the clock, but we will never get to=20 >> change the >> > > > parent. >> > > > >> > > > However, It's hard to tell whether it's been done on purpose=20 >> or >> > > not. >> > > > >> > > > Since we'll start mandating a determine_rate()=20 >> implementation, >> > > let's >> > > > convert the round_rate() implementation to a=20 >> determine_rate(), >> > > which >> > > > will also make the current behavior explicit. And if it was=20 >> an >> > > > oversight, the clock behaviour can be adjusted later on. >> > > >> > > So it's partly on purpose, partly because I didn't know about >> > > .determine_rate. >> > > >> > > There's nothing odd about having a lonely .set_parent=20 >> callback; in >> > > my case >> > > the clocks are parented from the device tree. >> > > >> > > Having the clocks driver trigger a parent change when=20 >> requesting a >> > > rate >> > > change sounds very dangerous, IMHO. My MMC controller can be >> > > parented to the >> > > external 48 MHz oscillator, and if the card requests 50 MHz, it >> > > could switch >> > > to one of the PLLs. That works as long as the PLLs don't change >> > > rate, but if >> > > one is configured as driving the CPU clock, it becomes messy. >> > > The thing is, the clocks driver has no way to know whether or=20 >> not >> > > it is >> > > "safe" to use a designated parent. >> > > >> > > For that reason, in practice, I never actually want to have a=20 >> clock >> > > re-parented - it's almost always a bad idea vs. sticking to the >> > > parent clock >> > > configured in the DTS. >> > >> > Yeah, and this is totally fine. But we need to be explicit about=20 >> it. The >> > determine_rate implementation I did in all the patches is an exact >> > equivalent to the round_rate one if there was one. We will never=20 >> ask to >> > change the parent. >> > >> > Given what you just said, I would suggest to set the >> > CLK_SET_RATE_NO_REPARENT flag as well. >>=20 >> But that would introduce policy into the driver... >=20 > I'm not sure why you're bringing policies into that discussion.=20 > There's > plenty of policy in the driver already, and the current code doesn't=20 > do > something that the old wasn't doing (implicitly). Yes, I was just talking about the CLK_SET_RATE_NO_REPARENT flag adding=20 policy. The fact that there's plenty of policy in the driver already is=20 not an argument for adding some more. > And there's plenty of policies in drivers in general. Whether you=20 > limit > the rate or not, whether you allow reparenting or not, even the > CLK_SET_RATE_NO_REPARENT flag mentioned above is a policy decision set > by drivers. Allowing reparenting and not limiting the rates is not a policy, it's=20 just following what the hardware allows you to do. The absence of=20 policy means that the driver allows you to configure the hardware in=20 any way you might want to. Limiting rates, forbidding reparenting, that's policy, and it doesn't=20 belong in a driver. You can argue that choosing not to reparent on rate change is a policy,=20 and it is. That's why we need a way to enforce these policies outside=20 the driver. >> The fact that I don't want the MMC parented to the PLLs, doesn't=20 >> mean >> that it's an invalid configuration per se. >=20 > Sure, and that's another policy :) A policy that is not enforced by the driver. Going back to the patch itself... I am fine with the change, although=20 the patch description should probably be updated. We have .set_parent=20 callbacks to configure clocks from DT, there's nothing more to it. Cheers, -Paul 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 85944C4332F for ; Wed, 9 Nov 2022 11:37:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-Id:Cc:To :Subject:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=MXkg6Hb5IahZ25ddUuZik3wLYxXABPs15yrCYACSRH0=; b=tEvXb2mE76YDHxpWNgpOZwsMPI HcSEEqSzrMOJAkp0t2KG0jQy+Kdge3Rix5541Gqm/3n2gLOgYjlyOuJ7kCznLDo3TtA3nY9+QAzzP UWKVmQCo9PPImwyo97k1xV66S51bOomiHyOYpWbGyGUO4/BEBt5EP48BnJwC7hhA68R8UJGpNlGx0 697RqDk43a+ca3EbqRhRrcsRTdmmlhU7i7mS/Rues/AGlJjAYoQ69kAcnbMTodnv/f3EoYjkdBG3I XzECd8FcAo/MqvBIG/760lVRud5sjqBXY6unjWBwZr2KC+6mXAqm282GuBK3UIg/QzAArdpfKVBPQ qud7AdHA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1osjON-00D2DP-UJ; Wed, 09 Nov 2022 11:37:15 +0000 Received: from aposti.net ([89.234.176.197]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1osjOI-00D2Bf-L2; Wed, 09 Nov 2022 11:37:12 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=crapouillou.net; s=mail; t=1667993821; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3HeCzuAd5DrGQl3yX0PoEWG+18EHx/4Mkssb32++3BI=; b=WwCPTNJPOIDWtdzd5piGmEgZU4J4b1t9NVy+qRbY3+ISBvVQ6apE6t+8jx2p/zEgIxnja5 9nWJEIf1duGHYh5JmCtFyh2CbFVaBUveP5Si2RGhwuWsnzKNfsXmdpkN0q+yUABhtgFuRh Y+C1HQZU871ihv7USL4Dk3I6J/l65gU= Date: Wed, 09 Nov 2022 11:36:35 +0000 From: Paul Cercueil Subject: Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate To: Maxime Ripard Cc: Stephen Boyd , 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 =?iso-8859-1?q?F=E4rber?= , 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 Message-Id: In-Reply-To: <20221109105301.ueus7o3b75j5yeff@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> <20221109105301.ueus7o3b75j5yeff@houat> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221109_033710_874006_CB84D86F X-CRM114-Status: GOOD ( 49.62 ) X-BeenThere: linux-phy@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux Phy Mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Sender: "linux-phy" Errors-To: linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org Hi Maxime, Le mer. 9 nov. 2022 =E0 11:53:01 +0100, Maxime Ripard = a =E9crit : > Hi Paul, > = > On Sat, Nov 05, 2022 at 10:33:54AM +0000, Paul Cercueil wrote: >> Hi Maxime, >> = >> Le ven. 4 nov. 2022 =E0 15:59:46 +0100, Maxime Ripard = >> a >> =E9crit : >> > Hi Paul, >> > >> > On Fri, Nov 04, 2022 at 02:31:20PM +0000, Paul Cercueil wrote: >> > > Le ven. 4 nov. 2022 =E0 14:18:13 +0100, Maxime Ripard >> > > a >> > > =E9crit : >> > > > The Ingenic CGU clocks implements a mux with a set_parent = >> hook, >> > > but >> > > > doesn't provide a determine_rate implementation. >> > > > >> > > > This is a bit odd, since set_parent() is there to, as its = >> name >> > > implies, >> > > > change the parent of a clock. However, the most likely = >> candidate >> > > to >> > > > trigger that parent change is a call to clk_set_rate(), with >> > > > determine_rate() figuring out which parent is the best = >> suited for >> > > a >> > > > given rate. >> > > > >> > > > The other trigger would be a call to clk_set_parent(), but = >> it's >> > > far less >> > > > used, and it doesn't look like there's any obvious user for = >> that >> > > clock. >> > > > >> > > > So, the set_parent hook is effectively unused, possibly = >> because >> > > of an >> > > > oversight. However, it could also be an explicit decision by = >> the >> > > > original author to avoid any reparenting but through an = >> explicit >> > > call to >> > > > clk_set_parent(). >> > > > >> > > > The driver does implement round_rate() though, which means = >> that >> > > we can >> > > > change the rate of the clock, but we will never get to = >> change the >> > > > parent. >> > > > >> > > > However, It's hard to tell whether it's been done on purpose = >> or >> > > not. >> > > > >> > > > Since we'll start mandating a determine_rate() = >> implementation, >> > > let's >> > > > convert the round_rate() implementation to a = >> determine_rate(), >> > > which >> > > > will also make the current behavior explicit. And if it was = >> an >> > > > oversight, the clock behaviour can be adjusted later on. >> > > >> > > So it's partly on purpose, partly because I didn't know about >> > > .determine_rate. >> > > >> > > There's nothing odd about having a lonely .set_parent = >> callback; in >> > > my case >> > > the clocks are parented from the device tree. >> > > >> > > Having the clocks driver trigger a parent change when = >> requesting a >> > > rate >> > > change sounds very dangerous, IMHO. My MMC controller can be >> > > parented to the >> > > external 48 MHz oscillator, and if the card requests 50 MHz, it >> > > could switch >> > > to one of the PLLs. That works as long as the PLLs don't change >> > > rate, but if >> > > one is configured as driving the CPU clock, it becomes messy. >> > > The thing is, the clocks driver has no way to know whether or = >> not >> > > it is >> > > "safe" to use a designated parent. >> > > >> > > For that reason, in practice, I never actually want to have a = >> clock >> > > re-parented - it's almost always a bad idea vs. sticking to the >> > > parent clock >> > > configured in the DTS. >> > >> > Yeah, and this is totally fine. But we need to be explicit about = >> it. The >> > determine_rate implementation I did in all the patches is an exact >> > equivalent to the round_rate one if there was one. We will never = >> ask to >> > change the parent. >> > >> > Given what you just said, I would suggest to set the >> > CLK_SET_RATE_NO_REPARENT flag as well. >> = >> But that would introduce policy into the driver... > = > I'm not sure why you're bringing policies into that discussion. = > There's > plenty of policy in the driver already, and the current code doesn't = > do > something that the old wasn't doing (implicitly). Yes, I was just talking about the CLK_SET_RATE_NO_REPARENT flag adding = policy. The fact that there's plenty of policy in the driver already is = not an argument for adding some more. > And there's plenty of policies in drivers in general. Whether you = > limit > the rate or not, whether you allow reparenting or not, even the > CLK_SET_RATE_NO_REPARENT flag mentioned above is a policy decision set > by drivers. Allowing reparenting and not limiting the rates is not a policy, it's = just following what the hardware allows you to do. The absence of = policy means that the driver allows you to configure the hardware in = any way you might want to. Limiting rates, forbidding reparenting, that's policy, and it doesn't = belong in a driver. You can argue that choosing not to reparent on rate change is a policy, = and it is. That's why we need a way to enforce these policies outside = the driver. >> The fact that I don't want the MMC parented to the PLLs, doesn't = >> mean >> that it's an invalid configuration per se. > = > Sure, and that's another policy :) A policy that is not enforced by the driver. Going back to the patch itself... I am fine with the change, although = the patch description should probably be updated. We have .set_parent = callbacks to configure clocks from DT, there's nothing more to it. Cheers, -Paul -- = linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy 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 alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (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 D227BC4332F for ; Wed, 9 Nov 2022 17:18:03 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 06A3A1686; Wed, 9 Nov 2022 18:17:12 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 06A3A1686 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1668014282; bh=smtpmkKjN841tEBNu+R+jZBfyuKEV1zGB2S9xqA0KRg=; h=Date:From:Subject:To:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=O7FKpu0PqwhEZBFTICUDZ0p2I1cvZvK4K0MvOpfQGfhcbZy3pQzTO38I5k0TGKNiw pDNA2GFKBLsRNLOa6WdcmniyvzRjxMNmAWBOPa7rpkbFMeLb+5VOYqoorz2zXGEVrT qY5dwFplfONTQg5yDATj68I21J4y27P1MGWb63NI= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id EE09FF80568; Wed, 9 Nov 2022 18:15:54 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 45371F80121; Wed, 9 Nov 2022 12:37:08 +0100 (CET) Received: from aposti.net (aposti.net [89.234.176.197]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 20BBFF80121 for ; Wed, 9 Nov 2022 12:37:01 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 20BBFF80121 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=crapouillou.net header.i=@crapouillou.net header.b="WwCPTNJP" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=crapouillou.net; s=mail; t=1667993821; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3HeCzuAd5DrGQl3yX0PoEWG+18EHx/4Mkssb32++3BI=; b=WwCPTNJPOIDWtdzd5piGmEgZU4J4b1t9NVy+qRbY3+ISBvVQ6apE6t+8jx2p/zEgIxnja5 9nWJEIf1duGHYh5JmCtFyh2CbFVaBUveP5Si2RGhwuWsnzKNfsXmdpkN0q+yUABhtgFuRh Y+C1HQZU871ihv7USL4Dk3I6J/l65gU= Date: Wed, 09 Nov 2022 11:36:35 +0000 From: Paul Cercueil Subject: Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate To: Maxime Ripard Message-Id: In-Reply-To: <20221109105301.ueus7o3b75j5yeff@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> <20221109105301.ueus7o3b75j5yeff@houat> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable X-Mailman-Approved-At: Wed, 09 Nov 2022 18:15:49 +0100 Cc: Ulf Hansson , Prashant Gaikwad , Alexandre Belloni , Liam Girdwood , Michael Turquette , Sekhar Nori , Alexandre Torgue , dri-devel@lists.freedesktop.org, Max Filippov , Thierry Reding , linux-phy@lists.infradead.org, David Airlie , Fabio Estevam , linux-stm32@st-md-mailman.stormreply.com, Abel Vesa , Kishon Vijay Abraham I , Geert Uytterhoeven , Samuel Holland , Chunyan Zhang , Takashi Iwai , linux-tegra@vger.kernel.org, Jernej Skrabec , Jonathan Hunter , Chen-Yu Tsai , NXP Linux Team , Orson Zhai , linux-mips@vger.kernel.org, Luca Ceresoli , Linus Walleij , linux-rtc@vger.kernel.org, linux-clk@vger.kernel.org, Charles Keepax , Daniel Vetter , alsa-devel@alsa-project.org, Manivannan Sadhasivam , linux-kernel@vger.kernel.org, Sascha Hauer , linux-actions@lists.infradead.org, Richard Fitzgerald , Mark Brown , linux-mediatek@lists.infradead.org, Baolin Wang , Matthias Brugger , Pengutronix Kernel Team , linux-arm-kernel@lists.infradead.org, AngeloGioacchino Del Regno , Alessandro Zummo , linux-sunxi@lists.linux.dev, Stephen Boyd , patches@opensource.cirrus.com, Peter De Schrijver , Nicolas Ferre , Andreas =?iso-8859-1?q?F=E4rber?= , linux-renesas-soc@vger.kernel.org, Dinh Nguyen , Vinod Koul , Maxime Coquelin , David Lechner , Shawn Guo , Claudiu Beznea X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" Hi Maxime, Le mer. 9 nov. 2022 =E0 11:53:01 +0100, Maxime Ripard=20 a =E9crit : > Hi Paul, >=20 > On Sat, Nov 05, 2022 at 10:33:54AM +0000, Paul Cercueil wrote: >> Hi Maxime, >>=20 >> Le ven. 4 nov. 2022 =E0 15:59:46 +0100, Maxime Ripard=20 >> a >> =E9crit : >> > Hi Paul, >> > >> > On Fri, Nov 04, 2022 at 02:31:20PM +0000, Paul Cercueil wrote: >> > > Le ven. 4 nov. 2022 =E0 14:18:13 +0100, Maxime Ripard >> > > a >> > > =E9crit : >> > > > The Ingenic CGU clocks implements a mux with a set_parent=20 >> hook, >> > > but >> > > > doesn't provide a determine_rate implementation. >> > > > >> > > > This is a bit odd, since set_parent() is there to, as its=20 >> name >> > > implies, >> > > > change the parent of a clock. However, the most likely=20 >> candidate >> > > to >> > > > trigger that parent change is a call to clk_set_rate(), with >> > > > determine_rate() figuring out which parent is the best=20 >> suited for >> > > a >> > > > given rate. >> > > > >> > > > The other trigger would be a call to clk_set_parent(), but=20 >> it's >> > > far less >> > > > used, and it doesn't look like there's any obvious user for=20 >> that >> > > clock. >> > > > >> > > > So, the set_parent hook is effectively unused, possibly=20 >> because >> > > of an >> > > > oversight. However, it could also be an explicit decision by=20 >> the >> > > > original author to avoid any reparenting but through an=20 >> explicit >> > > call to >> > > > clk_set_parent(). >> > > > >> > > > The driver does implement round_rate() though, which means=20 >> that >> > > we can >> > > > change the rate of the clock, but we will never get to=20 >> change the >> > > > parent. >> > > > >> > > > However, It's hard to tell whether it's been done on purpose=20 >> or >> > > not. >> > > > >> > > > Since we'll start mandating a determine_rate()=20 >> implementation, >> > > let's >> > > > convert the round_rate() implementation to a=20 >> determine_rate(), >> > > which >> > > > will also make the current behavior explicit. And if it was=20 >> an >> > > > oversight, the clock behaviour can be adjusted later on. >> > > >> > > So it's partly on purpose, partly because I didn't know about >> > > .determine_rate. >> > > >> > > There's nothing odd about having a lonely .set_parent=20 >> callback; in >> > > my case >> > > the clocks are parented from the device tree. >> > > >> > > Having the clocks driver trigger a parent change when=20 >> requesting a >> > > rate >> > > change sounds very dangerous, IMHO. My MMC controller can be >> > > parented to the >> > > external 48 MHz oscillator, and if the card requests 50 MHz, it >> > > could switch >> > > to one of the PLLs. That works as long as the PLLs don't change >> > > rate, but if >> > > one is configured as driving the CPU clock, it becomes messy. >> > > The thing is, the clocks driver has no way to know whether or=20 >> not >> > > it is >> > > "safe" to use a designated parent. >> > > >> > > For that reason, in practice, I never actually want to have a=20 >> clock >> > > re-parented - it's almost always a bad idea vs. sticking to the >> > > parent clock >> > > configured in the DTS. >> > >> > Yeah, and this is totally fine. But we need to be explicit about=20 >> it. The >> > determine_rate implementation I did in all the patches is an exact >> > equivalent to the round_rate one if there was one. We will never=20 >> ask to >> > change the parent. >> > >> > Given what you just said, I would suggest to set the >> > CLK_SET_RATE_NO_REPARENT flag as well. >>=20 >> But that would introduce policy into the driver... >=20 > I'm not sure why you're bringing policies into that discussion.=20 > There's > plenty of policy in the driver already, and the current code doesn't=20 > do > something that the old wasn't doing (implicitly). Yes, I was just talking about the CLK_SET_RATE_NO_REPARENT flag adding=20 policy. The fact that there's plenty of policy in the driver already is=20 not an argument for adding some more. > And there's plenty of policies in drivers in general. Whether you=20 > limit > the rate or not, whether you allow reparenting or not, even the > CLK_SET_RATE_NO_REPARENT flag mentioned above is a policy decision set > by drivers. Allowing reparenting and not limiting the rates is not a policy, it's=20 just following what the hardware allows you to do. The absence of=20 policy means that the driver allows you to configure the hardware in=20 any way you might want to. Limiting rates, forbidding reparenting, that's policy, and it doesn't=20 belong in a driver. You can argue that choosing not to reparent on rate change is a policy,=20 and it is. That's why we need a way to enforce these policies outside=20 the driver. >> The fact that I don't want the MMC parented to the PLLs, doesn't=20 >> mean >> that it's an invalid configuration per se. >=20 > Sure, and that's another policy :) A policy that is not enforced by the driver. Going back to the patch itself... I am fine with the change, although=20 the patch description should probably be updated. We have .set_parent=20 callbacks to configure clocks from DT, there's nothing more to it. Cheers, -Paul