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

  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?

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,
  

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

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

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

I really like this idea. Unfortunately, it seems that the divisor of the
GPU_CLK_REG must always be set to 1. I tried to convey this info in the
commit message for PATCH 6. If using a different divisor than 1 the bug
I'm trying to fix here occurs within minutes. Nevertheless, I gave your
proposal a quick try - it seems so elegant. Unfortunately, but not
unexpectedly, the bug occured almost immediately.

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

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?

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

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

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?

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

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.

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

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

Dne ponedeljek, 26. februar 2024 ob 08:13:42 CET je Frank Oltmanns napisal(a):
> 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?

I can't find original source of these points. So I'm good with removing
them. But instead of fully removing table, you can just leave one point and
it should work.

Best regards,
Jernej

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

Hello,

On Mon, Feb 26, 2024 at 6:29 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Dne ponedeljek, 26. februar 2024 ob 08:13:42 CET je Frank Oltmanns napisal(a):
> > 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?
>
> I can't find original source of these points. So I'm good with removing
> them. But instead of fully removing table, you can just leave one point and
> it should work.

I had posted in
https://gitlab.freedesktop.org/mesa/mesa/-/issues/8410#note_2216628
that I also noticed the A64 datasheet specifically claims that except
for PLL_CPUX and PLL_DDR1, other PLLs don't support frequency scaling.
I was never able to find any evidence that it is actually supposed to
work anyway (perhaps it was hope?). Since you also looked in the BSP
and there is still no evidence that it is supported, I support that we
should likely just remove the OPPs.

Also, I wanted to point out that my series
https://patchwork.freedesktop.org/series/128856/#rev2 was merged to
lima recently. That was the root cause of the "flipping between two
frames" issue that people most probably hit. I highly recommend that
people using the Pinephone update their kernel to include those fixes
to fix that issue. As you mentioned about that symptom here, I just
wanted to point out that it wouldn't be possible to fix the "flipping
frames" issue with just fixes to A64 clock, it does need lima driver
fixes.

Thanks

Erico