Message-ID: <jzl3mlzk4j7qvgcedvipgale5nhinznefodrnaehwsqfnseiwc@7zzlxd4dpueh>
Date: Mon, 5 Feb 2024 16:54:07 +0100
From: Ondřej Jirman <megi@....cz>
To: Frank Oltmanns <frank@...manns.dev>
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)

On Mon, Feb 05, 2024 at 04:22:23PM +0100, Frank Oltmanns wrote:
> On some pinephones the video output sometimes freezes (flips between two
> frames) [1]. It seems to be that the reason for this behaviour is that
> PLL-MIPI, PLL-GPU and GPU are operating outside their limits.
> 
> In this patch series I propose the followin changes:
>   1. sunxi-ng: Adhere to the following constraints given in the
>      Allwinner A64 Manual regarding PLL-MIPI:
>       * M/N <= 3
>       * (PLL_VIDEO0)/M >= 24MHz
>       * 500MHz <= clockrate <= 1400MHz
> 
>   2. Choose a higher clock rate for the ST7703 based XDB599 panel, so
>      that the panel function well with the Allwinner A64 SOC. PLL-MIPI
>      must run between 500 MHz and 1.4 GHz. As PLL-MIPI runs at 6 times
>      the panel's clock rate, we need the panel's clock to be at least
>      83.333 MHz.
> 
>   3. Increase the minimum frequency in the A64 DTS OPPs from 120 MHz to
>      192 MHz. This further reduces the issue.
> 
> Unfortunately, with these patches the issue [1] is not completely gone,
> but becomes less likely.
> 
> Note, that when pinning the GPU to 432 MHz the issue completely
> disappears for me. I've searched the BSP and could not find any
> indication that supports the idea of having the three OPPs. The only
> frequency I found in the BPSs for A64 is 432 MHz, that has also proven
> stable for me. So, while increasing the minimum frequency to 192 MHz
> reduces the issue, should we maybe instead set the GPU to a fixed 432
> MHz instead?

Per A64 User Manual 1.1 page 81:

(9). Clock output of PLL_GPU can be used for GPU;and dynamic frequency scaling is not supported;

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.

It's also unclear what happens when FRAC_CLK_OUT or PLL_MODE_SEL changes.
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)

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