Re: [PATCH 0/3] Make Allwinner A64's pll-mipi keep its rate when parent rate changes

[Frank Oltmanns]

On 2023-08-28 at 10:04:51 +0200, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> 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 was hoping that the discussion here would give me some clues (and it
does). You have already explained, that the issue is the locks. I'm
still confused why Icenowy's patches work. They use clk_set_rate() in a
notifier callback and despite that they work (up until kernel 6.5). The
only thing that has changed here (that I'm aware of), is that pll-mipi
now sets the parent rate in clk-next.

>> 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?

No, sorry, that's not what I said or meant. I was wondering if
ccu_nkm_determine_rate should check if the parent rate has no exclusive
lock, before assuming it can change the parent rate, so that Icenowy's
patches still work.

> 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.

Yes, it does. And that's why I thought that calling clk_set_rate() in
the notifier callback was never the right choice.

> 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).

I'm not trying to "fix" anything in the framework in the sense that the
framework has a bug. I propose to add a new feature to the framework, so
that I can extend pll-mipi, so that the A64 can drive both an LCD and
HDMI at the same time.

>> 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?

As I wrote in my other mail. The word "keep" was apparently a bad
choice. Maybe CLK_RESTORE_RATE_CLOSELY? The difference is that an
exclusive lock prevents other clocks from changing the parent, whereas
"CLK_KEEP_RATE" (sic) allows those changes to happen and deal with it by
restoring the previous rate as closely as possible after the parent rate
changed.

>> > 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.

See my other mail. I mis-remembered. That is already available via the
CLK_RECALC_NEW_RATES flag.

Best regards,
  Frank

>
> Maxime