On 2023-08-28 at 10:25:01 +0200, Maxime Ripard <mripard@xxxxxxxxxx> wrote: > On Sat, Aug 26, 2023 at 11:12:16AM +0200, Frank Oltmanns wrote: >> >> On 2023-08-25 at 17:07:58 +0200, Frank Oltmanns <frank@xxxxxxxxxxxx> wrote: >> > Thank you for your feedback, Maxime! >> > >> > On 2023-08-25 at 10:13:53 +0200, Maxime Ripard <mripard@xxxxxxxxxx> 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-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-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. 🤔 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. 🤷 >> > >> >> 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 function. >> >> >> >> 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 > > Not really. Both of those clocks can change (or not) at any point in > time. What triggers the rate set is a modeset which might never happen > (if the display driver or output is disabled, if the fbdev emulation is > disabled or if there's never a compositor starting) or possibly happen > each frame on both output for all you know. Ok, thank you for the explanation and I apologize for not having the terminology straight. This would mean that in my description above there are two modesets that trigger the rate set 1. for the tcon-data-clock on boot when the internal display is activated and 2. for hdmi when the external monitor is activated. For reference: In my scenario I'm using a pinephone which has an internal LCD display and an HDMI connector (not supported in mainline). I understand that there could be more. Let's put a pin in that, because my understandig is, that this is not the relevant part here. >> 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. > > I don't think any piece of code will be able to, really. In the first case, setting the pll-mipi clock starts from the bottom (tcon-data-clock) and uses clk_calc_new_rates() to get to the top-most clock that needs and can be changed. On it's way up to pll-video0 it passes pll-mipi. My proposal (PATCH 1) is to use that moment to store the rate in req_rate. In contrast, in the second case, setting the pll-mipi clock starts from the top. pll-video0's rate is changed and therefore a new rate is propagate for the whole subtree where, on its way down, it passes pll-mipi. My proposal (PATCH 1) is to use that moment to restore the rate from req_rate that was previously (see pragraph above) set. Since I did not see a way to achieve this using notifiers (and you seem to agree), I chose to propose a different path. > Your definition of CLK_KEEP_RATE is that it will "try to keep rate, if > parent rate changes" > > What happens if it fails, possibly because of rounding like you > mentioned already? Maybe "keep" is too strong of a word. I'm sorry for the confusion my poor choice of wording has caused. What I would like if for a clock to go back as closely as possible to the previous rate. And this is what PATCH 1 does by using the clocks determine_rate (or round_rate) operation. > Fundamentally, the problem is that you need different rates on two > subtrees, and we set both to have CLK_SET_RATE_PARENT and allow both to > change the parent rate if needed. This reads to me as if you are emphasizing the word "both" here. I'm aware that you know, but I state it here for the benefit of others: Up until kernel 6.5 only hdmi resets pll-video0. pll-mipi does not set pll-video0. This has changed in clk-next. Now also pll-mipi sets the parent rate. Icenowy's patches are aimed at (and work for) up to kernel 6.5. They restore pll-mipi's rate after pll-video0 has been changed by hdmi. > What would happen if we force pll-video0 to a known, fixed, value and > remove CLK_SET_RATE_PARENT from both the pll-mipi and hdmi clocks? Approximately three months ago, I wrote "one could argue that pll-video0 should be set to 297MHz at boot", to which Jernej responded: "Ideally, clock rate setting code should be immune on "initial" values, set by bootloader or default values. If it's not, then it should be improved in the way that it is.": https://lore.kernel.org/linux-clk/4831731.31r3eYUQgx@jernej-laptop/#t That's what got me startet on the whole process of allowing pll-mipi to set it's parent instead of simply setting it to a known rate of 297 MHz. Should I resume the other (297MHz) investigation? I had another idea, but don't know how/if that's possible: Maybe we could use a notifier to notify pll-mipi (or even better: tcon-data-clock) and use some mechanism to defer calling clk_set_rate() to a point in time when the whole process of setting the clocks is complete. Best regards, Frank > > Maxime
