| Message ID | 20240205-pinephone-pll-fixes-v2-0-96a46a2d8c9b@oltmanns.dev (mailing list archive) |
|---|---|
| Headers | show |
| Series | Pinephone video out fixes (flipping between two frames) | expand |
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.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> >>> >> >
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
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,