Message-ID: <87r0h8n64g.fsf@oltmanns.dev>
Date: Mon, 19 Feb 2024 10:41:19 +0100
From: Frank Oltmanns <frank@...manns.dev>
To: Ondřej Jirman <megi@....cz>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec
 <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, Guido
 Günther <agx@...xcpu.org>, Purism Kernel Team
 <kernel@...i.sm>, Neil
 Armstrong <neil.armstrong@...aro.org>, Jessica Zhang
 <quic_jesszhan@...cinc.com>, Sam Ravnborg <sam@...nborg.org>, Maarten
 Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
 <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David
 Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, Rob Herring
 <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 0/6] Pinephone video out fixes (flipping between two
 frames)

Hi Ondřej,

On 2024-02-11 at 20:25:29 +0100, Ondřej Jirman <megi@....cz> wrote:
> Hi Frank,
>
> On Sun, Feb 11, 2024 at 04:09:16PM +0100, Frank Oltmanns wrote:
>> Hi Ondřej,
>>
>> On 2024-02-05 at 17:02:00 +0100, Ondřej Jirman <megi@....cz> wrote:
>> > On Mon, Feb 05, 2024 at 04:54:07PM +0100, Ondřej Jirman wrote:
>> >> On Mon, Feb 05, 2024 at 04:22:23PM +0100, Frank Oltmanns wrote:
>> >>
>> >> [...]
>> >>
>> >> Also sunxi-ng clk driver does apply NM factors at once to PLL_GPU clock,
>> >> which can cause sudden frequency increase beyond intended output frequency,
>> >> because division is applied immediately while multiplication is reflected
>> >> slowly.
>> >>
>> >> Eg. if you're changing divider from 7 to 1, you can get a sudden 7x output
>> >> frequency spike, before PLL VCO manages to lower the frequency through N clk
>> >> divider feedback loop and lock on again. This can mess up whatever's connected
>> >> to the output quite badly.
>> >>
>> >> You'd have to put logging on kernel writes to PLL_GPU register to see what
>> >> is written in there and if divider is lowered significantly on some GPU
>> >> devfreq frequency transitions.
>>
>> By looking at the clocks in clk_summary in debugfs, the rate of PLL-GPU
>> always matches the rate of the GPU (at least at 120, 312, and 432 MHz).
>> This is further underlined by the fact, that none of the rates can be
>> achieved by integer dividing one of the other rates. sunxi-ng would
>> only favor a different rate for pll-gpu than the one that is requested
>> for the gpu, if pll-gpu is already running at a rate such that there
>> exists an M ∈ {1, 2, 3, 4, 5, 6, 7, 8}, where
>>   rate of pll-gpu / M = requested gpu rate
>> or if the requested rate could not be reached directly by pll-gpu. Both
>> is not the case for the rates in question (120, 192, 312, and 432 MHz).
>>
>> This means that the following divisor/multipliers are used by sunxi-ng's
>> ccu_nm:
>> N =  5, M = 1 for 120 MHz (min value without PATCH 6)
>> N =  8, M = 1 for 192 MHz (min value after applying PATCH 6)
>> N = 13, M = 1 for 312 MHz
>> N = 18, M = 1 for 432 MHz
>>
>> So, with or without PATCH 6, the divider stays constant and it's only
>> the multiplier that changes. This means, there should be no unexpected
>> frequency spikes, right?
>
> Maybe. Thanks for giving it a try. There may still be other kinds of glitches
> even if the divisor stays the same. It all depends how the register update is
> implemented in the PLL block. It's hard to say. I guess, unless Allwinner
> guarantees glitchless output from a given PLL when changing its parameters,
> you can't rely on the output being clean during changes.
>
>> >> It's also unclear what happens when FRAC_CLK_OUT or PLL_MODE_SEL changes.
>>
>> Those are not changed once the clock is initialized. The bug however
>> occurs hours or days after booting. IMO, this makes it unlikely that this
>> could be the culprit.
>>
>> >> Maybe not much because M is supposed to be set to 1, but you still need to
>> >> care when enabling fractional mode, and setting M to 1 because that's exactly
>> >> the bad scenario if M was previously higher than 1.
>> >>
>> >> It's tricky.
>> >>
>> >> Having GPU module clock gated during PLL config changes may help! You can
>> >> do that without locking yourself out, unlike with the CPU PLL.
>> >>
>> >> There's a gate enable bit for it at GPU_CLK_REG.SCLK_GATING. (page 122)
>>
>> The GPU should already be properly gated:
>> https://elixir.bootlin.com/linux/v6.7.4/source/drivers/clk/sunxi-ng/ccu-sun50i-a64.c#L599
>
> How so? That's just clock declaration. How does it guarantee the clock to the
> module is gated during parent PLL configuration changes?
>

