According to Allwinner's BSP code, in DSI mode, TCON0 clock needs to be
running at what's effectively the per-lane datarate of the DSI link.
Given that the TCON DCLK divider is fixed to 4 (SUN6I_DSI_TCON_DIV),
DCLK can't be set equal to the dotclock. Therefore labeling TCON DCLK
as sun4i_dotclock or tcon-pixel-clock shall be avoided.
With bpp bits per pixel transmitted over n DSI lanes, the target DCLK
rate for a given pixel clock is obtained as follows:
DCLK rate = 1/4 * bpp / n * pixel clock
Effect of this change can be observed through the rate of Vblank IRQs
which should now match refresh rate implied by set display mode. It
was verified to do so on a A64 board with a 2-lane and a 4-lane panel.
v2:
1. prevent reparent of tcon0 to pll-video0-2x
2. include pll-video0 in setting TCON0 DCLK rate
3. tested the whole thing also on a PinePhone
Roman Beranek (7):
clk: sunxi-ng: a64: propagate rate change from pll-mipi
clk: sunxi-ng: a64: export PLL_MIPI
clk: sunxi-ng: a64: prevent CLK_TCON0 being reparented
arm64: dts: allwinner: a64: assign PLL_MIPI to CLK_TCON0
ARM: dts: sunxi: rename tcon's clock output
drm: sun4i: rename sun4i_dotclock to sun4i_tcon_dclk
drm: sun4i: calculate proper DCLK rate for DSI
arch/arm/boot/dts/sun5i.dtsi | 2 +-
arch/arm/boot/dts/sun8i-a23-a33.dtsi | 2 +-
arch/arm/boot/dts/sun8i-a83t.dtsi | 2 +-
arch/arm/boot/dts/sun8i-v3s.dtsi | 2 +-
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 +-
drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 6 ++-
drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 4 +-
drivers/gpu/drm/sun4i/Makefile | 2 +-
drivers/gpu/drm/sun4i/sun4i_tcon.c | 46 +++++++++++--------
.../{sun4i_dotclock.c => sun4i_tcon_dclk.c} | 2 +-
.../{sun4i_dotclock.h => sun4i_tcon_dclk.h} | 0
include/dt-bindings/clock/sun50i-a64-ccu.h | 1 +
12 files changed, 43 insertions(+), 30 deletions(-)
rename drivers/gpu/drm/sun4i/{sun4i_dotclock.c => sun4i_tcon_dclk.c} (99%)
rename drivers/gpu/drm/sun4i/{sun4i_dotclock.h => sun4i_tcon_dclk.h} (100%)
base-commit: 4aa35a0130d6b8afbefc9ef530a521fb0fb9b8e1
--
2.34.1
Propagating rate change from tcon0 all the way to pll-video0 allows for
greater precision in matching requested display timing.
Signed-off-by: Roman Beranek <[email protected]>
---
drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 41519185600a..dd6212286dcd 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -179,7 +179,8 @@ static struct ccu_nkm pll_mipi_clk = {
.common = {
.reg = 0x040,
.hw.init = CLK_HW_INIT("pll-mipi", "pll-video0",
- &ccu_nkm_ops, CLK_SET_RATE_UNGATE),
+ &ccu_nkm_ops, CLK_SET_RATE_UNGATE
+ | CLK_SET_RATE_PARENT),
},
};
--
2.34.1
PLL_MIPI will be referenced as assigned parent to TCON0
Signed-off-by: Roman Beranek <[email protected]>
---
drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 4 +++-
include/dt-bindings/clock/sun50i-a64-ccu.h | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
index a8c11c0b4e06..35ab84e03e77 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
@@ -32,7 +32,9 @@
#define CLK_PLL_PERIPH1_2X 14
#define CLK_PLL_VIDEO1 15
#define CLK_PLL_GPU 16
-#define CLK_PLL_MIPI 17
+
+/* PLL_MIPI exported for TCON0 */
+
#define CLK_PLL_HSIC 18
#define CLK_PLL_DE 19
#define CLK_PLL_DDR1 20
diff --git a/include/dt-bindings/clock/sun50i-a64-ccu.h b/include/dt-bindings/clock/sun50i-a64-ccu.h
index 175892189e9d..5ad769a29c4e 100644
--- a/include/dt-bindings/clock/sun50i-a64-ccu.h
+++ b/include/dt-bindings/clock/sun50i-a64-ccu.h
@@ -45,6 +45,7 @@
#define CLK_PLL_VIDEO0 7
#define CLK_PLL_PERIPH0 11
+#define CLK_PLL_MIPI 17
#define CLK_CPUX 21
#define CLK_BUS_MIPI_DSI 28
--
2.34.1
TCON0's source clock can be fed from either pll-mipi, or pll-video0-2x,
however MIPI DSI output only seem to work when pll-mipi is selected and
thus some restriction have to be put on reparenting CLK_TCON0.
Functionally, there's no harm to other TCON0 users (LVDS, parallel RGB)
in also forcing them to settle on pll-mipi. The parent will be assigned
during boot based off of tcon0's DT node.
Signed-off-by: Roman Beranek <[email protected]>
---
drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index dd6212286dcd..ac9bf4e316bf 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -533,7 +533,8 @@ static const char * const tcon0_parents[] = { "pll-mipi", "pll-video0-2x" };
static const u8 tcon0_table[] = { 0, 2, };
static SUNXI_CCU_MUX_TABLE_WITH_GATE(tcon0_clk, "tcon0", tcon0_parents,
tcon0_table, 0x118, 24, 3, BIT(31),
- CLK_SET_RATE_PARENT);
+ CLK_SET_RATE_PARENT |
+ CLK_SET_RATE_NO_REPARENT);
static const char * const tcon1_parents[] = { "pll-video0", "pll-video1" };
static const u8 tcon1_table[] = { 0, 2, };
--
2.34.1
Assign pll-mipi parent to tcon0's source clock via 'assigned-clocks'.
Signed-off-by: Roman Beranek <[email protected]>
---
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 62f45f71ec65..e6a194db420d 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -411,6 +411,8 @@ tcon0: lcd-controller@1c0c000 {
#clock-cells = <0>;
resets = <&ccu RST_BUS_TCON0>, <&ccu RST_BUS_LVDS>;
reset-names = "lcd", "lvds";
+ assigned-clocks = <&ccu CLK_TCON0>;
+ assigned-clock-parents = <&ccu CLK_PLL_MIPI>;
ports {
#address-cells = <1>;
--
2.34.1
While the rate of TCON0's DCLK matches dotclock for parallel and LVDS
outputs, this doesn't hold for DSI. According manuals from Allwinner,
DCLK is an abbreviation of Data Clock, not dotclock, so go with that
instead.
Signed-off-by: Roman Beranek <[email protected]>
---
arch/arm/boot/dts/sun5i.dtsi | 2 +-
arch/arm/boot/dts/sun8i-a23-a33.dtsi | 2 +-
arch/arm/boot/dts/sun8i-a83t.dtsi | 2 +-
arch/arm/boot/dts/sun8i-v3s.dtsi | 2 +-
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/arm/boot/dts/sun5i.dtsi b/arch/arm/boot/dts/sun5i.dtsi
index 250d6b87ab4d..2f901a013676 100644
--- a/arch/arm/boot/dts/sun5i.dtsi
+++ b/arch/arm/boot/dts/sun5i.dtsi
@@ -286,7 +286,7 @@ tcon0: lcd-controller@1c0c000 {
clock-names = "ahb",
"tcon-ch0",
"tcon-ch1";
- clock-output-names = "tcon-pixel-clock";
+ clock-output-names = "tcon-data-clock";
#clock-cells = <0>;
status = "disabled";
diff --git a/arch/arm/boot/dts/sun8i-a23-a33.dtsi b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
index f630ab55bb6a..ddc87cc15e51 100644
--- a/arch/arm/boot/dts/sun8i-a23-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
@@ -190,7 +190,7 @@ tcon0: lcd-controller@1c0c000 {
clock-names = "ahb",
"tcon-ch0",
"lvds-alt";
- clock-output-names = "tcon-pixel-clock";
+ clock-output-names = "tcon-data-clock";
#clock-cells = <0>;
resets = <&ccu RST_BUS_LCD>,
<&ccu RST_BUS_LVDS>;
diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 82fdb04122ca..94eb3bfc989e 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -456,7 +456,7 @@ tcon0: lcd-controller@1c0c000 {
interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&ccu CLK_BUS_TCON0>, <&ccu CLK_TCON0>;
clock-names = "ahb", "tcon-ch0";
- clock-output-names = "tcon-pixel-clock";
+ clock-output-names = "tcon-data-clock";
#clock-cells = <0>;
resets = <&ccu RST_BUS_TCON0>, <&ccu RST_BUS_LVDS>;
reset-names = "lcd", "lvds";
diff --git a/arch/arm/boot/dts/sun8i-v3s.dtsi b/arch/arm/boot/dts/sun8i-v3s.dtsi
index db194c606fdc..ab2a0e1235e4 100644
--- a/arch/arm/boot/dts/sun8i-v3s.dtsi
+++ b/arch/arm/boot/dts/sun8i-v3s.dtsi
@@ -191,7 +191,7 @@ tcon0: lcd-controller@1c0c000 {
<&ccu CLK_TCON0>;
clock-names = "ahb",
"tcon-ch0";
- clock-output-names = "tcon-pixel-clock";
+ clock-output-names = "tcon-data-clock";
#clock-cells = <0>;
resets = <&ccu RST_BUS_TCON0>;
reset-names = "lcd";
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index e6a194db420d..caf35d2dd5d0 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -407,7 +407,7 @@ tcon0: lcd-controller@1c0c000 {
interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&ccu CLK_BUS_TCON0>, <&ccu CLK_TCON0>;
clock-names = "ahb", "tcon-ch0";
- clock-output-names = "tcon-pixel-clock";
+ clock-output-names = "tcon-data-clock";
#clock-cells = <0>;
resets = <&ccu RST_BUS_TCON0>, <&ccu RST_BUS_LVDS>;
reset-names = "lcd", "lvds";
--
2.34.1
While the rate of TCON0's DCLK matches dotclock for parallel and LVDS
outputs, this doesn't hold for DSI. The 'D' in DCLK actually stands for
'Data' according to Allwinner's manuals. The clock is mostly referred to
as dclk throughout this driver already anyway, so stick with that.
Signed-off-by: Roman Beranek <[email protected]>
---
drivers/gpu/drm/sun4i/Makefile | 2 +-
drivers/gpu/drm/sun4i/sun4i_tcon.c | 10 +++++-----
.../drm/sun4i/{sun4i_dotclock.c => sun4i_tcon_dclk.c} | 2 +-
.../drm/sun4i/{sun4i_dotclock.h => sun4i_tcon_dclk.h} | 0
4 files changed, 7 insertions(+), 7 deletions(-)
rename drivers/gpu/drm/sun4i/{sun4i_dotclock.c => sun4i_tcon_dclk.c} (99%)
rename drivers/gpu/drm/sun4i/{sun4i_dotclock.h => sun4i_tcon_dclk.h} (100%)
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index 0d04f2447b01..bad7497a0d11 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -19,7 +19,7 @@ sun8i-mixer-y += sun8i_mixer.o sun8i_ui_layer.o \
sun8i_vi_scaler.o sun8i_csc.o
sun4i-tcon-y += sun4i_crtc.o
-sun4i-tcon-y += sun4i_dotclock.o
+sun4i-tcon-y += sun4i_tcon_dclk.o
sun4i-tcon-y += sun4i_lvds.o
sun4i-tcon-y += sun4i_tcon.o
sun4i-tcon-y += sun4i_rgb.o
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 523a6d787921..eec26b1faa4b 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -31,12 +31,12 @@
#include <uapi/drm/drm_mode.h>
#include "sun4i_crtc.h"
-#include "sun4i_dotclock.h"
#include "sun4i_drv.h"
#include "sun4i_lvds.h"
#include "sun4i_rgb.h"
#include "sun4i_tcon.h"
#include "sun6i_mipi_dsi.h"
+#include "sun4i_tcon_dclk.h"
#include "sun8i_tcon_top.h"
#include "sunxi_engine.h"
@@ -1237,14 +1237,14 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
ret = sun4i_tcon_init_irq(dev, tcon);
if (ret) {
dev_err(dev, "Couldn't init our TCON interrupts\n");
- goto err_free_dotclock;
+ goto err_free_dclk;
}
tcon->crtc = sun4i_crtc_init(drm, engine, tcon);
if (IS_ERR(tcon->crtc)) {
dev_err(dev, "Couldn't create our CRTC\n");
ret = PTR_ERR(tcon->crtc);
- goto err_free_dotclock;
+ goto err_free_dclk;
}
if (tcon->quirks->has_channel_0) {
@@ -1264,7 +1264,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
of_node_put(remote);
if (ret < 0)
- goto err_free_dotclock;
+ goto err_free_dclk;
}
if (tcon->quirks->needs_de_be_mux) {
@@ -1290,7 +1290,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
return 0;
-err_free_dotclock:
+err_free_dclk:
if (tcon->quirks->has_channel_0)
sun4i_dclk_free(tcon);
err_free_clocks:
diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_tcon_dclk.c
similarity index 99%
rename from drivers/gpu/drm/sun4i/sun4i_dotclock.c
rename to drivers/gpu/drm/sun4i/sun4i_tcon_dclk.c
index 417ade3d2565..03d7de1911cd 100644
--- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon_dclk.c
@@ -10,7 +10,7 @@
#include <linux/regmap.h>
#include "sun4i_tcon.h"
-#include "sun4i_dotclock.h"
+#include "sun4i_tcon_dclk.h"
struct sun4i_dclk {
struct clk_hw hw;
diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.h b/drivers/gpu/drm/sun4i/sun4i_tcon_dclk.h
similarity index 100%
rename from drivers/gpu/drm/sun4i/sun4i_dotclock.h
rename to drivers/gpu/drm/sun4i/sun4i_tcon_dclk.h
--
2.34.1
In DSI mode, TCON0's data clock is required to run at 1/4 the per-lane
bit rate.
Signed-off-by: Roman Beranek <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_tcon.c | 36 +++++++++++++++++-------------
1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index eec26b1faa4b..b263de7a8237 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -291,18 +291,6 @@ static int sun4i_tcon_get_clk_delay(const struct drm_display_mode *mode,
return delay;
}
-static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
- const struct drm_display_mode *mode)
-{
- /* Configure the dot clock */
- clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
-
- /* Set the resolution */
- regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
- SUN4I_TCON0_BASIC0_X(mode->crtc_hdisplay) |
- SUN4I_TCON0_BASIC0_Y(mode->crtc_vdisplay));
-}
-
static void sun4i_tcon0_mode_set_dithering(struct sun4i_tcon *tcon,
const struct drm_connector *connector)
{
@@ -367,10 +355,18 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
u32 block_space, start_delay;
u32 tcon_div;
+ /*
+ * 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;
+ clk_set_rate(tcon->dclk, mode->crtc_clock * 1000 * (bpp / lanes)
+ / SUN6I_DSI_TCON_DIV);
- sun4i_tcon0_mode_set_common(tcon, mode);
+ /* Set the resolution */
+ regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
+ SUN4I_TCON0_BASIC0_X(mode->crtc_hdisplay) |
+ SUN4I_TCON0_BASIC0_Y(mode->crtc_vdisplay));
/* Set dithering if needed */
sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
@@ -438,7 +434,12 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
tcon->dclk_min_div = 7;
tcon->dclk_max_div = 7;
- sun4i_tcon0_mode_set_common(tcon, mode);
+ clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
+
+ /* Set the resolution */
+ regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
+ SUN4I_TCON0_BASIC0_X(mode->crtc_hdisplay) |
+ SUN4I_TCON0_BASIC0_Y(mode->crtc_vdisplay));
/* Set dithering if needed */
sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
@@ -515,7 +516,12 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
tcon->dclk_min_div = tcon->quirks->dclk_min_div;
tcon->dclk_max_div = 127;
- sun4i_tcon0_mode_set_common(tcon, mode);
+ clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
+
+ /* Set the resolution */
+ regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
+ SUN4I_TCON0_BASIC0_X(mode->crtc_hdisplay) |
+ SUN4I_TCON0_BASIC0_Y(mode->crtc_vdisplay));
/* Set dithering if needed */
sun4i_tcon0_mode_set_dithering(tcon, connector);
--
2.34.1
Hi Roman,
On 2023-04-18 at 09:40:01 +0200, Roman Beranek <[email protected]> wrote:
> According to Allwinner's BSP code, in DSI mode, TCON0 clock needs to be
> running at what's effectively the per-lane datarate of the DSI link.
> Given that the TCON DCLK divider is fixed to 4 (SUN6I_DSI_TCON_DIV),
> DCLK can't be set equal to the dotclock. Therefore labeling TCON DCLK
> as sun4i_dotclock or tcon-pixel-clock shall be avoided.
>
> With bpp bits per pixel transmitted over n DSI lanes, the target DCLK
> rate for a given pixel clock is obtained as follows:
>
> DCLK rate = 1/4 * bpp / n * pixel clock
>
> Effect of this change can be observed through the rate of Vblank IRQs
> which should now match refresh rate implied by set display mode. It
> was verified to do so on a A64 board with a 2-lane and a 4-lane panel.
>
> v2:
> 1. prevent reparent of tcon0 to pll-video0-2x
> 2. include pll-video0 in setting TCON0 DCLK rate
> 3. tested the whole thing also on a PinePhone
>
> Roman Beranek (7):
> clk: sunxi-ng: a64: propagate rate change from pll-mipi
> clk: sunxi-ng: a64: export PLL_MIPI
> clk: sunxi-ng: a64: prevent CLK_TCON0 being reparented
> arm64: dts: allwinner: a64: assign PLL_MIPI to CLK_TCON0
> ARM: dts: sunxi: rename tcon's clock output
> drm: sun4i: rename sun4i_dotclock to sun4i_tcon_dclk
> drm: sun4i: calculate proper DCLK rate for DSI
>
> arch/arm/boot/dts/sun5i.dtsi | 2 +-
> arch/arm/boot/dts/sun8i-a23-a33.dtsi | 2 +-
> arch/arm/boot/dts/sun8i-a83t.dtsi | 2 +-
> arch/arm/boot/dts/sun8i-v3s.dtsi | 2 +-
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 +-
> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 6 ++-
> drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 4 +-
> drivers/gpu/drm/sun4i/Makefile | 2 +-
> drivers/gpu/drm/sun4i/sun4i_tcon.c | 46 +++++++++++--------
> .../{sun4i_dotclock.c => sun4i_tcon_dclk.c} | 2 +-
> .../{sun4i_dotclock.h => sun4i_tcon_dclk.h} | 0
> include/dt-bindings/clock/sun50i-a64-ccu.h | 1 +
> 12 files changed, 43 insertions(+), 30 deletions(-)
> rename drivers/gpu/drm/sun4i/{sun4i_dotclock.c => sun4i_tcon_dclk.c} (99%)
> rename drivers/gpu/drm/sun4i/{sun4i_dotclock.h => sun4i_tcon_dclk.h} (100%)
>
>
> base-commit: 4aa35a0130d6b8afbefc9ef530a521fb0fb9b8e1
I've tried your patches on my pinephone. I also set the panel's clock to
72 MHz, so at 24 bpp and 4 lanes that should result in a data clock of
108 MHz. This should be possible when pll-video0 is at 297 MHz.
Unfortunately, pll-video0 is not set and therefore the relevant part of
the clk_summary looks like this:
enable prepare protect hardware
clock count count count rate enable
------------------------------------------------------------------------
pll-video0 1 1 1 294000000 Y
hdmi 0 0 0 294000000 N
tcon1 0 0 0 294000000 N
pll-mipi 1 1 1 431200000 Y
tcon0 2 2 1 431200000 Y
tcon-data-clock 1 1 1 107800000 Y
pll-video0-2x 0 0 0 588000000 Y
Note, I've cut the columns accuracy, phase, and duty cycle, because they
show the same values for all clocks (0, 0, 50000).
My understanding was that with this patchset setting the parent clock
should be possible. Do you have any idea why it doesn't work on the
pinephone? Or maybe it does work on yours and I'm making some kind of
mistake?
On a brighter note, when I initialize pll-video0 to 297 MHz in
sunxi-ng/ccu-sun50i-a64.c:sun50i_a64_ccu_probe() I get an even 108 Mhz
for the data clock. The patch is:
writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
+ /*
+ * Initialize PLL VIDEO0 to default values (297 MHz)
+ * to clean up any changes made by bootloader
+ */
+ writel(0x03006207, reg + 0x10);
+
ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_a64_ccu_desc);
if (ret)
return ret;
Best,
Frank
Hi,
On Tue, Apr 18, 2023 at 09:40:02AM +0200, Roman Beranek wrote:
> Propagating rate change from tcon0 all the way to pll-video0 allows for
> greater precision in matching requested display timing.
>
> Signed-off-by: Roman Beranek <[email protected]>
> ---
> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> index 41519185600a..dd6212286dcd 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> @@ -179,7 +179,8 @@ static struct ccu_nkm pll_mipi_clk = {
> .common = {
> .reg = 0x040,
> .hw.init = CLK_HW_INIT("pll-mipi", "pll-video0",
> - &ccu_nkm_ops, CLK_SET_RATE_UNGATE),
> + &ccu_nkm_ops, CLK_SET_RATE_UNGATE
> + | CLK_SET_RATE_PARENT),
The OR should be on the previous line
Maxime
On Tue, Apr 18, 2023 at 09:40:05AM +0200, Roman Beranek wrote:
> Assign pll-mipi parent to tcon0's source clock via 'assigned-clocks'.
>
> Signed-off-by: Roman Beranek <[email protected]>
> ---
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 62f45f71ec65..e6a194db420d 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -411,6 +411,8 @@ tcon0: lcd-controller@1c0c000 {
> #clock-cells = <0>;
> resets = <&ccu RST_BUS_TCON0>, <&ccu RST_BUS_LVDS>;
> reset-names = "lcd", "lvds";
> + assigned-clocks = <&ccu CLK_TCON0>;
> + assigned-clock-parents = <&ccu CLK_PLL_MIPI>;
assigned-clock-parents is fairly fragile, and it's essentially an OS
decision, so that doesn't have much to do with the platform.
Just force the parent in the clock driver, and prevent it from being
reparented. It will be more robust, and we will be able to change it in
the future easily.
Maxime
Maxime, Jernej, I was trying to understand why pll-video0 is not updated
and I tracked down the culprit to ccu_nkm.c.
On 2023-04-23 at 15:24:33 +0200, Frank Oltmanns <[email protected]> wrote:
> On 2023-04-18 at 09:40:01 +0200, Roman Beranek <[email protected]> wrote:
>> According to Allwinner's BSP code, in DSI mode, TCON0 clock needs to be
>> running at what's effectively the per-lane datarate of the DSI link.
>> Given that the TCON DCLK divider is fixed to 4 (SUN6I_DSI_TCON_DIV),
>> DCLK can't be set equal to the dotclock. Therefore labeling TCON DCLK
>> as sun4i_dotclock or tcon-pixel-clock shall be avoided.
>>
>> With bpp bits per pixel transmitted over n DSI lanes, the target DCLK
>> rate for a given pixel clock is obtained as follows:
>>
>> DCLK rate = 1/4 * bpp / n * pixel clock
>>
>> Effect of this change can be observed through the rate of Vblank IRQs
>> which should now match refresh rate implied by set display mode. It
>> was verified to do so on a A64 board with a 2-lane and a 4-lane panel.
[...]
> I've tried your patches on my pinephone. I also set the panel's clock to
> 72 MHz, so at 24 bpp and 4 lanes that should result in a data clock of
> 108 MHz. This should be possible when pll-video0 is at 297 MHz.
>
> Unfortunately, pll-video0 is not set and therefore the relevant part of
> the clk_summary looks like this:
>
> enable prepare protect hardware
> clock count count count rate enable
> ------------------------------------------------------------------------
> pll-video0 1 1 1 294000000 Y
> hdmi 0 0 0 294000000 N
> tcon1 0 0 0 294000000 N
> pll-mipi 1 1 1 431200000 Y
> tcon0 2 2 1 431200000 Y
> tcon-data-clock 1 1 1 107800000 Y
> pll-video0-2x 0 0 0 588000000 Y
>
> Note, I've cut the columns accuracy, phase, and duty cycle, because they
> show the same values for all clocks (0, 0, 50000).
>
> My understanding was that with this patchset setting the parent clock
> should be possible. Do you have any idea why it doesn't work on the
> pinephone? Or maybe it does work on yours and I'm making some kind of
> mistake?
To better understand what's going on I've extended the clk_rate_request
class to also output the requested rate. The relevant output is this
(leading line numbers by me for referencing the lines below):
line 1: kworker/u8:2-49 [002] ..... 1.850141: clk_rate_request_start: tcon-data-clock rate 108000000 min 0 max 18446744073709551615, parent tcon0 (588000000)
line 2: kworker/u8:2-49 [002] ..... 1.850149: clk_rate_request_start: tcon0 rate 432000000 min 0 max 18446744073709551615, parent pll-mipi (588000000)
line 3: kworker/u8:2-49 [002] ..... 1.850154: clk_rate_request_start: pll-mipi rate 432000000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 4: kworker/u8:2-49 [002] ..... 1.850168: clk_rate_request_done: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 5: kworker/u8:2-49 [002] ..... 1.850169: clk_rate_request_done: tcon0 rate 431200000 min 0 max 18446744073709551615, parent pll-mipi (431200000)
line 6: kworker/u8:2-49 [002] ..... 1.850171: clk_rate_request_done: tcon-data-clock rate 107800000 min 0 max 18446744073709551615, parent tcon0 (431200000)
line 7: kworker/u8:2-49 [002] ..... 1.850172: clk_rate_request_start: tcon-data-clock rate 108000000 min 0 max 18446744073709551615, parent tcon0 (588000000)
line 8: kworker/u8:2-49 [002] ..... 1.850174: clk_rate_request_start: tcon0 rate 432000000 min 0 max 18446744073709551615, parent pll-mipi (588000000)
line 9: kworker/u8:2-49 [002] ..... 1.850179: clk_rate_request_start: pll-mipi rate 432000000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 10: kworker/u8:2-49 [002] ..... 1.850190: clk_rate_request_done: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 11: kworker/u8:2-49 [002] ..... 1.850191: clk_rate_request_done: tcon0 rate 431200000 min 0 max 18446744073709551615, parent pll-mipi (431200000)
line 12: kworker/u8:2-49 [002] ..... 1.850192: clk_rate_request_done: tcon-data-clock rate 107800000 min 0 max 18446744073709551615, parent tcon0 (431200000)
line 13: kworker/u8:2-49 [002] ..... 1.850193: clk_rate_request_start: tcon0 rate 431200000 min 0 max 18446744073709551615, parent pll-mipi (588000000)
line 14: kworker/u8:2-49 [002] ..... 1.850195: clk_rate_request_start: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 15: kworker/u8:2-49 [002] ..... 1.850205: clk_rate_request_done: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 16: kworker/u8:2-49 [002] ..... 1.850206: clk_rate_request_done: tcon0 rate 431200000 min 0 max 18446744073709551615, parent pll-mipi (431200000)
line 17: kworker/u8:2-49 [002] ..... 1.850208: clk_rate_request_start: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 18: kworker/u8:2-49 [002] ..... 1.850219: clk_rate_request_done: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 19: kworker/u8:2-49 [002] ..... 1.850229: clk_set_rate: pll-mipi 431200000
line 20: kworker/u8:2-49 [003] ..... 1.850508: clk_set_rate_complete: pll-mipi 431200000
line 21: kworker/u8:2-49 [003] ..... 1.850513: clk_set_rate: tcon0 431200000
line 22: kworker/u8:2-49 [003] ..... 1.850515: clk_set_rate_complete: tcon0 431200000
line 23: kworker/u8:2-49 [003] ..... 1.850516: clk_set_rate: tcon-data-clock 107800000
line 24: kworker/u8:2-49 [003] ..... 1.850524: clk_set_rate_complete: tcon-data-clock 107800000
line 25: kworker/u8:2-49 [003] ..... 1.853320: clk_prepare: tcon-data-clock
line 26: kworker/u8:2-49 [003] ..... 1.853324: clk_prepare_complete: tcon-data-clock
line 27: kworker/u8:2-49 [003] d..1. 1.853328: clk_enable: tcon-data-clock
line 28: kworker/u8:2-49 [003] d..1. 1.853333: clk_enable_complete: tcon-data-clock
In line 1 we can see that a rate of 108 MHz is requested for
tcon-data-clock. In lines 2 and 3 this is forwarded to tcon0 and
pll-mipi (432 MHz). What surprised me, is that there is no request to
set the rate of pll-video0. Instead pll-mipi (and subsequently tcon0)
are set to 431.2 MHz (lines 4,5) and consequently tcon-data-clock is at
107.8 MHz (line 6) as I also reported in my previous mail (see quote
above).
When figuring out the call stack, I traced the whole thing down to
ccu_nkm_determine_rate(). The simplified call stack looks like this:
clk_set_rate(tcon-data-clock, 108MHz)
clk_core_set_rate_nolock(tcon-data-clock, 108MHz)
clk_core_req_round_rate_nolock(tcon-data-clock, 108MHz)
clk_core_round_rate_nolock(tcon-data-clock, 108MHz)
sun4i_dclk_round_rate(tcon-data-clock)
clk_hw_round_rate(tcon0, 432MHz)
clk_core_round_rate_nolock(tcon0, 432MHz)
clk_mux_determine_rate_flags(tcon0, 432MHz)
clk_core_round_rate_nolock(pll-mipi, 432MHz)
ccu_nkm_determine_rate(pll-mipi, 432MHz)
Looking at ccu_nkm_determine_rate(), we've found our culprit because it
does not try parent clock rates other than the current one. The same
applies to all other ccu_nkm_* functions.
So, I can see two options:
a. Set pll-video0 to 297 MHz on boot
b. Add functionality to ccu_nkm_* to also update the parent clock rate.
I'm actually interested in tackling b, but I can't make any promises as
to if and when I'll be able to solve it. I'm not certain about any side
effects this might have.
Until then, is option a acceptable in mainline?
Thanks,
Frank
>
> On a brighter note, when I initialize pll-video0 to 297 MHz in
> sunxi-ng/ccu-sun50i-a64.c:sun50i_a64_ccu_probe() I get an even 108 Mhz
> for the data clock. The patch is:
>
> writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
>
> + /*
> + * Initialize PLL VIDEO0 to default values (297 MHz)
> + * to clean up any changes made by bootloader
> + */
> + writel(0x03006207, reg + 0x10);
> +
> ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_a64_ccu_desc);
> if (ret)
> return ret;
>
> Best,
> Frank
Hello everyone,
I apologize for my absence from the discussion during past week, I got
hit with tonsillitis.
On Mon May 1, 2023 at 3:40 PM CEST, Frank Oltmanns wrote:
> Looking at ccu_nkm_determine_rate(), we've found our culprit because it
> does not try parent clock rates other than the current one. The same
> applies to all other ccu_nkm_* functions.
Yes, that's why I dropped CLK_SET_RATE_PARENT from pll-mipi in v3.
> b. Add functionality to ccu_nkm_* to also update the parent clock rate.
>
> I'm actually interested in tackling b, but I can't make any promises as
> to if and when I'll be able to solve it. I'm not certain about any side
> effects this might have.
It sounds like an interesting exercise. But what if HDMI is then added
to the mix?
Best regards
Roman
Hello Roman,
On 2023-05-03 at 16:22:32 +0200, "Roman Beranek" <[email protected]> wrote:
> Hello everyone,
>
> I apologize for my absence from the discussion during past week, I got
> hit with tonsillitis.
I hope you feel better!
> On Mon May 1, 2023 at 3:40 PM CEST, Frank Oltmanns wrote:
>> Looking at ccu_nkm_determine_rate(), we've found our culprit because it
>> does not try parent clock rates other than the current one. The same
>> applies to all other ccu_nkm_* functions.
>
> Yes, that's why I dropped CLK_SET_RATE_PARENT from pll-mipi in v3.
>
>> b. Add functionality to ccu_nkm_* to also update the parent clock rate.
>>
>> I'm actually interested in tackling b, but I can't make any promises as
>> to if and when I'll be able to solve it. I'm not certain about any side
>> effects this might have.
>
> It sounds like an interesting exercise. But what if HDMI is then added
> to the mix?
Thanks for interest in this discussion! I really appreciate it!
First of all, let me admit that I'm no expert on this. But nobody else
has replied so far, and I want to keep this conversation going, so let
me share my view.
My understanding is that pll-mipi being able to set pll-video0's rate
should not have an impact on HDMI, neither positive nor negative. If I'm
not mistaken those two things are orthogonal.
The relevant part of the clk_summary with your v4 [1] patch on top of
drm-next looks like this:
enable protect hardware
clock count count rate enable
----------------------------------------------------------------------
pll-video0 1 1 294000000 Y
hdmi 0 0 294000000 N
tcon1 0 0 294000000 N
pll-mipi 1 1 431200000 Y
tcon0 2 1 431200000 Y
tcon-data-clock 1 1 107800000 Y
pll-video0-2x 0 0 588000000 Y
Note, that pll-video0 is protected.
I don't own any boards that support HDMI in mainline. For the pinephone
this support is added e.g. in megi's kernel, where connecting an HDMI
output results in pll-video0's rate being set to 297MHz, even though it
is 294MHz after boot.
So, for reference, this is the same part of the clk_summary with megi's
6.3.0 kernel, USB-C dock unplugged:
enable protect hardware
clock count count rate enable
----------------------------------------------------------------------
pll-video0 3 0 294000000 Y
hdmi-phy-clk 1 0 73500000 Y
hdmi 1 0 294000000 Y
tcon1 0 0 294000000 N
pll-mipi 1 0 431200000 Y
tcon0 2 0 431200000 Y
tcon-pixel-clock 1 0 107800000 Y
pll-video0-2x 0 0 588000000 Y
pll-video0 is not protected. When plugging in the USB-C dock with an HDMI
monitor connected, the situation looks like this:
enable protect hardware
clock count count rate enable
----------------------------------------------------------------------
pll-video0 4 1 297000000 Y
hdmi-phy-clk 1 0 148500000 Y
hdmi 1 0 148500000 Y
tcon1 1 1 148500000 Y
pll-mipi 1 0 424285714 Y
tcon0 2 0 424285714 Y
tcon-pixel-clock 1 0 106071428 Y
pll-video0-2x 0 0 594000000 Y
As you can see, pll-video0 is updated to 297 MHz. My understanding is
(again: not an expert here) this is only possible due to the missing
protection.
What I'm trying to say is that I don't see a connection between HDMI and
having the functionality in ccu_nkm_* to update the parent clock rate.
But I do think it would be preferable to have pll-video0 at 297 MHz
after boot on the pinephone. We could achieve this with my two previous
proposals:
a) Set pll-video0 to 297 MHz on boot
b) Add functionality to ccu_nkm_* to also update the parent clock rate.
If solution b is viable, the goal for the pinephone (and any other
boards supporting HDMI) would then be to select a pixel-data-clock so
that the rate for pll-video0 is set to 297 MHz (by selecting an
appropriate clock speed for the internal panel).
Maybe I'm misunderstanding something. If so, I'd appreciate any
corrections.
Thanks,
Frank
[1]: https://lore.kernel.org/all/[email protected]/
>
> Best regards
> Roman
Hello again,
On 2023-05-08 at 08:54:28 +0200, Frank Oltmanns <[email protected]> wrote:
> Hello Roman,
>
> On 2023-05-03 at 16:22:32 +0200, "Roman Beranek" <[email protected]> wrote:
>> Hello everyone,
>>
>> I apologize for my absence from the discussion during past week, I got
>> hit with tonsillitis.
>
> I hope you feel better!
>
>> On Mon May 1, 2023 at 3:40 PM CEST, Frank Oltmanns wrote:
>>> Looking at ccu_nkm_determine_rate(), we've found our culprit because it
>>> does not try parent clock rates other than the current one. The same
>>> applies to all other ccu_nkm_* functions.
>>
>> Yes, that's why I dropped CLK_SET_RATE_PARENT from pll-mipi in v3.
>>
>>> b. Add functionality to ccu_nkm_* to also update the parent clock rate.
>>>
>>> I'm actually interested in tackling b, but I can't make any promises as
>>> to if and when I'll be able to solve it. I'm not certain about any side
>>> effects this might have.
>>
>> It sounds like an interesting exercise. But what if HDMI is then added
>> to the mix?
>
> Thanks for interest in this discussion! I really appreciate it!
>
> First of all, let me admit that I'm no expert on this. But nobody else
> has replied so far, and I want to keep this conversation going, so let
> me share my view.
>
> My understanding is that pll-mipi being able to set pll-video0's rate
> should not have an impact on HDMI, neither positive nor negative. If I'm
> not mistaken those two things are orthogonal.
>
> The relevant part of the clk_summary with your v4 [1] patch on top of
> drm-next looks like this:
>
> enable protect hardware
> clock count count rate enable
> ----------------------------------------------------------------------
> pll-video0 1 1 294000000 Y
> hdmi 0 0 294000000 N
> tcon1 0 0 294000000 N
> pll-mipi 1 1 431200000 Y
> tcon0 2 1 431200000 Y
> tcon-data-clock 1 1 107800000 Y
> pll-video0-2x 0 0 588000000 Y
>
> Note, that pll-video0 is protected.
>
> I don't own any boards that support HDMI in mainline. For the pinephone
> this support is added e.g. in megi's kernel, where connecting an HDMI
> output results in pll-video0's rate being set to 297MHz, even though it
> is 294MHz after boot.
>
> So, for reference, this is the same part of the clk_summary with megi's
> 6.3.0 kernel, USB-C dock unplugged:
>
> enable protect hardware
> clock count count rate enable
> ----------------------------------------------------------------------
> pll-video0 3 0 294000000 Y
> hdmi-phy-clk 1 0 73500000 Y
> hdmi 1 0 294000000 Y
> tcon1 0 0 294000000 N
> pll-mipi 1 0 431200000 Y
> tcon0 2 0 431200000 Y
> tcon-pixel-clock 1 0 107800000 Y
> pll-video0-2x 0 0 588000000 Y
>
> pll-video0 is not protected. When plugging in the USB-C dock with an HDMI
> monitor connected, the situation looks like this:
Just for reference, the protection count is disabled by this commit [1]
in megi's kernel.
In the commit message Icenowy Zheng refers to "the ability to keep TCON0
clock stable when HDMI changes its parent's clock." She implemented this
in these two previous commits [2] [3]. None of this is in mainline.
Best regards,
Frank
[1]: https://github.com/megous/linux/commit/039f7ee3f44adfbe4c6b7c2f1798b9a70c9fb9ee
[2]: https://github.com/megous/linux/commit/a927843932f16e5a7f5ff57fbfd2d5f11c712b67
[3]: https://github.com/megous/linux/commit/0e305371eaa49128856acce9830e6af079442ad6
>
> enable protect hardware
> clock count count rate enable
> ----------------------------------------------------------------------
> pll-video0 4 1 297000000 Y
> hdmi-phy-clk 1 0 148500000 Y
> hdmi 1 0 148500000 Y
> tcon1 1 1 148500000 Y
> pll-mipi 1 0 424285714 Y
> tcon0 2 0 424285714 Y
> tcon-pixel-clock 1 0 106071428 Y
> pll-video0-2x 0 0 594000000 Y
>
> As you can see, pll-video0 is updated to 297 MHz. My understanding is
> (again: not an expert here) this is only possible due to the missing
> protection.
>
> What I'm trying to say is that I don't see a connection between HDMI and
> having the functionality in ccu_nkm_* to update the parent clock rate.
>
> But I do think it would be preferable to have pll-video0 at 297 MHz
> after boot on the pinephone. We could achieve this with my two previous
> proposals:
>
> a) Set pll-video0 to 297 MHz on boot
> b) Add functionality to ccu_nkm_* to also update the parent clock rate.
>
> If solution b is viable, the goal for the pinephone (and any other
> boards supporting HDMI) would then be to select a pixel-data-clock so
> that the rate for pll-video0 is set to 297 MHz (by selecting an
> appropriate clock speed for the internal panel).
>
> Maybe I'm misunderstanding something. If so, I'd appreciate any
> corrections.
>
> Thanks,
> Frank
>
> [1]: https://lore.kernel.org/all/[email protected]/
>
>>
>> Best regards
>> Roman
Dne ponedeljek, 08. maj 2023 ob 16:08:32 CEST je Frank Oltmanns napisal(a):
> Hello again,
>
> On 2023-05-08 at 08:54:28 +0200, Frank Oltmanns <[email protected]> wrote:
> > Hello Roman,
> >
> > On 2023-05-03 at 16:22:32 +0200, "Roman Beranek" <[email protected]> wrote:
> >> Hello everyone,
> >>
> >> I apologize for my absence from the discussion during past week, I got
> >> hit with tonsillitis.
> >
> > I hope you feel better!
> >
> >> On Mon May 1, 2023 at 3:40 PM CEST, Frank Oltmanns wrote:
> >>> Looking at ccu_nkm_determine_rate(), we've found our culprit because it
> >>> does not try parent clock rates other than the current one. The same
> >>> applies to all other ccu_nkm_* functions.
> >>
> >> Yes, that's why I dropped CLK_SET_RATE_PARENT from pll-mipi in v3.
> >>
> >>> b. Add functionality to ccu_nkm_* to also update the parent clock rate.
> >>>
> >>> I'm actually interested in tackling b, but I can't make any promises as
> >>> to if and when I'll be able to solve it. I'm not certain about any side
> >>> effects this might have.
> >>
> >> It sounds like an interesting exercise. But what if HDMI is then added
> >> to the mix?
> >
> > Thanks for interest in this discussion! I really appreciate it!
> >
> > First of all, let me admit that I'm no expert on this. But nobody else
> > has replied so far, and I want to keep this conversation going, so let
> > me share my view.
> >
> > My understanding is that pll-mipi being able to set pll-video0's rate
> > should not have an impact on HDMI, neither positive nor negative. If I'm
> > not mistaken those two things are orthogonal.
> >
> > The relevant part of the clk_summary with your v4 [1] patch on top of
> >
> > drm-next looks like this:
> > enable protect hardware
> >
> > clock count count rate enable
> >
> > ----------------------------------------------------------------------
> >
> > pll-video0 1 1 294000000 Y
> >
> > hdmi 0 0 294000000 N
> > tcon1 0 0 294000000 N
> > pll-mipi 1 1 431200000 Y
> >
> > tcon0 2 1 431200000 Y
> >
> > tcon-data-clock 1 1 107800000 Y
> >
> > pll-video0-2x 0 0 588000000 Y
> >
> > Note, that pll-video0 is protected.
> >
> > I don't own any boards that support HDMI in mainline. For the pinephone
> > this support is added e.g. in megi's kernel, where connecting an HDMI
> > output results in pll-video0's rate being set to 297MHz, even though it
> > is 294MHz after boot.
> >
> > So, for reference, this is the same part of the clk_summary with megi's
> >
> > 6.3.0 kernel, USB-C dock unplugged:
> > enable protect hardware
> >
> > clock count count rate enable
> >
> > ----------------------------------------------------------------------
> >
> > pll-video0 3 0 294000000 Y
> >
> > hdmi-phy-clk 1 0 73500000 Y
> > hdmi 1 0 294000000 Y
> > tcon1 0 0 294000000 N
> > pll-mipi 1 0 431200000 Y
> >
> > tcon0 2 0 431200000 Y
> >
> > tcon-pixel-clock 1 0 107800000 Y
> >
> > pll-video0-2x 0 0 588000000 Y
> >
> > pll-video0 is not protected. When plugging in the USB-C dock with an HDMI
>
> > monitor connected, the situation looks like this:
> Just for reference, the protection count is disabled by this commit [1]
> in megi's kernel.
>
> In the commit message Icenowy Zheng refers to "the ability to keep TCON0
> clock stable when HDMI changes its parent's clock." She implemented this
> in these two previous commits [2] [3]. None of this is in mainline.
Those commits are good follow up series to this, if anyone wants to improve
things further.
Best regards,
Jernej
>
> Best regards,
> Frank
>
> [1]:
> https://github.com/megous/linux/commit/039f7ee3f44adfbe4c6b7c2f1798b9a70c9f
> b9ee [2]:
> https://github.com/megous/linux/commit/a927843932f16e5a7f5ff57fbfd2d5f11c71
> 2b67 [3]:
> https://github.com/megous/linux/commit/0e305371eaa49128856acce9830e6af07944
> 2ad6
> > enable protect hardware
> >
> > clock count count rate enable
> >
> > ----------------------------------------------------------------------
> >
> > pll-video0 4 1 297000000 Y
> >
> > hdmi-phy-clk 1 0 148500000 Y
> > hdmi 1 0 148500000 Y
> > tcon1 1 1 148500000 Y
> > pll-mipi 1 0 424285714 Y
> >
> > tcon0 2 0 424285714 Y
> >
> > tcon-pixel-clock 1 0 106071428 Y
> >
> > pll-video0-2x 0 0 594000000 Y
> >
> > As you can see, pll-video0 is updated to 297 MHz. My understanding is
> > (again: not an expert here) this is only possible due to the missing
> > protection.
> >
> > What I'm trying to say is that I don't see a connection between HDMI and
> > having the functionality in ccu_nkm_* to update the parent clock rate.
> >
> > But I do think it would be preferable to have pll-video0 at 297 MHz
> > after boot on the pinephone. We could achieve this with my two previous
> >
> > proposals:
> > a) Set pll-video0 to 297 MHz on boot
> > b) Add functionality to ccu_nkm_* to also update the parent clock rate.
> >
> > If solution b is viable, the goal for the pinephone (and any other
> > boards supporting HDMI) would then be to select a pixel-data-clock so
> > that the rate for pll-video0 is set to 297 MHz (by selecting an
> > appropriate clock speed for the internal panel).
> >
> > Maybe I'm misunderstanding something. If so, I'd appreciate any
> > corrections.
> >
> > Thanks,
> >
> > Frank
> >
> > [1]: https://lore.kernel.org/all/[email protected]/
> >
> >> Best regards
> >> Roman