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

On 2023-08-25 at 17:07:58 +0200, Frank Oltmanns [off-list ref] wrote:

Thank you for your feedback, Maxime!

On 2023-08-25 at 10:13:53 +0200, Maxime Ripard [off-list ref] wrote:

quoted

[[PGP Signed Part:Undecided]]
Hi,

On Fri, Aug 25, 2023 at 07:36:36AM +0200, Frank Oltmanns wrote:

quoted

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

quoted

It's not really obvious to me why the notifiers don't work there.

quoted

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

quoted

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

quoted

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

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel