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 <[email protected]>
---
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/[email protected]
---
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 <[email protected]>
The Allwinner A64 manual lists the following constraints for the
PLL-MIPI clock:
- M/N <= 3
- (PLL_VIDEO0)/M >= 24MHz
The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for
these constraints.
Signed-off-by: Frank Oltmanns <[email protected]>
---
drivers/clk/sunxi-ng/ccu_nkm.c | 21 +++++++++++++++++++++
drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
2 files changed, 23 insertions(+)
diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
index 853f84398e2b..1168d894d636 100644
--- a/drivers/clk/sunxi-ng/ccu_nkm.c
+++ b/drivers/clk/sunxi-ng/ccu_nkm.c
@@ -16,6 +16,20 @@ struct _ccu_nkm {
unsigned long m, min_m, max_m;
};
+static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent,
+ unsigned long n, unsigned long m)
+{
+ struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common);
+
+ if (nkm->max_m_n_ratio && (m > nkm->max_m_n_ratio * n))
+ return false;
+
+ if (nkm->min_parent_m_ratio && (parent < nkm->min_parent_m_ratio * m))
+ return false;
+
+ return true;
+}
+
static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common,
struct clk_hw *parent_hw,
unsigned long *parent, unsigned long rate,
@@ -31,6 +45,10 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common
unsigned long tmp_rate, tmp_parent;
tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k));
+
+ if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m))
+ continue;
+
tmp_rate = tmp_parent * _n * _k / _m;
if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) ||
@@ -64,6 +82,9 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
for (_k = nkm->min_k; _k <= nkm->max_k; _k++) {
for (_n = nkm->min_n; _n <= nkm->max_n; _n++) {
for (_m = nkm->min_m; _m <= nkm->max_m; _m++) {
+ if (!ccu_nkm_is_valid_rate(common, parent, _n, _m))
+ continue;
+
unsigned long tmp_rate;
tmp_rate = parent * _n * _k / _m;
diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
index 6601defb3f38..c409212ee40e 100644
--- a/drivers/clk/sunxi-ng/ccu_nkm.h
+++ b/drivers/clk/sunxi-ng/ccu_nkm.h
@@ -27,6 +27,8 @@ struct ccu_nkm {
struct ccu_mux_internal mux;
unsigned int fixed_post_div;
+ unsigned long max_m_n_ratio;
+ unsigned long min_parent_m_ratio;
struct ccu_common common;
};
--
2.43.0
The Allwinner A64 manual lists the following constraints for the
PLL-MIPI clock:
- M/N <= 3
- (PLL_VIDEO0)/M >= 24MHz
Use these constraints.
Signed-off-by: Frank Oltmanns <[email protected]>
---
drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 8951ffc14ff5..df679dada792 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -171,11 +171,13 @@ static struct ccu_nkm pll_mipi_clk = {
* user manual, and by experiments the PLL doesn't work without
* these bits toggled.
*/
- .enable = BIT(31) | BIT(23) | BIT(22),
- .lock = BIT(28),
- .n = _SUNXI_CCU_MULT(8, 4),
- .k = _SUNXI_CCU_MULT_MIN(4, 2, 2),
- .m = _SUNXI_CCU_DIV(0, 4),
+ .enable = BIT(31) | BIT(23) | BIT(22),
+ .lock = BIT(28),
+ .n = _SUNXI_CCU_MULT(8, 4),
+ .k = _SUNXI_CCU_MULT_MIN(4, 2, 2),
+ .m = _SUNXI_CCU_DIV(0, 4),
+ .max_m_n_ratio = 3,
+ .min_parent_m_ratio = 24000000,
.common = {
.reg = 0x040,
.hw.init = CLK_HW_INIT("pll-mipi", "pll-video0",
--
2.43.0
Set the minimum and maximum rate of Allwinner A64's PLL-MIPI according
to the Allwinner User Manual.
Signed-off-by: Frank Oltmanns <[email protected]>
---
drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index df679dada792..99c2ce11da74 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -178,6 +178,8 @@ static struct ccu_nkm pll_mipi_clk = {
.m = _SUNXI_CCU_DIV(0, 4),
.max_m_n_ratio = 3,
.min_parent_m_ratio = 24000000,
+ .min_rate = 500000000,
+ .max_rate = 1400000000,
.common = {
.reg = 0x040,
.hw.init = CLK_HW_INIT("pll-mipi", "pll-video0",
--
2.43.0
The PLL-GPU has a minimum rate of 192 MHz according to the A64 manual.
If run at less than 192 MHz the pinephone (based on the A64) sometimes
replays the last few frames that were displayed over and over (see first
link below).
Note, that running PLL-GPU at 240 MHz and using a divisor of 2 *should*
circumvent this problem as well. But unfortunately, the GPU shows the
erratic behaviour even more often, even though its parent is driven at a
supported range. This might be due to a similar issue to the one
observed on the Allwinner H6 (see second link).
Running both the GPU and PLL-GPU at more then 192 MHz reduces the
occurrenc of the issue.
Therefore, increase the minimum rate in the GPU OPP table to 192 MHz.
Link: https://gitlab.com/postmarketOS/pmaports/-/issues/805
Link: https://lore.kernel.org/linux-arm-kernel/2562485.k3LOHGUjKi@kista/T/
Signed-off-by: Frank Oltmanns <[email protected]>
---
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 57ac18738c99..448d7fbdd1a9 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -107,8 +107,8 @@ de: display-engine {
gpu_opp_table: opp-table-gpu {
compatible = "operating-points-v2";
- opp-120000000 {
- opp-hz = /bits/ 64 <120000000>;
+ opp-192000000 {
+ opp-hz = /bits/ 64 <192000000>;
};
opp-312000000 {
--
2.43.0
According to the Allwinner User Manual, the Allwinner A64 requires
PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
Signed-off-by: Frank Oltmanns <[email protected]>
---
drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
2 files changed, 15 insertions(+)
diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
index 1168d894d636..7d135908d6e0 100644
--- a/drivers/clk/sunxi-ng/ccu_nkm.c
+++ b/drivers/clk/sunxi-ng/ccu_nkm.c
@@ -181,6 +181,12 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
rate *= nkm->fixed_post_div;
+ if (nkm->min_rate && rate < nkm->min_rate)
+ rate = nkm->min_rate;
+
+ if (nkm->max_rate && rate > nkm->max_rate)
+ rate = nkm->max_rate;
+
if (!clk_hw_can_set_rate_parent(&nkm->common.hw))
rate = ccu_nkm_find_best(*parent_rate, rate, &_nkm, &nkm->common);
else
@@ -220,6 +226,13 @@ static int ccu_nkm_set_rate(struct clk_hw *hw, unsigned long rate,
_nkm.min_m = 1;
_nkm.max_m = nkm->m.max ?: 1 << nkm->m.width;
+
+ if (nkm->min_rate && rate < nkm->min_rate)
+ rate = nkm->min_rate;
+
+ if (nkm->max_rate && rate > nkm->max_rate)
+ rate = nkm->max_rate;
+
ccu_nkm_find_best(parent_rate, rate, &_nkm, &nkm->common);
spin_lock_irqsave(nkm->common.lock, flags);
diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
index c409212ee40e..358a9df6b6a0 100644
--- a/drivers/clk/sunxi-ng/ccu_nkm.h
+++ b/drivers/clk/sunxi-ng/ccu_nkm.h
@@ -27,6 +27,8 @@ struct ccu_nkm {
struct ccu_mux_internal mux;
unsigned int fixed_post_div;
+ unsigned long min_rate;
+ unsigned long max_rate;
unsigned long max_m_n_ratio;
unsigned long min_parent_m_ratio;
--
2.43.0
This panel is used in the pinephone that runs on a Allwinner A64 SOC.
The SOC requires pll-mipi to run at more than 500 MHz.
This is the relevant clock tree:
pll-mipi
tcon0
tcon-data-clock
tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
has 24 bpp and 4 lanes. Therefore, the resulting requested
tcon-data-clock rate is:
crtc_clock * 1000 * (24 / 4) / 4
tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
parent rate of
4 * (crtc_clock * 1000 * (24 / 4) / 4)
Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
pll-mipi's constraint to run at 500MHz or higher forces us to have a
crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
so that it is high enough to align with pll-pipi limits.
Signed-off-by: Frank Oltmanns <[email protected]>
---
drivers/gpu/drm/panel/panel-sitronix-st7703.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
index b55bafd1a8be..6886fd7f765e 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
@@ -320,14 +320,14 @@ static int xbd599_init_sequence(struct st7703 *ctx)
static const struct drm_display_mode xbd599_mode = {
.hdisplay = 720,
- .hsync_start = 720 + 40,
- .hsync_end = 720 + 40 + 40,
- .htotal = 720 + 40 + 40 + 40,
+ .hsync_start = 720 + 65,
+ .hsync_end = 720 + 65 + 65,
+ .htotal = 720 + 65 + 65 + 65,
.vdisplay = 1440,
- .vsync_start = 1440 + 18,
- .vsync_end = 1440 + 18 + 10,
- .vtotal = 1440 + 18 + 10 + 17,
- .clock = 69000,
+ .vsync_start = 1440 + 30,
+ .vsync_end = 1440 + 30 + 22,
+ .vtotal = 1440 + 30 + 22 + 29,
+ .clock = (720 + 65 + 65 + 65) * (1440 + 30 + 22 + 29) * 60 / 1000,
.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
.width_mm = 68,
.height_mm = 136,
--
2.43.0
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 <[email protected]>
> ---
> 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/[email protected]
>
> ---
> 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 <[email protected]>
>
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 <[email protected]>
> > ---
> > 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/[email protected]
> >
> > ---
> > 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 <[email protected]>
> >
Dne ponedeljek, 05. februar 2024 ob 16:22:24 CET je Frank Oltmanns napisal(a):
> The Allwinner A64 manual lists the following constraints for the
> PLL-MIPI clock:
> - M/N <= 3
> - (PLL_VIDEO0)/M >= 24MHz
>
> The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for
> these constraints.
>
> Signed-off-by: Frank Oltmanns <[email protected]>
Haven't we discussed that this patch is unnecessary because same effect can
be reached by limiting minimum frequency?
Best regards,
Jernej
> ---
> drivers/clk/sunxi-ng/ccu_nkm.c | 21 +++++++++++++++++++++
> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
> 2 files changed, 23 insertions(+)
>
> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
> index 853f84398e2b..1168d894d636 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> @@ -16,6 +16,20 @@ struct _ccu_nkm {
> unsigned long m, min_m, max_m;
> };
>
> +static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent,
> + unsigned long n, unsigned long m)
> +{
> + struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common);
> +
> + if (nkm->max_m_n_ratio && (m > nkm->max_m_n_ratio * n))
> + return false;
> +
> + if (nkm->min_parent_m_ratio && (parent < nkm->min_parent_m_ratio * m))
> + return false;
> +
> + return true;
> +}
> +
> static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common,
> struct clk_hw *parent_hw,
> unsigned long *parent, unsigned long rate,
> @@ -31,6 +45,10 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common
> unsigned long tmp_rate, tmp_parent;
>
> tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k));
> +
> + if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m))
> + continue;
> +
> tmp_rate = tmp_parent * _n * _k / _m;
>
> if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) ||
> @@ -64,6 +82,9 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
> for (_k = nkm->min_k; _k <= nkm->max_k; _k++) {
> for (_n = nkm->min_n; _n <= nkm->max_n; _n++) {
> for (_m = nkm->min_m; _m <= nkm->max_m; _m++) {
> + if (!ccu_nkm_is_valid_rate(common, parent, _n, _m))
> + continue;
> +
> unsigned long tmp_rate;
>
> tmp_rate = parent * _n * _k / _m;
> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
> index 6601defb3f38..c409212ee40e 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkm.h
> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h
> @@ -27,6 +27,8 @@ struct ccu_nkm {
> struct ccu_mux_internal mux;
>
> unsigned int fixed_post_div;
> + unsigned long max_m_n_ratio;
> + unsigned long min_parent_m_ratio;
>
> struct ccu_common common;
> };
>
>
Hi Jernej,
On 2024-02-05 at 18:45:27 +0100, Jernej Škrabec <[email protected]> wrote:
> Dne ponedeljek, 05. februar 2024 ob 16:22:24 CET je Frank Oltmanns napisal(a):
>> The Allwinner A64 manual lists the following constraints for the
>> PLL-MIPI clock:
>> - M/N <= 3
>> - (PLL_VIDEO0)/M >= 24MHz
>>
>> The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for
>> these constraints.
>>
>> Signed-off-by: Frank Oltmanns <[email protected]>
>
> Haven't we discussed that this patch is unnecessary because same effect can
> be reached by limiting minimum frequency?
The patch for ccu_nm was unnecessary:
https://lore.kernel.org/all/[email protected]/
Unfortunately, we still need this one.
Best regards,
Frank
>
> Best regards,
> Jernej
>
>> ---
>> drivers/clk/sunxi-ng/ccu_nkm.c | 21 +++++++++++++++++++++
>> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
>> 2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
>> index 853f84398e2b..1168d894d636 100644
>> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
>> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
>> @@ -16,6 +16,20 @@ struct _ccu_nkm {
>> unsigned long m, min_m, max_m;
>> };
>>
>> +static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent,
>> + unsigned long n, unsigned long m)
>> +{
>> + struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common);
>> +
>> + if (nkm->max_m_n_ratio && (m > nkm->max_m_n_ratio * n))
>> + return false;
>> +
>> + if (nkm->min_parent_m_ratio && (parent < nkm->min_parent_m_ratio * m))
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common,
>> struct clk_hw *parent_hw,
>> unsigned long *parent, unsigned long rate,
>> @@ -31,6 +45,10 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common
>> unsigned long tmp_rate, tmp_parent;
>>
>> tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k));
>> +
>> + if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m))
>> + continue;
>> +
>> tmp_rate = tmp_parent * _n * _k / _m;
>>
>> if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) ||
>> @@ -64,6 +82,9 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
>> for (_k = nkm->min_k; _k <= nkm->max_k; _k++) {
>> for (_n = nkm->min_n; _n <= nkm->max_n; _n++) {
>> for (_m = nkm->min_m; _m <= nkm->max_m; _m++) {
>> + if (!ccu_nkm_is_valid_rate(common, parent, _n, _m))
>> + continue;
>> +
>> unsigned long tmp_rate;
>>
>> tmp_rate = parent * _n * _k / _m;
>> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
>> index 6601defb3f38..c409212ee40e 100644
>> --- a/drivers/clk/sunxi-ng/ccu_nkm.h
>> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h
>> @@ -27,6 +27,8 @@ struct ccu_nkm {
>> struct ccu_mux_internal mux;
>>
>> unsigned int fixed_post_div;
>> + unsigned long max_m_n_ratio;
>> + unsigned long min_parent_m_ratio;
>>
>> struct ccu_common common;
>> };
>>
>>
Dne ponedeljek, 05. februar 2024 ob 16:22:26 CET je Frank Oltmanns napisal(a):
> According to the Allwinner User Manual, the Allwinner A64 requires
> PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
>
> Signed-off-by: Frank Oltmanns <[email protected]>
> ---
> drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
> index 1168d894d636..7d135908d6e0 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> @@ -181,6 +181,12 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
> if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
> rate *= nkm->fixed_post_div;
>
> + if (nkm->min_rate && rate < nkm->min_rate)
> + rate = nkm->min_rate;
> +
> + if (nkm->max_rate && rate > nkm->max_rate)
> + rate = nkm->max_rate;
Please take a look at ccu_nm_round_rate() code. You need to consider postdiv
and you can return immediately.
> +
> if (!clk_hw_can_set_rate_parent(&nkm->common.hw))
> rate = ccu_nkm_find_best(*parent_rate, rate, &_nkm, &nkm->common);
> else
> @@ -220,6 +226,13 @@ static int ccu_nkm_set_rate(struct clk_hw *hw, unsigned long rate,
> _nkm.min_m = 1;
> _nkm.max_m = nkm->m.max ?: 1 << nkm->m.width;
>
> +
> + if (nkm->min_rate && rate < nkm->min_rate)
> + rate = nkm->min_rate;
> +
> + if (nkm->max_rate && rate > nkm->max_rate)
> + rate = nkm->max_rate;
> +
No need for this, clk subsystem calls round rate before setting actual clock
rate.
Best regards,
Jernej
> ccu_nkm_find_best(parent_rate, rate, &_nkm, &nkm->common);
>
> spin_lock_irqsave(nkm->common.lock, flags);
> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
> index c409212ee40e..358a9df6b6a0 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkm.h
> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h
> @@ -27,6 +27,8 @@ struct ccu_nkm {
> struct ccu_mux_internal mux;
>
> unsigned int fixed_post_div;
> + unsigned long min_rate;
> + unsigned long max_rate;
> unsigned long max_m_n_ratio;
> unsigned long min_parent_m_ratio;
>
>
>
Dne ponedeljek, 05. februar 2024 ob 16:22:27 CET je Frank Oltmanns napisal(a):
> Set the minimum and maximum rate of Allwinner A64's PLL-MIPI according
> to the Allwinner User Manual.
>
> Signed-off-by: Frank Oltmanns <[email protected]>
Reviewed-by: Jernej Skrabec <[email protected]>
Best regards,
Jernej
Dne ponedeljek, 05. februar 2024 ob 16:22:28 CET je Frank Oltmanns napisal(a):
> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
> The SOC requires pll-mipi to run at more than 500 MHz.
>
> This is the relevant clock tree:
> pll-mipi
> tcon0
> tcon-data-clock
>
> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
> has 24 bpp and 4 lanes. Therefore, the resulting requested
> tcon-data-clock rate is:
> crtc_clock * 1000 * (24 / 4) / 4
>
> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
> parent rate of
> 4 * (crtc_clock * 1000 * (24 / 4) / 4)
>
> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
>
> pll-mipi's constraint to run at 500MHz or higher forces us to have a
> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
>
> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
> so that it is high enough to align with pll-pipi limits.
Typo: pll-pipi -> pll-mipi
Best regards,
Jernej
>
> Signed-off-by: Frank Oltmanns <[email protected]>
> ---
> drivers/gpu/drm/panel/panel-sitronix-st7703.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> index b55bafd1a8be..6886fd7f765e 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> @@ -320,14 +320,14 @@ static int xbd599_init_sequence(struct st7703 *ctx)
>
> static const struct drm_display_mode xbd599_mode = {
> .hdisplay = 720,
> - .hsync_start = 720 + 40,
> - .hsync_end = 720 + 40 + 40,
> - .htotal = 720 + 40 + 40 + 40,
> + .hsync_start = 720 + 65,
> + .hsync_end = 720 + 65 + 65,
> + .htotal = 720 + 65 + 65 + 65,
> .vdisplay = 1440,
> - .vsync_start = 1440 + 18,
> - .vsync_end = 1440 + 18 + 10,
> - .vtotal = 1440 + 18 + 10 + 17,
> - .clock = 69000,
> + .vsync_start = 1440 + 30,
> + .vsync_end = 1440 + 30 + 22,
> + .vtotal = 1440 + 30 + 22 + 29,
> + .clock = (720 + 65 + 65 + 65) * (1440 + 30 + 22 + 29) * 60 / 1000,
> .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> .width_mm = 68,
> .height_mm = 136,
>
>
On 2024-02-05 at 18:56:09 +0100, Jernej Škrabec <[email protected]> wrote:
> Dne ponedeljek, 05. februar 2024 ob 16:22:26 CET je Frank Oltmanns napisal(a):
>> According to the Allwinner User Manual, the Allwinner A64 requires
>> PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
>>
>> Signed-off-by: Frank Oltmanns <[email protected]>
>> ---
>> drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
>> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
>> index 1168d894d636..7d135908d6e0 100644
>> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
>> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
>> @@ -181,6 +181,12 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
>> if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
>> rate *= nkm->fixed_post_div;
>>
>> + if (nkm->min_rate && rate < nkm->min_rate)
>> + rate = nkm->min_rate;
>> +
>> + if (nkm->max_rate && rate > nkm->max_rate)
>> + rate = nkm->max_rate;
>
> Please take a look at ccu_nm_round_rate() code. You need to consider postdiv
> and you can return immediately.
There is a difference here insofar that ccu_nm is always connected to a
fixed rate parent (at least that's my understanding). Therefore, in
ccu_nm_round_rate() we can be sure that the min or max rate can really
be set. In ccu_nkm we don't have that luxury, we actually have to find a
rate that is approximately equal to the min and max rate, based on the
parent rate. Therefore, we can't return immediately.
Also, I'm not sure what you mean about me needing to consider postdiv.
That's what I did. The check is after multiplying with the postdiv. It's
the same as in ccu_nm_round_rate() (just minus the immediate return).
>
>> +
>> if (!clk_hw_can_set_rate_parent(&nkm->common.hw))
>> rate = ccu_nkm_find_best(*parent_rate, rate, &_nkm, &nkm->common);
>> else
>> @@ -220,6 +226,13 @@ static int ccu_nkm_set_rate(struct clk_hw *hw, unsigned long rate,
>> _nkm.min_m = 1;
>> _nkm.max_m = nkm->m.max ?: 1 << nkm->m.width;
>>
>> +
>> + if (nkm->min_rate && rate < nkm->min_rate)
>> + rate = nkm->min_rate;
>> +
>> + if (nkm->max_rate && rate > nkm->max_rate)
>> + rate = nkm->max_rate;
>> +
>
> No need for this, clk subsystem calls round rate before setting actual clock
> rate.
I'll remove the checks in V3.
Best regards,
Frank
>
> Best regards,
> Jernej
>
>> ccu_nkm_find_best(parent_rate, rate, &_nkm, &nkm->common);
>>
>> spin_lock_irqsave(nkm->common.lock, flags);
>> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
>> index c409212ee40e..358a9df6b6a0 100644
>> --- a/drivers/clk/sunxi-ng/ccu_nkm.h
>> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h
>> @@ -27,6 +27,8 @@ struct ccu_nkm {
>> struct ccu_mux_internal mux;
>>
>> unsigned int fixed_post_div;
>> + unsigned long min_rate;
>> + unsigned long max_rate;
>> unsigned long max_m_n_ratio;
>> unsigned long min_parent_m_ratio;
>>
>>
>>
Dne ponedeljek, 05. februar 2024 ob 18:50:27 CET je Frank Oltmanns napisal(a):
> Hi Jernej,
>
> On 2024-02-05 at 18:45:27 +0100, Jernej Škrabec <[email protected]> wrote:
> > Dne ponedeljek, 05. februar 2024 ob 16:22:24 CET je Frank Oltmanns napisal(a):
> >> The Allwinner A64 manual lists the following constraints for the
> >> PLL-MIPI clock:
> >> - M/N <= 3
> >> - (PLL_VIDEO0)/M >= 24MHz
> >>
> >> The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for
> >> these constraints.
> >>
> >> Signed-off-by: Frank Oltmanns <[email protected]>
> >
> > Haven't we discussed that this patch is unnecessary because same effect can
> > be reached by limiting minimum frequency?
>
> The patch for ccu_nm was unnecessary:
> https://lore.kernel.org/all/[email protected]/
>
> Unfortunately, we still need this one.
Ok, then:
Reviewed-by: Jernej Skrabec <[email protected]>
Best regards,
Jernej
>
> Best regards,
> Frank
>
> >
> > Best regards,
> > Jernej
> >
> >> ---
> >> drivers/clk/sunxi-ng/ccu_nkm.c | 21 +++++++++++++++++++++
> >> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
> >> 2 files changed, 23 insertions(+)
> >>
> >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
> >> index 853f84398e2b..1168d894d636 100644
> >> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> >> @@ -16,6 +16,20 @@ struct _ccu_nkm {
> >> unsigned long m, min_m, max_m;
> >> };
> >>
> >> +static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent,
> >> + unsigned long n, unsigned long m)
> >> +{
> >> + struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common);
> >> +
> >> + if (nkm->max_m_n_ratio && (m > nkm->max_m_n_ratio * n))
> >> + return false;
> >> +
> >> + if (nkm->min_parent_m_ratio && (parent < nkm->min_parent_m_ratio * m))
> >> + return false;
> >> +
> >> + return true;
> >> +}
> >> +
> >> static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common,
> >> struct clk_hw *parent_hw,
> >> unsigned long *parent, unsigned long rate,
> >> @@ -31,6 +45,10 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common
> >> unsigned long tmp_rate, tmp_parent;
> >>
> >> tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k));
> >> +
> >> + if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m))
> >> + continue;
> >> +
> >> tmp_rate = tmp_parent * _n * _k / _m;
> >>
> >> if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) ||
> >> @@ -64,6 +82,9 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
> >> for (_k = nkm->min_k; _k <= nkm->max_k; _k++) {
> >> for (_n = nkm->min_n; _n <= nkm->max_n; _n++) {
> >> for (_m = nkm->min_m; _m <= nkm->max_m; _m++) {
> >> + if (!ccu_nkm_is_valid_rate(common, parent, _n, _m))
> >> + continue;
> >> +
> >> unsigned long tmp_rate;
> >>
> >> tmp_rate = parent * _n * _k / _m;
> >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
> >> index 6601defb3f38..c409212ee40e 100644
> >> --- a/drivers/clk/sunxi-ng/ccu_nkm.h
> >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h
> >> @@ -27,6 +27,8 @@ struct ccu_nkm {
> >> struct ccu_mux_internal mux;
> >>
> >> unsigned int fixed_post_div;
> >> + unsigned long max_m_n_ratio;
> >> + unsigned long min_parent_m_ratio;
> >>
> >> struct ccu_common common;
> >> };
> >>
> >>
>
Dne ponedeljek, 05. februar 2024 ob 16:22:25 CET je Frank Oltmanns napisal(a):
> The Allwinner A64 manual lists the following constraints for the
> PLL-MIPI clock:
> - M/N <= 3
> - (PLL_VIDEO0)/M >= 24MHz
>
> Use these constraints.
>
> Signed-off-by: Frank Oltmanns <[email protected]>
Reviewed-by: Jernej Skrabec <[email protected]>
Best regards,
Jernej
Dne ponedeljek, 05. februar 2024 ob 21:34:04 CET je Frank Oltmanns napisal(a):
>
> On 2024-02-05 at 18:56:09 +0100, Jernej Škrabec <[email protected]> wrote:
> > Dne ponedeljek, 05. februar 2024 ob 16:22:26 CET je Frank Oltmanns napisal(a):
> >> According to the Allwinner User Manual, the Allwinner A64 requires
> >> PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
> >>
> >> Signed-off-by: Frank Oltmanns <[email protected]>
> >> ---
> >> drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
> >> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
> >> 2 files changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
> >> index 1168d894d636..7d135908d6e0 100644
> >> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> >> @@ -181,6 +181,12 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
> >> if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
> >> rate *= nkm->fixed_post_div;
> >>
> >> + if (nkm->min_rate && rate < nkm->min_rate)
> >> + rate = nkm->min_rate;
> >> +
> >> + if (nkm->max_rate && rate > nkm->max_rate)
> >> + rate = nkm->max_rate;
> >
> > Please take a look at ccu_nm_round_rate() code. You need to consider postdiv
> > and you can return immediately.
>
> There is a difference here insofar that ccu_nm is always connected to a
> fixed rate parent (at least that's my understanding). Therefore, in
> ccu_nm_round_rate() we can be sure that the min or max rate can really
> be set. In ccu_nkm we don't have that luxury, we actually have to find a
> rate that is approximately equal to the min and max rate, based on the
> parent rate. Therefore, we can't return immediately.
Good point.
>
> Also, I'm not sure what you mean about me needing to consider postdiv.
> That's what I did. The check is after multiplying with the postdiv. It's
> the same as in ccu_nm_round_rate() (just minus the immediate return).
Nevermind, this applies only for immediate return.
Best regards,
Jernej
>
> >
> >> +
> >> if (!clk_hw_can_set_rate_parent(&nkm->common.hw))
> >> rate = ccu_nkm_find_best(*parent_rate, rate, &_nkm, &nkm->common);
> >> else
> >> @@ -220,6 +226,13 @@ static int ccu_nkm_set_rate(struct clk_hw *hw, unsigned long rate,
> >> _nkm.min_m = 1;
> >> _nkm.max_m = nkm->m.max ?: 1 << nkm->m.width;
> >>
> >> +
> >> + if (nkm->min_rate && rate < nkm->min_rate)
> >> + rate = nkm->min_rate;
> >> +
> >> + if (nkm->max_rate && rate > nkm->max_rate)
> >> + rate = nkm->max_rate;
> >> +
> >
> > No need for this, clk subsystem calls round rate before setting actual clock
> > rate.
>
> I'll remove the checks in V3.
>
> Best regards,
> Frank
>
> >
> > Best regards,
> > Jernej
> >
> >> ccu_nkm_find_best(parent_rate, rate, &_nkm, &nkm->common);
> >>
> >> spin_lock_irqsave(nkm->common.lock, flags);
> >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
> >> index c409212ee40e..358a9df6b6a0 100644
> >> --- a/drivers/clk/sunxi-ng/ccu_nkm.h
> >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h
> >> @@ -27,6 +27,8 @@ struct ccu_nkm {
> >> struct ccu_mux_internal mux;
> >>
> >> unsigned int fixed_post_div;
> >> + unsigned long min_rate;
> >> + unsigned long max_rate;
> >> unsigned long max_m_n_ratio;
> >> unsigned long min_parent_m_ratio;
> >>
> >>
> >>
>
On Mon, Feb 05, 2024 at 04:22:26PM +0100, Frank Oltmanns wrote:
> According to the Allwinner User Manual, the Allwinner A64 requires
> PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
>
> Signed-off-by: Frank Oltmanns <[email protected]>
> ---
> drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
> index 1168d894d636..7d135908d6e0 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> @@ -181,6 +181,12 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
> if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
> rate *= nkm->fixed_post_div;
>
> + if (nkm->min_rate && rate < nkm->min_rate)
> + rate = nkm->min_rate;
> +
> + if (nkm->max_rate && rate > nkm->max_rate)
> + rate = nkm->max_rate;
> +
This is provided by the clock range already. If you call
clk_hw_set_rate_range, it should work just fine.
Maxime
Hi Frank,
On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
> The SOC requires pll-mipi to run at more than 500 MHz.
>
> This is the relevant clock tree:
> pll-mipi
> tcon0
> tcon-data-clock
>
> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
> has 24 bpp and 4 lanes. Therefore, the resulting requested
> tcon-data-clock rate is:
> crtc_clock * 1000 * (24 / 4) / 4
>
> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
> parent rate of
> 4 * (crtc_clock * 1000 * (24 / 4) / 4)
>
> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
>
> pll-mipi's constraint to run at 500MHz or higher forces us to have a
> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
>
> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
> so that it is high enough to align with pll-pipi limits.
>
> Signed-off-by: Frank Oltmanns <[email protected]>
That commit log is great, but it's kind of off-topic. It's a panel
driver, it can be used on any MIPI-DSI controller, the only relevant
information there should be the panel timings required in the datasheet.
The PLL setup is something for the MIPI-DSI driver to adjust, not for
the panel to care for.
Maxime
Hi Ondřej,
On 2024-02-05 at 17:02:00 +0100, Ondřej Jirman <[email protected]> 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 <[email protected]>
>> > ---
>> > 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/[email protected]
>> >
>> > ---
>> > 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 <[email protected]>
>> >
On 2024-02-08 at 20:05:08 +0100, Maxime Ripard <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> Hi Frank,
>
> On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
>> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
>> The SOC requires pll-mipi to run at more than 500 MHz.
>>
>> This is the relevant clock tree:
>> pll-mipi
>> tcon0
>> tcon-data-clock
>>
>> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
>> has 24 bpp and 4 lanes. Therefore, the resulting requested
>> tcon-data-clock rate is:
>> crtc_clock * 1000 * (24 / 4) / 4
>>
>> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
>> parent rate of
>> 4 * (crtc_clock * 1000 * (24 / 4) / 4)
>>
>> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
>>
>> pll-mipi's constraint to run at 500MHz or higher forces us to have a
>> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
>>
>> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
>> so that it is high enough to align with pll-pipi limits.
>>
>> Signed-off-by: Frank Oltmanns <[email protected]>
>
> That commit log is great, but it's kind of off-topic. It's a panel
> driver, it can be used on any MIPI-DSI controller, the only relevant
> information there should be the panel timings required in the datasheet.
>
> The PLL setup is something for the MIPI-DSI driver to adjust, not for
> the panel to care for.
>
I absolutely agree. It even was the reason for my submission of a
sunxi-ng patch series last year that was accepted, to make pll-mipi more
flexible. :)
The only remaining option I currently see for adjusting the sunxi-ng
driver to further accomodate the panel, is trying to use a higher
divisor than 4 for calculating tcon-data-clock from tcon0. I remember
reading a discussion about this, but as far as I remember that proposal
was rejected (by you, IIRC).
While I appreciate other suggestion as well, I'll look into options for
using a different divisor than 4.
Best regards,
Frank
>
> Maxime
>
> [[End of PGP Signed Part]]
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 <[email protected]> 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 <[email protected]>
> >> > ---
> >> > 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/[email protected]
> >> >
> >> > ---
> >> > 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 <[email protected]>
> >> >
On 2024-02-11 at 16:42:43 +0100, Frank Oltmanns <[email protected]> wrote:
> On 2024-02-08 at 20:05:08 +0100, Maxime Ripard <[email protected]> wrote:
>> [[PGP Signed Part:Undecided]]
>> Hi Frank,
>>
>> On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
>>> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
>>> The SOC requires pll-mipi to run at more than 500 MHz.
>>>
>>> This is the relevant clock tree:
>>> pll-mipi
>>> tcon0
>>> tcon-data-clock
>>>
>>> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
>>> has 24 bpp and 4 lanes. Therefore, the resulting requested
>>> tcon-data-clock rate is:
>>> crtc_clock * 1000 * (24 / 4) / 4
>>>
>>> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
>>> parent rate of
>>> 4 * (crtc_clock * 1000 * (24 / 4) / 4)
>>>
>>> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
>>>
>>> pll-mipi's constraint to run at 500MHz or higher forces us to have a
>>> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
>>>
>>> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
>>> so that it is high enough to align with pll-pipi limits.
>>>
>>> Signed-off-by: Frank Oltmanns <[email protected]>
>>
>> That commit log is great, but it's kind of off-topic. It's a panel
>> driver, it can be used on any MIPI-DSI controller, the only relevant
>> information there should be the panel timings required in the datasheet.
>>
>> The PLL setup is something for the MIPI-DSI driver to adjust, not for
>> the panel to care for.
>>
>
> I absolutely agree. It even was the reason for my submission of a
> sunxi-ng patch series last year that was accepted, to make pll-mipi more
> flexible. :)
>
> The only remaining option I currently see for adjusting the sunxi-ng
> driver to further accomodate the panel, is trying to use a higher
> divisor than 4 for calculating tcon-data-clock from tcon0. I remember
> reading a discussion about this, but as far as I remember that proposal
> was rejected (by you, IIRC).
>
> While I appreciate other suggestion as well, I'll look into options for
> using a different divisor than 4.
I tried the following:
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -391,7 +391,7 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
* dclk is required to run at 1/4 the DSI per-lane bit rate.
*/
tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
- tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
+ tcon->dclk_max_div = 127;
clk_set_rate(tcon->dclk, mode->crtc_clock * 1000 * (bpp / lanes)
/ SUN6I_DSI_TCON_DIV);
On the pinephone, this selects a divisor of 6 resulting in a 25% frame
drop. I.e., unless I'm missing something using a divisor other than 4 is
not an option. This also matches your report from 2019: "Well, it's also
breaking another panel." [1]
I can currently see the following options:
a. Drive PLL-MIPI outside spec and panel within spec (current situation,
but missing a small patch [2] that fixes the crtc_clock and nothing
else) and live with the fact that some pinephones will run
unreliably.
b. Drive PLL-MIPI and panel within spec and shove data into the panel at
a too high rate (i.e., apply the rest of this series but not this
specific patch). This seems to mostly work, but hasn't seen any long
term testing. Short term testing showed that this approach results in
a slight but noticable unsmooth scrolling behavior.
c. Drive PLL-MIPI within spec and panel outside spec (i.e., apply a
future version of the whole series). This has been tested for over a
month on three devices that I know of. There are no reports of panels
not working with the suggested parameters.
All options require additional work on the GPU rate which is currently
being discussed in a parallel thread of this series. Unless somebody
comes up with a better idea, I will pause working on fixing PLL-MIPI and
focus on the GPU instead. While I doubt it, maybe fixing the GPU is
sufficient and continuing to drive PLL-MIPI outside spec proves to be
ok.
[1]: https://lore.kernel.org/all/20190828130341.s5z76wejulwdgxlc@flea/
[2]: https://lore.kernel.org/all/[email protected]/
Best regards,
Frank
>
> Best regards,
> Frank
>
>>
>> Maxime
>>
>> [[End of PGP Signed Part]]
Hi Maxime,
On 2024-02-08 at 13:16:27 +0100, Maxime Ripard <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> On Mon, Feb 05, 2024 at 04:22:26PM +0100, Frank Oltmanns wrote:
>> According to the Allwinner User Manual, the Allwinner A64 requires
>> PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
>>
>> Signed-off-by: Frank Oltmanns <[email protected]>
>> ---
>> drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
>> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
>> index 1168d894d636..7d135908d6e0 100644
>> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
>> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
>> @@ -181,6 +181,12 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
>> if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
>> rate *= nkm->fixed_post_div;
>>
>> + if (nkm->min_rate && rate < nkm->min_rate)
>> + rate = nkm->min_rate;
>> +
>> + if (nkm->max_rate && rate > nkm->max_rate)
>> + rate = nkm->max_rate;
>> +
>
> This is provided by the clock range already. If you call
> clk_hw_set_rate_range, it should work just fine.
I have to admit, that I don't know that much about sunxi-ng or the CCF
and therefore humbly request some guidance.
I've looked at other examples of clk_hw_set_rate_range() usage and it
seems there is not a lot of adoption for this functionality even though
it was already introduced mid-2015. This makes me wonder, why that is.
Anyhow, it seems in all examples I found, clk_hw_set_rate_range() is
called immediately after registering the clk_hw. So, in the case of
sunxi-ng, we'd need to do that in sunxi_ccu_probe, which is a common
function for all sunxi-ng clock types. Correct?
If so, surely, you don't want me to introduce clock type specific code
to a common function, so I assume you want min_rate and max_rate to
become members of struct ccu_common. Correct?
If so, since there already are some clock types in sunxi-ng that support
having a minimum and maximum rate, these clocks should be refactored
eventually. Correct?
Finally, in sunxi-ng there is a feature of having a fixed_post_div (see,
e.g., the first to lines of the diff above). It seems to me that CCF
cannot know about these post_divs, so we'd also need to transfer the
fixed_post_div to ccu_common and use that when calling
clk_hw_set_rate_range. Correct?
The fact that you casually dropped the two sentences above and me
deducing you want a somewhat large refactoring of the functionality for
sunxi-ng, makes me wonder if I completely misunderstood your request.
Best regards,
Frank
>
> Maxime
>
> [[End of PGP Signed Part]]
Hi Ondřej,
On 2024-02-11 at 20:25:29 +0100, Ondřej Jirman <[email protected]> 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 <[email protected]> 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 <[email protected]>
*/
+#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 <[email protected]>
>> >> > ---
>> >> > 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/[email protected]
>> >> >
>> >> > ---
>> >> > 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 <[email protected]>
>> >> >
Hi Jernej,
hi Maxime,
On 2024-02-05 at 16:22:26 +0100, Frank Oltmanns <[email protected]> wrote:
> According to the Allwinner User Manual, the Allwinner A64 requires
> PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
I should point out that limiting PLL-MIPI also fixes a regression
that was introduced in 6.5, specifically
ca1170b69968233b34d26432245eddf7d265186b "clk: sunxi-ng: a64: force
select PLL_MIPI in TCON0 mux". This has been bisected and reported by
Diego [1].
I don't know the procedure (yet), but probably the fix (if and when
accepted) should be backported at least to 6.6 (first broken LTS), 6.7
(stable), and 6.8 (next stable).
My suggestion:
- In V3 of this series, I will reorder the patches, so that what is now
PATCH 3 and 4 becomes 1 and 2 respectively, so that they can be
applied to 6.6 more easily.
- Maxime, IIUC you requested some refactoring for handling the rate
limits [2]. I propose, we use my current proposal as-is, and I will
do a follow-up series for the refactoring.
Please let me know how you would like me to proceed.
Thanks,
Frank
[1]: https://groups.google.com/g/linux-sunxi/c/Rh-Uqqa66bw
[2]: https://lore.kernel.org/all/exb2lvjcozak5fayrgyenrd3ntii4jfxgvqork4klyz5pky2aq@dj2zyw5su6pv/
>
> Signed-off-by: Frank Oltmanns <[email protected]>
> ---
> drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
> index 1168d894d636..7d135908d6e0 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> @@ -181,6 +181,12 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
> if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
> rate *= nkm->fixed_post_div;
>
> + if (nkm->min_rate && rate < nkm->min_rate)
> + rate = nkm->min_rate;
> +
> + if (nkm->max_rate && rate > nkm->max_rate)
> + rate = nkm->max_rate;
> +
> if (!clk_hw_can_set_rate_parent(&nkm->common.hw))
> rate = ccu_nkm_find_best(*parent_rate, rate, &_nkm, &nkm->common);
> else
> @@ -220,6 +226,13 @@ static int ccu_nkm_set_rate(struct clk_hw *hw, unsigned long rate,
> _nkm.min_m = 1;
> _nkm.max_m = nkm->m.max ?: 1 << nkm->m.width;
>
> +
> + if (nkm->min_rate && rate < nkm->min_rate)
> + rate = nkm->min_rate;
> +
> + if (nkm->max_rate && rate > nkm->max_rate)
> + rate = nkm->max_rate;
> +
> ccu_nkm_find_best(parent_rate, rate, &_nkm, &nkm->common);
>
> spin_lock_irqsave(nkm->common.lock, flags);
> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
> index c409212ee40e..358a9df6b6a0 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkm.h
> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h
> @@ -27,6 +27,8 @@ struct ccu_nkm {
> struct ccu_mux_internal mux;
>
> unsigned int fixed_post_div;
> + unsigned long min_rate;
> + unsigned long max_rate;
> unsigned long max_m_n_ratio;
> unsigned long min_parent_m_ratio;
On Wed, Feb 21, 2024 at 11:38:39AM +0100, Frank Oltmanns wrote:
> Hi Jernej,
> hi Maxime,
>
> On 2024-02-05 at 16:22:26 +0100, Frank Oltmanns <[email protected]> wrote:
> > According to the Allwinner User Manual, the Allwinner A64 requires
> > PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
>
> I should point out that limiting PLL-MIPI also fixes a regression
> that was introduced in 6.5, specifically
> ca1170b69968233b34d26432245eddf7d265186b "clk: sunxi-ng: a64: force
> select PLL_MIPI in TCON0 mux". This has been bisected and reported by
> Diego [1].
>
> I don't know the procedure (yet), but probably the fix (if and when
> accepted) should be backported at least to 6.6 (first broken LTS), 6.7
> (stable), and 6.8 (next stable).
https://www.kernel.org/doc/html/next/process/stable-kernel-rules.html#procedure-for-submitting-patches-to-the-stable-tree
> My suggestion:
> - In V3 of this series, I will reorder the patches, so that what is now
> PATCH 3 and 4 becomes 1 and 2 respectively, so that they can be
> applied to 6.6 more easily.
> - Maxime, IIUC you requested some refactoring for handling the rate
> limits [2]. I propose, we use my current proposal as-is, and I will
> do a follow-up series for the refactoring.
I'd really like to not introduce a new ad-hoc implementation of range
handling. It's fine for older users to not be converted yet, but we
shouldn't introduce more users.
Maxime
Hi,
On Sun, Feb 18, 2024 at 09:29:15AM +0100, Frank Oltmanns wrote:
> Hi Maxime,
>
> On 2024-02-08 at 13:16:27 +0100, Maxime Ripard <[email protected]> wrote:
> > [[PGP Signed Part:Undecided]]
> > On Mon, Feb 05, 2024 at 04:22:26PM +0100, Frank Oltmanns wrote:
> >> According to the Allwinner User Manual, the Allwinner A64 requires
> >> PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
> >>
> >> Signed-off-by: Frank Oltmanns <[email protected]>
> >> ---
> >> drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
> >> drivers/clk/sunxi-ng/ccu_nkm.h | 2 ++
> >> 2 files changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
> >> index 1168d894d636..7d135908d6e0 100644
> >> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> >> @@ -181,6 +181,12 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
> >> if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
> >> rate *= nkm->fixed_post_div;
> >>
> >> + if (nkm->min_rate && rate < nkm->min_rate)
> >> + rate = nkm->min_rate;
> >> +
> >> + if (nkm->max_rate && rate > nkm->max_rate)
> >> + rate = nkm->max_rate;
> >> +
> >
> > This is provided by the clock range already. If you call
> > clk_hw_set_rate_range, it should work just fine.
>
> I have to admit, that I don't know that much about sunxi-ng or the CCF
> and therefore humbly request some guidance.
>
> I've looked at other examples of clk_hw_set_rate_range() usage and it
> seems there is not a lot of adoption for this functionality even though
> it was already introduced mid-2015. This makes me wonder, why that is.
There's no reason, really. I would expect a big part of it to be "if it
works don't fix it" :)
> Anyhow, it seems in all examples I found, clk_hw_set_rate_range() is
> called immediately after registering the clk_hw. So, in the case of
> sunxi-ng, we'd need to do that in sunxi_ccu_probe, which is a common
> function for all sunxi-ng clock types. Correct?
Yup.
> If so, surely, you don't want me to introduce clock type specific code
> to a common function, so I assume you want min_rate and max_rate to
> become members of struct ccu_common. Correct?
Yes, that would be reasonable indeed.
> If so, since there already are some clock types in sunxi-ng that support
> having a minimum and maximum rate, these clocks should be refactored
> eventually. Correct?
I guess. I don't consider it to be a pre-requisite to your patch though.
> Finally, in sunxi-ng there is a feature of having a fixed_post_div (see,
> e.g., the first to lines of the diff above). It seems to me that CCF
> cannot know about these post_divs, so we'd also need to transfer the
> fixed_post_div to ccu_common and use that when calling
> clk_hw_set_rate_range. Correct?
Not really, no. The fixed post divider is an additional divider that
needs to be considered for the clock rate.
See the A64's periph0 PLL for example. Its fixed post divider is 2, and
its rate is 24MHz * N * K / 2. The rate should be bounded by its minimal
and maximal rate taking the post divider into account. The CCF doesn't
have to know about it.
Maxime
On Sun, Feb 11, 2024 at 04:42:43PM +0100, Frank Oltmanns wrote:
>
> On 2024-02-08 at 20:05:08 +0100, Maxime Ripard <[email protected]> wrote:
> > [[PGP Signed Part:Undecided]]
> > Hi Frank,
> >
> > On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
> >> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
> >> The SOC requires pll-mipi to run at more than 500 MHz.
> >>
> >> This is the relevant clock tree:
> >> pll-mipi
> >> tcon0
> >> tcon-data-clock
> >>
> >> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
> >> has 24 bpp and 4 lanes. Therefore, the resulting requested
> >> tcon-data-clock rate is:
> >> crtc_clock * 1000 * (24 / 4) / 4
> >>
> >> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
> >> parent rate of
> >> 4 * (crtc_clock * 1000 * (24 / 4) / 4)
> >>
> >> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
> >>
> >> pll-mipi's constraint to run at 500MHz or higher forces us to have a
> >> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
> >>
> >> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
> >> so that it is high enough to align with pll-pipi limits.
> >>
> >> Signed-off-by: Frank Oltmanns <[email protected]>
> >
> > That commit log is great, but it's kind of off-topic. It's a panel
> > driver, it can be used on any MIPI-DSI controller, the only relevant
> > information there should be the panel timings required in the datasheet.
> >
> > The PLL setup is something for the MIPI-DSI driver to adjust, not for
> > the panel to care for.
> >
>
> I absolutely agree. It even was the reason for my submission of a
> sunxi-ng patch series last year that was accepted, to make pll-mipi more
> flexible. :)
>
> The only remaining option I currently see for adjusting the sunxi-ng
> driver to further accomodate the panel, is trying to use a higher
> divisor than 4 for calculating tcon-data-clock from tcon0. I remember
> reading a discussion about this, but as far as I remember that proposal
> was rejected (by you, IIRC).
>
> While I appreciate other suggestion as well, I'll look into options for
> using a different divisor than 4.
Like I said, I'm not against the patch at all, it looks great to me on
principle. I just think you should completely rephrase the commit log
using the datasheet as the only reliable source of the display timings.
Whether sun4i can work around the panel requirements is something
completely orthogonal to the discussion, and thus the commit log.
Maxime
Hi Maxime,
On 2024-02-22 at 11:29:51 +0100, Maxime Ripard <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> On Sun, Feb 11, 2024 at 04:42:43PM +0100, Frank Oltmanns wrote:
>>
>> On 2024-02-08 at 20:05:08 +0100, Maxime Ripard <[email protected]> wrote:
>> > [[PGP Signed Part:Undecided]]
>> > Hi Frank,
>> >
>> > On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
>> >> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
>> >> The SOC requires pll-mipi to run at more than 500 MHz.
>> >>
>> >> This is the relevant clock tree:
>> >> pll-mipi
>> >> tcon0
>> >> tcon-data-clock
>> >>
>> >> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
>> >> has 24 bpp and 4 lanes. Therefore, the resulting requested
>> >> tcon-data-clock rate is:
>> >> crtc_clock * 1000 * (24 / 4) / 4
>> >>
>> >> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
>> >> parent rate of
>> >> 4 * (crtc_clock * 1000 * (24 / 4) / 4)
>> >>
>> >> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
>> >>
>> >> pll-mipi's constraint to run at 500MHz or higher forces us to have a
>> >> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
>> >>
>> >> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
>> >> so that it is high enough to align with pll-pipi limits.
>> >>
>> >> Signed-off-by: Frank Oltmanns <[email protected]>
>> >
>> > That commit log is great, but it's kind of off-topic. It's a panel
>> > driver, it can be used on any MIPI-DSI controller, the only relevant
>> > information there should be the panel timings required in the datasheet.
>> >
>> > The PLL setup is something for the MIPI-DSI driver to adjust, not for
>> > the panel to care for.
>> >
>>
>> I absolutely agree. It even was the reason for my submission of a
>> sunxi-ng patch series last year that was accepted, to make pll-mipi more
>> flexible. :)
>>
>> The only remaining option I currently see for adjusting the sunxi-ng
>> driver to further accomodate the panel, is trying to use a higher
>> divisor than 4 for calculating tcon-data-clock from tcon0. I remember
>> reading a discussion about this, but as far as I remember that proposal
>> was rejected (by you, IIRC).
>>
>> While I appreciate other suggestion as well, I'll look into options for
>> using a different divisor than 4.
>
> Like I said, I'm not against the patch at all, it looks great to me on
> principle. I just think you should completely rephrase the commit log
> using the datasheet as the only reliable source of the display timings.
> Whether sun4i can work around the panel requirements is something
> completely orthogonal to the discussion, and thus the commit log.
>
I was trying to follow the guidelines [1] for describing the reason
behind my changes to the panel. My original commit message was a lot
shorter, which, understandably, resulted in follow up questions [2].
With the current commit log, I'm trying to address those questions.
According to the device tree, the panel is only used in the pinephone.
The only reason for the change is that the SoC used by the only user of
this panel can not provide the rate the panel requests with the current
values. I think this information is relevant.
Unfortunately, as described in [2], I cannot back these values with any
datasheets because I couldn't find any. I could only find hints that
they are not publicly available. Icenowy (added to CC) submitted the
original values.
Best regards,
Frank
[1]: https://www.kernel.org/doc/html/v6.7/process/submitting-patches.html#describe-your-changes
[2]: https://lore.kernel.org/lkml/[email protected]/
>
> Maxime
>
> [[End of PGP Signed Part]]
在 2024-02-25星期日的 17:46 +0100，Frank Oltmanns写道：
> Hi Maxime,
>
> On 2024-02-22 at 11:29:51 +0100, Maxime Ripard <[email protected]>
> wrote:
> > [[PGP Signed Part:Undecided]]
> > On Sun, Feb 11, 2024 at 04:42:43PM +0100, Frank Oltmanns wrote:
> > >
> > > On 2024-02-08 at 20:05:08 +0100, Maxime Ripard
> > > <[email protected]> wrote:
> > > > [[PGP Signed Part:Undecided]]
> > > > Hi Frank,
> > > >
> > > > On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
> > > > > This panel is used in the pinephone that runs on a Allwinner
> > > > > A64 SOC.
> > > > > The SOC requires pll-mipi to run at more than 500 MHz.
> > > > >
> > > > > This is the relevant clock tree:
> > > > > pll-mipi
> > > > > tcon0
> > > > > tcon-data-clock
> > > > >
> > > > > tcon-data-clock has to run at 1/4 the DSI per-lane bit rate.
> > > > > The XBD599
> > > > > has 24 bpp and 4 lanes. Therefore, the resulting requested
> > > > > tcon-data-clock rate is:
> > > > > crtc_clock * 1000 * (24 / 4) / 4
> > > > >
> > > > > tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it
> > > > > requests a
> > > > > parent rate of
> > > > > 4 * (crtc_clock * 1000 * (24 / 4) / 4)
> > > > >
> > > > > Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate
> > > > > of pll-mipi.
> > > > >
> > > > > pll-mipi's constraint to run at 500MHz or higher forces us to
> > > > > have a
> > > > > crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh
> > > > > rate.
> > > > >
> > > > > Change [hv]sync_(start|end) so that we reach a clock rate of
> > > > > 83502 kHz
> > > > > so that it is high enough to align with pll-pipi limits.
> > > > >
> > > > > Signed-off-by: Frank Oltmanns <[email protected]>
> > > >
> > > > That commit log is great, but it's kind of off-topic. It's a
> > > > panel
> > > > driver, it can be used on any MIPI-DSI controller, the only
> > > > relevant
> > > > information there should be the panel timings required in the
> > > > datasheet.
> > > >
> > > > The PLL setup is something for the MIPI-DSI driver to adjust,
> > > > not for
> > > > the panel to care for.
> > > >
> > >
> > > I absolutely agree. It even was the reason for my submission of a
> > > sunxi-ng patch series last year that was accepted, to make pll-
> > > mipi more
> > > flexible. :)
> > >
> > > The only remaining option I currently see for adjusting the
> > > sunxi-ng
> > > driver to further accomodate the panel, is trying to use a higher
> > > divisor than 4 for calculating tcon-data-clock from tcon0. I
> > > remember
> > > reading a discussion about this, but as far as I remember that
> > > proposal
> > > was rejected (by you, IIRC).
> > >
> > > While I appreciate other suggestion as well, I'll look into
> > > options for
> > > using a different divisor than 4.
> >
> > Like I said, I'm not against the patch at all, it looks great to me
> > on
> > principle. I just think you should completely rephrase the commit
> > log
> > using the datasheet as the only reliable source of the display
> > timings.
> > Whether sun4i can work around the panel requirements is something
> > completely orthogonal to the discussion, and thus the commit log.
> >
>
> I was trying to follow the guidelines [1] for describing the reason
> behind my changes to the panel. My original commit message was a lot
> shorter, which, understandably, resulted in follow up questions [2].
> With the current commit log, I'm trying to address those questions.
> According to the device tree, the panel is only used in the
> pinephone.
> The only reason for the change is that the SoC used by the only user
> of
> this panel can not provide the rate the panel requests with the
> current
> values. I think this information is relevant.
>
> Unfortunately, as described in [2], I cannot back these values with
> any
> datasheets because I couldn't find any. I could only find hints that
> they are not publicly available. Icenowy (added to CC) submitted the
> original values.
Sorry but this kind of things are just magic from the vendor that I
could hardly explain...
>
> Best regards,
> Frank
>
> [1]:
> https://www.kernel.org/doc/html/v6.7/process/submitting-patches.html#describe-your-changes
> [2]: https://lore.kernel.org/lkml/[email protected]/
>
> >
> > Maxime
> >
> > [[End of PGP Signed Part]]
>
Hi Jernej,
hi Maxime,
hi Ondřej,
On 2024-02-19 at 10:41:19 +0100, Frank Oltmanns <[email protected]> wrote:
> Hi Ondřej,
>
> On 2024-02-11 at 20:25:29 +0100, Ondřej Jirman <[email protected]> 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 <[email protected]> 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 <[email protected]>
> */
>
> +#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 <[email protected]>
>>> >> > ---
>>> >> > 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/[email protected]
>>> >> >
>>> >> > ---
>>> >> > 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 <[email protected]>
>>> >> >
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 <[email protected]> wrote:
> > Hi Ondřej,
> >
> > On 2024-02-11 at 20:25:29 +0100, Ondřej Jirman <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > */
> >
> > +#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 <[email protected]>
> >>> >> > ---
> >>> >> > 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/[email protected]
> >>> >> >
> >>> >> > ---
> >>> >> > 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 <[email protected]>
> >>> >> >
>
Hello,
On Mon, Feb 26, 2024 at 6:29 PM Jernej Škrabec <[email protected]> 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