You're of course right.

I now tried using a similar approach like the one for changes for on
PLL-CPU. It's using a notifier to connect the CPU to the 24 MHz
oscillator and, after PLL-CPU is at its new rate, connecting it back to
PLL-CPU.

For the GPU my approach was to disable the GPU prior to changing
PLL-GPU's rate and then re-enabling it, once the rate change is
complete. I think, that's what you were proposing, right?

Unfortunately, this results in a frozen phone even more quickly.

Below is my code. Again, it doesn't solve the problem, but maybe
somebody can spot what I'm doing wrong.

Best regards,
  Frank

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index d68bdf7dd342..74538259d67a 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -977,6 +977,11 @@ static struct ccu_rate_reset_nb sun50i_a64_pll_video0_reset_tcon0_nb = {

 #define CCU_MIPI_DSI_CLK 0x168

+static struct ccu_div_nb sun50i_a64_gpu_nb = {
+	.common		= &gpu_clk.common,
+	.delay_us	= 1, /* ??? */
+};
+
 static int sun50i_a64_ccu_probe(struct platform_device *pdev)
 {
 	void __iomem *reg;
@@ -1025,6 +1030,10 @@ static int sun50i_a64_ccu_probe(struct platform_device *pdev)
 	sun50i_a64_pll_video0_reset_tcon0_nb.target_clk = tcon0_clk.common.hw.clk;
 	ccu_rate_reset_notifier_register(&sun50i_a64_pll_video0_reset_tcon0_nb);

+	/* Gate then ungate GPU on PLL-GPU changes */
+	ccu_div_notifier_register(pll_gpu_clk.common.hw.clk,
+				  &sun50i_a64_gpu_nb);
+
 	return 0;
 }

diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
index cb10a3ea23f9..83813c54fb2f 100644
--- a/drivers/clk/sunxi-ng/ccu_div.c
+++ b/drivers/clk/sunxi-ng/ccu_div.c
@@ -4,7 +4,9 @@
  * Maxime Ripard <maxime.ripard@...e-electrons.com>
  */

+#include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/delay.h>
 #include <linux/io.h>

 #include "ccu_gate.h"
@@ -142,3 +144,37 @@ const struct clk_ops ccu_div_ops = {
 	.set_rate	= ccu_div_set_rate,
 };
 EXPORT_SYMBOL_NS_GPL(ccu_div_ops, SUNXI_CCU);
+
+static int ccu_div_notifier_cb(struct notifier_block *nb,
+			       unsigned long event, void *data)
+{
+	struct ccu_div_nb *div_nb = to_ccu_div_nb(nb);
+
+	if (event == PRE_RATE_CHANGE) {
+		div_nb->original_enable = ccu_div_is_enabled(&div_nb->common->hw);
+		if (div_nb->original_enable) {
+			ccu_div_disable(&div_nb->common->hw);
+			udelay(div_nb->delay_us);
+		}
+	} else if (event == POST_RATE_CHANGE) {
+		if (div_nb->original_enable) {
+			ccu_div_enable(&div_nb->common->hw);
+			udelay(div_nb->delay_us);
+		}
+	}
+
+	return NOTIFY_OK;
+}
+
+int ccu_div_notifier_register(struct clk *clk, struct ccu_div_nb *div_nb)
+{
+	div_nb->clk_nb.notifier_call = ccu_div_notifier_cb;
+
+	return clk_notifier_register(clk, &div_nb->clk_nb);
+}
diff --git a/drivers/clk/sunxi-ng/ccu_div.h b/drivers/clk/sunxi-ng/ccu_div.h
index 90d49ee8e0cc..e096c7be5dca 100644
--- a/drivers/clk/sunxi-ng/ccu_div.h
+++ b/drivers/clk/sunxi-ng/ccu_div.h
@@ -283,4 +283,16 @@ static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)

 extern const struct clk_ops ccu_div_ops;

