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

[Frank Oltmanns]

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

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.

Best regards,
  Frank

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