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. 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? 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. 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. 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
Attachment:
signature.asc
Description: PGP signature
