Re: [PATCH v2 0/6] Pinephone video out fixes (flipping between two frames)

[Frank Oltmanns <frank@oltmanns.dev>]

Hi Jernej,
hi Maxime,
hi Ondřej,

On 2024-02-19 at 10:41:19 +0100, Frank Oltmanns <frank@oltmanns.dev> wrote:
> Hi Ondřej,
>
> On 2024-02-11 at 20:25:29 +0100, Ondřej Jirman <megi@xff.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@xff.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.

It seems to me that all options for changing the GPU's rate in a stable
manner have been exhausted. There seems to be no common interpretation
what the phrase "Clock output of PLL_GPU can be used for GPU;and dynamic
frequency scaling is not supported" in the Allwinner A64 manual (chapter
3.3.3) means.

The BSP uses a fixed rate of 432 MHz. Unless one of you has a clever
idea, I suggest to remove the OPPs from the device tree and set the GPU
to 432 MHz.

What are your thoughts on that?

Best regards,
  Frank

>
> 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@free-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@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>
>>> >> >