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

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

[Frank Oltmanns]

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,
-- 
Frank Oltmanns <frank@oltmanns.dev>

[PATCH 1/3] clk: keep clock rate when parent rate changes

[Frank Oltmanns]

Allow clocks to keep their rate when parent (or grandparent) rate
changes.

Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
 drivers/clk/clk.c            | 48 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/clk-provider.h |  2 ++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c249f9791ae8..a382876c18da 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2245,6 +2245,9 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	    best_parent_rate != parent->rate)
 		top = clk_calc_new_rates(parent, best_parent_rate);
 
+	if ((core->flags & CLK_KEEP_RATE))
+		core->req_rate = new_rate;
+
 out:
 	clk_calc_subtree(core, new_rate, parent, p_index);
 
@@ -2343,9 +2346,51 @@ static void clk_change_rate(struct clk_core *core)
 		clk_core_prepare_enable(parent);
 
 	trace_clk_set_rate(core, core->new_rate);
+	if (!skip_set_rate && core->ops->set_rate) {
+		if (core->flags & CLK_KEEP_RATE && clk_core_can_round(core)) {
+			struct clk_rate_request req;
+			unsigned long flags;
+			int ret;
+
+			clk_core_init_rate_req(core, &req, core->req_rate);
 
-	if (!skip_set_rate && core->ops->set_rate)
+			/*
+			 * Re-determine the new rate for the clock based on the requested rate.
+			 *
+			 * In this stage, the clock must not set a new parent rate or try a
+			 * different parent, so temporarily prevent that from happening.
+			 */
+			flags = core->flags;
+			core->flags &= ~(CLK_SET_RATE_PARENT);
+			core->flags |= CLK_SET_RATE_NO_REPARENT;
+			ret = clk_core_determine_round_nolock(core, &req);
+			core->flags = flags;
+
+			/*
+			 * If necessary, store the new rate and propagate to the subtree.
+			 *
+			 * The previously calculated rates (new_rate) of this core's subtree are no
+			 * longer correct, because this clock will be set to a rate that differs
+			 * from the rate that was used to calculate the subtree.
+			 *
+			 * FIXME: This means that the rate also differs from the new_rate that was
+			 *        announced in the PRE_RATE_CHANGE notification. Be careful when
+			 *        applying this flag, that the subtree does not depend on the
+			 *        correct new rate being propagated.
+			 */
+			if (ret >= 0 && req.rate != core->new_rate) {
+				core->new_rate = req.rate;
+				pr_debug("%s: clk %s: keeping rate %lu as %lu\n",
+				       __func__, core->name, core->req_rate, core->new_rate);
+
+				hlist_for_each_entry(child, &core->children, child_node) {
+					child->new_rate = clk_recalc(child, core->new_rate);
+					clk_calc_subtree(child, child->new_rate, NULL, 0);
+				}
+			}
+		}
 		core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);
+	}
 
 	trace_clk_set_rate_complete(core, core->new_rate);
 
@@ -3388,6 +3433,7 @@ static const struct {
 	ENTRY(CLK_IS_CRITICAL),
 	ENTRY(CLK_OPS_PARENT_ENABLE),
 	ENTRY(CLK_DUTY_CYCLE_PARENT),
+	ENTRY(CLK_KEEP_RATE),
 #undef ENTRY
 };
 
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index ec32ec58c59f..fba78a99ac56 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -32,6 +32,8 @@
 #define CLK_OPS_PARENT_ENABLE	BIT(12)
 /* duty cycle call may be forwarded to the parent clock */
 #define CLK_DUTY_CYCLE_PARENT	BIT(13)
+/* try to keep rate, if parent rate changes */
+#define CLK_KEEP_RATE		BIT(14)
 
 struct clk;
 struct clk_hw;

-- 
2.41.0

[PATCH 2/3] clk: sunxi-ng: a64: keep rate of pll-mipi stable across parent rate changes

[Frank Oltmanns]

