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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 84954C83F18 for ; Sat, 26 Aug 2023 09:13:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229735AbjHZJMr (ORCPT ); Sat, 26 Aug 2023 05:12:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55188 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232197AbjHZJMi (ORCPT ); Sat, 26 Aug 2023 05:12:38 -0400 Received: from mout-p-102.mailbox.org (mout-p-102.mailbox.org [80.241.56.152]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C4D8A1BD2; Sat, 26 Aug 2023 02:12:33 -0700 (PDT) Received: from smtp102.mailbox.org (smtp102.mailbox.org [10.196.197.102]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-102.mailbox.org (Postfix) with ESMTPS id 4RXrgd1zMnz9ssD; Sat, 26 Aug 2023 11:12:29 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oltmanns.dev; s=MBO0001; t=1693041149; h=from:from: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=qDVX91eJSqetV4AI7HaZ27+vMHk/EcMEhN+LW66E61U=; b=N1C7C8Vr1bF2I5w/dCj0qlrUk7A5AsZQgvgEVyRfmhKhjZAfrE/UxA7ojFPk39T7FzkOj5 OoeLrx+KpMeXvTchQ6pjZbnhM3EFQ7l5eewlEoShR0RqnvEJKgXBKn0646aSNL1AsxLUs1 ojwyeDeAyZSpDmfk/gXID1Ye0NXV7Lz8kGF3THxtr6ScWqrS+epvrDXrogcci+XFyJE/r6 1M8Qr5luHg4Q+4HIQIYL6a3ZOtfdapGJ2KJO90hi13t1AFi5x30AWtyOhhj1WbpdpVKs86 usdv3SMuSVSEllmWovO5Ec7N1aUzwGzq0OCO0otbthwshHAdbKGHHEmQwOh6Jw== References: <20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev> <87ledzqhwx.fsf@oltmanns.dev> From: Frank Oltmanns To: Maxime Ripard Cc: Michael Turquette , Stephen Boyd , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , David Airlie , Daniel Vetter , Ondrej Jirman , Icenowy Zheng , linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, dri-devel@lists.freedesktop.org, Icenowy Zheng Subject: Re: [PATCH 0/3] Make Allwinner A64's pll-mipi keep its rate when parent rate changes In-reply-to: <87ledzqhwx.fsf@oltmanns.dev> Date: Sat, 26 Aug 2023 11:12:16 +0200 Message-ID: <878r9yb21b.fsf@oltmanns.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-clk@vger.kernel.org On 2023-08-25 at 17:07:58 +0200, Frank Oltmanns wrote: > Thank you for your feedback, Maxime! > > On 2023-08-25 at 10:13:53 +0200, Maxime Ripard wrote: >> [[PGP Signed Part:Undecided]] >> Hi, >> >> On Fri, Aug 25, 2023 at 07:36:36AM +0200, Frank Oltmanns wrote: >>> I would like to make the Allwinner A64's pll-mipi to keep its rate when >>> its parent's (pll-video0) rate changes. Keeping pll-mipi's rate is >>> required, to let the A64 drive both an LCD and HDMI display at the same >>> time, because both have pll-video0 as an ancestor. >>> >>> PATCH 1 adds this functionality as a feature into the clk framework (new >>> flag: CLK_KEEP_RATE). >>> >>> Cores that use this flag, store a rate as req_rate when it or one of its >>> descendants requests a new rate. >>> >>> That rate is then restored in the clk_change_rate recursion, which walks >>> through the tree. It will reach the flagged core (e.g. pll-mipi) after >>> the parent's rate (e.g. pll-video0) has already been set to the new >>> rate. It will then call determine_rate (which requests the parent's >>> current, i.e. new, rate) to determine a rate that is close to the >>> flagged core's previous rate. Afterward it will re-calculate the rates >>> for the flagged core's subtree. >> >> I don't think it's the right way forward. It makes the core logic more >> complicated, for something that is redundant with the notifiers >> mechanism that has been the go-to for that kind of things so far. > > Yeah, that was my initial idea as well. But I couldn't get it to work. > See details below. > > Do you have an example of a clock that restores its previous rate after > the parent rate has changed? I've looked left and right, but to me it > seems that notifiers are mainly used for setting clocks into some kind > of "safe mode" prior to the rate change. Examples: > > sunxi-ng: > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/sunxi-ng/ccu_= mux.c#L273 > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/sunxi-ng/ccu_= common.c#L60 > > but also others: > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/at91/clk-mast= er.c#L248 > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/meson/meson8b= .c#L3755 > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/qcom/clk-cpu-= 8996.c#L546 > >> It's not really obvious to me why the notifiers don't work there. >> >>> This work is inspired by an out-of-tree patchset [1] [2] [3]. >>> Unfortunately, the patchset uses clk_set_rate() in a notifier callback, >>> which the following comment on clk_notifier_register() forbids: "The >>> callbacks associated with the notifier must not re-enter into the clk >>> framework by calling any top-level clk APIs." [4] Furthermore, that >>> out-of-tree patchset no longer works with the current linux-next, >>> because setting pll-mipi is now also resetting pll-video0 [5]. >> >> Is it because of the "The callbacks associated with the notifier must >> not re-enter into the clk framework by calling any top-level clk APIs." >> comment? > > I don't think that's the reason. I'm fairly certain that the problem is, > that pll-mipi tries to set the parent rate. Maybe it should check if the > parent is locked, before determining a rate that requires the parent > rate to change. =F0=9F=A4=94 Currently, it only calls clk_hw_can_set_rate= _parent() > which only checks the flag, but does not check if it is really possible > to change the parent's rate. > > Regardless, please don't prematurely dismiss my proposal. It has the > advantage that it is not specific for sunxi-ng, but could be used for > other drivers as well. Maybe there other instances of exclusive locks > today where the CLK_KEEP_RATE flag might work equally well. =F0=9F=A4=B7 > >> If so, I think the thing we should emphasize is that it's about *any >> top-level clk API*, as in clk_set_rate() or clk_set_parent(). >> >> The issue is that any consumer-facing API is taking the clk_prepare lock >> and thus we would have reentrancy. But we're a provider there, and none >> of the clk_hw_* functions are taking that lock. Neither do our own funct= ion. >> >> So we could call in that notifier our set_rate callback directly, or we >> could create a clk_hw_set_rate() function. >> >> The first one will create cache issue between the actual rate that the >> common clock framework is running and the one we actually enforced, but >> we could create a function to flush the CCF cache. >> >> The second one is probably simpler. > > I'm probably missing something, because I don't think this would work. > For reference, this is our tree: > > pll-video0 > hdmi-phy-clk > hdmi > tcon1 > pll-mipi > tcon0 > tcon-data-clock > > When pll-video0's rate is changed (e.g. because a HDMI monitor is > plugged in), the rates of the complete subtree for pll-video0 are > recalculated, including tcon0 and tcon-data-clock. The rate of tcon0 is > based on the rate that was recalculated for pll-mipi, which - in turn - > was of course recalculated based on the pll-video0's new rate. These > values are stored by the clk framework in a private struct. They are > calculated before actually performing any rate changes. > > So, if a notifier sets pll-mipi's rate to something else than was > previously recalculated, the clk framework would still try to set tcon0 > to the value that it previously calculated. > > So, we would have to recalculate pll-mipi's subtree after changing its > rate (that's what PATCH 1 is doing). Sorry, I forgot that this actually was possible by flagging pll-mipi with CLK_RECALC_NEW_RATES. But the real problem I was fighting with when trying to use the notifiers is something else. Initially, pll-video0 is set by the bootloader. In my case uboot sets it to 294 MHz. pll-mipi is set to 588 MHz. Afterward, there are actually two types of calls for setting pll-mipi in my scenario: 1. during boot when tcon-data-clock is set to drive the LCD panel 2. when the HDMI cable is plugged in In the first case, the rate for pll-mipi is based on the rate that tcon-data-clock requests. In that case, we do not want to restore the previous rate. In the second case, pll-mipi should try to remain running at the previous rate (the one that was requested by tcon-data-clock). That's the reason for setting core->req_rate in PATCH 1. Unfortunately, the notifier does not provide us with enough context to distinguish the two cases. Best regards, Frank >> Another option could be that we turn clk_set_rate_exclusive into >> something more subtle that allows to change a parent rate as long as the >> clock rate doesn't. > > I don't think this would work either. Only in rare circumstances > pll-mipi can be set to the exact previous rate, normally it will be set > to a rate that is close to it's previous rate. > > Note there is another option, we could analyze: pll-video0's > RRE_RATE_CHANGE notifier could be used to set pll-mipi into a mode that > lets it recalculate a rate that is close to the previous rate. A > POST_RATE_CHANGE notifier could be used to switch it back to "normal" > recalc mode. I don't know if pll-video0's notifier works or if we also > need to be notified after pll-mipi has finished setting it's rate. > However, this seems a little hacky and I haven't tried if it works at > all. I prefer the current proposal (i.e. the CLK_KEEP_RATE flag). > > Best regards, > Frank > >> It would ease the requirement that >> clk_set_rate_exclusive() has on a clock subtree (which I think prevents >> its usage to some extent), but I have no issue on how that would work in >> practice. >> >> So yeah, I think adding a clk_hw_set_rate() that would be callable from >> a notifier is the right way forward there. >> >> Maxime >> >> [[End of PGP Signed Part]] 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 B2CB7C83F11 for ; Sat, 26 Aug 2023 09:13:19 +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-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-ID:Date: In-reply-to:Subject:Cc:To:From:References:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=8IKsvoYX/00vwPIiX6YEFR8Lha//JDkmqYOaXbV4030=; b=jv2MEcup4RGEM6FdLjccs4MvGV hGCqlebhopV1MxVTJ+hg+cLC4dpPj478aAXvzpW5XLvJhAEAd86ZzRJ5davfgog1c0/KOMW2oV+Rt /01huDUQHLzWmwtkcRVl3ah3tQq/0mBmM49s1YMfLHpOAu9ehSzTMMVWi8IzyPxWIoOnC95ZTZ9vo pYeUFTvI9RDAoq8tepY3ZzwbfCth6vyKaj8Wf1kVjodTVtAQpPnJprLWK+i21giTCnJ2BMMU8fwxE +Y5BxNJ5GEFpOgTPJ4f+zJFBlesMo4ldcRjfS1vMIvE2kukIn3VXcaHxuDKejf9ENqLGgsR4Dgteg 8UbnZGcw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qZpLc-006cD2-2D; Sat, 26 Aug 2023 09:12:48 +0000 Received: from mout-p-102.mailbox.org ([80.241.56.152]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qZpLZ-006cBz-03 for linux-arm-kernel@lists.infradead.org; Sat, 26 Aug 2023 09:12:47 +0000 Received: from smtp102.mailbox.org (smtp102.mailbox.org [10.196.197.102]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-102.mailbox.org (Postfix) with ESMTPS id 4RXrgd1zMnz9ssD; Sat, 26 Aug 2023 11:12:29 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oltmanns.dev; s=MBO0001; t=1693041149; h=from:from: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=qDVX91eJSqetV4AI7HaZ27+vMHk/EcMEhN+LW66E61U=; b=N1C7C8Vr1bF2I5w/dCj0qlrUk7A5AsZQgvgEVyRfmhKhjZAfrE/UxA7ojFPk39T7FzkOj5 OoeLrx+KpMeXvTchQ6pjZbnhM3EFQ7l5eewlEoShR0RqnvEJKgXBKn0646aSNL1AsxLUs1 ojwyeDeAyZSpDmfk/gXID1Ye0NXV7Lz8kGF3THxtr6ScWqrS+epvrDXrogcci+XFyJE/r6 1M8Qr5luHg4Q+4HIQIYL6a3ZOtfdapGJ2KJO90hi13t1AFi5x30AWtyOhhj1WbpdpVKs86 usdv3SMuSVSEllmWovO5Ec7N1aUzwGzq0OCO0otbthwshHAdbKGHHEmQwOh6Jw== References: <20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev> <87ledzqhwx.fsf@oltmanns.dev> From: Frank Oltmanns To: Maxime Ripard Cc: Michael Turquette , Stephen Boyd , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , David Airlie , Daniel Vetter , Ondrej Jirman , Icenowy Zheng , linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, dri-devel@lists.freedesktop.org, Icenowy Zheng Subject: Re: [PATCH 0/3] Make Allwinner A64's pll-mipi keep its rate when parent rate changes In-reply-to: <87ledzqhwx.fsf@oltmanns.dev> Date: Sat, 26 Aug 2023 11:12:16 +0200 Message-ID: <878r9yb21b.fsf@oltmanns.dev> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230826_021245_356855_4FE28FF3 X-CRM114-Status: GOOD ( 54.29 ) 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: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Ck9uIDIwMjMtMDgtMjUgYXQgMTc6MDc6NTggKzAyMDAsIEZyYW5rIE9sdG1hbm5zIDxmcmFua0Bv bHRtYW5ucy5kZXY+IHdyb3RlOgo+IFRoYW5rIHlvdSBmb3IgeW91ciBmZWVkYmFjaywgTWF4aW1l IQo+Cj4gT24gMjAyMy0wOC0yNSBhdCAxMDoxMzo1MyArMDIwMCwgTWF4aW1lIFJpcGFyZCA8bXJp cGFyZEBrZXJuZWwub3JnPiB3cm90ZToKPj4gW1tQR1AgU2lnbmVkIFBhcnQ6VW5kZWNpZGVkXV0K Pj4gSGksCj4+Cj4+IE9uIEZyaSwgQXVnIDI1LCAyMDIzIGF0IDA3OjM2OjM2QU0gKzAyMDAsIEZy YW5rIE9sdG1hbm5zIHdyb3RlOgo+Pj4gSSB3b3VsZCBsaWtlIHRvIG1ha2UgdGhlIEFsbHdpbm5l ciBBNjQncyBwbGwtbWlwaSB0byBrZWVwIGl0cyByYXRlIHdoZW4KPj4+IGl0cyBwYXJlbnQncyAo cGxsLXZpZGVvMCkgcmF0ZSBjaGFuZ2VzLiBLZWVwaW5nIHBsbC1taXBpJ3MgcmF0ZSBpcwo+Pj4g cmVxdWlyZWQsIHRvIGxldCB0aGUgQTY0IGRyaXZlIGJvdGggYW4gTENEIGFuZCBIRE1JIGRpc3Bs YXkgYXQgdGhlIHNhbWUKPj4+IHRpbWUsIGJlY2F1c2UgYm90aCBoYXZlIHBsbC12aWRlbzAgYXMg YW4gYW5jZXN0b3IuCj4+Pgo+Pj4gUEFUQ0ggMSBhZGRzIHRoaXMgZnVuY3Rpb25hbGl0eSBhcyBh IGZlYXR1cmUgaW50byB0aGUgY2xrIGZyYW1ld29yayAobmV3Cj4+PiBmbGFnOiBDTEtfS0VFUF9S QVRFKS4KPj4+Cj4+PiBDb3JlcyB0aGF0IHVzZSB0aGlzIGZsYWcsIHN0b3JlIGEgcmF0ZSBhcyBy ZXFfcmF0ZSB3aGVuIGl0IG9yIG9uZSBvZiBpdHMKPj4+IGRlc2NlbmRhbnRzIHJlcXVlc3RzIGEg bmV3IHJhdGUuCj4+Pgo+Pj4gVGhhdCByYXRlIGlzIHRoZW4gcmVzdG9yZWQgaW4gdGhlIGNsa19j aGFuZ2VfcmF0ZSByZWN1cnNpb24sIHdoaWNoIHdhbGtzCj4+PiB0aHJvdWdoIHRoZSB0cmVlLiBJ dCB3aWxsIHJlYWNoIHRoZSBmbGFnZ2VkIGNvcmUgKGUuZy4gcGxsLW1pcGkpIGFmdGVyCj4+PiB0 aGUgcGFyZW50J3MgcmF0ZSAoZS5nLiBwbGwtdmlkZW8wKSBoYXMgYWxyZWFkeSBiZWVuIHNldCB0 byB0aGUgbmV3Cj4+PiByYXRlLiBJdCB3aWxsIHRoZW4gY2FsbCBkZXRlcm1pbmVfcmF0ZSAod2hp Y2ggcmVxdWVzdHMgdGhlIHBhcmVudCdzCj4+PiBjdXJyZW50LCBpLmUuIG5ldywgcmF0ZSkgdG8g ZGV0ZXJtaW5lIGEgcmF0ZSB0aGF0IGlzIGNsb3NlIHRvIHRoZQo+Pj4gZmxhZ2dlZCBjb3JlJ3Mg cHJldmlvdXMgcmF0ZS4gQWZ0ZXJ3YXJkIGl0IHdpbGwgcmUtY2FsY3VsYXRlIHRoZSByYXRlcwo+ Pj4gZm9yIHRoZSBmbGFnZ2VkIGNvcmUncyBzdWJ0cmVlLgo+Pgo+PiBJIGRvbid0IHRoaW5rIGl0 J3MgdGhlIHJpZ2h0IHdheSBmb3J3YXJkLiBJdCBtYWtlcyB0aGUgY29yZSBsb2dpYyBtb3JlCj4+ IGNvbXBsaWNhdGVkLCBmb3Igc29tZXRoaW5nIHRoYXQgaXMgcmVkdW5kYW50IHdpdGggdGhlIG5v dGlmaWVycwo+PiBtZWNoYW5pc20gdGhhdCBoYXMgYmVlbiB0aGUgZ28tdG8gZm9yIHRoYXQga2lu ZCBvZiB0aGluZ3Mgc28gZmFyLgo+Cj4gWWVhaCwgdGhhdCB3YXMgbXkgaW5pdGlhbCBpZGVhIGFz IHdlbGwuIEJ1dCBJIGNvdWxkbid0IGdldCBpdCB0byB3b3JrLgo+IFNlZSBkZXRhaWxzIGJlbG93 Lgo+Cj4gRG8geW91IGhhdmUgYW4gZXhhbXBsZSBvZiBhIGNsb2NrIHRoYXQgcmVzdG9yZXMgaXRz IHByZXZpb3VzIHJhdGUgYWZ0ZXIKPiB0aGUgcGFyZW50IHJhdGUgaGFzIGNoYW5nZWQ/IEkndmUg bG9va2VkIGxlZnQgYW5kIHJpZ2h0LCBidXQgdG8gbWUgaXQKPiBzZWVtcyB0aGF0IG5vdGlmaWVy cyBhcmUgbWFpbmx5IHVzZWQgZm9yIHNldHRpbmcgY2xvY2tzIGludG8gc29tZSBraW5kCj4gb2Yg InNhZmUgbW9kZSIgcHJpb3IgdG8gdGhlIHJhdGUgY2hhbmdlLiBFeGFtcGxlczoKPgo+IHN1bnhp LW5nOgo+IGh0dHBzOi8vZWxpeGlyLmJvb3RsaW4uY29tL2xpbnV4L3Y2LjQuMTEvc291cmNlL2Ry aXZlcnMvY2xrL3N1bnhpLW5nL2NjdV9tdXguYyNMMjczCj4gaHR0cHM6Ly9lbGl4aXIuYm9vdGxp bi5jb20vbGludXgvdjYuNC4xMS9zb3VyY2UvZHJpdmVycy9jbGsvc3VueGktbmcvY2N1X2NvbW1v bi5jI0w2MAo+Cj4gYnV0IGFsc28gb3RoZXJzOgo+IGh0dHBzOi8vZWxpeGlyLmJvb3RsaW4uY29t L2xpbnV4L3Y2LjQuMTEvc291cmNlL2RyaXZlcnMvY2xrL2F0OTEvY2xrLW1hc3Rlci5jI0wyNDgK PiBodHRwczovL2VsaXhpci5ib290bGluLmNvbS9saW51eC92Ni40LjExL3NvdXJjZS9kcml2ZXJz L2Nsay9tZXNvbi9tZXNvbjhiLmMjTDM3NTUKPiBodHRwczovL2VsaXhpci5ib290bGluLmNvbS9s aW51eC92Ni40LjExL3NvdXJjZS9kcml2ZXJzL2Nsay9xY29tL2Nsay1jcHUtODk5Ni5jI0w1NDYK Pgo+PiBJdCdzIG5vdCByZWFsbHkgb2J2aW91cyB0byBtZSB3aHkgdGhlIG5vdGlmaWVycyBkb24n dCB3b3JrIHRoZXJlLgo+Pgo+Pj4gVGhpcyB3b3JrIGlzIGluc3BpcmVkIGJ5IGFuIG91dC1vZi10 cmVlIHBhdGNoc2V0IFsxXSBbMl0gWzNdLgo+Pj4gVW5mb3J0dW5hdGVseSwgdGhlIHBhdGNoc2V0 IHVzZXMgY2xrX3NldF9yYXRlKCkgaW4gYSBub3RpZmllciBjYWxsYmFjaywKPj4+IHdoaWNoIHRo ZSBmb2xsb3dpbmcgY29tbWVudCBvbiBjbGtfbm90aWZpZXJfcmVnaXN0ZXIoKSBmb3JiaWRzOiAi VGhlCj4+PiBjYWxsYmFja3MgYXNzb2NpYXRlZCB3aXRoIHRoZSBub3RpZmllciBtdXN0IG5vdCBy ZS1lbnRlciBpbnRvIHRoZSBjbGsKPj4+IGZyYW1ld29yayBieSBjYWxsaW5nIGFueSB0b3AtbGV2 ZWwgY2xrIEFQSXMuIiBbNF0gRnVydGhlcm1vcmUsIHRoYXQKPj4+IG91dC1vZi10cmVlIHBhdGNo c2V0IG5vIGxvbmdlciB3b3JrcyB3aXRoIHRoZSBjdXJyZW50IGxpbnV4LW5leHQsCj4+PiBiZWNh dXNlIHNldHRpbmcgcGxsLW1pcGkgaXMgbm93IGFsc28gcmVzZXR0aW5nIHBsbC12aWRlbzAgWzVd Lgo+Pgo+PiBJcyBpdCBiZWNhdXNlIG9mIHRoZSAiVGhlIGNhbGxiYWNrcyBhc3NvY2lhdGVkIHdp dGggdGhlIG5vdGlmaWVyIG11c3QKPj4gbm90IHJlLWVudGVyIGludG8gdGhlIGNsayBmcmFtZXdv cmsgYnkgY2FsbGluZyBhbnkgdG9wLWxldmVsIGNsayBBUElzLiIKPj4gY29tbWVudD8KPgo+IEkg ZG9uJ3QgdGhpbmsgdGhhdCdzIHRoZSByZWFzb24uIEknbSBmYWlybHkgY2VydGFpbiB0aGF0IHRo ZSBwcm9ibGVtIGlzLAo+IHRoYXQgcGxsLW1pcGkgdHJpZXMgdG8gc2V0IHRoZSBwYXJlbnQgcmF0 ZS4gTWF5YmUgaXQgc2hvdWxkIGNoZWNrIGlmIHRoZQo+IHBhcmVudCBpcyBsb2NrZWQsIGJlZm9y ZSBkZXRlcm1pbmluZyBhIHJhdGUgdGhhdCByZXF1aXJlcyB0aGUgcGFyZW50Cj4gcmF0ZSB0byBj aGFuZ2UuIPCfpJQgQ3VycmVudGx5LCBpdCBvbmx5IGNhbGxzIGNsa19od19jYW5fc2V0X3JhdGVf cGFyZW50KCkKPiB3aGljaCBvbmx5IGNoZWNrcyB0aGUgZmxhZywgYnV0IGRvZXMgbm90IGNoZWNr IGlmIGl0IGlzIHJlYWxseSBwb3NzaWJsZQo+IHRvIGNoYW5nZSB0aGUgcGFyZW50J3MgcmF0ZS4K Pgo+IFJlZ2FyZGxlc3MsIHBsZWFzZSBkb24ndCBwcmVtYXR1cmVseSBkaXNtaXNzIG15IHByb3Bv c2FsLiBJdCBoYXMgdGhlCj4gYWR2YW50YWdlIHRoYXQgaXQgaXMgbm90IHNwZWNpZmljIGZvciBz dW54aS1uZywgYnV0IGNvdWxkIGJlIHVzZWQgZm9yCj4gb3RoZXIgZHJpdmVycyBhcyB3ZWxsLiBN YXliZSB0aGVyZSBvdGhlciBpbnN0YW5jZXMgb2YgZXhjbHVzaXZlIGxvY2tzCj4gdG9kYXkgd2hl cmUgdGhlIENMS19LRUVQX1JBVEUgZmxhZyBtaWdodCB3b3JrIGVxdWFsbHkgd2VsbC4g8J+ktwo+ Cj4+IElmIHNvLCBJIHRoaW5rIHRoZSB0aGluZyB3ZSBzaG91bGQgZW1waGFzaXplIGlzIHRoYXQg aXQncyBhYm91dCAqYW55Cj4+IHRvcC1sZXZlbCBjbGsgQVBJKiwgYXMgaW4gY2xrX3NldF9yYXRl KCkgb3IgY2xrX3NldF9wYXJlbnQoKS4KPj4KPj4gVGhlIGlzc3VlIGlzIHRoYXQgYW55IGNvbnN1 bWVyLWZhY2luZyBBUEkgaXMgdGFraW5nIHRoZSBjbGtfcHJlcGFyZSBsb2NrCj4+IGFuZCB0aHVz IHdlIHdvdWxkIGhhdmUgcmVlbnRyYW5jeS4gQnV0IHdlJ3JlIGEgcHJvdmlkZXIgdGhlcmUsIGFu ZCBub25lCj4+IG9mIHRoZSBjbGtfaHdfKiBmdW5jdGlvbnMgYXJlIHRha2luZyB0aGF0IGxvY2su IE5laXRoZXIgZG8gb3VyIG93biBmdW5jdGlvbi4KPj4KPj4gU28gd2UgY291bGQgY2FsbCBpbiB0 aGF0IG5vdGlmaWVyIG91ciBzZXRfcmF0ZSBjYWxsYmFjayBkaXJlY3RseSwgb3Igd2UKPj4gY291 bGQgY3JlYXRlIGEgY2xrX2h3X3NldF9yYXRlKCkgZnVuY3Rpb24uCj4+Cj4+IFRoZSBmaXJzdCBv bmUgd2lsbCBjcmVhdGUgY2FjaGUgaXNzdWUgYmV0d2VlbiB0aGUgYWN0dWFsIHJhdGUgdGhhdCB0 aGUKPj4gY29tbW9uIGNsb2NrIGZyYW1ld29yayBpcyBydW5uaW5nIGFuZCB0aGUgb25lIHdlIGFj dHVhbGx5IGVuZm9yY2VkLCBidXQKPj4gd2UgY291bGQgY3JlYXRlIGEgZnVuY3Rpb24gdG8gZmx1 c2ggdGhlIENDRiBjYWNoZS4KPj4KPj4gVGhlIHNlY29uZCBvbmUgaXMgcHJvYmFibHkgc2ltcGxl ci4KPgo+IEknbSBwcm9iYWJseSBtaXNzaW5nIHNvbWV0aGluZywgYmVjYXVzZSBJIGRvbid0IHRo aW5rIHRoaXMgd291bGQgd29yay4KPiBGb3IgcmVmZXJlbmNlLCB0aGlzIGlzIG91ciB0cmVlOgo+ Cj4gICAgIHBsbC12aWRlbzAKPiAgICAgICAgaGRtaS1waHktY2xrCj4gICAgICAgIGhkbWkKPiAg ICAgICAgdGNvbjEKPiAgICAgICAgcGxsLW1pcGkKPiAgICAgICAgICAgdGNvbjAKPiAgICAgICAg ICAgICAgdGNvbi1kYXRhLWNsb2NrCj4KPiBXaGVuIHBsbC12aWRlbzAncyByYXRlIGlzIGNoYW5n ZWQgKGUuZy4gYmVjYXVzZSBhIEhETUkgbW9uaXRvciBpcwo+IHBsdWdnZWQgaW4pLCB0aGUgcmF0 ZXMgb2YgdGhlIGNvbXBsZXRlIHN1YnRyZWUgZm9yIHBsbC12aWRlbzAgYXJlCj4gcmVjYWxjdWxh dGVkLCBpbmNsdWRpbmcgdGNvbjAgYW5kIHRjb24tZGF0YS1jbG9jay4gVGhlIHJhdGUgb2YgdGNv bjAgaXMKPiBiYXNlZCBvbiB0aGUgcmF0ZSB0aGF0IHdhcyByZWNhbGN1bGF0ZWQgZm9yIHBsbC1t aXBpLCB3aGljaCAtIGluIHR1cm4gLQo+IHdhcyBvZiBjb3Vyc2UgcmVjYWxjdWxhdGVkIGJhc2Vk IG9uIHRoZSBwbGwtdmlkZW8wJ3MgbmV3IHJhdGUuIFRoZXNlCj4gdmFsdWVzIGFyZSBzdG9yZWQg YnkgdGhlIGNsayBmcmFtZXdvcmsgaW4gYSBwcml2YXRlIHN0cnVjdC4gVGhleSBhcmUKPiBjYWxj dWxhdGVkIGJlZm9yZSBhY3R1YWxseSBwZXJmb3JtaW5nIGFueSByYXRlIGNoYW5nZXMuCj4KPiBT bywgaWYgYSBub3RpZmllciBzZXRzIHBsbC1taXBpJ3MgcmF0ZSB0byBzb21ldGhpbmcgZWxzZSB0 aGFuIHdhcwo+IHByZXZpb3VzbHkgcmVjYWxjdWxhdGVkLCB0aGUgY2xrIGZyYW1ld29yayB3b3Vs ZCBzdGlsbCB0cnkgdG8gc2V0IHRjb24wCj4gdG8gdGhlIHZhbHVlIHRoYXQgaXQgcHJldmlvdXNs eSBjYWxjdWxhdGVkLgo+Cj4gU28sIHdlIHdvdWxkIGhhdmUgdG8gcmVjYWxjdWxhdGUgcGxsLW1p cGkncyBzdWJ0cmVlIGFmdGVyIGNoYW5naW5nIGl0cwo+IHJhdGUgKHRoYXQncyB3aGF0IFBBVENI IDEgaXMgZG9pbmcpLgoKU29ycnksIEkgZm9yZ290IHRoYXQgdGhpcyBhY3R1YWxseSB3YXMgcG9z c2libGUgYnkgZmxhZ2dpbmcgcGxsLW1pcGkKd2l0aCBDTEtfUkVDQUxDX05FV19SQVRFUy4gQnV0 IHRoZSByZWFsIHByb2JsZW0gSSB3YXMgZmlnaHRpbmcgd2l0aCB3aGVuCnRyeWluZyB0byB1c2Ug dGhlIG5vdGlmaWVycyBpcyBzb21ldGhpbmcgZWxzZS4KCkluaXRpYWxseSwgcGxsLXZpZGVvMCBp cyBzZXQgYnkgdGhlIGJvb3Rsb2FkZXIuIEluIG15IGNhc2UgdWJvb3Qgc2V0cyBpdAp0byAyOTQg TUh6LiBwbGwtbWlwaSBpcyBzZXQgdG8gNTg4IE1Iei4KCkFmdGVyd2FyZCwgdGhlcmUgYXJlIGFj dHVhbGx5IHR3byB0eXBlcyBvZiBjYWxscyBmb3Igc2V0dGluZyBwbGwtbWlwaSBpbgpteSBzY2Vu YXJpbzoKIDEuIGR1cmluZyBib290IHdoZW4gdGNvbi1kYXRhLWNsb2NrIGlzIHNldCB0byBkcml2 ZSB0aGUgTENEIHBhbmVsCiAyLiB3aGVuIHRoZSBIRE1JIGNhYmxlIGlzIHBsdWdnZWQgaW4KCklu IHRoZSBmaXJzdCBjYXNlLCB0aGUgcmF0ZSBmb3IgcGxsLW1pcGkgaXMgYmFzZWQgb24gdGhlIHJh dGUgdGhhdAp0Y29uLWRhdGEtY2xvY2sgcmVxdWVzdHMuIEluIHRoYXQgY2FzZSwgd2UgZG8gbm90 IHdhbnQgdG8gcmVzdG9yZSB0aGUKcHJldmlvdXMgcmF0ZS4KCkluIHRoZSBzZWNvbmQgY2FzZSwg cGxsLW1pcGkgc2hvdWxkIHRyeSB0byByZW1haW4gcnVubmluZyBhdCB0aGUKcHJldmlvdXMgcmF0 ZSAodGhlIG9uZSB0aGF0IHdhcyByZXF1ZXN0ZWQgYnkgdGNvbi1kYXRhLWNsb2NrKS4gVGhhdCdz CnRoZSByZWFzb24gZm9yIHNldHRpbmcgY29yZS0+cmVxX3JhdGUgaW4gUEFUQ0ggMS4KClVuZm9y dHVuYXRlbHksIHRoZSBub3RpZmllciBkb2VzIG5vdCBwcm92aWRlIHVzIHdpdGggZW5vdWdoIGNv bnRleHQgdG8KZGlzdGluZ3Vpc2ggdGhlIHR3byBjYXNlcy4KCkJlc3QgcmVnYXJkcywKICBGcmFu awoKPj4gQW5vdGhlciBvcHRpb24gY291bGQgYmUgdGhhdCB3ZSB0dXJuIGNsa19zZXRfcmF0ZV9l eGNsdXNpdmUgaW50bwo+PiBzb21ldGhpbmcgbW9yZSBzdWJ0bGUgdGhhdCBhbGxvd3MgdG8gY2hh bmdlIGEgcGFyZW50IHJhdGUgYXMgbG9uZyBhcyB0aGUKPj4gY2xvY2sgcmF0ZSBkb2Vzbid0Lgo+ Cj4gSSBkb24ndCB0aGluayB0aGlzIHdvdWxkIHdvcmsgZWl0aGVyLiBPbmx5IGluIHJhcmUgY2ly Y3Vtc3RhbmNlcwo+IHBsbC1taXBpIGNhbiBiZSBzZXQgdG8gdGhlIGV4YWN0IHByZXZpb3VzIHJh dGUsIG5vcm1hbGx5IGl0IHdpbGwgYmUgc2V0Cj4gdG8gYSByYXRlIHRoYXQgaXMgY2xvc2UgdG8g aXQncyBwcmV2aW91cyByYXRlLgo+Cj4gTm90ZSB0aGVyZSBpcyBhbm90aGVyIG9wdGlvbiwgd2Ug Y291bGQgYW5hbHl6ZTogcGxsLXZpZGVvMCdzCj4gUlJFX1JBVEVfQ0hBTkdFIG5vdGlmaWVyIGNv dWxkIGJlIHVzZWQgdG8gc2V0IHBsbC1taXBpIGludG8gYSBtb2RlIHRoYXQKPiBsZXRzIGl0IHJl Y2FsY3VsYXRlIGEgcmF0ZSB0aGF0IGlzIGNsb3NlIHRvIHRoZSBwcmV2aW91cyByYXRlLiBBCj4g UE9TVF9SQVRFX0NIQU5HRSBub3RpZmllciBjb3VsZCBiZSB1c2VkIHRvIHN3aXRjaCBpdCBiYWNr IHRvICJub3JtYWwiCj4gcmVjYWxjIG1vZGUuIEkgZG9uJ3Qga25vdyBpZiBwbGwtdmlkZW8wJ3Mg bm90aWZpZXIgd29ya3Mgb3IgaWYgd2UgYWxzbwo+IG5lZWQgdG8gYmUgbm90aWZpZWQgYWZ0ZXIg cGxsLW1pcGkgaGFzIGZpbmlzaGVkIHNldHRpbmcgaXQncyByYXRlLgo+IEhvd2V2ZXIsIHRoaXMg c2VlbXMgYSBsaXR0bGUgaGFja3kgYW5kIEkgaGF2ZW4ndCB0cmllZCBpZiBpdCB3b3JrcyBhdAo+ IGFsbC4gSSBwcmVmZXIgdGhlIGN1cnJlbnQgcHJvcG9zYWwgKGkuZS4gdGhlIENMS19LRUVQX1JB VEUgZmxhZykuCj4KPiBCZXN0IHJlZ2FyZHMsCj4gICBGcmFuawo+Cj4+IEl0IHdvdWxkIGVhc2Ug dGhlIHJlcXVpcmVtZW50IHRoYXQKPj4gY2xrX3NldF9yYXRlX2V4Y2x1c2l2ZSgpIGhhcyBvbiBh IGNsb2NrIHN1YnRyZWUgKHdoaWNoIEkgdGhpbmsgcHJldmVudHMKPj4gaXRzIHVzYWdlIHRvIHNv bWUgZXh0ZW50KSwgYnV0IEkgaGF2ZSBubyBpc3N1ZSBvbiBob3cgdGhhdCB3b3VsZCB3b3JrIGlu Cj4+IHByYWN0aWNlLgo+Pgo+PiBTbyB5ZWFoLCBJIHRoaW5rIGFkZGluZyBhIGNsa19od19zZXRf cmF0ZSgpIHRoYXQgd291bGQgYmUgY2FsbGFibGUgZnJvbQo+PiBhIG5vdGlmaWVyIGlzIHRoZSBy aWdodCB3YXkgZm9yd2FyZCB0aGVyZS4KPj4KPj4gTWF4aW1lCj4+Cj4+IFtbRW5kIG9mIFBHUCBT aWduZWQgUGFydF1dCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3Rz LmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5m by9saW51eC1hcm0ta2VybmVsCg== 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 6477CC83F11 for ; Sat, 26 Aug 2023 09:12:36 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 465CD10E150; Sat, 26 Aug 2023 09:12:35 +0000 (UTC) Received: from mout-p-102.mailbox.org (mout-p-102.mailbox.org [IPv6:2001:67c:2050:0:465::102]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9FCDE10E150 for ; Sat, 26 Aug 2023 09:12:33 +0000 (UTC) Received: from smtp102.mailbox.org (smtp102.mailbox.org [10.196.197.102]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-102.mailbox.org (Postfix) with ESMTPS id 4RXrgd1zMnz9ssD; Sat, 26 Aug 2023 11:12:29 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oltmanns.dev; s=MBO0001; t=1693041149; h=from:from: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=qDVX91eJSqetV4AI7HaZ27+vMHk/EcMEhN+LW66E61U=; b=N1C7C8Vr1bF2I5w/dCj0qlrUk7A5AsZQgvgEVyRfmhKhjZAfrE/UxA7ojFPk39T7FzkOj5 OoeLrx+KpMeXvTchQ6pjZbnhM3EFQ7l5eewlEoShR0RqnvEJKgXBKn0646aSNL1AsxLUs1 ojwyeDeAyZSpDmfk/gXID1Ye0NXV7Lz8kGF3THxtr6ScWqrS+epvrDXrogcci+XFyJE/r6 1M8Qr5luHg4Q+4HIQIYL6a3ZOtfdapGJ2KJO90hi13t1AFi5x30AWtyOhhj1WbpdpVKs86 usdv3SMuSVSEllmWovO5Ec7N1aUzwGzq0OCO0otbthwshHAdbKGHHEmQwOh6Jw== References: <20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev> <87ledzqhwx.fsf@oltmanns.dev> From: Frank Oltmanns To: Maxime Ripard Subject: Re: [PATCH 0/3] Make Allwinner A64's pll-mipi keep its rate when parent rate changes In-reply-to: <87ledzqhwx.fsf@oltmanns.dev> Date: Sat, 26 Aug 2023 11:12:16 +0200 Message-ID: <878r9yb21b.fsf@oltmanns.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 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: dri-devel@lists.freedesktop.org, Icenowy Zheng , Samuel Holland , Stephen Boyd , Michael Turquette , linux-kernel@vger.kernel.org, Jernej Skrabec , linux-sunxi@lists.linux.dev, Chen-Yu Tsai , Ondrej Jirman , linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Icenowy Zheng Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 2023-08-25 at 17:07:58 +0200, Frank Oltmanns wrote: > Thank you for your feedback, Maxime! > > On 2023-08-25 at 10:13:53 +0200, Maxime Ripard wrote: >> [[PGP Signed Part:Undecided]] >> Hi, >> >> On Fri, Aug 25, 2023 at 07:36:36AM +0200, Frank Oltmanns wrote: >>> I would like to make the Allwinner A64's pll-mipi to keep its rate when >>> its parent's (pll-video0) rate changes. Keeping pll-mipi's rate is >>> required, to let the A64 drive both an LCD and HDMI display at the same >>> time, because both have pll-video0 as an ancestor. >>> >>> PATCH 1 adds this functionality as a feature into the clk framework (new >>> flag: CLK_KEEP_RATE). >>> >>> Cores that use this flag, store a rate as req_rate when it or one of its >>> descendants requests a new rate. >>> >>> That rate is then restored in the clk_change_rate recursion, which walks >>> through the tree. It will reach the flagged core (e.g. pll-mipi) after >>> the parent's rate (e.g. pll-video0) has already been set to the new >>> rate. It will then call determine_rate (which requests the parent's >>> current, i.e. new, rate) to determine a rate that is close to the >>> flagged core's previous rate. Afterward it will re-calculate the rates >>> for the flagged core's subtree. >> >> I don't think it's the right way forward. It makes the core logic more >> complicated, for something that is redundant with the notifiers >> mechanism that has been the go-to for that kind of things so far. > > Yeah, that was my initial idea as well. But I couldn't get it to work. > See details below. > > Do you have an example of a clock that restores its previous rate after > the parent rate has changed? I've looked left and right, but to me it > seems that notifiers are mainly used for setting clocks into some kind > of "safe mode" prior to the rate change. Examples: > > sunxi-ng: > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/sunxi-ng/ccu_= mux.c#L273 > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/sunxi-ng/ccu_= common.c#L60 > > but also others: > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/at91/clk-mast= er.c#L248 > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/meson/meson8b= .c#L3755 > https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/qcom/clk-cpu-= 8996.c#L546 > >> It's not really obvious to me why the notifiers don't work there. >> >>> This work is inspired by an out-of-tree patchset [1] [2] [3]. >>> Unfortunately, the patchset uses clk_set_rate() in a notifier callback, >>> which the following comment on clk_notifier_register() forbids: "The >>> callbacks associated with the notifier must not re-enter into the clk >>> framework by calling any top-level clk APIs." [4] Furthermore, that >>> out-of-tree patchset no longer works with the current linux-next, >>> because setting pll-mipi is now also resetting pll-video0 [5]. >> >> Is it because of the "The callbacks associated with the notifier must >> not re-enter into the clk framework by calling any top-level clk APIs." >> comment? > > I don't think that's the reason. I'm fairly certain that the problem is, > that pll-mipi tries to set the parent rate. Maybe it should check if the > parent is locked, before determining a rate that requires the parent > rate to change. =F0=9F=A4=94 Currently, it only calls clk_hw_can_set_rate= _parent() > which only checks the flag, but does not check if it is really possible > to change the parent's rate. > > Regardless, please don't prematurely dismiss my proposal. It has the > advantage that it is not specific for sunxi-ng, but could be used for > other drivers as well. Maybe there other instances of exclusive locks > today where the CLK_KEEP_RATE flag might work equally well. =F0=9F=A4=B7 > >> If so, I think the thing we should emphasize is that it's about *any >> top-level clk API*, as in clk_set_rate() or clk_set_parent(). >> >> The issue is that any consumer-facing API is taking the clk_prepare lock >> and thus we would have reentrancy. But we're a provider there, and none >> of the clk_hw_* functions are taking that lock. Neither do our own funct= ion. >> >> So we could call in that notifier our set_rate callback directly, or we >> could create a clk_hw_set_rate() function. >> >> The first one will create cache issue between the actual rate that the >> common clock framework is running and the one we actually enforced, but >> we could create a function to flush the CCF cache. >> >> The second one is probably simpler. > > I'm probably missing something, because I don't think this would work. > For reference, this is our tree: > > pll-video0 > hdmi-phy-clk > hdmi > tcon1 > pll-mipi > tcon0 > tcon-data-clock > > When pll-video0's rate is changed (e.g. because a HDMI monitor is > plugged in), the rates of the complete subtree for pll-video0 are > recalculated, including tcon0 and tcon-data-clock. The rate of tcon0 is > based on the rate that was recalculated for pll-mipi, which - in turn - > was of course recalculated based on the pll-video0's new rate. These > values are stored by the clk framework in a private struct. They are > calculated before actually performing any rate changes. > > So, if a notifier sets pll-mipi's rate to something else than was > previously recalculated, the clk framework would still try to set tcon0 > to the value that it previously calculated. > > So, we would have to recalculate pll-mipi's subtree after changing its > rate (that's what PATCH 1 is doing). Sorry, I forgot that this actually was possible by flagging pll-mipi with CLK_RECALC_NEW_RATES. But the real problem I was fighting with when trying to use the notifiers is something else. Initially, pll-video0 is set by the bootloader. In my case uboot sets it to 294 MHz. pll-mipi is set to 588 MHz. Afterward, there are actually two types of calls for setting pll-mipi in my scenario: 1. during boot when tcon-data-clock is set to drive the LCD panel 2. when the HDMI cable is plugged in In the first case, the rate for pll-mipi is based on the rate that tcon-data-clock requests. In that case, we do not want to restore the previous rate. In the second case, pll-mipi should try to remain running at the previous rate (the one that was requested by tcon-data-clock). That's the reason for setting core->req_rate in PATCH 1. Unfortunately, the notifier does not provide us with enough context to distinguish the two cases. Best regards, Frank >> Another option could be that we turn clk_set_rate_exclusive into >> something more subtle that allows to change a parent rate as long as the >> clock rate doesn't. > > I don't think this would work either. Only in rare circumstances > pll-mipi can be set to the exact previous rate, normally it will be set > to a rate that is close to it's previous rate. > > Note there is another option, we could analyze: pll-video0's > RRE_RATE_CHANGE notifier could be used to set pll-mipi into a mode that > lets it recalculate a rate that is close to the previous rate. A > POST_RATE_CHANGE notifier could be used to switch it back to "normal" > recalc mode. I don't know if pll-video0's notifier works or if we also > need to be notified after pll-mipi has finished setting it's rate. > However, this seems a little hacky and I haven't tried if it works at > all. I prefer the current proposal (i.e. the CLK_KEEP_RATE flag). > > Best regards, > Frank > >> It would ease the requirement that >> clk_set_rate_exclusive() has on a clock subtree (which I think prevents >> its usage to some extent), but I have no issue on how that would work in >> practice. >> >> So yeah, I think adding a clk_hw_set_rate() that would be callable from >> a notifier is the right way forward there. >> >> Maxime >> >> [[End of PGP Signed Part]]