+struct ccu_div_nb {
+	struct notifier_block	clk_nb;
+	struct ccu_common	*common;
+
+	u32	delay_us;	/* us to wait after changing parent rate */
+	int	original_enable;/* This is set by the notifier callback */
+};
+
+#define to_ccu_div_nb(_nb) container_of(_nb, struct ccu_div_nb, clk_nb)
+
+int ccu_div_notifier_register(struct clk *clk, struct ccu_div_nb *mux_nb);
+
 #endif /* _CCU_DIV_H_ */



>
> CLK_SET_RATE_PARENT only gates output on re-parenting, not on parent rate changes,
> according to the header:
>
>   https://elixir.bootlin.com/linux/v6.7.4/source/include/linux/clk-provider.h#L19
>
> You'd need perhaps CLK_SET_RATE_GATE *and* still verify that it actually works
> as expected via some tracing of gpu clock enable/disable/set_rate and pll-gpu
> set_rate. CLK_SET_RATE_GATE seems confusingly docummented:
>
>   https://elixir.bootlin.com/linux/v6.7.4/source/drivers/clk/clk.c#L1034
>
> so I don't particularly trust it does exaclty what the header claims and what
> would be needed to test the theory that gating gpu clock during rate change
> might help.
>
> kind regards,
> 	o.
>
>> Thank you for your detailed proposal! It was insightful to read. But
>> while those were all great ideas, they have all already been taken care
>> of. I'm fresh out of ideas again (except for pinning the GPU rate).
>>
>> Again, thank you so much,
>>   Frank
>>
>> >>
>> >> Kind regards,
>> >> 	o.
>> >>
>> >> > I very much appreciate your feedback!
>> >> >
>> >> > [1] https://gitlab.com/postmarketOS/pmaports/-/issues/805
>> >> >
>> >> > Signed-off-by: Frank Oltmanns <frank@...manns.dev>
>> >> > ---
>> >> > Changes in v2:
>> >> > - dts: Increase minimum GPU frequency to 192 MHz.
>> >> > - nkm and a64: Add minimum and maximum rate for PLL-MIPI.
>> >> > - nkm: Use the same approach for skipping invalid rates in
>> >> >   ccu_nkm_find_best() as in ccu_nkm_find_best_with_parent_adj().
>> >> > - nkm: Improve names for ratio struct members and hence get rid of
>> >> >   describing comments.
>> >> > - nkm and a64: Correct description in the commit messages: M/N <= 3
>> >> > - Remove patches for nm as they were not needed.
>> >> > - st7703: Rework the commit message to cover more background for the
>> >> >   change.
>> >> > - Link to v1: https://lore.kernel.org/r/20231218-pinephone-pll-fixes-v1-0-e238b6ed6dc1@oltmanns.dev
>> >> >
>> >> > ---
>> >> > Frank Oltmanns (6):
>> >> >       clk: sunxi-ng: nkm: Support constraints on m/n ratio and parent rate
>> >> >       clk: sunxi-ng: a64: Add constraints on PLL-MIPI's n/m ratio and parent rate
>> >> >       clk: sunxi-ng: nkm: Support minimum and maximum rate
>> >> >       clk: sunxi-ng: a64: Set minimum and maximum rate for PLL-MIPI
>> >> >       drm/panel: st7703: Drive XBD599 panel at higher clock rate
>> >> >       arm64: dts: allwinner: a64: Fix minimum GPU OPP rate
>> >> >
>> >> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  4 ++--
>> >> >  drivers/clk/sunxi-ng/ccu-sun50i-a64.c         | 14 +++++++----
>> >> >  drivers/clk/sunxi-ng/ccu_nkm.c                | 34 +++++++++++++++++++++++++++
>> >> >  drivers/clk/sunxi-ng/ccu_nkm.h                |  4 ++++
>> >> >  drivers/gpu/drm/panel/panel-sitronix-st7703.c | 14 +++++------
>> >> >  5 files changed, 56 insertions(+), 14 deletions(-)
>> >> > ---
>> >> > base-commit: 059c53e877ca6e723e10490c27c1487a63e66efe
>> >> > change-id: 20231218-pinephone-pll-fixes-0ccdfde273e4
>> >> >
>> >> > Best regards,
>> >> > --
>> >> > Frank Oltmanns <frank@...manns.dev>
>> >> >

Powered by blists - more mailing lists