Keep the clock rate of Allwinner A64's pll-mipi even when the parent
(pll-video0) changes its rate. This is required, to drive both an LCD
and HDMI display, because both have pll-video0 as an ancestor.

Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 8951ffc14ff5..d22094ce1d66 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -180,7 +180,8 @@ static struct ccu_nkm pll_mipi_clk = {
 		.reg		= 0x040,
 		.hw.init	= CLK_HW_INIT("pll-mipi", "pll-video0",
 					      &ccu_nkm_ops,
-					      CLK_SET_RATE_UNGATE | CLK_SET_RATE_PARENT),
+					      CLK_SET_RATE_UNGATE | CLK_SET_RATE_PARENT |
+					      CLK_KEEP_RATE),
 		.features	= CCU_FEATURE_CLOSEST_RATE,
 	},
 };

-- 
2.41.0

[PATCH 3/3] drm/sun4i: tcon: parent keeps TCON0 clock stable on A64

[Frank Oltmanns]

From: Icenowy Zheng <icenowy@aosc.io>

As the clk framework keeps A64's TCON0 clock stable when HDMI changes
its parent's clock, do not protect TCON0 clock on A64 in the TCON driver
to allow PLL-Video0 to get changed by HDMI.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 15 +++++++++++++--
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 6a52fb12cbfb..4439e62b7a34 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -108,9 +108,11 @@ static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
 
 	if (enabled) {
 		clk_prepare_enable(clk);
-		clk_rate_exclusive_get(clk);
+		if (!tcon->quirks->rate_kept_by_parent)
+			clk_rate_exclusive_get(clk);
 	} else {
-		clk_rate_exclusive_put(clk);
+		if (!tcon->quirks->rate_kept_by_parent)
+			clk_rate_exclusive_put(clk);
 		clk_disable_unprepare(clk);
 	}
 }
@@ -1505,6 +1507,14 @@ static const struct sun4i_tcon_quirks sun8i_a33_quirks = {
 	.supports_lvds		= true,
 };
 
