On Fri, Aug 25, 2023 at 05:07:58PM +0200, Frank Oltmanns 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 There's examples for phases and parents, but not for rates afaics. We shouldn't behave any differently though. > > 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 not sure I follow you there. How can we find a solution to a problem you don't know about or can't know for sure? > 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. 🤔 Why would the clock framework documentation mention an issue that only arises with a single clock on a single SoC? That comment in the clock framework you linked to clearly stated that you can't use a top-level clock function in a notifier, and that's because of the locking. If it's not what you're trying to fix, then I'd really like to know what issue you're trying to fix *in the framework* (so, not on the pll-mipi clock, or the A64). > 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. Just like the two solutions I provided. > Maybe there other instances of exclusive locks today where the > CLK_KEEP_RATE flag might work equally well. 🤷 If exclusive locks work equally well, why would we need CLK_KEEP_RATE? > > 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). Then we should make that function I was telling you about deal with all this. Maxime
