mbox series

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

Message ID 20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev (mailing list archive)
Headers show
Series Make Allwinner A64's pll-mipi keep its rate when parent rate changes | expand

Message

Frank Oltmanns Aug. 25, 2023, 5:36 a.m. UTC
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.

PATCH 2 & 3 demonstrate how the new flag can be used for A64's pll-mipi.
By setting this flag, it is no longer required to get an exclusive lock
when setting tcon0's rate, because the rate will be restored when its
parent's (pll-mipi) rate is restored.

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

Thank you for considering this contribution,
  Frank

[1] https://github.com/megous/linux/commit/4124e115de82797f604808aaa5caad4512a9a1ed
[2] https://github.com/megous/linux/commit/edc93fd70ee759fd989664fcb85996cb48a006e6
[3] https://github.com/megous/linux/commit/40f5fc5b08b21142931662147d039ec217c9ba2f
[4] https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/clk.c#L4578
[5] https://lore.kernel.org/linux-kernel/20230807-pll-mipi_set_rate_parent-v6-0-f173239a4b59@oltmanns.dev/

Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
Frank Oltmanns (2):
      clk: keep clock rate when parent rate changes
      clk: sunxi-ng: a64: keep rate of pll-mipi stable across parent rate changes

Icenowy Zheng (1):
      drm/sun4i: tcon: parent keeps TCON0 clock stable on A64

 drivers/clk/clk.c                     | 48 ++++++++++++++++++++++++++++++++++-
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c |  3 ++-
 drivers/gpu/drm/sun4i/sun4i_tcon.c    | 15 +++++++++--
 drivers/gpu/drm/sun4i/sun4i_tcon.h    |  1 +
 include/linux/clk-provider.h          |  2 ++
 5 files changed, 65 insertions(+), 4 deletions(-)
---
base-commit: c539c5c0a7ccafe7169c02564cceeb50317b540b
change-id: 20230824-pll-mipi_keep_rate-0a3a0d3574cf

Best regards,

Comments

Maxime Ripard Aug. 25, 2023, 8:13 a.m. UTC | #1
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
Frank Oltmanns Aug. 25, 2023, 3:07 p.m. UTC | #2
Thank you for your feedback, Maxime!

On 2023-08-25 at 10:13:53 +0200, Maxime Ripard <mripard@kernel.org> 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. 
Frank Oltmanns Aug. 26, 2023, 9:12 a.m. UTC | #3
On 2023-08-25 at 17:07:58 +0200, Frank Oltmanns <frank@oltmanns.dev> wrote:
> Thank you for your feedback, Maxime!
>
> On 2023-08-25 at 10:13:53 +0200, Maxime Ripard <mripard@kernel.org> 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. 
Maxime Ripard Aug. 28, 2023, 8:04 a.m. UTC | #4
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@kernel.org> 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. 
Maxime Ripard Aug. 28, 2023, 8:25 a.m. UTC | #5
On Sat, Aug 26, 2023 at 11:12:16AM +0200, Frank Oltmanns wrote:
> 
> On 2023-08-25 at 17:07:58 +0200, Frank Oltmanns <frank@oltmanns.dev> wrote:
> > Thank you for your feedback, Maxime!
> >
> > On 2023-08-25 at 10:13:53 +0200, Maxime Ripard <mripard@kernel.org> 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. 
Frank Oltmanns Aug. 28, 2023, 2:14 p.m. UTC | #6
On 2023-08-28 at 10:25:01 +0200, Maxime Ripard <mripard@kernel.org> wrote:
> On Sat, Aug 26, 2023 at 11:12:16AM +0200, Frank Oltmanns wrote:
>>
>> On 2023-08-25 at 17:07:58 +0200, Frank Oltmanns <frank@oltmanns.dev> wrote:
>> > Thank you for your feedback, Maxime!
>> >
>> > On 2023-08-25 at 10:13:53 +0200, Maxime Ripard <mripard@kernel.org> 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. 
Frank Oltmanns Aug. 28, 2023, 2:17 p.m. UTC | #7
On 2023-08-28 at 10:04:51 +0200, Maxime Ripard <mripard@kernel.org> 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@kernel.org> 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.