Received-SPF: pass (google.com: domain of linux-kernel+bounces-52989-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 5 Feb 2024 17:02:00 +0100
From: =?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>
To: Frank Oltmanns <frank@oltmanns.dev>, 
	Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Chen-Yu Tsai <wens@csie.org>, 
	Jernej Skrabec <jernej.skrabec@gmail.com>, Samuel Holland <samuel@sholland.org>, 
	Guido =?utf-8?Q?G=C3=BCnther?= <agx@sigxcpu.org>, Purism Kernel Team <kernel@puri.sm>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Sam Ravnborg <sam@ravnborg.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, 
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, 
	Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, 
	devicetree@vger.kernel.org
Subject: Re: [PATCH v2 0/6] Pinephone video out fixes (flipping between two
 frames)
Message-ID: <bgyyemyi4shj3spo6qy4icvk56nrp5sihnzqurnozqdyktwugc@ikurx4ojoxpi>
Mail-Followup-To: =?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>, 
	Frank Oltmanns <frank@oltmanns.dev>, Michael Turquette <mturquette@baylibre.com>, 
	Stephen Boyd <sboyd@kernel.org>, Chen-Yu Tsai <wens@csie.org>, 
	Jernej Skrabec <jernej.skrabec@gmail.com>, Samuel Holland <samuel@sholland.org>, 
	Guido =?utf-8?Q?G=C3=BCnther?= <agx@sigxcpu.org>, Purism Kernel Team <kernel@puri.sm>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Sam Ravnborg <sam@ravnborg.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, 
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, 
	Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, 
	devicetree@vger.kernel.org
References: <20240205-pinephone-pll-fixes-v2-0-96a46a2d8c9b@oltmanns.dev>
 <jzl3mlzk4j7qvgcedvipgale5nhinznefodrnaehwsqfnseiwc@7zzlxd4dpueh>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <jzl3mlzk4j7qvgcedvipgale5nhinznefodrnaehwsqfnseiwc@7zzlxd4dpueh>

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:
> > 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;

You may be able to elegantly work around this by pinning PLL_GPU to a certain
frequency (assing it in DT and then don't decalre gpu clock as
CLK_SET_RATE_PARENT). Say 432 MHz for PLL and then do the scaling via GPU_CLK_REG
N divider between 432MHz and 216MHz and maybe 108MHz if that brings any gains.

Then you can perhaps sidestep all these potential issues with PLL_GPU and
lack of DVFS support decalred in the manual.

regards,
	o.

> 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@oltmanns.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@oltmanns.dev>
> >