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 F2E6BC3DA6F for ; Fri, 25 Aug 2023 15:09:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229690AbjHYPIo (ORCPT ); Fri, 25 Aug 2023 11:08:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51812 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229448AbjHYPIQ (ORCPT ); Fri, 25 Aug 2023 11:08:16 -0400 Received: from mout-p-101.mailbox.org (mout-p-101.mailbox.org [IPv6:2001:67c:2050:0:465::101]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 441912127; Fri, 25 Aug 2023 08:08:13 -0700 (PDT) Received: from smtp202.mailbox.org (smtp202.mailbox.org [IPv6:2001:67c:2050:b231:465::202]) (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-101.mailbox.org (Postfix) with ESMTPS id 4RXNcS5hXvz9sy4; Fri, 25 Aug 2023 17:08:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oltmanns.dev; s=MBO0001; t=1692976088; 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=AAEOctatpX3/10zBsdBfZLy0AhwYTDuyHJC0ob0juM4=; b=IjIDPIFmVJawwxPDgRAAlZPbcizlZu2X0oO98CrY7GZLx1XpLW7KW6fiyGbFlWKGn7z8px LF8Yv+5laH7ak4UBi6248gTuwquMwszkDWa9XJG1NkJ/LtaAKl9Mef8iOsLi42PFCY54hP faju8xYIEdwPmuMm1IFQfz1a3knx/QWXtOScS4DmfAUPRhyrypK/LXCflX+Ed/vnSuT0p5 X6XBeKAXaruVsPIMxm0XyjGUgjuAJF9j+0rwkBdTo48ZmcyH1FgFET4bkJ3LLQYP8K903P V2fUr/uVExTnT01W5FiGDxwPHaR81Xyo5OyQSHKqolrMV8L6F355X+Isvsk0sA== References: <20230825-pll-mipi_keep_rate-v1-0-35bc43570730@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: Date: Fri, 25 Aug 2023 17:07:58 +0200 Message-ID: <87ledzqhwx.fsf@oltmanns.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4RXNcS5hXvz9sy4 Precedence: bulk List-ID: X-Mailing-List: linux-clk@vger.kernel.org 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_mu= x.c#L273 https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/sunxi-ng/ccu_co= mmon.c#L60 but also others: https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/at91/clk-master= .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-89= 96.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_p= arent() 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 functi= on. > > 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). > 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 5A1A5C3DA66 for ; Fri, 25 Aug 2023 15:08:52 +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=SrF02pc+QUO2wLGhA3M/H1cTgNMo8fghVui5yvBMYN8=; b=0qX5Fh60pM5xTvCxsrQLZ8gD0H HM6X5aVHK/7Aat1B5EQAej3DcT/I3CNAD5Zb7u84/5HoT7noFhjGCI4kJI89HMFHHzRxaF0J76clD gt4aWOXsKwo328X3eCIcPO/OnvSqRGtJRRP/trl1TmSV3YpfDkCf7qG2Dimr6c5dqvVdAsfTgVAgu k2a/LEZxwD+Dy8w9/cPGWNCNnK8CdTa8SHbYwp+030brZvnxtBfiLConTkpFsZLMa2ONPQhY4ULQP 0CBcW0lkJa6bqqaCkRh0WIsYYT7GhGgs8nZ9Kag5anMEJadME6k3/MBCJbWkr8Jd1Pk90MGuamWUx jBuhGmhg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qZYQ9-005eKQ-14; Fri, 25 Aug 2023 15:08:21 +0000 Received: from mout-p-101.mailbox.org ([2001:67c:2050:0:465::101]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qZYQ5-005eJ2-29 for linux-arm-kernel@lists.infradead.org; Fri, 25 Aug 2023 15:08:19 +0000 Received: from smtp202.mailbox.org (smtp202.mailbox.org [IPv6:2001:67c:2050:b231:465::202]) (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-101.mailbox.org (Postfix) with ESMTPS id 4RXNcS5hXvz9sy4; Fri, 25 Aug 2023 17:08:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oltmanns.dev; s=MBO0001; t=1692976088; 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=AAEOctatpX3/10zBsdBfZLy0AhwYTDuyHJC0ob0juM4=; b=IjIDPIFmVJawwxPDgRAAlZPbcizlZu2X0oO98CrY7GZLx1XpLW7KW6fiyGbFlWKGn7z8px LF8Yv+5laH7ak4UBi6248gTuwquMwszkDWa9XJG1NkJ/LtaAKl9Mef8iOsLi42PFCY54hP faju8xYIEdwPmuMm1IFQfz1a3knx/QWXtOScS4DmfAUPRhyrypK/LXCflX+Ed/vnSuT0p5 X6XBeKAXaruVsPIMxm0XyjGUgjuAJF9j+0rwkBdTo48ZmcyH1FgFET4bkJ3LLQYP8K903P V2fUr/uVExTnT01W5FiGDxwPHaR81Xyo5OyQSHKqolrMV8L6F355X+Isvsk0sA== References: <20230825-pll-mipi_keep_rate-v1-0-35bc43570730@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: Date: Fri, 25 Aug 2023 17:07:58 +0200 Message-ID: <87ledzqhwx.fsf@oltmanns.dev> MIME-Version: 1.0 X-Rspamd-Queue-Id: 4RXNcS5hXvz9sy4 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230825_080817_863637_220F1EDE X-CRM114-Status: GOOD ( 45.22 ) 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 VGhhbmsgeW91IGZvciB5b3VyIGZlZWRiYWNrLCBNYXhpbWUhCgpPbiAyMDIzLTA4LTI1IGF0IDEw OjEzOjUzICswMjAwLCBNYXhpbWUgUmlwYXJkIDxtcmlwYXJkQGtlcm5lbC5vcmc+IHdyb3RlOgo+ IFtbUEdQIFNpZ25lZCBQYXJ0OlVuZGVjaWRlZF1dCj4gSGksCj4KPiBPbiBGcmksIEF1ZyAyNSwg MjAyMyBhdCAwNzozNjozNkFNICswMjAwLCBGcmFuayBPbHRtYW5ucyB3cm90ZToKPj4gSSB3b3Vs ZCBsaWtlIHRvIG1ha2UgdGhlIEFsbHdpbm5lciBBNjQncyBwbGwtbWlwaSB0byBrZWVwIGl0cyBy YXRlIHdoZW4KPj4gaXRzIHBhcmVudCdzIChwbGwtdmlkZW8wKSByYXRlIGNoYW5nZXMuIEtlZXBp bmcgcGxsLW1pcGkncyByYXRlIGlzCj4+IHJlcXVpcmVkLCB0byBsZXQgdGhlIEE2NCBkcml2ZSBi b3RoIGFuIExDRCBhbmQgSERNSSBkaXNwbGF5IGF0IHRoZSBzYW1lCj4+IHRpbWUsIGJlY2F1c2Ug Ym90aCBoYXZlIHBsbC12aWRlbzAgYXMgYW4gYW5jZXN0b3IuCj4+Cj4+IFBBVENIIDEgYWRkcyB0 aGlzIGZ1bmN0aW9uYWxpdHkgYXMgYSBmZWF0dXJlIGludG8gdGhlIGNsayBmcmFtZXdvcmsgKG5l dwo+PiBmbGFnOiBDTEtfS0VFUF9SQVRFKS4KPj4KPj4gQ29yZXMgdGhhdCB1c2UgdGhpcyBmbGFn LCBzdG9yZSBhIHJhdGUgYXMgcmVxX3JhdGUgd2hlbiBpdCBvciBvbmUgb2YgaXRzCj4+IGRlc2Nl bmRhbnRzIHJlcXVlc3RzIGEgbmV3IHJhdGUuCj4+Cj4+IFRoYXQgcmF0ZSBpcyB0aGVuIHJlc3Rv cmVkIGluIHRoZSBjbGtfY2hhbmdlX3JhdGUgcmVjdXJzaW9uLCB3aGljaCB3YWxrcwo+PiB0aHJv dWdoIHRoZSB0cmVlLiBJdCB3aWxsIHJlYWNoIHRoZSBmbGFnZ2VkIGNvcmUgKGUuZy4gcGxsLW1p cGkpIGFmdGVyCj4+IHRoZSBwYXJlbnQncyByYXRlIChlLmcuIHBsbC12aWRlbzApIGhhcyBhbHJl YWR5IGJlZW4gc2V0IHRvIHRoZSBuZXcKPj4gcmF0ZS4gSXQgd2lsbCB0aGVuIGNhbGwgZGV0ZXJt aW5lX3JhdGUgKHdoaWNoIHJlcXVlc3RzIHRoZSBwYXJlbnQncwo+PiBjdXJyZW50LCBpLmUuIG5l dywgcmF0ZSkgdG8gZGV0ZXJtaW5lIGEgcmF0ZSB0aGF0IGlzIGNsb3NlIHRvIHRoZQo+PiBmbGFn Z2VkIGNvcmUncyBwcmV2aW91cyByYXRlLiBBZnRlcndhcmQgaXQgd2lsbCByZS1jYWxjdWxhdGUg dGhlIHJhdGVzCj4+IGZvciB0aGUgZmxhZ2dlZCBjb3JlJ3Mgc3VidHJlZS4KPgo+IEkgZG9uJ3Qg dGhpbmsgaXQncyB0aGUgcmlnaHQgd2F5IGZvcndhcmQuIEl0IG1ha2VzIHRoZSBjb3JlIGxvZ2lj IG1vcmUKPiBjb21wbGljYXRlZCwgZm9yIHNvbWV0aGluZyB0aGF0IGlzIHJlZHVuZGFudCB3aXRo IHRoZSBub3RpZmllcnMKPiBtZWNoYW5pc20gdGhhdCBoYXMgYmVlbiB0aGUgZ28tdG8gZm9yIHRo YXQga2luZCBvZiB0aGluZ3Mgc28gZmFyLgoKWWVhaCwgdGhhdCB3YXMgbXkgaW5pdGlhbCBpZGVh IGFzIHdlbGwuIEJ1dCBJIGNvdWxkbid0IGdldCBpdCB0byB3b3JrLgpTZWUgZGV0YWlscyBiZWxv dy4KCkRvIHlvdSBoYXZlIGFuIGV4YW1wbGUgb2YgYSBjbG9jayB0aGF0IHJlc3RvcmVzIGl0cyBw cmV2aW91cyByYXRlIGFmdGVyCnRoZSBwYXJlbnQgcmF0ZSBoYXMgY2hhbmdlZD8gSSd2ZSBsb29r ZWQgbGVmdCBhbmQgcmlnaHQsIGJ1dCB0byBtZSBpdApzZWVtcyB0aGF0IG5vdGlmaWVycyBhcmUg bWFpbmx5IHVzZWQgZm9yIHNldHRpbmcgY2xvY2tzIGludG8gc29tZSBraW5kCm9mICJzYWZlIG1v ZGUiIHByaW9yIHRvIHRoZSByYXRlIGNoYW5nZS4gRXhhbXBsZXM6CgpzdW54aS1uZzoKaHR0cHM6 Ly9lbGl4aXIuYm9vdGxpbi5jb20vbGludXgvdjYuNC4xMS9zb3VyY2UvZHJpdmVycy9jbGsvc3Vu eGktbmcvY2N1X211eC5jI0wyNzMKaHR0cHM6Ly9lbGl4aXIuYm9vdGxpbi5jb20vbGludXgvdjYu NC4xMS9zb3VyY2UvZHJpdmVycy9jbGsvc3VueGktbmcvY2N1X2NvbW1vbi5jI0w2MAoKYnV0IGFs c28gb3RoZXJzOgpodHRwczovL2VsaXhpci5ib290bGluLmNvbS9saW51eC92Ni40LjExL3NvdXJj ZS9kcml2ZXJzL2Nsay9hdDkxL2Nsay1tYXN0ZXIuYyNMMjQ4Cmh0dHBzOi8vZWxpeGlyLmJvb3Rs aW4uY29tL2xpbnV4L3Y2LjQuMTEvc291cmNlL2RyaXZlcnMvY2xrL21lc29uL21lc29uOGIuYyNM Mzc1NQpodHRwczovL2VsaXhpci5ib290bGluLmNvbS9saW51eC92Ni40LjExL3NvdXJjZS9kcml2 ZXJzL2Nsay9xY29tL2Nsay1jcHUtODk5Ni5jI0w1NDYKCj4gSXQncyBub3QgcmVhbGx5IG9idmlv dXMgdG8gbWUgd2h5IHRoZSBub3RpZmllcnMgZG9uJ3Qgd29yayB0aGVyZS4KPgo+PiBUaGlzIHdv cmsgaXMgaW5zcGlyZWQgYnkgYW4gb3V0LW9mLXRyZWUgcGF0Y2hzZXQgWzFdIFsyXSBbM10uCj4+ IFVuZm9ydHVuYXRlbHksIHRoZSBwYXRjaHNldCB1c2VzIGNsa19zZXRfcmF0ZSgpIGluIGEgbm90 aWZpZXIgY2FsbGJhY2ssCj4+IHdoaWNoIHRoZSBmb2xsb3dpbmcgY29tbWVudCBvbiBjbGtfbm90 aWZpZXJfcmVnaXN0ZXIoKSBmb3JiaWRzOiAiVGhlCj4+IGNhbGxiYWNrcyBhc3NvY2lhdGVkIHdp dGggdGhlIG5vdGlmaWVyIG11c3Qgbm90IHJlLWVudGVyIGludG8gdGhlIGNsawo+PiBmcmFtZXdv cmsgYnkgY2FsbGluZyBhbnkgdG9wLWxldmVsIGNsayBBUElzLiIgWzRdIEZ1cnRoZXJtb3JlLCB0 aGF0Cj4+IG91dC1vZi10cmVlIHBhdGNoc2V0IG5vIGxvbmdlciB3b3JrcyB3aXRoIHRoZSBjdXJy ZW50IGxpbnV4LW5leHQsCj4+IGJlY2F1c2Ugc2V0dGluZyBwbGwtbWlwaSBpcyBub3cgYWxzbyBy ZXNldHRpbmcgcGxsLXZpZGVvMCBbNV0uCj4KPiBJcyBpdCBiZWNhdXNlIG9mIHRoZSAiVGhlIGNh bGxiYWNrcyBhc3NvY2lhdGVkIHdpdGggdGhlIG5vdGlmaWVyIG11c3QKPiBub3QgcmUtZW50ZXIg aW50byB0aGUgY2xrIGZyYW1ld29yayBieSBjYWxsaW5nIGFueSB0b3AtbGV2ZWwgY2xrIEFQSXMu Igo+IGNvbW1lbnQ/CgpJIGRvbid0IHRoaW5rIHRoYXQncyB0aGUgcmVhc29uLiBJJ20gZmFpcmx5 IGNlcnRhaW4gdGhhdCB0aGUgcHJvYmxlbSBpcywKdGhhdCBwbGwtbWlwaSB0cmllcyB0byBzZXQg dGhlIHBhcmVudCByYXRlLiBNYXliZSBpdCBzaG91bGQgY2hlY2sgaWYgdGhlCnBhcmVudCBpcyBs b2NrZWQsIGJlZm9yZSBkZXRlcm1pbmluZyBhIHJhdGUgdGhhdCByZXF1aXJlcyB0aGUgcGFyZW50 CnJhdGUgdG8gY2hhbmdlLiDwn6SUIEN1cnJlbnRseSwgaXQgb25seSBjYWxscyBjbGtfaHdfY2Fu X3NldF9yYXRlX3BhcmVudCgpCndoaWNoIG9ubHkgY2hlY2tzIHRoZSBmbGFnLCBidXQgZG9lcyBu b3QgY2hlY2sgaWYgaXQgaXMgcmVhbGx5IHBvc3NpYmxlCnRvIGNoYW5nZSB0aGUgcGFyZW50J3Mg cmF0ZS4KClJlZ2FyZGxlc3MsIHBsZWFzZSBkb24ndCBwcmVtYXR1cmVseSBkaXNtaXNzIG15IHBy b3Bvc2FsLiBJdCBoYXMgdGhlCmFkdmFudGFnZSB0aGF0IGl0IGlzIG5vdCBzcGVjaWZpYyBmb3Ig c3VueGktbmcsIGJ1dCBjb3VsZCBiZSB1c2VkIGZvcgpvdGhlciBkcml2ZXJzIGFzIHdlbGwuIE1h eWJlIHRoZXJlIG90aGVyIGluc3RhbmNlcyBvZiBleGNsdXNpdmUgbG9ja3MKdG9kYXkgd2hlcmUg dGhlIENMS19LRUVQX1JBVEUgZmxhZyBtaWdodCB3b3JrIGVxdWFsbHkgd2VsbC4g8J+ktwoKPiBJ ZiBzbywgSSB0aGluayB0aGUgdGhpbmcgd2Ugc2hvdWxkIGVtcGhhc2l6ZSBpcyB0aGF0IGl0J3Mg YWJvdXQgKmFueQo+IHRvcC1sZXZlbCBjbGsgQVBJKiwgYXMgaW4gY2xrX3NldF9yYXRlKCkgb3Ig Y2xrX3NldF9wYXJlbnQoKS4KPgo+IFRoZSBpc3N1ZSBpcyB0aGF0IGFueSBjb25zdW1lci1mYWNp bmcgQVBJIGlzIHRha2luZyB0aGUgY2xrX3ByZXBhcmUgbG9jawo+IGFuZCB0aHVzIHdlIHdvdWxk IGhhdmUgcmVlbnRyYW5jeS4gQnV0IHdlJ3JlIGEgcHJvdmlkZXIgdGhlcmUsIGFuZCBub25lCj4g b2YgdGhlIGNsa19od18qIGZ1bmN0aW9ucyBhcmUgdGFraW5nIHRoYXQgbG9jay4gTmVpdGhlciBk byBvdXIgb3duIGZ1bmN0aW9uLgo+Cj4gU28gd2UgY291bGQgY2FsbCBpbiB0aGF0IG5vdGlmaWVy IG91ciBzZXRfcmF0ZSBjYWxsYmFjayBkaXJlY3RseSwgb3Igd2UKPiBjb3VsZCBjcmVhdGUgYSBj bGtfaHdfc2V0X3JhdGUoKSBmdW5jdGlvbi4KPgo+IFRoZSBmaXJzdCBvbmUgd2lsbCBjcmVhdGUg Y2FjaGUgaXNzdWUgYmV0d2VlbiB0aGUgYWN0dWFsIHJhdGUgdGhhdCB0aGUKPiBjb21tb24gY2xv Y2sgZnJhbWV3b3JrIGlzIHJ1bm5pbmcgYW5kIHRoZSBvbmUgd2UgYWN0dWFsbHkgZW5mb3JjZWQs IGJ1dAo+IHdlIGNvdWxkIGNyZWF0ZSBhIGZ1bmN0aW9uIHRvIGZsdXNoIHRoZSBDQ0YgY2FjaGUu Cj4KPiBUaGUgc2Vjb25kIG9uZSBpcyBwcm9iYWJseSBzaW1wbGVyLgoKSSdtIHByb2JhYmx5IG1p c3Npbmcgc29tZXRoaW5nLCBiZWNhdXNlIEkgZG9uJ3QgdGhpbmsgdGhpcyB3b3VsZCB3b3JrLgpG b3IgcmVmZXJlbmNlLCB0aGlzIGlzIG91ciB0cmVlOgoKICAgIHBsbC12aWRlbzAKICAgICAgIGhk bWktcGh5LWNsawogICAgICAgaGRtaQogICAgICAgdGNvbjEKICAgICAgIHBsbC1taXBpCiAgICAg ICAgICB0Y29uMAogICAgICAgICAgICAgdGNvbi1kYXRhLWNsb2NrCgpXaGVuIHBsbC12aWRlbzAn cyByYXRlIGlzIGNoYW5nZWQgKGUuZy4gYmVjYXVzZSBhIEhETUkgbW9uaXRvciBpcwpwbHVnZ2Vk IGluKSwgdGhlIHJhdGVzIG9mIHRoZSBjb21wbGV0ZSBzdWJ0cmVlIGZvciBwbGwtdmlkZW8wIGFy ZQpyZWNhbGN1bGF0ZWQsIGluY2x1ZGluZyB0Y29uMCBhbmQgdGNvbi1kYXRhLWNsb2NrLiBUaGUg cmF0ZSBvZiB0Y29uMCBpcwpiYXNlZCBvbiB0aGUgcmF0ZSB0aGF0IHdhcyByZWNhbGN1bGF0ZWQg Zm9yIHBsbC1taXBpLCB3aGljaCAtIGluIHR1cm4gLQp3YXMgb2YgY291cnNlIHJlY2FsY3VsYXRl ZCBiYXNlZCBvbiB0aGUgcGxsLXZpZGVvMCdzIG5ldyByYXRlLiBUaGVzZQp2YWx1ZXMgYXJlIHN0 b3JlZCBieSB0aGUgY2xrIGZyYW1ld29yayBpbiBhIHByaXZhdGUgc3RydWN0LiBUaGV5IGFyZQpj YWxjdWxhdGVkIGJlZm9yZSBhY3R1YWxseSBwZXJmb3JtaW5nIGFueSByYXRlIGNoYW5nZXMuCgpT bywgaWYgYSBub3RpZmllciBzZXRzIHBsbC1taXBpJ3MgcmF0ZSB0byBzb21ldGhpbmcgZWxzZSB0 aGFuIHdhcwpwcmV2aW91c2x5IHJlY2FsY3VsYXRlZCwgdGhlIGNsayBmcmFtZXdvcmsgd291bGQg c3RpbGwgdHJ5IHRvIHNldCB0Y29uMAp0byB0aGUgdmFsdWUgdGhhdCBpdCBwcmV2aW91c2x5IGNh bGN1bGF0ZWQuCgpTbywgd2Ugd291bGQgaGF2ZSB0byByZWNhbGN1bGF0ZSBwbGwtbWlwaSdzIHN1 YnRyZWUgYWZ0ZXIgY2hhbmdpbmcgaXRzCnJhdGUgKHRoYXQncyB3aGF0IFBBVENIIDEgaXMgZG9p bmcpLgoKPiBBbm90aGVyIG9wdGlvbiBjb3VsZCBiZSB0aGF0IHdlIHR1cm4gY2xrX3NldF9yYXRl X2V4Y2x1c2l2ZSBpbnRvCj4gc29tZXRoaW5nIG1vcmUgc3VidGxlIHRoYXQgYWxsb3dzIHRvIGNo YW5nZSBhIHBhcmVudCByYXRlIGFzIGxvbmcgYXMgdGhlCj4gY2xvY2sgcmF0ZSBkb2Vzbid0LgoK SSBkb24ndCB0aGluayB0aGlzIHdvdWxkIHdvcmsgZWl0aGVyLiBPbmx5IGluIHJhcmUgY2lyY3Vt c3RhbmNlcwpwbGwtbWlwaSBjYW4gYmUgc2V0IHRvIHRoZSBleGFjdCBwcmV2aW91cyByYXRlLCBu b3JtYWxseSBpdCB3aWxsIGJlIHNldAp0byBhIHJhdGUgdGhhdCBpcyBjbG9zZSB0byBpdCdzIHBy ZXZpb3VzIHJhdGUuCgpOb3RlIHRoZXJlIGlzIGFub3RoZXIgb3B0aW9uLCB3ZSBjb3VsZCBhbmFs eXplOiBwbGwtdmlkZW8wJ3MKUlJFX1JBVEVfQ0hBTkdFIG5vdGlmaWVyIGNvdWxkIGJlIHVzZWQg dG8gc2V0IHBsbC1taXBpIGludG8gYSBtb2RlIHRoYXQKbGV0cyBpdCByZWNhbGN1bGF0ZSBhIHJh dGUgdGhhdCBpcyBjbG9zZSB0byB0aGUgcHJldmlvdXMgcmF0ZS4gQQpQT1NUX1JBVEVfQ0hBTkdF IG5vdGlmaWVyIGNvdWxkIGJlIHVzZWQgdG8gc3dpdGNoIGl0IGJhY2sgdG8gIm5vcm1hbCIKcmVj YWxjIG1vZGUuIEkgZG9uJ3Qga25vdyBpZiBwbGwtdmlkZW8wJ3Mgbm90aWZpZXIgd29ya3Mgb3Ig aWYgd2UgYWxzbwpuZWVkIHRvIGJlIG5vdGlmaWVkIGFmdGVyIHBsbC1taXBpIGhhcyBmaW5pc2hl ZCBzZXR0aW5nIGl0J3MgcmF0ZS4KSG93ZXZlciwgdGhpcyBzZWVtcyBhIGxpdHRsZSBoYWNreSBh bmQgSSBoYXZlbid0IHRyaWVkIGlmIGl0IHdvcmtzIGF0CmFsbC4gSSBwcmVmZXIgdGhlIGN1cnJl bnQgcHJvcG9zYWwgKGkuZS4gdGhlIENMS19LRUVQX1JBVEUgZmxhZykuCgpCZXN0IHJlZ2FyZHMs CiAgRnJhbmsKCj4gSXQgd291bGQgZWFzZSB0aGUgcmVxdWlyZW1lbnQgdGhhdAo+IGNsa19zZXRf cmF0ZV9leGNsdXNpdmUoKSBoYXMgb24gYSBjbG9jayBzdWJ0cmVlICh3aGljaCBJIHRoaW5rIHBy ZXZlbnRzCj4gaXRzIHVzYWdlIHRvIHNvbWUgZXh0ZW50KSwgYnV0IEkgaGF2ZSBubyBpc3N1ZSBv biBob3cgdGhhdCB3b3VsZCB3b3JrIGluCj4gcHJhY3RpY2UuCj4KPiBTbyB5ZWFoLCBJIHRoaW5r IGFkZGluZyBhIGNsa19od19zZXRfcmF0ZSgpIHRoYXQgd291bGQgYmUgY2FsbGFibGUgZnJvbQo+ IGEgbm90aWZpZXIgaXMgdGhlIHJpZ2h0IHdheSBmb3J3YXJkIHRoZXJlLgo+Cj4gTWF4aW1lCj4K PiBbW0VuZCBvZiBQR1AgU2lnbmVkIFBhcnRdXQoKX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX18KbGludXgtYXJtLWtlcm5lbCBtYWlsaW5nIGxpc3QKbGludXgt YXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3Jn L21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtlcm5lbAo= 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 D08E2C3DA6F for ; Fri, 25 Aug 2023 15:08:16 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0007610E6BA; Fri, 25 Aug 2023 15:08:15 +0000 (UTC) Received: from mout-p-101.mailbox.org (mout-p-101.mailbox.org [80.241.56.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id 50F5610E6CC for ; Fri, 25 Aug 2023 15:08:13 +0000 (UTC) Received: from smtp202.mailbox.org (smtp202.mailbox.org [IPv6:2001:67c:2050:b231:465::202]) (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-101.mailbox.org (Postfix) with ESMTPS id 4RXNcS5hXvz9sy4; Fri, 25 Aug 2023 17:08:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oltmanns.dev; s=MBO0001; t=1692976088; 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=AAEOctatpX3/10zBsdBfZLy0AhwYTDuyHJC0ob0juM4=; b=IjIDPIFmVJawwxPDgRAAlZPbcizlZu2X0oO98CrY7GZLx1XpLW7KW6fiyGbFlWKGn7z8px LF8Yv+5laH7ak4UBi6248gTuwquMwszkDWa9XJG1NkJ/LtaAKl9Mef8iOsLi42PFCY54hP faju8xYIEdwPmuMm1IFQfz1a3knx/QWXtOScS4DmfAUPRhyrypK/LXCflX+Ed/vnSuT0p5 X6XBeKAXaruVsPIMxm0XyjGUgjuAJF9j+0rwkBdTo48ZmcyH1FgFET4bkJ3LLQYP8K903P V2fUr/uVExTnT01W5FiGDxwPHaR81Xyo5OyQSHKqolrMV8L6F355X+Isvsk0sA== References: <20230825-pll-mipi_keep_rate-v1-0-35bc43570730@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: Date: Fri, 25 Aug 2023 17:07:58 +0200 Message-ID: <87ledzqhwx.fsf@oltmanns.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4RXNcS5hXvz9sy4 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" 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_mu= x.c#L273 https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/sunxi-ng/ccu_co= mmon.c#L60 but also others: https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/at91/clk-master= .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-89= 96.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_p= arent() 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 functi= on. > > 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). > 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]]