2023-04-27 09:17:57

by Roman Beranek

[permalink] [raw]
Subject: [PATCH v3 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode

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

v3:
1. accept that pll-video0 can't be included in setting TCON0 DCLK rate
2. reset pll-video0 to its default rate in case u-boot changed it

Roman Beranek (7):
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
arm64: dts: allwinner: a64: reset pll-video0 rate
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 | 7 ++-
drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 +-
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, 44 insertions(+), 29 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: 40aeab044a352b9af106960542257c1b9b652e8a
--
2.34.1


2023-04-27 09:17:59

by Roman Beranek

[permalink] [raw]
Subject: [PATCH v3 1/7] clk: sunxi-ng: a64: export PLL_MIPI

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

2023-04-27 09:17:59

by Roman Beranek

[permalink] [raw]
Subject: [PATCH v3 2/7] clk: sunxi-ng: a64: prevent CLK_TCON0 being reparented

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 41519185600a..044f301a8c61 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -532,7 +532,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

2023-04-27 09:19:30

by Roman Beranek

[permalink] [raw]
Subject: [PATCH v3 3/7] arm64: dts: allwinner: a64: assign PLL_MIPI to CLK_TCON0

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

2023-04-27 09:19:30

by Roman Beranek

[permalink] [raw]
Subject: [PATCH v3 4/7] arm64: dts: allwinner: a64: reset pll-video0 rate

With pll-mipi as its source clock, the exact rate to which TCON0's data
clock can be set to is constrained by the current rate of pll-video0.
Unless changed on a request of another consumer, the rate of pll-video0
is left as inherited from the bootloader.

The default rate on reset is 297 MHz, a value preferable to what it is
later set to in u-boot (294 MHz). This happens unintentionally though,
as u-boot, for the sake of simplicity, rounds the rate requested by DE2
driver (297 MHz) to 6 MHz steps.

Reset the PLL to its default rate of 297 MHz.

Signed-off-by: Roman Beranek <[email protected]>
---
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index e6a194db420d..cfc60dce80b0 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -667,6 +667,9 @@ ccu: clock@1c20000 {
clock-names = "hosc", "losc";
#clock-cells = <1>;
#reset-cells = <1>;
+
+ assigned-clocks = <&ccu CLK_PLL_VIDEO0>;
+ assigned-clock-rates = <297000000>;
};

pio: pinctrl@1c20800 {
--
2.34.1

2023-04-27 09:19:32

by Roman Beranek

[permalink] [raw]
Subject: [PATCH v3 5/7] ARM: dts: sunxi: rename tcon's clock output

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

2023-04-27 09:19:40

by Roman Beranek

[permalink] [raw]
Subject: [PATCH v3 6/7] drm: sun4i: rename sun4i_dotclock to sun4i_tcon_dclk

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

2023-04-27 09:19:49

by Roman Beranek

[permalink] [raw]
Subject: [PATCH v3 7/7] drm: sun4i: calculate proper DCLK rate for DSI

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

2023-04-27 09:37:29

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH v3 3/7] arm64: dts: allwinner: a64: assign PLL_MIPI to CLK_TCON0

On Thu, Apr 27, 2023 at 11:16:07AM +0200, Roman Beranek wrote:
> Assign pll-mipi parent to tcon0's source clock via 'assigned-clocks'.
>
> Signed-off-by: Roman Beranek <[email protected]>

Again, you should be doing it in the driver, not the device tree.

Maxime


Attachments:
(No filename) (265.00 B)
signature.asc (235.00 B)
Download all attachments

2023-04-27 09:44:18

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH v3 4/7] arm64: dts: allwinner: a64: reset pll-video0 rate

Hi,

I'm not sure I understand what you're doing here

On Thu, Apr 27, 2023 at 11:16:08AM +0200, Roman Beranek wrote:
> With pll-mipi as its source clock, the exact rate to which TCON0's data
> clock can be set to is constrained by the current rate of pll-video0.

What in the TCON exactly is constrained by pll-video0 rate?

> Unless changed on a request of another consumer, the rate of pll-video0
> is left as inherited from the bootloader.
>
> The default rate on reset is 297 MHz, a value preferable to what it is
> later set to in u-boot (294 MHz). This happens unintentionally though,
> as u-boot, for the sake of simplicity, rounds the rate requested by DE2
> driver (297 MHz) to 6 MHz steps.
>
> Reset the PLL to its default rate of 297 MHz.
>
> Signed-off-by: Roman Beranek <[email protected]>

If the driver depends on the value being the reset value, then that's
the issue we should fix, either by reading the clock rate and adjusting
to it, or enforcing the rate we expect in the TCON driver.

Maxime


