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 X-Spam-Level: X-Spam-Status: No, score=-8.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 78B88C433E1 for ; Fri, 15 May 2020 08:19:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5630020657 for ; Fri, 15 May 2020 08:19:22 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=cerno.tech header.i=@cerno.tech header.b="GE1JM5qQ"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="B5lgUugU" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727917AbgEOITV (ORCPT ); Fri, 15 May 2020 04:19:21 -0400 Received: from wnew1-smtp.messagingengine.com ([64.147.123.26]:53101 "EHLO wnew1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726716AbgEOITV (ORCPT ); Fri, 15 May 2020 04:19:21 -0400 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailnew.west.internal (Postfix) with ESMTP id 27A99721; Fri, 15 May 2020 04:19:19 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute4.internal (MEProxy); Fri, 15 May 2020 04:19:20 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=djCJW+pef41wbgJqyHTwsU/Rmtd 91KY7CCIGjo6BjIc=; b=GE1JM5qQuwzd9LQciLs02d8zYqgVG3GcgFwGkVvl/wd kwq6PmqKFUN9oU3dZnDuS0g6ThupbiTJLX1Bqbz1KtQLPa3IZUchJaSK6OU5fJp0 bMhd9Yo8tztyMA9su5QT23cuJCRdk2ebNExERuMAq70bJgkAbxC7J6Bg33rkh7/Q 9kanDkWZmpXp+8D4pGyX1/R4pSzJj/aWv51cvChfGQbRLW6vBg6+B7OrR1DOn5bz jcPLwe9rx+OjYuDzloCSh3nC6lqI9KDQ2asT7EL/N1N1kEZNrjIAdjXd8FnycsDO +LCLDu1oj1R6wNnVRS+e/heyWeAgSAogXPU4UzH5D7A== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=djCJW+ pef41wbgJqyHTwsU/Rmtd91KY7CCIGjo6BjIc=; b=B5lgUugURolhl8/0mxXG49 GK0eO9WoYjuXIAIX+63DeBSVpn1o0eym0dGdGmsDupatqfMfX2XMo9qOSeFhgpGi W+52GT2wdS2zexmMErOIDJpxHzyqwkHMEg8Q675jrvf28RBN+EMfsmMqSsRM9wS4 8xyosmY2J140dPEFpE2MlLMcBmUkTBSbUQTUnfle8qHgtfRx+0L4B9DtGbAcNw+i /SuXCLBtLlpAYUbYs2fIA/8tRNa1a8IG+Ko8BubYWA0c83M3q1dFxPe8ccsmezrF 64JsZIrXh8ZuVsO7HRiHbDNA+On1zmPidMVVLIM57jAwj3jujcLwFVWWC2/nyaEg == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrleekgddtudcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomhepofgrgihimhgv ucftihhprghrugcuoehmrgigihhmvgestggvrhhnohdrthgvtghhqeenucggtffrrghtth gvrhhnpeevgeduteektefhtefggfdtkeekgfehhffffeegudelheegheeiueevfeegvdei geenucffohhmrghinhepghhithhhuhgsrdgtohhmnecukfhppeeltddrkeelrdeikedrje einecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgr gihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Received: from localhost (lfbn-tou-1-1502-76.w90-89.abo.wanadoo.fr [90.89.68.76]) by mail.messagingengine.com (Postfix) with ESMTPA id 8EF163280065; Fri, 15 May 2020 04:19:16 -0400 (EDT) Date: Fri, 15 May 2020 10:19:14 +0200 From: Maxime Ripard To: Nicolas Saenz Julienne Cc: Eric Anholt , Tim Gover , Dave Stevenson , Stephen Boyd , Michael Turquette , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-clk@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, linux-rpi-kernel@lists.infradead.org, Phil Elwell , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 20/91] clk: bcm: rpi: Discover the firmware clocks Message-ID: <20200515081914.lejcqqwezn3zwpft@gilmour.lan> References: <1a25b4f079dcdc669d4b29d3658ef0b72be2651e.1587742492.git-series.maxime@cerno.tech> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="m2rbr7aero7m5yan" Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --m2rbr7aero7m5yan Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Nicolas, On Mon, May 04, 2020 at 02:05:47PM +0200, Nicolas Saenz Julienne wrote: > Hi Maxime, as always, thanks for the series! > Some extra context, and comments below. >=20 > On Fri, 2020-04-24 at 17:34 +0200, Maxime Ripard wrote: > > The RaspberryPi4 firmware actually exposes more clocks than are current= ly > > handled by the driver and we will need to change some of them directly > > based on the pixel rate for the display related clocks, or the load for= the > > GPU. > >=20 > > This rate change can have a number of side-effects, including adjusting= the > > various PLL voltages or the PLL parents. The firmware will also update > > those clocks by itself for example if the SoC runs too hot. >=20 > To complete this: >=20 > RPi4's firmware implements DVFS. Clock frequency and SoC voltage are > correlated, if you can determine all clocks are running below a maximum t= hen it > should be safe to lower the voltage. Currently, firmware actively monitor= s arm, > core, h264, v3d, isp and hevc to determine what voltage can be reduced to= , and > if arm increases any of those clocks behind the firmware's back, when it = has a > lowered voltage, a crash is highly likely. >=20 > As pointed out to me by RPi foundation engineers hsm/pixel aren't current= ly > being monitored, as driving a high resolution display also requires a hig= h core > clock, which already sets an acceptable min voltage threshold. But that m= ight > change if needed. >=20 > Ultimately, from the DVFS, the safe way to change clocks from arm would b= e to > always use the firmware interface, which we're far from doing right now. = On the > other hand, AFAIK not all clocks have a firmware counterpart. >=20 > Note that we could also have a word with the RPi foundation and see if > disabling DVFS is possible (AFAIK it's not an option right now). Although= I > personally prefer to support this upstream, aside from the obvious benefi= ts to > battery powered use cases, not consuming power unnecessarily is always big > plus. >=20 > > In order to make Linux play as nice as possible with those constraints,= it > > makes sense to rely on the firmware clocks as much as possible. >=20 > As I comment above, not as simple as it seems. I suggest, for now, we only > register the clocks we're going to use and that are likely to be affected= by > DVFS. hsm being a contender here. >=20 > As we'd be settling on a hybrid solution here, which isn't ideal to say t= he > least, I'd like to gather some opinions on whether pushing towards a fully > firmware based solution is something you'd like to see. Thanks for the summary above, I'll try to adjust that commit log to reflect this. As for my opinion, I don't really think that an hybrid approach is practical, since that would leave us with weird interactions between the firmware (and possibly multiple versinos of it) and the linux driver, and t= his would be pretty hard to maintain in the long run. That leaves us either the MMIO-based driver or the firmware-based one, and = here with what you said above, at the moment, the firmware based one is a clear winner. > > Fortunately,t he firmware has an interface to discover the clocks it > > exposes. >=20 > nit: wrongly placed space >=20 > > Let's use it to discover, register the clocks in the clocks framework a= nd > > then expose them through the device tree for consumers to use them. > > > > Cc: Michael Turquette > > Cc: Stephen Boyd > > Cc: linux-clk@vger.kernel.org > > Signed-off-by: Maxime Ripard > > --- > > drivers/clk/bcm/clk-raspberrypi.c | 104 +++++++++++++++++++--- >=20 > [...] >=20 > > +static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk = *rpi, > > + unsigned int parent, > > + unsigned int id) > > +{ > > + struct raspberrypi_clk_data *data; > > + struct clk_init_data init =3D {}; > > + int ret; > > + > > + if (id =3D=3D RPI_FIRMWARE_ARM_CLK_ID) { > > + struct clk_hw *hw; > > + > > + hw =3D raspberrypi_register_pllb(rpi); > > + if (IS_ERR(hw)) { > > + dev_err(rpi->dev, "Failed to initialize pllb, %ld\n", > > + PTR_ERR(hw)); > > + return hw; > > + } > > + > > + return raspberrypi_register_pllb_arm(rpi); > > + } >=20 > We shouldn't create a special case for pllb. My suggestion here is that we > revert the commit that removed pllb from the mmio driver, in order to mai= ntain > a nice view of the clock tree, and register this as a regular fw-clk. >=20 > The only user to this is RPi's the cpufreq driver, which shouldn't mind t= he > change as long as you maintain the clk lookup creation. Ok, I'll change that. > On that topic, now that the clocks are available in DT we could even move > raspberrypi-cpufreq's registration there. But that's out of the scope of = this > series. >=20 > > + > > + data =3D devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return ERR_PTR(-ENOMEM); > > + data->rpi =3D rpi; > > + data->id =3D id; > > + > > + init.name =3D devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%u", id); >=20 > I'd really like more descriptive names here, sadly the firmware interface > doesn't provide them, but they are set in stone nonetheless. Something li= ke > 'fw-clk-arm' and 'fw-clk-hsm' comes to mind (SCMI does provide naming BTW= ). >=20 > See here for a list of all clocks: > https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface#c= locks That's a good idea, I'll add that too. Thanks! Maxime --m2rbr7aero7m5yan Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCXr5QggAKCRDj7w1vZxhR xT8LAP9pbcW7g3FHYczItDO6C6nd+Zdy+BRd6Tu8sNXBS4ykbwEA5UyzGXR2yeUy +/AuByhHVQhGRrgDpir1aJvVMN4YtAI= =xbLM -----END PGP SIGNATURE----- --m2rbr7aero7m5yan-- 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 X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DCE01C433DF for ; Fri, 15 May 2020 08:19:48 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id A6CE420657 for ; Fri, 15 May 2020 08:19:48 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="O+FilmSx"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=cerno.tech header.i=@cerno.tech header.b="GE1JM5qQ"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="B5lgUugU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A6CE420657 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cerno.tech Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type:Cc: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject: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=NH0A0Nf0zCXVPE3Zx6ei5ZnSQnJZPSUSmGuHHN6l8n8=; b=O+FilmSxU6wpj7swP9qzL632I omHa0NoAVLjAwgCks/En82mpGgR7bQQcJ2EPQNfS6AE4inNKLoqsKvGi8JRZX7uzkiWFgmIuN2dPO OUnsSJfwWidHyatnpKDznM84AGSCLWYv8h+8rzN15FU4Z4c+6nrFUZHj0Nv2ijOHsOx8AEBrfOAzi W7qrqIHm0eJFsYLJHkNWrDdtqNZj7NuazapFKslQUN67T/XIhd3G7ySShqoVmqlrqls1pZcm8aCja njZUfRxX862uYHMud7umDhj7Lu5LLpWTNm1L8mGZ9etLQw7UXeBJs+S8V48mJVg6jcyms5u60ZEaZ beLTSXezg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jZVZK-0006un-7G; Fri, 15 May 2020 08:19:46 +0000 Received: from wnew1-smtp.messagingengine.com ([64.147.123.26]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jZVYw-0006dS-Rn; Fri, 15 May 2020 08:19:28 +0000 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailnew.west.internal (Postfix) with ESMTP id 27A99721; Fri, 15 May 2020 04:19:19 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute4.internal (MEProxy); Fri, 15 May 2020 04:19:20 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=djCJW+pef41wbgJqyHTwsU/Rmtd 91KY7CCIGjo6BjIc=; b=GE1JM5qQuwzd9LQciLs02d8zYqgVG3GcgFwGkVvl/wd kwq6PmqKFUN9oU3dZnDuS0g6ThupbiTJLX1Bqbz1KtQLPa3IZUchJaSK6OU5fJp0 bMhd9Yo8tztyMA9su5QT23cuJCRdk2ebNExERuMAq70bJgkAbxC7J6Bg33rkh7/Q 9kanDkWZmpXp+8D4pGyX1/R4pSzJj/aWv51cvChfGQbRLW6vBg6+B7OrR1DOn5bz jcPLwe9rx+OjYuDzloCSh3nC6lqI9KDQ2asT7EL/N1N1kEZNrjIAdjXd8FnycsDO +LCLDu1oj1R6wNnVRS+e/heyWeAgSAogXPU4UzH5D7A== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=djCJW+ pef41wbgJqyHTwsU/Rmtd91KY7CCIGjo6BjIc=; b=B5lgUugURolhl8/0mxXG49 GK0eO9WoYjuXIAIX+63DeBSVpn1o0eym0dGdGmsDupatqfMfX2XMo9qOSeFhgpGi W+52GT2wdS2zexmMErOIDJpxHzyqwkHMEg8Q675jrvf28RBN+EMfsmMqSsRM9wS4 8xyosmY2J140dPEFpE2MlLMcBmUkTBSbUQTUnfle8qHgtfRx+0L4B9DtGbAcNw+i /SuXCLBtLlpAYUbYs2fIA/8tRNa1a8IG+Ko8BubYWA0c83M3q1dFxPe8ccsmezrF 64JsZIrXh8ZuVsO7HRiHbDNA+On1zmPidMVVLIM57jAwj3jujcLwFVWWC2/nyaEg == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrleekgddtudcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomhepofgrgihimhgv ucftihhprghrugcuoehmrgigihhmvgestggvrhhnohdrthgvtghhqeenucggtffrrghtth gvrhhnpeevgeduteektefhtefggfdtkeekgfehhffffeegudelheegheeiueevfeegvdei geenucffohhmrghinhepghhithhhuhgsrdgtohhmnecukfhppeeltddrkeelrdeikedrje einecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgr gihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Received: from localhost (lfbn-tou-1-1502-76.w90-89.abo.wanadoo.fr [90.89.68.76]) by mail.messagingengine.com (Postfix) with ESMTPA id 8EF163280065; Fri, 15 May 2020 04:19:16 -0400 (EDT) Date: Fri, 15 May 2020 10:19:14 +0200 From: Maxime Ripard To: Nicolas Saenz Julienne Subject: Re: [PATCH v2 20/91] clk: bcm: rpi: Discover the firmware clocks Message-ID: <20200515081914.lejcqqwezn3zwpft@gilmour.lan> References: <1a25b4f079dcdc669d4b29d3658ef0b72be2651e.1587742492.git-series.maxime@cerno.tech> MIME-Version: 1.0 In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200515_011923_198142_7B5D8927 X-CRM114-Status: GOOD ( 31.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Tim Gover , Dave Stevenson , Stephen Boyd , Michael Turquette , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Phil Elwell , Eric Anholt , bcm-kernel-feedback-list@broadcom.com, linux-rpi-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: multipart/mixed; boundary="===============6404264601053167752==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============6404264601053167752== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="m2rbr7aero7m5yan" Content-Disposition: inline --m2rbr7aero7m5yan Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Nicolas, On Mon, May 04, 2020 at 02:05:47PM +0200, Nicolas Saenz Julienne wrote: > Hi Maxime, as always, thanks for the series! > Some extra context, and comments below. >=20 > On Fri, 2020-04-24 at 17:34 +0200, Maxime Ripard wrote: > > The RaspberryPi4 firmware actually exposes more clocks than are current= ly > > handled by the driver and we will need to change some of them directly > > based on the pixel rate for the display related clocks, or the load for= the > > GPU. > >=20 > > This rate change can have a number of side-effects, including adjusting= the > > various PLL voltages or the PLL parents. The firmware will also update > > those clocks by itself for example if the SoC runs too hot. >=20 > To complete this: >=20 > RPi4's firmware implements DVFS. Clock frequency and SoC voltage are > correlated, if you can determine all clocks are running below a maximum t= hen it > should be safe to lower the voltage. Currently, firmware actively monitor= s arm, > core, h264, v3d, isp and hevc to determine what voltage can be reduced to= , and > if arm increases any of those clocks behind the firmware's back, when it = has a > lowered voltage, a crash is highly likely. >=20 > As pointed out to me by RPi foundation engineers hsm/pixel aren't current= ly > being monitored, as driving a high resolution display also requires a hig= h core > clock, which already sets an acceptable min voltage threshold. But that m= ight > change if needed. >=20 > Ultimately, from the DVFS, the safe way to change clocks from arm would b= e to > always use the firmware interface, which we're far from doing right now. = On the > other hand, AFAIK not all clocks have a firmware counterpart. >=20 > Note that we could also have a word with the RPi foundation and see if > disabling DVFS is possible (AFAIK it's not an option right now). Although= I > personally prefer to support this upstream, aside from the obvious benefi= ts to > battery powered use cases, not consuming power unnecessarily is always big > plus. >=20 > > In order to make Linux play as nice as possible with those constraints,= it > > makes sense to rely on the firmware clocks as much as possible. >=20 > As I comment above, not as simple as it seems. I suggest, for now, we only > register the clocks we're going to use and that are likely to be affected= by > DVFS. hsm being a contender here. >=20 > As we'd be settling on a hybrid solution here, which isn't ideal to say t= he > least, I'd like to gather some opinions on whether pushing towards a fully > firmware based solution is something you'd like to see. Thanks for the summary above, I'll try to adjust that commit log to reflect this. As for my opinion, I don't really think that an hybrid approach is practical, since that would leave us with weird interactions between the firmware (and possibly multiple versinos of it) and the linux driver, and t= his would be pretty hard to maintain in the long run. That leaves us either the MMIO-based driver or the firmware-based one, and = here with what you said above, at the moment, the firmware based one is a clear winner. > > Fortunately,t he firmware has an interface to discover the clocks it > > exposes. >=20 > nit: wrongly placed space >=20 > > Let's use it to discover, register the clocks in the clocks framework a= nd > > then expose them through the device tree for consumers to use them. > > > > Cc: Michael Turquette > > Cc: Stephen Boyd > > Cc: linux-clk@vger.kernel.org > > Signed-off-by: Maxime Ripard > > --- > > drivers/clk/bcm/clk-raspberrypi.c | 104 +++++++++++++++++++--- >=20 > [...] >=20 > > +static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk = *rpi, > > + unsigned int parent, > > + unsigned int id) > > +{ > > + struct raspberrypi_clk_data *data; > > + struct clk_init_data init =3D {}; > > + int ret; > > + > > + if (id =3D=3D RPI_FIRMWARE_ARM_CLK_ID) { > > + struct clk_hw *hw; > > + > > + hw =3D raspberrypi_register_pllb(rpi); > > + if (IS_ERR(hw)) { > > + dev_err(rpi->dev, "Failed to initialize pllb, %ld\n", > > + PTR_ERR(hw)); > > + return hw; > > + } > > + > > + return raspberrypi_register_pllb_arm(rpi); > > + } >=20 > We shouldn't create a special case for pllb. My suggestion here is that we > revert the commit that removed pllb from the mmio driver, in order to mai= ntain > a nice view of the clock tree, and register this as a regular fw-clk. >=20 > The only user to this is RPi's the cpufreq driver, which shouldn't mind t= he > change as long as you maintain the clk lookup creation. Ok, I'll change that. > On that topic, now that the clocks are available in DT we could even move > raspberrypi-cpufreq's registration there. But that's out of the scope of = this > series. >=20 > > + > > + data =3D devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return ERR_PTR(-ENOMEM); > > + data->rpi =3D rpi; > > + data->id =3D id; > > + > > + init.name =3D devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%u", id); >=20 > I'd really like more descriptive names here, sadly the firmware interface > doesn't provide them, but they are set in stone nonetheless. Something li= ke > 'fw-clk-arm' and 'fw-clk-hsm' comes to mind (SCMI does provide naming BTW= ). >=20 > See here for a list of all clocks: > https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface#c= locks That's a good idea, I'll add that too. Thanks! Maxime --m2rbr7aero7m5yan Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCXr5QggAKCRDj7w1vZxhR xT8LAP9pbcW7g3FHYczItDO6C6nd+Zdy+BRd6Tu8sNXBS4ykbwEA5UyzGXR2yeUy +/AuByhHVQhGRrgDpir1aJvVMN4YtAI= =xbLM -----END PGP SIGNATURE----- --m2rbr7aero7m5yan-- --===============6404264601053167752== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============6404264601053167752==-- 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 X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6C8ABC433E0 for ; Sat, 16 May 2020 09:59:15 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id CFE29206D4 for ; Sat, 16 May 2020 09:59:14 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=cerno.tech header.i=@cerno.tech header.b="GE1JM5qQ"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="B5lgUugU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CFE29206D4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cerno.tech Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 1D52C6E198; Sat, 16 May 2020 09:59:01 +0000 (UTC) Received: from wnew1-smtp.messagingengine.com (wnew1-smtp.messagingengine.com [64.147.123.26]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6DCCB6E1D7 for ; Fri, 15 May 2020 08:19:22 +0000 (UTC) Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailnew.west.internal (Postfix) with ESMTP id 27A99721; Fri, 15 May 2020 04:19:19 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute4.internal (MEProxy); Fri, 15 May 2020 04:19:20 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=djCJW+pef41wbgJqyHTwsU/Rmtd 91KY7CCIGjo6BjIc=; b=GE1JM5qQuwzd9LQciLs02d8zYqgVG3GcgFwGkVvl/wd kwq6PmqKFUN9oU3dZnDuS0g6ThupbiTJLX1Bqbz1KtQLPa3IZUchJaSK6OU5fJp0 bMhd9Yo8tztyMA9su5QT23cuJCRdk2ebNExERuMAq70bJgkAbxC7J6Bg33rkh7/Q 9kanDkWZmpXp+8D4pGyX1/R4pSzJj/aWv51cvChfGQbRLW6vBg6+B7OrR1DOn5bz jcPLwe9rx+OjYuDzloCSh3nC6lqI9KDQ2asT7EL/N1N1kEZNrjIAdjXd8FnycsDO +LCLDu1oj1R6wNnVRS+e/heyWeAgSAogXPU4UzH5D7A== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=djCJW+ pef41wbgJqyHTwsU/Rmtd91KY7CCIGjo6BjIc=; b=B5lgUugURolhl8/0mxXG49 GK0eO9WoYjuXIAIX+63DeBSVpn1o0eym0dGdGmsDupatqfMfX2XMo9qOSeFhgpGi W+52GT2wdS2zexmMErOIDJpxHzyqwkHMEg8Q675jrvf28RBN+EMfsmMqSsRM9wS4 8xyosmY2J140dPEFpE2MlLMcBmUkTBSbUQTUnfle8qHgtfRx+0L4B9DtGbAcNw+i /SuXCLBtLlpAYUbYs2fIA/8tRNa1a8IG+Ko8BubYWA0c83M3q1dFxPe8ccsmezrF 64JsZIrXh8ZuVsO7HRiHbDNA+On1zmPidMVVLIM57jAwj3jujcLwFVWWC2/nyaEg == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrleekgddtudcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomhepofgrgihimhgv ucftihhprghrugcuoehmrgigihhmvgestggvrhhnohdrthgvtghhqeenucggtffrrghtth gvrhhnpeevgeduteektefhtefggfdtkeekgfehhffffeegudelheegheeiueevfeegvdei geenucffohhmrghinhepghhithhhuhgsrdgtohhmnecukfhppeeltddrkeelrdeikedrje einecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgr gihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Received: from localhost (lfbn-tou-1-1502-76.w90-89.abo.wanadoo.fr [90.89.68.76]) by mail.messagingengine.com (Postfix) with ESMTPA id 8EF163280065; Fri, 15 May 2020 04:19:16 -0400 (EDT) Date: Fri, 15 May 2020 10:19:14 +0200 From: Maxime Ripard To: Nicolas Saenz Julienne Subject: Re: [PATCH v2 20/91] clk: bcm: rpi: Discover the firmware clocks Message-ID: <20200515081914.lejcqqwezn3zwpft@gilmour.lan> References: <1a25b4f079dcdc669d4b29d3658ef0b72be2651e.1587742492.git-series.maxime@cerno.tech> MIME-Version: 1.0 In-Reply-To: X-Mailman-Approved-At: Sat, 16 May 2020 09:58:59 +0000 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: Tim Gover , Dave Stevenson , Stephen Boyd , Michael Turquette , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Phil Elwell , bcm-kernel-feedback-list@broadcom.com, linux-rpi-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: multipart/mixed; boundary="===============0695095956==" Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" --===============0695095956== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="m2rbr7aero7m5yan" Content-Disposition: inline --m2rbr7aero7m5yan Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Nicolas, On Mon, May 04, 2020 at 02:05:47PM +0200, Nicolas Saenz Julienne wrote: > Hi Maxime, as always, thanks for the series! > Some extra context, and comments below. >=20 > On Fri, 2020-04-24 at 17:34 +0200, Maxime Ripard wrote: > > The RaspberryPi4 firmware actually exposes more clocks than are current= ly > > handled by the driver and we will need to change some of them directly > > based on the pixel rate for the display related clocks, or the load for= the > > GPU. > >=20 > > This rate change can have a number of side-effects, including adjusting= the > > various PLL voltages or the PLL parents. The firmware will also update > > those clocks by itself for example if the SoC runs too hot. >=20 > To complete this: >=20 > RPi4's firmware implements DVFS. Clock frequency and SoC voltage are > correlated, if you can determine all clocks are running below a maximum t= hen it > should be safe to lower the voltage. Currently, firmware actively monitor= s arm, > core, h264, v3d, isp and hevc to determine what voltage can be reduced to= , and > if arm increases any of those clocks behind the firmware's back, when it = has a > lowered voltage, a crash is highly likely. >=20 > As pointed out to me by RPi foundation engineers hsm/pixel aren't current= ly > being monitored, as driving a high resolution display also requires a hig= h core > clock, which already sets an acceptable min voltage threshold. But that m= ight > change if needed. >=20 > Ultimately, from the DVFS, the safe way to change clocks from arm would b= e to > always use the firmware interface, which we're far from doing right now. = On the > other hand, AFAIK not all clocks have a firmware counterpart. >=20 > Note that we could also have a word with the RPi foundation and see if > disabling DVFS is possible (AFAIK it's not an option right now). Although= I > personally prefer to support this upstream, aside from the obvious benefi= ts to > battery powered use cases, not consuming power unnecessarily is always big > plus. >=20 > > In order to make Linux play as nice as possible with those constraints,= it > > makes sense to rely on the firmware clocks as much as possible. >=20 > As I comment above, not as simple as it seems. I suggest, for now, we only > register the clocks we're going to use and that are likely to be affected= by > DVFS. hsm being a contender here. >=20 > As we'd be settling on a hybrid solution here, which isn't ideal to say t= he > least, I'd like to gather some opinions on whether pushing towards a fully > firmware based solution is something you'd like to see. Thanks for the summary above, I'll try to adjust that commit log to reflect this. As for my opinion, I don't really think that an hybrid approach is practical, since that would leave us with weird interactions between the firmware (and possibly multiple versinos of it) and the linux driver, and t= his would be pretty hard to maintain in the long run. That leaves us either the MMIO-based driver or the firmware-based one, and = here with what you said above, at the moment, the firmware based one is a clear winner. > > Fortunately,t he firmware has an interface to discover the clocks it > > exposes. >=20 > nit: wrongly placed space >=20 > > Let's use it to discover, register the clocks in the clocks framework a= nd > > then expose them through the device tree for consumers to use them. > > > > Cc: Michael Turquette > > Cc: Stephen Boyd > > Cc: linux-clk@vger.kernel.org > > Signed-off-by: Maxime Ripard > > --- > > drivers/clk/bcm/clk-raspberrypi.c | 104 +++++++++++++++++++--- >=20 > [...] >=20 > > +static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk = *rpi, > > + unsigned int parent, > > + unsigned int id) > > +{ > > + struct raspberrypi_clk_data *data; > > + struct clk_init_data init =3D {}; > > + int ret; > > + > > + if (id =3D=3D RPI_FIRMWARE_ARM_CLK_ID) { > > + struct clk_hw *hw; > > + > > + hw =3D raspberrypi_register_pllb(rpi); > > + if (IS_ERR(hw)) { > > + dev_err(rpi->dev, "Failed to initialize pllb, %ld\n", > > + PTR_ERR(hw)); > > + return hw; > > + } > > + > > + return raspberrypi_register_pllb_arm(rpi); > > + } >=20 > We shouldn't create a special case for pllb. My suggestion here is that we > revert the commit that removed pllb from the mmio driver, in order to mai= ntain > a nice view of the clock tree, and register this as a regular fw-clk. >=20 > The only user to this is RPi's the cpufreq driver, which shouldn't mind t= he > change as long as you maintain the clk lookup creation. Ok, I'll change that. > On that topic, now that the clocks are available in DT we could even move > raspberrypi-cpufreq's registration there. But that's out of the scope of = this > series. >=20 > > + > > + data =3D devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return ERR_PTR(-ENOMEM); > > + data->rpi =3D rpi; > > + data->id =3D id; > > + > > + init.name =3D devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%u", id); >=20 > I'd really like more descriptive names here, sadly the firmware interface > doesn't provide them, but they are set in stone nonetheless. Something li= ke > 'fw-clk-arm' and 'fw-clk-hsm' comes to mind (SCMI does provide naming BTW= ). >=20 > See here for a list of all clocks: > https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface#c= locks That's a good idea, I'll add that too. Thanks! Maxime --m2rbr7aero7m5yan Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCXr5QggAKCRDj7w1vZxhR xT8LAP9pbcW7g3FHYczItDO6C6nd+Zdy+BRd6Tu8sNXBS4ykbwEA5UyzGXR2yeUy +/AuByhHVQhGRrgDpir1aJvVMN4YtAI= =xbLM -----END PGP SIGNATURE----- --m2rbr7aero7m5yan-- --===============0695095956== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel --===============0695095956==--