+static const struct sun4i_tcon_quirks sun50i_a64_lcd_quirks = {
+	.supports_lvds		= true,
+	.has_channel_0		= true,
+	.rate_kept_by_parent	= true,
+	.dclk_min_div		= 1,
+	.setup_lvds_phy		= sun6i_tcon_setup_lvds_phy,
+};
+
 static const struct sun4i_tcon_quirks sun8i_a83t_lcd_quirks = {
 	.supports_lvds		= true,
 	.has_channel_0		= true,
@@ -1563,6 +1573,7 @@ const struct of_device_id sun4i_tcon_of_table[] = {
 	{ .compatible = "allwinner,sun9i-a80-tcon-tv", .data = &sun9i_a80_tcon_tv_quirks },
 	{ .compatible = "allwinner,sun20i-d1-tcon-lcd", .data = &sun20i_d1_lcd_quirks },
 	{ .compatible = "allwinner,sun20i-d1-tcon-tv", .data = &sun8i_r40_tv_quirks },
+	{ .compatible = "allwinner,sun50i-a64-tcon-lcd", .data = &sun50i_a64_lcd_quirks },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sun4i_tcon_of_table);
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index fa23aa23fe4a..c4ce7c29192e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -243,6 +243,7 @@ struct sun4i_tcon_quirks {
 	bool    needs_edp_reset; /* a80 edp reset needed for tcon0 access */
 	bool	supports_lvds;   /* Does the TCON support an LVDS output? */
 	bool	polarity_in_ch0; /* some tcon1 channels have polarity bits in tcon0 pol register */
+	bool	rate_kept_by_parent; /* Does parent keep TCON0 clock stable? */
 	u8	dclk_min_div;	/* minimum divider for TCON0 DCLK */
 
 	/* callback to handle tcon muxing options */

-- 
2.41.0

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Frank Oltmanns]

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

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

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

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

[Maxime Ripard]

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

Not really. Both of those clocks can change (or not) at any point in
time. What triggers the rate set is a modeset which might never happen
(if the display driver or output is disabled, if the fbdev emulation is
disabled or if there's never a compositor starting) or possibly happen
each frame on both output for all you know.

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

I don't think any piece of code will be able to, really.

Your definition of CLK_KEEP_RATE is that it will "try to keep rate, if
parent rate changes"

What happens if it fails, possibly because of rounding like you
mentioned already?

Fundamentally, the problem is that you need different rates on two
subtrees, and we set both to have CLK_SET_RATE_PARENT and allow both to
change the parent rate if needed.

What would happen if we force pll-video0 to a known, fixed, value and
remove CLK_SET_RATE_PARENT from both the pll-mipi and hdmi clocks?

Maxime

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: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. 🤔 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
>
> Not really. Both of those clocks can change (or not) at any point in
> time. What triggers the rate set is a modeset which might never happen
> (if the display driver or output is disabled, if the fbdev emulation is
> disabled or if there's never a compositor starting) or possibly happen
> each frame on both output for all you know.

Ok, thank you for the explanation and I apologize for not having the
terminology straight. This would mean that in my description above there
are two modesets that trigger the rate set
 1. for the tcon-data-clock on boot when the internal display is
    activated and
 2. for hdmi when the external monitor is activated.

For reference: In my scenario I'm using a pinephone which has an
internal LCD display and an HDMI connector (not supported in mainline).

I understand that there could be more. Let's put a pin in that, because
my understandig is, that this is not the relevant part here.

>> 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.
>
> I don't think any piece of code will be able to, really.

In the first case, setting the pll-mipi clock starts from the bottom
(tcon-data-clock) and uses clk_calc_new_rates() to get to the top-most
clock that needs and can be changed. On it's way up to pll-video0 it
passes pll-mipi. My proposal (PATCH 1) is to use that moment to store
the rate in req_rate.

In contrast, in the second case, setting the pll-mipi clock starts from
the top. pll-video0's rate is changed and therefore a new rate is
propagate for the whole subtree where, on its way down, it passes
pll-mipi. My proposal (PATCH 1) is to use that moment to restore the
rate from req_rate that was previously (see pragraph above) set.

Since I did not see a way to achieve this using notifiers (and you seem
to agree), I chose to propose a different path.

> Your definition of CLK_KEEP_RATE is that it will "try to keep rate, if
> parent rate changes"
>
> What happens if it fails, possibly because of rounding like you
> mentioned already?

Maybe "keep" is too strong of a word. I'm sorry for the confusion my
poor choice of wording has caused.

What I would like if for a clock to go back as closely as possible to
the previous rate. And this is what PATCH 1 does by using the clocks
determine_rate (or round_rate) operation.

> Fundamentally, the problem is that you need different rates on two
> subtrees, and we set both to have CLK_SET_RATE_PARENT and allow both to
> change the parent rate if needed.

This reads to me as if you are emphasizing the word "both" here. I'm
aware that you know, but I state it here for the benefit of others: Up
until kernel 6.5 only hdmi resets pll-video0. pll-mipi does not set
pll-video0. This has changed in clk-next. Now also pll-mipi sets the
parent rate.

Icenowy's patches are aimed at (and work for) up to kernel 6.5. They
restore pll-mipi's rate after pll-video0 has been changed by hdmi.

> What would happen if we force pll-video0 to a known, fixed, value and
> remove CLK_SET_RATE_PARENT from both the pll-mipi and hdmi clocks?

Approximately three months ago, I wrote "one could argue that pll-video0
should be set to 297MHz at boot", to which Jernej responded: "Ideally,
clock rate setting code should be immune on "initial" values, set by
bootloader or default values. If it's not, then it should be improved in
the way that it is.":
https://lore.kernel.org/linux-clk/4831731.31r3eYUQgx@jernej-laptop/#t

That's what got me startet on the whole process of allowing pll-mipi to
set it's parent instead of simply setting it to a known rate of 297 MHz.

Should I resume the other (297MHz) investigation?

I had another idea, but don't know how/if that's possible: Maybe we
could use a notifier to notify pll-mipi (or even better:
tcon-data-clock) and use some mechanism to defer calling clk_set_rate()
to a point in time when the whole process of setting the clocks is
complete.

Best regards,
  Frank


>
> Maxime

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

[Maxime Ripard]

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

Why would the clock framework documentation mention an issue that only
arises with a single clock on a single SoC?

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.

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

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

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

Maxime

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