Attachments:
(No filename) (1.01 kB)
signature.asc (235.00 B)
Download all attachments

2023-04-28 06:43:22

by Jernej Škrabec

[permalink] [raw]
Subject: Re: [PATCH v3 3/7] arm64: dts: allwinner: a64: assign PLL_MIPI to CLK_TCON0

Dne četrtek, 27. april 2023 ob 11:27:11 CEST je Maxime Ripard napisal(a):
> On Thu, Apr 27, 2023 at 11:16:07AM +0200, Roman Beranek wrote:
> > Assign pll-mipi parent to tcon0's source clock via 'assigned-clocks'.
> >
> > Signed-off-by: Roman Beranek <[email protected]>
>
> Again, you should be doing it in the driver, not the device tree.

Agreed, fixing this in driver instead of DT is better as it allows kernel to
work with older DTs and still have proper DSI output.

Best regards,
Jernej

>
> Maxime




2023-04-28 06:48:36

by Jernej Škrabec

[permalink] [raw]
Subject: Re: [PATCH v3 4/7] arm64: dts: allwinner: a64: reset pll-video0 rate

Dne četrtek, 27. april 2023 ob 11:16:08 CEST je Roman Beranek napisal(a):
> With pll-mipi as its source clock, the exact rate to which TCON0's data
> clock can be set to is constrained by the current rate of pll-video0.
> Unless changed on a request of another consumer, the rate of pll-video0
> is left as inherited from the bootloader.
>
> The default rate on reset is 297 MHz, a value preferable to what it is
> later set to in u-boot (294 MHz). This happens unintentionally though,
> as u-boot, for the sake of simplicity, rounds the rate requested by DE2
> driver (297 MHz) to 6 MHz steps.
>
> Reset the PLL to its default rate of 297 MHz.

Why would that be preferable? You actually dropped "clk: sunxi-ng: a64:
propagate rate change from pll-mipi" patch which would take care for adjusting
parent rate to correct value.

Best regards,
Jernej

>
> Signed-off-by: Roman Beranek <[email protected]>
> ---
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index
> e6a194db420d..cfc60dce80b0 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -667,6 +667,9 @@ ccu: clock@1c20000 {
> clock-names = "hosc", "losc";
> #clock-cells = <1>;
> #reset-cells = <1>;
> +
> + assigned-clocks = <&ccu CLK_PLL_VIDEO0>;
> + assigned-clock-rates = <297000000>;
> };
>
> pio: pinctrl@1c20800 {




2023-04-28 10:37:04

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v3 1/7] clk: sunxi-ng: a64: export PLL_MIPI

On 27/04/2023 11:16, Roman Beranek wrote:
> 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 +

No, bindings are always separate patches.


Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Best regards,
Krzysztof

2023-04-29 19:49:58

by Frank Oltmanns

[permalink] [raw]
Subject: Re: [PATCH v3 4/7] arm64: dts: allwinner: a64: reset pll-video0 rate

Hi Jernej,

On 2023-04-28 at 08:43:29 +0200, Jernej Škrabec <[email protected]> wrote:
> Dne četrtek, 27. april 2023 ob 11:16:08 CEST je Roman Beranek napisal(a):
>> With pll-mipi as its source clock, the exact rate to which TCON0's data
>> clock can be set to is constrained by the current rate of pll-video0.
>> Unless changed on a request of another consumer, the rate of pll-video0
>> is left as inherited from the bootloader.
>>
>> The default rate on reset is 297 MHz, a value preferable to what it is
>> later set to in u-boot (294 MHz). This happens unintentionally though,
>> as u-boot, for the sake of simplicity, rounds the rate requested by DE2
>> driver (297 MHz) to 6 MHz steps.
>>
>> Reset the PLL to its default rate of 297 MHz.
>
> Why would that be preferable? You actually dropped "clk: sunxi-ng: a64:
> propagate rate change from pll-mipi" patch which would take care for adjusting
> parent rate to correct value.

For me, on the pinephone, it somehow doesn't. Please see here:
https://lore.kernel.org/all/[email protected]/

I haven't figured out yet why that is. But hopefully, I'll find time in
the coming days / weeks to look into that.

Best regards,
Frank

> Best regards,
> Jernej
>
>>
>> Signed-off-by: Roman Beranek <[email protected]>
>> ---
>> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index
>> e6a194db420d..cfc60dce80b0 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> @@ -667,6 +667,9 @@ ccu: clock@1c20000 {
>> clock-names = "hosc", "losc";
>> #clock-cells = <1>;
>> #reset-cells = <1>;
>> +
>> + assigned-clocks = <&ccu CLK_PLL_VIDEO0>;
>> + assigned-clock-rates = <297000000>;
>> };
>>
>> pio: pinctrl@1c20800 {