This series adds support for R40 HDMI pipeline. It is a bit special than other already supported pipelines because it has additional unit called TCON TOP responsible for relationship configuration between mixers, TCONs and HDMI. Additionally, it has additional gates for DSI and TV TCONs, TV encoder clock settings and pin muxing between LCD and TV encoders.
However, it seems that TCON TOP will become a norm, since newer Allwinner SoCs like H6 also have this unit.
I tested different possible configurations: - mixer0 <> TCON-TV0 <> HDMI - mixer0 <> TCON-TV1 <> HDMI - mixer1 <> TCON-TV0 <> HDMI - mixer1 <> TCON-TV1 <> HDMI
Please review.
Best regards, Jernej
Jernej Skrabec (15): clk: sunxi-ng: r40: Add minimal rate for video PLLs clk: sunxi-ng: r40: Allow setting parent rate to display related clocks clk: sunxi-ng: r40: Export video PLLs dt-bindings: display: sunxi-drm: Add TCON TOP description drm/sun4i: Add TCON TOP driver drm/sun4i: tcon: Add support for tcon-top dt-bindings: display: sun4i-drm: Add R40 HDMI pipeline drm/sun4i: DE2 mixer: Add index quirk drm/sun4i: Add support for R40 mixers drm/sun4i: Add support for R40 TV TCONs drm/sun4i: DW HDMI PHY: Add support for second PLL drm/sun4i: Add support for second clock parent to DW HDMI PHY clk driver drm/sun4i: Add support for A64 HDMI PHY ARM: dts: sun8i: r40: Add HDMI pipeline ARM: dts: sun8i: r40: Enable HDMI output on BananaPi M2 Ultra
.../bindings/display/sunxi/sun4i-drm.txt | 36 ++- .../boot/dts/sun8i-r40-bananapi-m2-ultra.dts | 50 ++++ arch/arm/boot/dts/sun8i-r40.dtsi | 166 ++++++++++++ drivers/clk/sunxi-ng/ccu-sun8i-r40.c | 58 ++-- drivers/clk/sunxi-ng/ccu-sun8i-r40.h | 8 +- drivers/gpu/drm/sun4i/Makefile | 3 +- drivers/gpu/drm/sun4i/sun4i_tcon.c | 67 +++++ drivers/gpu/drm/sun4i/sun4i_tcon.h | 8 + drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 8 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 61 ++++- drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++-- drivers/gpu/drm/sun4i/sun8i_mixer.c | 30 +- drivers/gpu/drm/sun4i/sun8i_mixer.h | 2 + drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 256 ++++++++++++++++++ drivers/gpu/drm/sun4i/sun8i_tcon_top.h | 20 ++ include/dt-bindings/clock/sun8i-r40-ccu.h | 4 + include/dt-bindings/clock/sun8i-tcon-top.h | 11 + 17 files changed, 813 insertions(+), 65 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.c create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.h create mode 100644 include/dt-bindings/clock/sun8i-tcon-top.h
According to documentation and experience with other similar SoCs, video PLLs don't work stable if their output frequency is set below 192 MHz.
Because of that, set minimal rate to both R40 video PLLs to 192 MHz.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/clk/sunxi-ng/ccu-sun8i-r40.c | 46 +++++++++++++++------------- 1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c index 933f2e68f42a..c16a62a7bdbd 100644 --- a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c +++ b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c @@ -65,17 +65,18 @@ static SUNXI_CCU_NM_WITH_GATE_LOCK(pll_audio_base_clk, "pll-audio-base", CLK_SET_RATE_UNGATE);
/* TODO: The result of N/M is required to be in [8, 25] range. */ -static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_video0_clk, "pll-video0", - "osc24M", 0x0010, - 8, 7, /* N */ - 0, 4, /* M */ - BIT(24), /* frac enable */ - BIT(25), /* frac select */ - 270000000, /* frac rate 0 */ - 297000000, /* frac rate 1 */ - BIT(31), /* gate */ - BIT(28), /* lock */ - CLK_SET_RATE_UNGATE); +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN(pll_video0_clk, "pll-video0", + "osc24M", 0x0010, + 192000000, /* Minimum rate */ + 8, 7, /* N */ + 0, 4, /* M */ + BIT(24), /* frac enable */ + BIT(25), /* frac select */ + 270000000, /* frac rate 0 */ + 297000000, /* frac rate 1 */ + BIT(31), /* gate */ + BIT(28), /* lock */ + CLK_SET_RATE_UNGATE);
/* TODO: The result of N/M is required to be in [8, 25] range. */ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_ve_clk, "pll-ve", @@ -151,17 +152,18 @@ static struct ccu_nk pll_periph1_clk = { };
/* TODO: The result of N/M is required to be in [8, 25] range. */ -static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_video1_clk, "pll-video1", - "osc24M", 0x030, - 8, 7, /* N */ - 0, 4, /* M */ - BIT(24), /* frac enable */ - BIT(25), /* frac select */ - 270000000, /* frac rate 0 */ - 297000000, /* frac rate 1 */ - BIT(31), /* gate */ - BIT(28), /* lock */ - CLK_SET_RATE_UNGATE); +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN(pll_video1_clk, "pll-video1", + "osc24M", 0x030, + 192000000, /* Minimum rate */ + 8, 7, /* N */ + 0, 4, /* M */ + BIT(24), /* frac enable */ + BIT(25), /* frac select */ + 270000000, /* frac rate 0 */ + 297000000, /* frac rate 1 */ + BIT(31), /* gate */ + BIT(28), /* lock */ + CLK_SET_RATE_UNGATE);
static struct ccu_nkm pll_sata_clk = { .enable = BIT(31),
Display related peripherals need precise clocks to operate correctly.
Allow DE2, TCONs and HDMI to set parent clock.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/clk/sunxi-ng/ccu-sun8i-r40.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c index c16a62a7bdbd..fa5317719684 100644 --- a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c +++ b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c @@ -655,7 +655,8 @@ static SUNXI_CCU_GATE(dram_deinterlace_clk, "dram-deinterlace", "dram",
static const char * const de_parents[] = { "pll-periph0-2x", "pll-de" }; static SUNXI_CCU_M_WITH_MUX_GATE(de_clk, "de", de_parents, - 0x104, 0, 4, 24, 3, BIT(31), 0); + 0x104, 0, 4, 24, 3, BIT(31), + CLK_SET_RATE_PARENT); static SUNXI_CCU_M_WITH_MUX_GATE(mp_clk, "mp", de_parents, 0x108, 0, 4, 24, 3, BIT(31), 0);
@@ -667,9 +668,11 @@ static SUNXI_CCU_MUX_WITH_GATE(tcon_lcd0_clk, "tcon-lcd0", tcon_parents, static SUNXI_CCU_MUX_WITH_GATE(tcon_lcd1_clk, "tcon-lcd1", tcon_parents, 0x114, 24, 3, BIT(31), CLK_SET_RATE_PARENT); static SUNXI_CCU_M_WITH_MUX_GATE(tcon_tv0_clk, "tcon-tv0", tcon_parents, - 0x118, 0, 4, 24, 3, BIT(31), 0); + 0x118, 0, 4, 24, 3, BIT(31), + CLK_SET_RATE_PARENT); static SUNXI_CCU_M_WITH_MUX_GATE(tcon_tv1_clk, "tcon-tv1", tcon_parents, - 0x11c, 0, 4, 24, 3, BIT(31), 0); + 0x11c, 0, 4, 24, 3, BIT(31), + CLK_SET_RATE_PARENT);
static const char * const deinterlace_parents[] = { "pll-periph0", "pll-periph1" }; @@ -699,7 +702,8 @@ static SUNXI_CCU_GATE(avs_clk, "avs", "osc24M",
static const char * const hdmi_parents[] = { "pll-video0", "pll-video1" }; static SUNXI_CCU_M_WITH_MUX_GATE(hdmi_clk, "hdmi", hdmi_parents, - 0x150, 0, 4, 24, 2, BIT(31), 0); + 0x150, 0, 4, 24, 2, BIT(31), + CLK_SET_RATE_PARENT);
static SUNXI_CCU_GATE(hdmi_slow_clk, "hdmi-slow", "osc24M", 0x154, BIT(31), 0);
Video PLLs need to be referenced in R40 DT as possible HDMI PHY parent.
Export them.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/clk/sunxi-ng/ccu-sun8i-r40.h | 8 ++++++-- include/dt-bindings/clock/sun8i-r40-ccu.h | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r40.h b/drivers/clk/sunxi-ng/ccu-sun8i-r40.h index 0db8e1e97af8..db2a1243f9ff 100644 --- a/drivers/clk/sunxi-ng/ccu-sun8i-r40.h +++ b/drivers/clk/sunxi-ng/ccu-sun8i-r40.h @@ -25,7 +25,9 @@ #define CLK_PLL_AUDIO_2X 4 #define CLK_PLL_AUDIO_4X 5 #define CLK_PLL_AUDIO_8X 6 -#define CLK_PLL_VIDEO0 7 + +/* PLL_VIDEO0 is exported */ + #define CLK_PLL_VIDEO0_2X 8 #define CLK_PLL_VE 9 #define CLK_PLL_DDR0 10 @@ -34,7 +36,9 @@ #define CLK_PLL_PERIPH0_2X 13 #define CLK_PLL_PERIPH1 14 #define CLK_PLL_PERIPH1_2X 15 -#define CLK_PLL_VIDEO1 16 + +/* PLL_VIDEO1 is exported */ + #define CLK_PLL_VIDEO1_2X 17 #define CLK_PLL_SATA 18 #define CLK_PLL_SATA_OUT 19 diff --git a/include/dt-bindings/clock/sun8i-r40-ccu.h b/include/dt-bindings/clock/sun8i-r40-ccu.h index 4fa5f69fc297..f9e15a235626 100644 --- a/include/dt-bindings/clock/sun8i-r40-ccu.h +++ b/include/dt-bindings/clock/sun8i-r40-ccu.h @@ -43,6 +43,10 @@ #ifndef _DT_BINDINGS_CLK_SUN8I_R40_H_ #define _DT_BINDINGS_CLK_SUN8I_R40_H_
+#define CLK_PLL_VIDEO0 7 + +#define CLK_PLL_VIDEO1 16 + #define CLK_CPU 24
#define CLK_BUS_MIPI_DSI 29
On Sat, May 19, 2018 at 08:31:15PM +0200, Jernej Skrabec wrote:
Video PLLs need to be referenced in R40 DT as possible HDMI PHY parent.
Export them.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/clk/sunxi-ng/ccu-sun8i-r40.h | 8 ++++++-- include/dt-bindings/clock/sun8i-r40-ccu.h | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-)
Reviewed-by: Rob Herring robh@kernel.org
TCON TOP main purpose is to configure whole display pipeline. It determines relationships between mixers and TCONs, selects source TCON for HDMI, muxes LCD and TV encoder GPIO output, selects TV encoder clock source and contains additional TV TCON and DSI gates.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- .../bindings/display/sunxi/sun4i-drm.txt | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt index 3346c1e2a7a0..a099957ab62a 100644 --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt @@ -187,6 +187,26 @@ And on the A23, A31, A31s and A33, you need one more clock line: - 'lvds-alt': An alternative clock source, separate from the TCON channel 0 clock, that can be used to drive the LVDS clock
+TCON TOP +-------- + +TCON TOPs main purpose is to configure whole display pipeline. It determines +relationships between mixers and TCONs, selects source TCON for HDMI, muxes +LCD and TV encoder GPIO output, selects TV encoder clock source and contains +additional TV TCON and DSI gates. + +Required properties: + - compatible: value must be one of: + * allwinner,sun8i-r40-tcon-top + - reg: base address and size of the memory-mapped region. + - clocks: phandle to the clocks feeding the TCON TOP + * bus: TCON TOP interface clock + - clock-names: clock name mentioned above + - resets: phandle to the reset line driving the DRC + * rst: TCON TOP reset line + - reset-names: reset name mentioned above + - #clock-cells : must contain 1 + DRC ---
Hi,
On Sat, May 19, 2018 at 08:31:16PM +0200, Jernej Skrabec wrote:
TCON TOP main purpose is to configure whole display pipeline. It determines relationships between mixers and TCONs, selects source TCON for HDMI, muxes LCD and TV encoder GPIO output,
I'm not sure you mean GPIO here, but rather pin?
selects TV encoder clock source and contains additional TV TCON and DSI gates.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
.../bindings/display/sunxi/sun4i-drm.txt | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt index 3346c1e2a7a0..a099957ab62a 100644 --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt @@ -187,6 +187,26 @@ And on the A23, A31, A31s and A33, you need one more clock line: - 'lvds-alt': An alternative clock source, separate from the TCON channel 0 clock, that can be used to drive the LVDS clock
+TCON TOP +--------
+TCON TOPs main purpose is to configure whole display pipeline. It determines +relationships between mixers and TCONs, selects source TCON for HDMI, muxes +LCD and TV encoder GPIO output, selects TV encoder clock source and contains +additional TV TCON and DSI gates.
+Required properties:
- compatible: value must be one of:
- allwinner,sun8i-r40-tcon-top
- reg: base address and size of the memory-mapped region.
- clocks: phandle to the clocks feeding the TCON TOP
- bus: TCON TOP interface clock
- clock-names: clock name mentioned above
- resets: phandle to the reset line driving the DRC
- rst: TCON TOP reset line
- reset-names: reset name mentioned above
- #clock-cells : must contain 1
I guess you should better describe the OF-graph endpoints, and the clocks output. Just using the binding additions here doesn't allow to get a clear idea of how the DT should look like.
Maxime
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:01:47 CEST je Maxime Ripard napisal(a):
Hi,
On Sat, May 19, 2018 at 08:31:16PM +0200, Jernej Skrabec wrote:
TCON TOP main purpose is to configure whole display pipeline. It determines relationships between mixers and TCONs, selects source TCON for HDMI, muxes LCD and TV encoder GPIO output,
I'm not sure you mean GPIO here, but rather pin?
Right, I'll fix that.
selects TV encoder clock source and contains additional TV TCON and DSI gates.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
.../bindings/display/sunxi/sun4i-drm.txt | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt index 3346c1e2a7a0..a099957ab62a 100644 --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
@@ -187,6 +187,26 @@ And on the A23, A31, A31s and A33, you need one more
clock line:
- 'lvds-alt': An alternative clock source, separate from the TCON channel 0 clock, that can be used to drive the LVDS clock+TCON TOP +--------
+TCON TOPs main purpose is to configure whole display pipeline. It determines +relationships between mixers and TCONs, selects source TCON for HDMI, muxes +LCD and TV encoder GPIO output, selects TV encoder clock source and contains +additional TV TCON and DSI gates.
+Required properties:
- compatible: value must be one of:
- allwinner,sun8i-r40-tcon-top
- reg: base address and size of the memory-mapped region.
- clocks: phandle to the clocks feeding the TCON TOP
- bus: TCON TOP interface clock
- clock-names: clock name mentioned above
- resets: phandle to the reset line driving the DRC
- rst: TCON TOP reset line
- reset-names: reset name mentioned above
- #clock-cells : must contain 1
I guess you should better describe the OF-graph endpoints, and the clocks output. Just using the binding additions here doesn't allow to get a clear idea of how the DT should look like.
With my idea of implementation, OF graph is the same as before, so I didn't mention anything about it.
My idea of dependencies (you should view it in fixed width font): TCON-TOP ^ | mixer <-> TCON <-> HDMI
I'll explain my design decision as response to other mail.
Best regards, Jernej
As already described in DT binding, TCON TOP is responsible for configuring display pipeline. In this initial driver focus is on HDMI pipeline, so TVE and LCD configuration is not implemented.
Implemented features: - HDMI source selection - clock driver (TCON and DSI gating) - connecting mixers and TCONS
Something similar also existed in previous SoCs, except that it was part of first TCON.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/sun4i/Makefile | 3 +- drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 256 +++++++++++++++++++++ drivers/gpu/drm/sun4i/sun8i_tcon_top.h | 20 ++ include/dt-bindings/clock/sun8i-tcon-top.h | 11 + 4 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.c create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.h create mode 100644 include/dt-bindings/clock/sun8i-tcon-top.h
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index 2589f4acd5ae..09fbfd6304ba 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -16,7 +16,8 @@ sun8i-drm-hdmi-y += sun8i_hdmi_phy_clk.o
sun8i-mixer-y += sun8i_mixer.o sun8i_ui_layer.o \ sun8i_vi_layer.o sun8i_ui_scaler.o \ - sun8i_vi_scaler.o sun8i_csc.o + sun8i_vi_scaler.o sun8i_csc.o \ + sun8i_tcon_top.o
sun4i-tcon-y += sun4i_crtc.o sun4i-tcon-y += sun4i_dotclock.o diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c new file mode 100644 index 000000000000..075a356a6dfa --- /dev/null +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright (c) 2018 Jernej Skrabec jernej.skrabec@siol.net */ + +#include <drm/drmP.h> + +#include <dt-bindings/clock/sun8i-tcon-top.h> + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/spinlock.h> + +#include "sun8i_tcon_top.h" + +#define TCON_TOP_PORT_SEL_REG 0x1C +#define TCON_TOP_PORT_DE0_MSK GENMASK(1, 0) +#define TCON_TOP_PORT_DE1_MSK GENMASK(5, 4) +#define TCON_TOP_PORT_TCON_LCD0 0 +#define TCON_TOP_PORT_TCON_LCD1 1 +#define TCON_TOP_PORT_TCON_TV0 2 +#define TCON_TOP_PORT_TCON_TV1 3 + +#define TCON_TOP_GATE_SRC_REG 0x20 +#define TCON_TOP_HDMI_SRC_MSK GENMASK(29, 28) +#define TCON_TOP_HDMI_SRC_NONE 0 +#define TCON_TOP_HDMI_SRC_TCON_TV0 1 +#define TCON_TOP_HDMI_SRC_TCON_TV1 2 +#define TCON_TOP_TCON_TV1_GATE 24 +#define TCON_TOP_TCON_TV0_GATE 20 +#define TCON_TOP_TCON_DSI_GATE 16 + +#define CLK_NUM 3 + +struct sun8i_tcon_top { + struct clk *bus; + void __iomem *regs; + struct reset_control *rst; + + /* + * spinlock is used for locking access to registers from different + * places - tcon driver and clk subsystem. + */ + spinlock_t reg_lock; +}; + +struct sun8i_tcon_top_gate { + const char *name; + u8 bit; + int index; +}; + +static const struct sun8i_tcon_top_gate gates[] = { + {"bus-tcon-top-dsi", TCON_TOP_TCON_DSI_GATE, CLK_BUS_TCON_TOP_DSI}, + {"bus-tcon-top-tv0", TCON_TOP_TCON_TV0_GATE, CLK_BUS_TCON_TOP_TV0}, + {"bus-tcon-top-tv1", TCON_TOP_TCON_TV1_GATE, CLK_BUS_TCON_TOP_TV1}, +}; + +void sun8i_tcon_top_set_hdmi_src(struct sun8i_tcon_top *tcon_top, int tcon) +{ + unsigned long flags; + u32 val; + + if (tcon > 1) { + DRM_ERROR("TCON index is too high!\n"); + return; + } + + spin_lock_irqsave(&tcon_top->reg_lock, flags); + + val = readl(tcon_top->regs + TCON_TOP_GATE_SRC_REG); + val &= ~TCON_TOP_HDMI_SRC_MSK; + val |= FIELD_PREP(TCON_TOP_HDMI_SRC_MSK, + TCON_TOP_HDMI_SRC_TCON_TV0 + tcon); + writel(val, tcon_top->regs + TCON_TOP_GATE_SRC_REG); + + spin_unlock_irqrestore(&tcon_top->reg_lock, flags); +} + +void sun8i_tcon_top_de_config(struct sun8i_tcon_top *tcon_top, + int mixer, enum tcon_type tcon_type, int tcon) +{ + unsigned long flags; + u32 val, reg; + + if (mixer > 1) { + DRM_ERROR("Mixer index is too high!\n"); + return; + } + + if (tcon > 1) { + DRM_ERROR("TCON index is too high!\n"); + return; + } + + switch (tcon_type) { + case tcon_type_lcd: + val = TCON_TOP_PORT_TCON_LCD0 + tcon; + break; + case tcon_type_tv: + val = TCON_TOP_PORT_TCON_TV0 + tcon; + break; + default: + DRM_ERROR("Invalid TCON type!\n"); + return; + } + + spin_lock_irqsave(&tcon_top->reg_lock, flags); + + reg = readl(tcon_top->regs + TCON_TOP_PORT_SEL_REG); + if (mixer == 0) { + reg &= ~TCON_TOP_PORT_DE0_MSK; + reg |= FIELD_PREP(TCON_TOP_PORT_DE0_MSK, val); + } else { + reg &= ~TCON_TOP_PORT_DE1_MSK; + reg |= FIELD_PREP(TCON_TOP_PORT_DE1_MSK, val); + } + writel(reg, tcon_top->regs + TCON_TOP_PORT_SEL_REG); + + spin_unlock_irqrestore(&tcon_top->reg_lock, flags); +} + +static int sun8i_tcon_top_probe(struct platform_device *pdev) +{ + struct clk_hw_onecell_data *clk_data; + struct sun8i_tcon_top *tcon_top; + struct device *dev = &pdev->dev; + struct resource *res; + int ret, i; + + tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL); + if (!tcon_top) + return -ENOMEM; + + clk_data = devm_kzalloc(&pdev->dev, sizeof(*clk_data) + + sizeof(*clk_data->hws) * CLK_NUM, + GFP_KERNEL); + if (!clk_data) + return -ENOMEM; + + spin_lock_init(&tcon_top->reg_lock); + + tcon_top->rst = devm_reset_control_get(dev, "rst"); + if (IS_ERR(tcon_top->rst)) { + dev_err(dev, "Couldn't get our reset line\n"); + return PTR_ERR(tcon_top->rst); + } + + tcon_top->bus = devm_clk_get(dev, "bus"); + if (IS_ERR(tcon_top->bus)) { + dev_err(dev, "Couldn't get the bus clock\n"); + return PTR_ERR(tcon_top->bus); + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + tcon_top->regs = devm_ioremap_resource(dev, res); + if (IS_ERR(tcon_top->regs)) + return PTR_ERR(tcon_top->regs); + + ret = reset_control_deassert(tcon_top->rst); + if (ret) { + dev_err(dev, "Could not deassert ctrl reset control\n"); + return ret; + } + + ret = clk_prepare_enable(tcon_top->bus); + if (ret) { + dev_err(dev, "Could not enable bus clock\n"); + goto err_assert_reset; + } + + /* + * Default register values might have some reserved bits set, which + * prevents TCON TOP from working properly. Set them to 0 here. + */ + writel(0, tcon_top->regs + TCON_TOP_PORT_SEL_REG); + writel(0, tcon_top->regs + TCON_TOP_GATE_SRC_REG); + + for (i = 0; i < CLK_NUM; i++) { + const char *parent_name = "bus-tcon-top"; + struct clk_init_data init; + struct clk_gate *gate; + + gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL); + if (!gate) { + ret = -ENOMEM; + goto err_disable_clock; + } + + init.name = gates[i].name; + init.ops = &clk_gate_ops; + init.flags = CLK_IS_BASIC; + init.parent_names = &parent_name; + init.num_parents = 1; + + gate->reg = tcon_top->regs + TCON_TOP_GATE_SRC_REG; + gate->bit_idx = gates[i].bit; + gate->lock = &tcon_top->reg_lock; + gate->hw.init = &init; + + ret = devm_clk_hw_register(dev, &gate->hw); + if (ret) + goto err_disable_clock; + + clk_data->hws[gates[i].index] = &gate->hw; + } + + clk_data->num = CLK_NUM; + + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); + if (ret) + goto err_disable_clock; + + platform_set_drvdata(pdev, tcon_top); + + return 0; + +err_disable_clock: + clk_disable_unprepare(tcon_top->bus); +err_assert_reset: + reset_control_assert(tcon_top->rst); + + return ret; +} + +static int sun8i_tcon_top_remove(struct platform_device *pdev) +{ + struct sun8i_tcon_top *tcon_top = platform_get_drvdata(pdev); + + clk_disable_unprepare(tcon_top->bus); + reset_control_assert(tcon_top->rst); + + return 0; +} + +const struct of_device_id sun8i_tcon_top_of_table[] = { + { .compatible = "allwinner,sun8i-r40-tcon-top" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, sun8i_tcon_top_of_table); + +static struct platform_driver sun8i_tcon_top_platform_driver = { + .probe = sun8i_tcon_top_probe, + .remove = sun8i_tcon_top_remove, + .driver = { + .name = "sun8i-tcon-top", + .of_match_table = sun8i_tcon_top_of_table, + }, +}; +module_platform_driver(sun8i_tcon_top_platform_driver); + +MODULE_AUTHOR("Jernej Skrabec jernej.skrabec@siol.net"); +MODULE_DESCRIPTION("Allwinner R40 TCON TOP driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.h b/drivers/gpu/drm/sun4i/sun8i_tcon_top.h new file mode 100644 index 000000000000..19126e07d2a6 --- /dev/null +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* Copyright (c) 2018 Jernej Skrabec jernej.skrabec@siol.net */ + +#ifndef _SUN8I_TCON_TOP_H_ +#define _SUN8I_TCON_TOP_H_ + +#include <linux/device.h> + +struct sun8i_tcon_top; + +enum tcon_type { + tcon_type_lcd, + tcon_type_tv, +}; + +void sun8i_tcon_top_set_hdmi_src(struct sun8i_tcon_top *tcon_top, int tcon); +void sun8i_tcon_top_de_config(struct sun8i_tcon_top *tcon_top, + int mixer, enum tcon_type tcon_type, int tcon); + +#endif /* _SUN8I_TCON_TOP_H_ */ diff --git a/include/dt-bindings/clock/sun8i-tcon-top.h b/include/dt-bindings/clock/sun8i-tcon-top.h new file mode 100644 index 000000000000..c05e92770402 --- /dev/null +++ b/include/dt-bindings/clock/sun8i-tcon-top.h @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ +/* Copyright (C) 2018 Jernej Skrabec jernej.skrabec@siol.net */ + +#ifndef _DT_BINDINGS_CLOCK_SUN8I_TCON_TOP_H_ +#define _DT_BINDINGS_CLOCK_SUN8I_TCON_TOP_H_ + +#define CLK_BUS_TCON_TOP_DSI 0 +#define CLK_BUS_TCON_TOP_TV0 1 +#define CLK_BUS_TCON_TOP_TV1 2 + +#endif /* _DT_BINDINGS_CLOCK_SUN8I_TCON_TOP_H_ */
On Sat, May 19, 2018 at 08:31:17PM +0200, Jernej Skrabec wrote:
As already described in DT binding, TCON TOP is responsible for configuring display pipeline. In this initial driver focus is on HDMI pipeline, so TVE and LCD configuration is not implemented.
Implemented features:
- HDMI source selection
- clock driver (TCON and DSI gating)
- connecting mixers and TCONS
Something similar also existed in previous SoCs, except that it was part of first TCON.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/Makefile | 3 +- drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 256 +++++++++++++++++++++ drivers/gpu/drm/sun4i/sun8i_tcon_top.h | 20 ++ include/dt-bindings/clock/sun8i-tcon-top.h | 11 + 4 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.c create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.h create mode 100644 include/dt-bindings/clock/sun8i-tcon-top.h
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index 2589f4acd5ae..09fbfd6304ba 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -16,7 +16,8 @@ sun8i-drm-hdmi-y += sun8i_hdmi_phy_clk.o
sun8i-mixer-y += sun8i_mixer.o sun8i_ui_layer.o \ sun8i_vi_layer.o sun8i_ui_scaler.o \
sun8i_vi_scaler.o sun8i_csc.o
sun8i_vi_scaler.o sun8i_csc.o \sun8i_tcon_top.osun4i-tcon-y += sun4i_crtc.o sun4i-tcon-y += sun4i_dotclock.o diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c new file mode 100644 index 000000000000..075a356a6dfa --- /dev/null +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright (c) 2018 Jernej Skrabec jernej.skrabec@siol.net */
+#include <drm/drmP.h>
+#include <dt-bindings/clock/sun8i-tcon-top.h>
+#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/spinlock.h>
+#include "sun8i_tcon_top.h"
+#define TCON_TOP_PORT_SEL_REG 0x1C +#define TCON_TOP_PORT_DE0_MSK GENMASK(1, 0) +#define TCON_TOP_PORT_DE1_MSK GENMASK(5, 4) +#define TCON_TOP_PORT_TCON_LCD0 0 +#define TCON_TOP_PORT_TCON_LCD1 1 +#define TCON_TOP_PORT_TCON_TV0 2 +#define TCON_TOP_PORT_TCON_TV1 3
+#define TCON_TOP_GATE_SRC_REG 0x20 +#define TCON_TOP_HDMI_SRC_MSK GENMASK(29, 28) +#define TCON_TOP_HDMI_SRC_NONE 0 +#define TCON_TOP_HDMI_SRC_TCON_TV0 1 +#define TCON_TOP_HDMI_SRC_TCON_TV1 2 +#define TCON_TOP_TCON_TV1_GATE 24 +#define TCON_TOP_TCON_TV0_GATE 20 +#define TCON_TOP_TCON_DSI_GATE 16
+#define CLK_NUM 3
+struct sun8i_tcon_top {
- struct clk *bus;
- void __iomem *regs;
- struct reset_control *rst;
- /*
* spinlock is used for locking access to registers from different* places - tcon driver and clk subsystem.*/- spinlock_t reg_lock;
+};
+struct sun8i_tcon_top_gate {
- const char *name;
- u8 bit;
- int index;
+};
+static const struct sun8i_tcon_top_gate gates[] = {
- {"bus-tcon-top-dsi", TCON_TOP_TCON_DSI_GATE, CLK_BUS_TCON_TOP_DSI},
- {"bus-tcon-top-tv0", TCON_TOP_TCON_TV0_GATE, CLK_BUS_TCON_TOP_TV0},
- {"bus-tcon-top-tv1", TCON_TOP_TCON_TV1_GATE, CLK_BUS_TCON_TOP_TV1},
+};
+void sun8i_tcon_top_set_hdmi_src(struct sun8i_tcon_top *tcon_top, int tcon) +{
- unsigned long flags;
- u32 val;
- if (tcon > 1) {
DRM_ERROR("TCON index is too high!\n");return;- }
- spin_lock_irqsave(&tcon_top->reg_lock, flags);
- val = readl(tcon_top->regs + TCON_TOP_GATE_SRC_REG);
- val &= ~TCON_TOP_HDMI_SRC_MSK;
- val |= FIELD_PREP(TCON_TOP_HDMI_SRC_MSK,
TCON_TOP_HDMI_SRC_TCON_TV0 + tcon);- writel(val, tcon_top->regs + TCON_TOP_GATE_SRC_REG);
- spin_unlock_irqrestore(&tcon_top->reg_lock, flags);
+}
+void sun8i_tcon_top_de_config(struct sun8i_tcon_top *tcon_top,
int mixer, enum tcon_type tcon_type, int tcon)+{
- unsigned long flags;
- u32 val, reg;
- if (mixer > 1) {
DRM_ERROR("Mixer index is too high!\n");return;- }
- if (tcon > 1) {
DRM_ERROR("TCON index is too high!\n");return;- }
- switch (tcon_type) {
- case tcon_type_lcd:
val = TCON_TOP_PORT_TCON_LCD0 + tcon;break;- case tcon_type_tv:
val = TCON_TOP_PORT_TCON_TV0 + tcon;break;- default:
DRM_ERROR("Invalid TCON type!\n");return;- }
- spin_lock_irqsave(&tcon_top->reg_lock, flags);
- reg = readl(tcon_top->regs + TCON_TOP_PORT_SEL_REG);
- if (mixer == 0) {
reg &= ~TCON_TOP_PORT_DE0_MSK;reg |= FIELD_PREP(TCON_TOP_PORT_DE0_MSK, val);- } else {
reg &= ~TCON_TOP_PORT_DE1_MSK;reg |= FIELD_PREP(TCON_TOP_PORT_DE1_MSK, val);- }
- writel(reg, tcon_top->regs + TCON_TOP_PORT_SEL_REG);
- spin_unlock_irqrestore(&tcon_top->reg_lock, flags);
+}
+static int sun8i_tcon_top_probe(struct platform_device *pdev) +{
- struct clk_hw_onecell_data *clk_data;
- struct sun8i_tcon_top *tcon_top;
- struct device *dev = &pdev->dev;
- struct resource *res;
- int ret, i;
- tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL);
- if (!tcon_top)
return -ENOMEM;- clk_data = devm_kzalloc(&pdev->dev, sizeof(*clk_data) +
sizeof(*clk_data->hws) * CLK_NUM,GFP_KERNEL);- if (!clk_data)
return -ENOMEM;- spin_lock_init(&tcon_top->reg_lock);
- tcon_top->rst = devm_reset_control_get(dev, "rst");
- if (IS_ERR(tcon_top->rst)) {
dev_err(dev, "Couldn't get our reset line\n");return PTR_ERR(tcon_top->rst);- }
- tcon_top->bus = devm_clk_get(dev, "bus");
- if (IS_ERR(tcon_top->bus)) {
dev_err(dev, "Couldn't get the bus clock\n");return PTR_ERR(tcon_top->bus);- }
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- tcon_top->regs = devm_ioremap_resource(dev, res);
- if (IS_ERR(tcon_top->regs))
return PTR_ERR(tcon_top->regs);- ret = reset_control_deassert(tcon_top->rst);
- if (ret) {
dev_err(dev, "Could not deassert ctrl reset control\n");return ret;- }
- ret = clk_prepare_enable(tcon_top->bus);
- if (ret) {
dev_err(dev, "Could not enable bus clock\n");goto err_assert_reset;- }
- /*
* Default register values might have some reserved bits set, which* prevents TCON TOP from working properly. Set them to 0 here.*/- writel(0, tcon_top->regs + TCON_TOP_PORT_SEL_REG);
- writel(0, tcon_top->regs + TCON_TOP_GATE_SRC_REG);
- for (i = 0; i < CLK_NUM; i++) {
const char *parent_name = "bus-tcon-top";
I guess retrieving the parent's clock name at runtime would be more flexible.
struct clk_init_data init;struct clk_gate *gate;gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);if (!gate) {ret = -ENOMEM;goto err_disable_clock;}init.name = gates[i].name;init.ops = &clk_gate_ops;init.flags = CLK_IS_BASIC;init.parent_names = &parent_name;init.num_parents = 1;gate->reg = tcon_top->regs + TCON_TOP_GATE_SRC_REG;gate->bit_idx = gates[i].bit;gate->lock = &tcon_top->reg_lock;gate->hw.init = &init;ret = devm_clk_hw_register(dev, &gate->hw);if (ret)goto err_disable_clock;
Isn't it what clk_hw_register_gate is doing?
clk_data->hws[gates[i].index] = &gate->hw;- }
- clk_data->num = CLK_NUM;
- ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
- if (ret)
goto err_disable_clock;- platform_set_drvdata(pdev, tcon_top);
- return 0;
+err_disable_clock:
- clk_disable_unprepare(tcon_top->bus);
+err_assert_reset:
- reset_control_assert(tcon_top->rst);
- return ret;
+}
+static int sun8i_tcon_top_remove(struct platform_device *pdev) +{
- struct sun8i_tcon_top *tcon_top = platform_get_drvdata(pdev);
- clk_disable_unprepare(tcon_top->bus);
- reset_control_assert(tcon_top->rst);
- return 0;
+}
+const struct of_device_id sun8i_tcon_top_of_table[] = {
- { .compatible = "allwinner,sun8i-r40-tcon-top" },
- { /* sentinel */ }
+}; +MODULE_DEVICE_TABLE(of, sun8i_tcon_top_of_table);
+static struct platform_driver sun8i_tcon_top_platform_driver = {
- .probe = sun8i_tcon_top_probe,
- .remove = sun8i_tcon_top_remove,
- .driver = {
.name = "sun8i-tcon-top",.of_match_table = sun8i_tcon_top_of_table,- },
+}; +module_platform_driver(sun8i_tcon_top_platform_driver);
+MODULE_AUTHOR("Jernej Skrabec jernej.skrabec@siol.net"); +MODULE_DESCRIPTION("Allwinner R40 TCON TOP driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.h b/drivers/gpu/drm/sun4i/sun8i_tcon_top.h new file mode 100644 index 000000000000..19126e07d2a6 --- /dev/null +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* Copyright (c) 2018 Jernej Skrabec jernej.skrabec@siol.net */
+#ifndef _SUN8I_TCON_TOP_H_ +#define _SUN8I_TCON_TOP_H_
+#include <linux/device.h>
+struct sun8i_tcon_top;
+enum tcon_type {
- tcon_type_lcd,
- tcon_type_tv,
The usual practice is to have the enum values upper-case.
Thanks! Maxime
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:05:17 CEST je Maxime Ripard napisal(a):
On Sat, May 19, 2018 at 08:31:17PM +0200, Jernej Skrabec wrote:
As already described in DT binding, TCON TOP is responsible for configuring display pipeline. In this initial driver focus is on HDMI pipeline, so TVE and LCD configuration is not implemented.
Implemented features:
- HDMI source selection
- clock driver (TCON and DSI gating)
- connecting mixers and TCONS
Something similar also existed in previous SoCs, except that it was part of first TCON.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/Makefile | 3 +- drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 256 +++++++++++++++++++++ drivers/gpu/drm/sun4i/sun8i_tcon_top.h | 20 ++ include/dt-bindings/clock/sun8i-tcon-top.h | 11 + 4 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.c create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.h create mode 100644 include/dt-bindings/clock/sun8i-tcon-top.h
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index 2589f4acd5ae..09fbfd6304ba 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -16,7 +16,8 @@ sun8i-drm-hdmi-y += sun8i_hdmi_phy_clk.o
sun8i-mixer-y += sun8i_mixer.o sun8i_ui_layer.o \
sun8i_vi_layer.o sun8i_ui_scaler.o \
sun8i_vi_scaler.o sun8i_csc.o
sun8i_vi_scaler.o sun8i_csc.o \sun8i_tcon_top.osun4i-tcon-y += sun4i_crtc.o sun4i-tcon-y += sun4i_dotclock.o
diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c new file mode 100644 index 000000000000..075a356a6dfa --- /dev/null +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright (c) 2018 Jernej Skrabec jernej.skrabec@siol.net */
+#include <drm/drmP.h>
+#include <dt-bindings/clock/sun8i-tcon-top.h>
+#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/spinlock.h>
+#include "sun8i_tcon_top.h"
+#define TCON_TOP_PORT_SEL_REG 0x1C +#define TCON_TOP_PORT_DE0_MSK GENMASK(1, 0) +#define TCON_TOP_PORT_DE1_MSK GENMASK(5, 4) +#define TCON_TOP_PORT_TCON_LCD0 0 +#define TCON_TOP_PORT_TCON_LCD1 1 +#define TCON_TOP_PORT_TCON_TV0 2 +#define TCON_TOP_PORT_TCON_TV1 3
+#define TCON_TOP_GATE_SRC_REG 0x20 +#define TCON_TOP_HDMI_SRC_MSK GENMASK(29, 28) +#define TCON_TOP_HDMI_SRC_NONE 0 +#define TCON_TOP_HDMI_SRC_TCON_TV0 1 +#define TCON_TOP_HDMI_SRC_TCON_TV1 2 +#define TCON_TOP_TCON_TV1_GATE 24 +#define TCON_TOP_TCON_TV0_GATE 20 +#define TCON_TOP_TCON_DSI_GATE 16
+#define CLK_NUM 3
+struct sun8i_tcon_top {
- struct clk *bus;
- void __iomem *regs;
- struct reset_control *rst;
- /*
* spinlock is used for locking access to registers from different* places - tcon driver and clk subsystem.*/- spinlock_t reg_lock;
+};
+struct sun8i_tcon_top_gate {
- const char *name;
- u8 bit;
- int index;
+};
+static const struct sun8i_tcon_top_gate gates[] = {
- {"bus-tcon-top-dsi", TCON_TOP_TCON_DSI_GATE, CLK_BUS_TCON_TOP_DSI},
- {"bus-tcon-top-tv0", TCON_TOP_TCON_TV0_GATE, CLK_BUS_TCON_TOP_TV0},
- {"bus-tcon-top-tv1", TCON_TOP_TCON_TV1_GATE, CLK_BUS_TCON_TOP_TV1},
+};
+void sun8i_tcon_top_set_hdmi_src(struct sun8i_tcon_top *tcon_top, int tcon) +{
- unsigned long flags;
- u32 val;
- if (tcon > 1) {
DRM_ERROR("TCON index is too high!\n");return;- }
- spin_lock_irqsave(&tcon_top->reg_lock, flags);
- val = readl(tcon_top->regs + TCON_TOP_GATE_SRC_REG);
- val &= ~TCON_TOP_HDMI_SRC_MSK;
- val |= FIELD_PREP(TCON_TOP_HDMI_SRC_MSK,
TCON_TOP_HDMI_SRC_TCON_TV0 + tcon);- writel(val, tcon_top->regs + TCON_TOP_GATE_SRC_REG);
- spin_unlock_irqrestore(&tcon_top->reg_lock, flags);
+}
+void sun8i_tcon_top_de_config(struct sun8i_tcon_top *tcon_top,
int mixer, enum tcon_type tcon_type, int tcon)+{
- unsigned long flags;
- u32 val, reg;
- if (mixer > 1) {
DRM_ERROR("Mixer index is too high!\n");return;- }
- if (tcon > 1) {
DRM_ERROR("TCON index is too high!\n");return;- }
- switch (tcon_type) {
- case tcon_type_lcd:
val = TCON_TOP_PORT_TCON_LCD0 + tcon;break;- case tcon_type_tv:
val = TCON_TOP_PORT_TCON_TV0 + tcon;break;- default:
DRM_ERROR("Invalid TCON type!\n");return;- }
- spin_lock_irqsave(&tcon_top->reg_lock, flags);
- reg = readl(tcon_top->regs + TCON_TOP_PORT_SEL_REG);
- if (mixer == 0) {
reg &= ~TCON_TOP_PORT_DE0_MSK;reg |= FIELD_PREP(TCON_TOP_PORT_DE0_MSK, val);- } else {
reg &= ~TCON_TOP_PORT_DE1_MSK;reg |= FIELD_PREP(TCON_TOP_PORT_DE1_MSK, val);- }
- writel(reg, tcon_top->regs + TCON_TOP_PORT_SEL_REG);
- spin_unlock_irqrestore(&tcon_top->reg_lock, flags);
+}
+static int sun8i_tcon_top_probe(struct platform_device *pdev) +{
- struct clk_hw_onecell_data *clk_data;
- struct sun8i_tcon_top *tcon_top;
- struct device *dev = &pdev->dev;
- struct resource *res;
- int ret, i;
- tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL);
- if (!tcon_top)
return -ENOMEM;- clk_data = devm_kzalloc(&pdev->dev, sizeof(*clk_data) +
sizeof(*clk_data->hws) * CLK_NUM,GFP_KERNEL);- if (!clk_data)
return -ENOMEM;- spin_lock_init(&tcon_top->reg_lock);
- tcon_top->rst = devm_reset_control_get(dev, "rst");
- if (IS_ERR(tcon_top->rst)) {
dev_err(dev, "Couldn't get our reset line\n");return PTR_ERR(tcon_top->rst);- }
- tcon_top->bus = devm_clk_get(dev, "bus");
- if (IS_ERR(tcon_top->bus)) {
dev_err(dev, "Couldn't get the bus clock\n");return PTR_ERR(tcon_top->bus);- }
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- tcon_top->regs = devm_ioremap_resource(dev, res);
- if (IS_ERR(tcon_top->regs))
return PTR_ERR(tcon_top->regs);- ret = reset_control_deassert(tcon_top->rst);
- if (ret) {
dev_err(dev, "Could not deassert ctrl reset control\n");return ret;- }
- ret = clk_prepare_enable(tcon_top->bus);
- if (ret) {
dev_err(dev, "Could not enable bus clock\n");goto err_assert_reset;- }
- /*
* Default register values might have some reserved bits set, which* prevents TCON TOP from working properly. Set them to 0 here.*/- writel(0, tcon_top->regs + TCON_TOP_PORT_SEL_REG);
- writel(0, tcon_top->regs + TCON_TOP_GATE_SRC_REG);
- for (i = 0; i < CLK_NUM; i++) {
const char *parent_name = "bus-tcon-top";I guess retrieving the parent's clock name at runtime would be more flexible.
It is, but will it ever be anything else?
struct clk_init_data init;struct clk_gate *gate;gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);if (!gate) {ret = -ENOMEM;goto err_disable_clock;}init.name = gates[i].name;init.ops = &clk_gate_ops;init.flags = CLK_IS_BASIC;init.parent_names = &parent_name;init.num_parents = 1;gate->reg = tcon_top->regs + TCON_TOP_GATE_SRC_REG;gate->bit_idx = gates[i].bit;gate->lock = &tcon_top->reg_lock;gate->hw.init = &init;ret = devm_clk_hw_register(dev, &gate->hw);if (ret)goto err_disable_clock;Isn't it what clk_hw_register_gate is doing?
Almost, but not exactly. My goal was to use devm_* functions, so there is no need to do any special cleanup.
clk_data->hws[gates[i].index] = &gate->hw;- }
- clk_data->num = CLK_NUM;
- ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
clk_data);
- if (ret)
goto err_disable_clock;- platform_set_drvdata(pdev, tcon_top);
- return 0;
+err_disable_clock:
- clk_disable_unprepare(tcon_top->bus);
+err_assert_reset:
- reset_control_assert(tcon_top->rst);
- return ret;
+}
+static int sun8i_tcon_top_remove(struct platform_device *pdev) +{
- struct sun8i_tcon_top *tcon_top = platform_get_drvdata(pdev);
- clk_disable_unprepare(tcon_top->bus);
- reset_control_assert(tcon_top->rst);
- return 0;
+}
+const struct of_device_id sun8i_tcon_top_of_table[] = {
- { .compatible = "allwinner,sun8i-r40-tcon-top" },
- { /* sentinel */ }
+}; +MODULE_DEVICE_TABLE(of, sun8i_tcon_top_of_table);
+static struct platform_driver sun8i_tcon_top_platform_driver = {
- .probe = sun8i_tcon_top_probe,
- .remove = sun8i_tcon_top_remove,
- .driver = {
.name = "sun8i-tcon-top",.of_match_table = sun8i_tcon_top_of_table,- },
+}; +module_platform_driver(sun8i_tcon_top_platform_driver);
+MODULE_AUTHOR("Jernej Skrabec jernej.skrabec@siol.net"); +MODULE_DESCRIPTION("Allwinner R40 TCON TOP driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.h b/drivers/gpu/drm/sun4i/sun8i_tcon_top.h new file mode 100644 index 000000000000..19126e07d2a6 --- /dev/null +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* Copyright (c) 2018 Jernej Skrabec jernej.skrabec@siol.net */
+#ifndef _SUN8I_TCON_TOP_H_ +#define _SUN8I_TCON_TOP_H_
+#include <linux/device.h>
+struct sun8i_tcon_top;
+enum tcon_type {
- tcon_type_lcd,
- tcon_type_tv,
The usual practice is to have the enum values upper-case.
Ok.
Best regards, Jernej
Hi,
On Mon, May 21, 2018 at 05:15:15PM +0200, Jernej Škrabec wrote:
- /*
* Default register values might have some reserved bits set, which* prevents TCON TOP from working properly. Set them to 0 here.*/- writel(0, tcon_top->regs + TCON_TOP_PORT_SEL_REG);
- writel(0, tcon_top->regs + TCON_TOP_GATE_SRC_REG);
- for (i = 0; i < CLK_NUM; i++) {
const char *parent_name = "bus-tcon-top";I guess retrieving the parent's clock name at runtime would be more flexible.
It is, but will it ever be anything else?
Probably not, but when the complexity is exactly the same (using __clk_get_name), we'd better use the more appropriate solution. If we ever need to change that clock name, or to use the driver with an SoC that wouldn't have the same clock name for whatever reason, it will just work.
struct clk_init_data init;struct clk_gate *gate;gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);if (!gate) {ret = -ENOMEM;goto err_disable_clock;}init.name = gates[i].name;init.ops = &clk_gate_ops;init.flags = CLK_IS_BASIC;init.parent_names = &parent_name;init.num_parents = 1;gate->reg = tcon_top->regs + TCON_TOP_GATE_SRC_REG;gate->bit_idx = gates[i].bit;gate->lock = &tcon_top->reg_lock;gate->hw.init = &init;ret = devm_clk_hw_register(dev, &gate->hw);if (ret)goto err_disable_clock;Isn't it what clk_hw_register_gate is doing?
Almost, but not exactly. My goal was to use devm_* functions, so there is no need to do any special cleanup.
Is it the only difference? If so, you can just create a devm_clk_hw_register gate.
Maxime
Hi,
Dne četrtek, 24. maj 2018 ob 10:43:51 CEST je Maxime Ripard napisal(a):
Hi,
On Mon, May 21, 2018 at 05:15:15PM +0200, Jernej Škrabec wrote:
- /*
* Default register values might have some reserved bits set,
which
* prevents TCON TOP from working properly. Set them to 0 here.*/- writel(0, tcon_top->regs + TCON_TOP_PORT_SEL_REG);
- writel(0, tcon_top->regs + TCON_TOP_GATE_SRC_REG);
- for (i = 0; i < CLK_NUM; i++) {
const char *parent_name = "bus-tcon-top";I guess retrieving the parent's clock name at runtime would be more flexible.
It is, but will it ever be anything else?
Probably not, but when the complexity is exactly the same (using __clk_get_name), we'd better use the more appropriate solution. If we ever need to change that clock name, or to use the driver with an SoC that wouldn't have the same clock name for whatever reason, it will just work.
struct clk_init_data init;struct clk_gate *gate;gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);if (!gate) {ret = -ENOMEM;goto err_disable_clock;}init.name = gates[i].name;init.ops = &clk_gate_ops;init.flags = CLK_IS_BASIC;init.parent_names = &parent_name;init.num_parents = 1;gate->reg = tcon_top->regs + TCON_TOP_GATE_SRC_REG;gate->bit_idx = gates[i].bit;gate->lock = &tcon_top->reg_lock;gate->hw.init = &init;ret = devm_clk_hw_register(dev, &gate->hw);if (ret)goto err_disable_clock;Isn't it what clk_hw_register_gate is doing?
Almost, but not exactly. My goal was to use devm_* functions, so there is no need to do any special cleanup.
Is it the only difference? If so, you can just create a devm_clk_hw_register gate.
I checked around and it seems that in clk core there are only non devm_* helpers like clk_hw_register_gate() for some reason. I guess I'll just use that and manually unregister all the clocks in cleanup function.
Best regards, Jernej
Hi Jernej,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm/drm-next] [also build test ERROR on v4.17-rc6 next-20180517] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jernej-Skrabec/Add-support-for-R40-... base: git://people.freedesktop.org/~airlied/linux.git drm-next config: arm-multi_v7_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm
All errors (new ones prefixed by >>):
drivers/gpu/drm/sun4i/sun8i_tcon_top.o: In function `init_module':
sun8i_tcon_top.c:(.init.text+0x0): multiple definition of `init_module'
drivers/gpu/drm/sun4i/sun8i_mixer.o:sun8i_mixer.c:(.init.text+0x0): first defined here drivers/gpu/drm/sun4i/sun8i_tcon_top.o: In function `cleanup_module':
sun8i_tcon_top.c:(.exit.text+0x0): multiple definition of `cleanup_module'
drivers/gpu/drm/sun4i/sun8i_mixer.o:sun8i_mixer.c:(.exit.text+0x0): first defined here
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, May 19, 2018 at 08:31:17PM +0200, Jernej Skrabec wrote:
As already described in DT binding, TCON TOP is responsible for configuring display pipeline. In this initial driver focus is on HDMI pipeline, so TVE and LCD configuration is not implemented.
Implemented features:
- HDMI source selection
- clock driver (TCON and DSI gating)
- connecting mixers and TCONS
Something similar also existed in previous SoCs, except that it was part of first TCON.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/Makefile | 3 +- drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 256 +++++++++++++++++++++ drivers/gpu/drm/sun4i/sun8i_tcon_top.h | 20 ++ include/dt-bindings/clock/sun8i-tcon-top.h | 11 +
This belongs with the binding doc patch.
4 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.c create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.h create mode 100644 include/dt-bindings/clock/sun8i-tcon-top.h
If SoC has TCON TOP unit, it has to be configured from TCON, since it has all information needed. Additionally, if it is TCON TV, it must also enable bus gate inside TCON TOP unit.
Add support for such TCONs.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++++ drivers/gpu/drm/sun4i/sun4i_tcon.h | 8 ++++++++ 2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 08747fc3ee71..e0c562ce1c22 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -688,6 +688,16 @@ static int sun4i_tcon_init_clocks(struct device *dev, dev_err(dev, "Couldn't get the TCON bus clock\n"); return PTR_ERR(tcon->clk); } + + if (tcon->quirks->needs_tcon_top && tcon->quirks->has_channel_1) { + tcon->top_clk = devm_clk_get(dev, "tcon-top"); + if (IS_ERR(tcon->top_clk)) { + dev_err(dev, "Couldn't get the TCON TOP bus clock\n"); + return PTR_ERR(tcon->top_clk); + } + clk_prepare_enable(tcon->top_clk); + } + clk_prepare_enable(tcon->clk);
if (tcon->quirks->has_channel_0) { @@ -712,6 +722,7 @@ static int sun4i_tcon_init_clocks(struct device *dev, static void sun4i_tcon_free_clocks(struct sun4i_tcon *tcon) { clk_disable_unprepare(tcon->clk); + clk_disable_unprepare(tcon->top_clk); }
static int sun4i_tcon_init_irq(struct device *dev, @@ -980,6 +991,23 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, tcon->id = engine->id; tcon->quirks = of_device_get_match_data(dev);
+ if (tcon->quirks->needs_tcon_top) { + struct device_node *np; + + np = of_parse_phandle(dev->of_node, "allwinner,tcon-top", 0); + if (np) { + struct platform_device *pdev; + + pdev = of_find_device_by_node(np); + if (pdev) + tcon->tcon_top = platform_get_drvdata(pdev); + of_node_put(np); + + if (!tcon->tcon_top) + return -EPROBE_DEFER; + } + } + tcon->lcd_rst = devm_reset_control_get(dev, "lcd"); if (IS_ERR(tcon->lcd_rst)) { dev_err(dev, "Couldn't get our reset line\n"); diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h index f6a071cd5a6f..26be0d317a38 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h @@ -20,6 +20,8 @@ #include <linux/list.h> #include <linux/reset.h>
+#include "sun8i_tcon_top.h" + #define SUN4I_TCON_GCTL_REG 0x0 #define SUN4I_TCON_GCTL_TCON_ENABLE BIT(31) #define SUN4I_TCON_GCTL_IOMAP_MASK BIT(0) @@ -224,6 +226,7 @@ struct sun4i_tcon_quirks { bool needs_de_be_mux; /* sun6i needs mux to select backend */ bool needs_edp_reset; /* a80 edp reset needed for tcon0 access */ bool supports_lvds; /* Does the TCON support an LVDS output? */ + bool needs_tcon_top; /* TCON TOP holds additional muxing configuration */
/* callback to handle tcon muxing options */ int (*set_mux)(struct sun4i_tcon *, const struct drm_encoder *); @@ -249,6 +252,9 @@ struct sun4i_tcon { u8 dclk_max_div; u8 dclk_min_div;
+ /* TCON TOP clock */ + struct clk *top_clk; + /* Reset control */ struct reset_control *lcd_rst; struct reset_control *lvds_rst; @@ -263,6 +269,8 @@ struct sun4i_tcon {
int id;
+ struct sun8i_tcon_top *tcon_top; + /* TCON list management */ struct list_head list; };
On Sat, May 19, 2018 at 08:31:18PM +0200, Jernej Skrabec wrote:
If SoC has TCON TOP unit, it has to be configured from TCON, since it has all information needed. Additionally, if it is TCON TV, it must also enable bus gate inside TCON TOP unit.
Why?
Add support for such TCONs.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++++ drivers/gpu/drm/sun4i/sun4i_tcon.h | 8 ++++++++ 2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 08747fc3ee71..e0c562ce1c22 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -688,6 +688,16 @@ static int sun4i_tcon_init_clocks(struct device *dev, dev_err(dev, "Couldn't get the TCON bus clock\n"); return PTR_ERR(tcon->clk); }
if (tcon->quirks->needs_tcon_top && tcon->quirks->has_channel_1) {
tcon->top_clk = devm_clk_get(dev, "tcon-top");if (IS_ERR(tcon->top_clk)) {dev_err(dev, "Couldn't get the TCON TOP bus clock\n");return PTR_ERR(tcon->top_clk);}clk_prepare_enable(tcon->top_clk);}
clk_prepare_enable(tcon->clk);
if (tcon->quirks->has_channel_0) {
@@ -712,6 +722,7 @@ static int sun4i_tcon_init_clocks(struct device *dev, static void sun4i_tcon_free_clocks(struct sun4i_tcon *tcon) { clk_disable_unprepare(tcon->clk);
- clk_disable_unprepare(tcon->top_clk);
}
static int sun4i_tcon_init_irq(struct device *dev, @@ -980,6 +991,23 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, tcon->id = engine->id; tcon->quirks = of_device_get_match_data(dev);
- if (tcon->quirks->needs_tcon_top) {
struct device_node *np;np = of_parse_phandle(dev->of_node, "allwinner,tcon-top", 0);if (np) {struct platform_device *pdev;pdev = of_find_device_by_node(np);if (pdev)tcon->tcon_top = platform_get_drvdata(pdev);of_node_put(np);if (!tcon->tcon_top)return -EPROBE_DEFER;}- }
I might have missed it, but I've not seen the bindings additions for that property. This shouldn't really be done that way anyway, instead of using a direct phandle, you should be using the of-graph, with the TCON-top sitting where it belongs in the flow of data.
Maxime
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:07:59 CEST je Maxime Ripard napisal(a):
On Sat, May 19, 2018 at 08:31:18PM +0200, Jernej Skrabec wrote:
If SoC has TCON TOP unit, it has to be configured from TCON, since it has all information needed. Additionally, if it is TCON TV, it must also enable bus gate inside TCON TOP unit.
Why?
I'll explain my design decision below.
Add support for such TCONs.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++++ drivers/gpu/drm/sun4i/sun4i_tcon.h | 8 ++++++++ 2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 08747fc3ee71..e0c562ce1c22 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -688,6 +688,16 @@ static int sun4i_tcon_init_clocks(struct device *dev,
dev_err(dev, "Couldn't get the TCON bus clock\n"); return PTR_ERR(tcon->clk);}
if (tcon->quirks->needs_tcon_top && tcon->quirks->has_channel_1) {
tcon->top_clk = devm_clk_get(dev, "tcon-top");if (IS_ERR(tcon->top_clk)) {dev_err(dev, "Couldn't get the TCON TOP bus clock\n");return PTR_ERR(tcon->top_clk);}clk_prepare_enable(tcon->top_clk);}
clk_prepare_enable(tcon->clk);
if (tcon->quirks->has_channel_0) {
@@ -712,6 +722,7 @@ static int sun4i_tcon_init_clocks(struct device *dev,
static void sun4i_tcon_free_clocks(struct sun4i_tcon *tcon) {
clk_disable_unprepare(tcon->clk);
- clk_disable_unprepare(tcon->top_clk);
}
static int sun4i_tcon_init_irq(struct device *dev,
@@ -980,6 +991,23 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,> tcon->id = engine->id; tcon->quirks = of_device_get_match_data(dev);
- if (tcon->quirks->needs_tcon_top) {
struct device_node *np;np = of_parse_phandle(dev->of_node, "allwinner,tcon-top", 0);if (np) {struct platform_device *pdev;pdev = of_find_device_by_node(np);if (pdev)tcon->tcon_top = platform_get_drvdata(pdev);of_node_put(np);if (!tcon->tcon_top)return -EPROBE_DEFER;}- }
I might have missed it, but I've not seen the bindings additions for that property. This shouldn't really be done that way anyway, instead of using a direct phandle, you should be using the of-graph, with the TCON-top sitting where it belongs in the flow of data.
Just to answer to the first question, it did describe it in "[PATCH 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline".
As why I designed it that way - HW representation could be described that way (ASCII art makes sense when fixed width font is used to view it):
/ LCD0/LVDS0 / TCON-LCD0 | \ MIPI DSI mixer0 | \ / TCON-LCD1 - LCD1/LVDS1 TCON-TOP / \ TCON-TV0 - TVE0/RGB mixer1 | \ | TCON-TOP - HDMI | / \ TCON-TV1 - TVE1/RGB
This is a bit simplified, since there is also TVE-TOP, which is responsible for sharing 4 DACs between both TVE encoders. You can have two TV outs (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It even seems that you can arbitrarly choose which DAC is responsible for which signal, so there is a ton of possible end combinations, but I'm not 100% sure.
Even though I wrote TCON-TOP twice, this is same unit in HW. R40 manual suggest more possibilities, although some of them seem wrong, like RGB feeding from LCD TCON. That is confirmed to be wrong when checking BSP code.
Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, TVE1 and LCD1 for pin muxing, although I'm not sure why is that needed at all, since according to R40 datasheet, TVE0 and TVE1 pins are dedicated and not on PORT D and PORT H, respectively, as TCON-TOP documentation suggest. However, HSYNC and PSYNC lines might be shared between TVE (when it works in RGB mode) and LCD. But that is just my guess since I'm not really familiar with RGB and LCD interfaces.
I'm really not sure what would be the best representation in OF-graph. Can you suggest one?
On the other hand, mux callback in TCON driver has all available informations at hand. It knows mixer ID, TCON ID and most importantly, encoder type. Based on all that informations, it's easy to configure TCON TOP.
I hope you understand. If you have better idea, I'm all ears, since phandle seems a bit weird to me too, but I think it's the only future proof, when adding LVDS, RGB, TVE or LCD support.
Best regards, Jernej
On Mon, May 21, 2018 at 07:27:46PM +0200, Jernej Škrabec wrote:
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:07:59 CEST je Maxime Ripard napisal(a):
On Sat, May 19, 2018 at 08:31:18PM +0200, Jernej Skrabec wrote:
If SoC has TCON TOP unit, it has to be configured from TCON, since it has all information needed. Additionally, if it is TCON TV, it must also enable bus gate inside TCON TOP unit.
Why?
I'll explain my design decision below.
Add support for such TCONs.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++++ drivers/gpu/drm/sun4i/sun4i_tcon.h | 8 ++++++++ 2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 08747fc3ee71..e0c562ce1c22 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -688,6 +688,16 @@ static int sun4i_tcon_init_clocks(struct device *dev,
dev_err(dev, "Couldn't get the TCON bus clock\n"); return PTR_ERR(tcon->clk);}
if (tcon->quirks->needs_tcon_top && tcon->quirks->has_channel_1) {
tcon->top_clk = devm_clk_get(dev, "tcon-top");if (IS_ERR(tcon->top_clk)) {dev_err(dev, "Couldn't get the TCON TOP bus clock\n");return PTR_ERR(tcon->top_clk);}clk_prepare_enable(tcon->top_clk);}
clk_prepare_enable(tcon->clk);
if (tcon->quirks->has_channel_0) {
@@ -712,6 +722,7 @@ static int sun4i_tcon_init_clocks(struct device *dev,
static void sun4i_tcon_free_clocks(struct sun4i_tcon *tcon) {
clk_disable_unprepare(tcon->clk);
- clk_disable_unprepare(tcon->top_clk);
}
static int sun4i_tcon_init_irq(struct device *dev,
@@ -980,6 +991,23 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,> tcon->id = engine->id; tcon->quirks = of_device_get_match_data(dev);
- if (tcon->quirks->needs_tcon_top) {
struct device_node *np;np = of_parse_phandle(dev->of_node, "allwinner,tcon-top", 0);if (np) {struct platform_device *pdev;pdev = of_find_device_by_node(np);if (pdev)tcon->tcon_top = platform_get_drvdata(pdev);of_node_put(np);if (!tcon->tcon_top)return -EPROBE_DEFER;}- }
I might have missed it, but I've not seen the bindings additions for that property. This shouldn't really be done that way anyway, instead of using a direct phandle, you should be using the of-graph, with the TCON-top sitting where it belongs in the flow of data.
Just to answer to the first question, it did describe it in "[PATCH 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline".
As why I designed it that way - HW representation could be described that way (ASCII art makes sense when fixed width font is used to view it):
/ LCD0/LVDS0 / TCON-LCD0 | \ MIPI DSImixer0 | \ / TCON-LCD1 - LCD1/LVDS1 TCON-TOP / \ TCON-TV0 - TVE0/RGB mixer1 | \ | TCON-TOP - HDMI | / \ TCON-TV1 - TVE1/RGB
This is a bit simplified, since there is also TVE-TOP, which is responsible for sharing 4 DACs between both TVE encoders. You can have two TV outs (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It even seems that you can arbitrarly choose which DAC is responsible for which signal, so there is a ton of possible end combinations, but I'm not 100% sure.
Even though I wrote TCON-TOP twice, this is same unit in HW. R40 manual suggest more possibilities, although some of them seem wrong, like RGB feeding from LCD TCON. That is confirmed to be wrong when checking BSP code.
Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, TVE1 and LCD1 for pin muxing, although I'm not sure why is that needed at all, since according to R40 datasheet, TVE0 and TVE1 pins are dedicated and not on PORT D and PORT H, respectively, as TCON-TOP documentation suggest. However, HSYNC and PSYNC lines might be shared between TVE (when it works in RGB mode) and LCD. But that is just my guess since I'm not really familiar with RGB and LCD interfaces.
I'm really not sure what would be the best representation in OF-graph. Can you suggest one?
Rob might disagree on this one, but I don't see anything wrong with having loops in the graph. If the TCON-TOP can be both the input and output of the TCONs, then so be it, and have it described that way in the graph.
The code is already able to filter out nodes that have already been added to the list of devices we need to wait for in the component framework, so that should work as well.
And we'd need to describe TVE-TOP as well, even though we don't have a driver for it yet. That will simplify the backward compatibility later on.
Maxime
On Thu, May 24, 2018 at 1:50 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
On Mon, May 21, 2018 at 07:27:46PM +0200, Jernej Škrabec wrote:
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:07:59 CEST je Maxime Ripard napisal(a):
On Sat, May 19, 2018 at 08:31:18PM +0200, Jernej Skrabec wrote:
If SoC has TCON TOP unit, it has to be configured from TCON, since it has all information needed. Additionally, if it is TCON TV, it must also enable bus gate inside TCON TOP unit.
Why?
I'll explain my design decision below.
Add support for such TCONs.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++++ drivers/gpu/drm/sun4i/sun4i_tcon.h | 8 ++++++++ 2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 08747fc3ee71..e0c562ce1c22 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -688,6 +688,16 @@ static int sun4i_tcon_init_clocks(struct device *dev,
dev_err(dev, "Couldn't get the TCON bus clock\n"); return PTR_ERR(tcon->clk);}
if (tcon->quirks->needs_tcon_top && tcon->quirks->has_channel_1) {
tcon->top_clk = devm_clk_get(dev, "tcon-top");if (IS_ERR(tcon->top_clk)) {dev_err(dev, "Couldn't get the TCON TOP bus clock\n");return PTR_ERR(tcon->top_clk);}clk_prepare_enable(tcon->top_clk);}
clk_prepare_enable(tcon->clk);
if (tcon->quirks->has_channel_0) {
@@ -712,6 +722,7 @@ static int sun4i_tcon_init_clocks(struct device *dev,
static void sun4i_tcon_free_clocks(struct sun4i_tcon *tcon) {
clk_disable_unprepare(tcon->clk);
- clk_disable_unprepare(tcon->top_clk);
}
static int sun4i_tcon_init_irq(struct device *dev,
@@ -980,6 +991,23 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,> tcon->id = engine->id; tcon->quirks = of_device_get_match_data(dev);
- if (tcon->quirks->needs_tcon_top) {
struct device_node *np;np = of_parse_phandle(dev->of_node, "allwinner,tcon-top", 0);if (np) {struct platform_device *pdev;pdev = of_find_device_by_node(np);if (pdev)tcon->tcon_top = platform_get_drvdata(pdev);of_node_put(np);if (!tcon->tcon_top)return -EPROBE_DEFER;}- }
I might have missed it, but I've not seen the bindings additions for that property. This shouldn't really be done that way anyway, instead of using a direct phandle, you should be using the of-graph, with the TCON-top sitting where it belongs in the flow of data.
Just to answer to the first question, it did describe it in "[PATCH 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline".
As why I designed it that way - HW representation could be described that way (ASCII art makes sense when fixed width font is used to view it):
/ LCD0/LVDS0 / TCON-LCD0 | \ MIPI DSImixer0 | \ / TCON-LCD1 - LCD1/LVDS1 TCON-TOP / \ TCON-TV0 - TVE0/RGB mixer1 | \ | TCON-TOP - HDMI | / \ TCON-TV1 - TVE1/RGB
This is a bit simplified, since there is also TVE-TOP, which is responsible for sharing 4 DACs between both TVE encoders. You can have two TV outs (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It even seems that you can arbitrarly choose which DAC is responsible for which signal, so there is a ton of possible end combinations, but I'm not 100% sure.
Even though I wrote TCON-TOP twice, this is same unit in HW. R40 manual suggest more possibilities, although some of them seem wrong, like RGB feeding from LCD TCON. That is confirmed to be wrong when checking BSP code.
Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, TVE1 and LCD1 for pin muxing, although I'm not sure why is that needed at all, since according to R40 datasheet, TVE0 and TVE1 pins are dedicated and not on PORT D and PORT H, respectively, as TCON-TOP documentation suggest. However, HSYNC and PSYNC lines might be shared between TVE (when it works in RGB mode) and LCD. But that is just my guess since I'm not really familiar with RGB and LCD interfaces.
I'm really not sure what would be the best representation in OF-graph. Can you suggest one?
Rob might disagree on this one, but I don't see anything wrong with having loops in the graph. If the TCON-TOP can be both the input and output of the TCONs, then so be it, and have it described that way in the graph.
The code is already able to filter out nodes that have already been added to the list of devices we need to wait for in the component framework, so that should work as well.
And we'd need to describe TVE-TOP as well, even though we don't have a driver for it yet. That will simplify the backward compatibility later on.
I'm getting the feeling that TCON-TOP / TVE-TOP is the glue layer that binds everything together, and provides signal routing, kind of like DE-TOP on A64. So the signal mux controls that were originally found in TCON0 and TVE0 were moved out.
The driver needs to know about that, but the graph about doesn't make much sense directly. Without looking at the manual, I understand it to likely be one mux between the mixers and TCONs, and one between the TCON-TVs and HDMI. Would it make more sense to just have the graph connections between the muxed components, and remove TCON-TOP from it, like we had in the past? A phandle could be used to reference the TCON-TOP for mux controls, in addition to the clocks and resets.
For TVE, we would need something to represent each of the output pins, so the device tree can actually describe what kind of signal, be it each component of RGB/YUV or composite video, is wanted on each pin, if any. This is also needed on the A20 for the Cubietruck, so we can describe which pins are tied to the VGA connector, and which one does R, G, or B.
Regards ChenYu
On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote:
- if (tcon->quirks->needs_tcon_top) {
struct device_node *np;np = of_parse_phandle(dev->of_node, "allwinner,tcon-top", 0);if (np) {struct platform_device *pdev;pdev = of_find_device_by_node(np);if (pdev)tcon->tcon_top = platform_get_drvdata(pdev);of_node_put(np);if (!tcon->tcon_top)return -EPROBE_DEFER;}- }
I might have missed it, but I've not seen the bindings additions for that property. This shouldn't really be done that way anyway, instead of using a direct phandle, you should be using the of-graph, with the TCON-top sitting where it belongs in the flow of data.
Just to answer to the first question, it did describe it in "[PATCH 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline".
As why I designed it that way - HW representation could be described that way (ASCII art makes sense when fixed width font is used to view it):
/ LCD0/LVDS0 / TCON-LCD0 | \ MIPI DSImixer0 | \ / TCON-LCD1 - LCD1/LVDS1 TCON-TOP / \ TCON-TV0 - TVE0/RGB mixer1 | \ | TCON-TOP - HDMI | / \ TCON-TV1 - TVE1/RGB
This is a bit simplified, since there is also TVE-TOP, which is responsible for sharing 4 DACs between both TVE encoders. You can have two TV outs (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It even seems that you can arbitrarly choose which DAC is responsible for which signal, so there is a ton of possible end combinations, but I'm not 100% sure.
Even though I wrote TCON-TOP twice, this is same unit in HW. R40 manual suggest more possibilities, although some of them seem wrong, like RGB feeding from LCD TCON. That is confirmed to be wrong when checking BSP code.
Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, TVE1 and LCD1 for pin muxing, although I'm not sure why is that needed at all, since according to R40 datasheet, TVE0 and TVE1 pins are dedicated and not on PORT D and PORT H, respectively, as TCON-TOP documentation suggest. However, HSYNC and PSYNC lines might be shared between TVE (when it works in RGB mode) and LCD. But that is just my guess since I'm not really familiar with RGB and LCD interfaces.
I'm really not sure what would be the best representation in OF-graph. Can you suggest one?
Rob might disagree on this one, but I don't see anything wrong with having loops in the graph. If the TCON-TOP can be both the input and output of the TCONs, then so be it, and have it described that way in the graph.
The code is already able to filter out nodes that have already been added to the list of devices we need to wait for in the component framework, so that should work as well.
And we'd need to describe TVE-TOP as well, even though we don't have a driver for it yet. That will simplify the backward compatibility later on.
I'm getting the feeling that TCON-TOP / TVE-TOP is the glue layer that binds everything together, and provides signal routing, kind of like DE-TOP on A64. So the signal mux controls that were originally found in TCON0 and TVE0 were moved out.
The driver needs to know about that, but the graph about doesn't make much sense directly. Without looking at the manual, I understand it to likely be one mux between the mixers and TCONs, and one between the TCON-TVs and HDMI. Would it make more sense to just have the graph connections between the muxed components, and remove TCON-TOP from it, like we had in the past? A phandle could be used to reference the TCON-TOP for mux controls, in addition to the clocks and resets.
For TVE, we would need something to represent each of the output pins, so the device tree can actually describe what kind of signal, be it each component of RGB/YUV or composite video, is wanted on each pin, if any. This is also needed on the A20 for the Cubietruck, so we can describe which pins are tied to the VGA connector, and which one does R, G, or B.
I guess we'll see how the DT maintainers feel about this, but my impression is that the OF graph should model the flow of data between the devices. If there's a mux somewhere, then the data is definitely going through it, and as such it should be part of the graph.
Maxime
Dne četrtek, 31. maj 2018 ob 11:21:33 CEST je Maxime Ripard napisal(a):
On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote:
- if (tcon->quirks->needs_tcon_top) {
struct device_node *np;np = of_parse_phandle(dev->of_node, "allwinner,tcon-top",0);
if (np) {struct platform_device *pdev;pdev = of_find_device_by_node(np);if (pdev)tcon->tcon_top =platform_get_drvdata(pdev);
of_node_put(np);if (!tcon->tcon_top)return -EPROBE_DEFER;}- }
I might have missed it, but I've not seen the bindings additions for that property. This shouldn't really be done that way anyway, instead of using a direct phandle, you should be using the of-graph, with the TCON-top sitting where it belongs in the flow of data.
Just to answer to the first question, it did describe it in "[PATCH 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline".
As why I designed it that way - HW representation could be described that way> >> (ASCII art makes sense when fixed width font is used to view it): / LCD0/LVDS0
/ TCON-LCD0 | \ MIPI DSImixer0 |
\ / TCON-LCD1 - LCD1/LVDS1 TCON-TOP / \ TCON-TV0 - TVE0/RGBmixer1 | \
| TCON-TOP - HDMI | | / \ TCON-TV1 - TVE1/RGBThis is a bit simplified, since there is also TVE-TOP, which is responsible for sharing 4 DACs between both TVE encoders. You can have two TV outs (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It even seems that you can arbitrarly choose which DAC is responsible for which signal, so there is a ton of possible end combinations, but I'm not 100% sure.
Even though I wrote TCON-TOP twice, this is same unit in HW. R40 manual suggest more possibilities, although some of them seem wrong, like RGB feeding from LCD TCON. That is confirmed to be wrong when checking BSP code.
Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, TVE1 and LCD1 for pin muxing, although I'm not sure why is that needed at all, since according to R40 datasheet, TVE0 and TVE1 pins are dedicated and not on PORT D and PORT H, respectively, as TCON-TOP documentation suggest. However, HSYNC and PSYNC lines might be shared between TVE (when it works in RGB mode) and LCD. But that is just my guess since I'm not really familiar with RGB and LCD interfaces.
I'm really not sure what would be the best representation in OF-graph. Can you suggest one?
Rob might disagree on this one, but I don't see anything wrong with having loops in the graph. If the TCON-TOP can be both the input and output of the TCONs, then so be it, and have it described that way in the graph.
The code is already able to filter out nodes that have already been added to the list of devices we need to wait for in the component framework, so that should work as well.
And we'd need to describe TVE-TOP as well, even though we don't have a driver for it yet. That will simplify the backward compatibility later on.
I'm getting the feeling that TCON-TOP / TVE-TOP is the glue layer that binds everything together, and provides signal routing, kind of like DE-TOP on A64. So the signal mux controls that were originally found in TCON0 and TVE0 were moved out.
The driver needs to know about that, but the graph about doesn't make much sense directly. Without looking at the manual, I understand it to likely be one mux between the mixers and TCONs, and one between the TCON-TVs and HDMI. Would it make more sense to just have the graph connections between the muxed components, and remove TCON-TOP from it, like we had in the past? A phandle could be used to reference the TCON-TOP for mux controls, in addition to the clocks and resets.
For TVE, we would need something to represent each of the output pins, so the device tree can actually describe what kind of signal, be it each component of RGB/YUV or composite video, is wanted on each pin, if any. This is also needed on the A20 for the Cubietruck, so we can describe which pins are tied to the VGA connector, and which one does R, G, or B.
I guess we'll see how the DT maintainers feel about this, but my impression is that the OF graph should model the flow of data between the devices. If there's a mux somewhere, then the data is definitely going through it, and as such it should be part of the graph.
I concur, but I spent few days thinking how to represent this sanely in graph, but I didn't find any good solution. I'll represent here my idea and please tell your opinion before I start implementing it.
First, let me be clear that mixer0 and mixer1 don't have same capabilities (different number of planes, mixer0 supports writeback, mixer1 does not, etc.). Thus, it does matter which mixer is connected to which TCON/encoder. mixer0 is meant to be connected to main display and mixer1 to auxiliary. That obviously depends on end system.
So, TCON TOP has 3 muxes, which have to be represented in graph. Two of them are for mixer/TCON relationship (each of them 1 input and 4 possible outputs) and one for TV TCON/HDMI pair selection (2 possible inputs, 1 output).
According to current practice in sun4i-drm driver, graph has to have port 0, representing input and port 1, representing output. This would mean that graph looks something like that:
tcon_top: tcon-top@1c70000 { compatible = "allwinner,sun8i-r40-tcon-top"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_top_in: port@0 { #address-cells = <1>; #size-cells = <0>; reg = <0>;
tcon_top_in_mixer0: endpoint@0 { reg = <0>; remote-endpoint = <&mixer0_out_tcon_top>; };
tcon_top_in_mixer1: endpoint@1 { reg = <1>; remote-endpoint = <&mixer1_out_tcon_top>; };
tcon_top_in_tcon_tv: endpoint@2 { reg = <2>; // here is HDMI input connection, part of board DTS remote-endpoint = <board specific phandle to TV TCON output>; }; };
tcon_top_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>;
tcon_top_out_tcon0: endpoint@0 { reg = <0>; // here is mixer0 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; };
tcon_top_out_tcon1: endpoint@1 { reg = <1>; // here is mixer1 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; };
tcon_top_out_hdmi: endpoint@2 { reg = <2>; remote-endpoint = <&hdmi_in_tcon_top>; }; }; }; };
tcon_tv0: lcd-controller@1c73000 { compatible = "allwinner,sun8i-r40-tcon-tv-0"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_tv0_in: port@0 { reg = <0>;
tcon_tv0_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>; }; };
tcon_tv0_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>;
// endpoints to TV TOP and TCON TOP HDMI input ... }; }; };
tcon_tv1: lcd-controller@1c74000 { compatible = "allwinner,sun8i-r40-tcon-tv-1"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_tv1_in: port@0 { reg = <0>;
tcon_tv1_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>; }; };
tcon_tv1_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>;
// endpoints to TV TOP and TCON TOP HDMI input ... }; }; };
tcon_lcd0 and tcon_lcd1 would have similar connections, except that for outputs would be LCD or LVDS panels or MIPI DSI encoder.
Please note that each TCON (there are 4 of them) would need to have unique compatible and have HW index stored in quirks data. It would be used by TCON TOP driver for configuring muxes.
What do you think about above suggestion?
Best regards, Jernej
On Thu, May 31, 2018 at 07:54:08PM +0200, Jernej Škrabec wrote:
Dne četrtek, 31. maj 2018 ob 11:21:33 CEST je Maxime Ripard napisal(a):
On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote:
> + if (tcon->quirks->needs_tcon_top) { > + struct device_node *np; > + > + np = of_parse_phandle(dev->of_node, "allwinner,tcon-top", > 0); > + if (np) { > + struct platform_device *pdev; > + > + pdev = of_find_device_by_node(np); > + if (pdev) > + tcon->tcon_top = > platform_get_drvdata(pdev); > + of_node_put(np); > + > + if (!tcon->tcon_top) > + return -EPROBE_DEFER; > + } > + } > +
I might have missed it, but I've not seen the bindings additions for that property. This shouldn't really be done that way anyway, instead of using a direct phandle, you should be using the of-graph, with the TCON-top sitting where it belongs in the flow of data.
Just to answer to the first question, it did describe it in "[PATCH 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline".
As why I designed it that way - HW representation could be described that way> >> (ASCII art makes sense when fixed width font is used to view it): / LCD0/LVDS0
/ TCON-LCD0 | \ MIPI DSImixer0 |
\ / TCON-LCD1 - LCD1/LVDS1 TCON-TOP / \ TCON-TV0 - TVE0/RGBmixer1 | \
| TCON-TOP - HDMI | | / \ TCON-TV1 - TVE1/RGBThis is a bit simplified, since there is also TVE-TOP, which is responsible for sharing 4 DACs between both TVE encoders. You can have two TV outs (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It even seems that you can arbitrarly choose which DAC is responsible for which signal, so there is a ton of possible end combinations, but I'm not 100% sure.
Even though I wrote TCON-TOP twice, this is same unit in HW. R40 manual suggest more possibilities, although some of them seem wrong, like RGB feeding from LCD TCON. That is confirmed to be wrong when checking BSP code.
Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, TVE1 and LCD1 for pin muxing, although I'm not sure why is that needed at all, since according to R40 datasheet, TVE0 and TVE1 pins are dedicated and not on PORT D and PORT H, respectively, as TCON-TOP documentation suggest. However, HSYNC and PSYNC lines might be shared between TVE (when it works in RGB mode) and LCD. But that is just my guess since I'm not really familiar with RGB and LCD interfaces.
I'm really not sure what would be the best representation in OF-graph. Can you suggest one?
Rob might disagree on this one, but I don't see anything wrong with having loops in the graph. If the TCON-TOP can be both the input and output of the TCONs, then so be it, and have it described that way in the graph.
The code is already able to filter out nodes that have already been added to the list of devices we need to wait for in the component framework, so that should work as well.
And we'd need to describe TVE-TOP as well, even though we don't have a driver for it yet. That will simplify the backward compatibility later on.
I'm getting the feeling that TCON-TOP / TVE-TOP is the glue layer that binds everything together, and provides signal routing, kind of like DE-TOP on A64. So the signal mux controls that were originally found in TCON0 and TVE0 were moved out.
The driver needs to know about that, but the graph about doesn't make much sense directly. Without looking at the manual, I understand it to likely be one mux between the mixers and TCONs, and one between the TCON-TVs and HDMI. Would it make more sense to just have the graph connections between the muxed components, and remove TCON-TOP from it, like we had in the past? A phandle could be used to reference the TCON-TOP for mux controls, in addition to the clocks and resets.
For TVE, we would need something to represent each of the output pins, so the device tree can actually describe what kind of signal, be it each component of RGB/YUV or composite video, is wanted on each pin, if any. This is also needed on the A20 for the Cubietruck, so we can describe which pins are tied to the VGA connector, and which one does R, G, or B.
I guess we'll see how the DT maintainers feel about this, but my impression is that the OF graph should model the flow of data between the devices. If there's a mux somewhere, then the data is definitely going through it, and as such it should be part of the graph.
I concur, but I spent few days thinking how to represent this sanely in graph, but I didn't find any good solution. I'll represent here my idea and please tell your opinion before I start implementing it.
First, let me be clear that mixer0 and mixer1 don't have same capabilities (different number of planes, mixer0 supports writeback, mixer1 does not, etc.). Thus, it does matter which mixer is connected to which TCON/encoder. mixer0 is meant to be connected to main display and mixer1 to auxiliary. That obviously depends on end system.
So, TCON TOP has 3 muxes, which have to be represented in graph. Two of them are for mixer/TCON relationship (each of them 1 input and 4 possible outputs) and one for TV TCON/HDMI pair selection (2 possible inputs, 1 output).
According to current practice in sun4i-drm driver, graph has to have port 0, representing input and port 1, representing output. This would mean that graph looks something like that:
tcon_top: tcon-top@1c70000 { compatible = "allwinner,sun8i-r40-tcon-top"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_top_in: port@0 { #address-cells = <1>; #size-cells = <0>; reg = <0>; tcon_top_in_mixer0: endpoint@0 { reg = <0>; remote-endpoint = <&mixer0_out_tcon_top>; }; tcon_top_in_mixer1: endpoint@1 { reg = <1>; remote-endpoint = <&mixer1_out_tcon_top>; }; tcon_top_in_tcon_tv: endpoint@2 { reg = <2>; // here is HDMI input connection, part of board DTS remote-endpoint = <board specific phandle to TV TCON output>; }; }; tcon_top_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; tcon_top_out_tcon0: endpoint@0 { reg = <0>; // here is mixer0 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_tcon1: endpoint@1 { reg = <1>; // here is mixer1 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_hdmi: endpoint@2 { reg = <2>; remote-endpoint = <&hdmi_in_tcon_top>; }; };}; };
IIRC, each port is supposed to be one route for the data, so we would have multiple ports, one for the mixers in input and for the tcon in output, and one for the TCON in input and for the HDMI/TV in output. Rob might correct me here.
tcon_tv0: lcd-controller@1c73000 { compatible = "allwinner,sun8i-r40-tcon-tv-0"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_tv0_in: port@0 { reg = <0>; tcon_tv0_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>;
Just curious, what would be there?
}; }; tcon_tv0_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; // endpoints to TV TOP and TCON TOP HDMI input ... };}; };
tcon_tv1: lcd-controller@1c74000 { compatible = "allwinner,sun8i-r40-tcon-tv-1"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_tv1_in: port@0 { reg = <0>; tcon_tv1_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>; }; }; tcon_tv1_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; // endpoints to TV TOP and TCON TOP HDMI input ... };}; };
tcon_lcd0 and tcon_lcd1 would have similar connections, except that for outputs would be LCD or LVDS panels or MIPI DSI encoder.
Please note that each TCON (there are 4 of them) would need to have unique compatible and have HW index stored in quirks data. It would be used by TCON TOP driver for configuring muxes.
Can't we use the port/endpoint ID instead? If the mux is the only thing that changes, the compatible has no reason to. It's the same IP, and the only thing that changes is something that is not part of that IP.
Maxime
On Fri, Jun 1, 2018 at 8:29 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
On Thu, May 31, 2018 at 07:54:08PM +0200, Jernej Škrabec wrote:
Dne četrtek, 31. maj 2018 ob 11:21:33 CEST je Maxime Ripard napisal(a):
On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote:
> > + if (tcon->quirks->needs_tcon_top) { > > + struct device_node *np; > > + > > + np = of_parse_phandle(dev->of_node, "allwinner,tcon-top", > > 0); > > + if (np) { > > + struct platform_device *pdev; > > + > > + pdev = of_find_device_by_node(np); > > + if (pdev) > > + tcon->tcon_top = > > platform_get_drvdata(pdev); > > + of_node_put(np); > > + > > + if (!tcon->tcon_top) > > + return -EPROBE_DEFER; > > + } > > + } > > + > > I might have missed it, but I've not seen the bindings additions for > that property. This shouldn't really be done that way anyway, instead > of using a direct phandle, you should be using the of-graph, with the > TCON-top sitting where it belongs in the flow of data.
Just to answer to the first question, it did describe it in "[PATCH 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline".
As why I designed it that way - HW representation could be described that way> >> (ASCII art makes sense when fixed width font is used to view it): / LCD0/LVDS0
/ TCON-LCD0 | \ MIPI DSImixer0 |
\ / TCON-LCD1 - LCD1/LVDS1 TCON-TOP / \ TCON-TV0 - TVE0/RGBmixer1 | \
| TCON-TOP - HDMI | | / \ TCON-TV1 - TVE1/RGBThis is a bit simplified, since there is also TVE-TOP, which is responsible for sharing 4 DACs between both TVE encoders. You can have two TV outs (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It even seems that you can arbitrarly choose which DAC is responsible for which signal, so there is a ton of possible end combinations, but I'm not 100% sure.
Even though I wrote TCON-TOP twice, this is same unit in HW. R40 manual suggest more possibilities, although some of them seem wrong, like RGB feeding from LCD TCON. That is confirmed to be wrong when checking BSP code.
Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, TVE1 and LCD1 for pin muxing, although I'm not sure why is that needed at all, since according to R40 datasheet, TVE0 and TVE1 pins are dedicated and not on PORT D and PORT H, respectively, as TCON-TOP documentation suggest. However, HSYNC and PSYNC lines might be shared between TVE (when it works in RGB mode) and LCD. But that is just my guess since I'm not really familiar with RGB and LCD interfaces.
I'm really not sure what would be the best representation in OF-graph. Can you suggest one?
Rob might disagree on this one, but I don't see anything wrong with having loops in the graph. If the TCON-TOP can be both the input and output of the TCONs, then so be it, and have it described that way in the graph.
The code is already able to filter out nodes that have already been added to the list of devices we need to wait for in the component framework, so that should work as well.
And we'd need to describe TVE-TOP as well, even though we don't have a driver for it yet. That will simplify the backward compatibility later on.
I'm getting the feeling that TCON-TOP / TVE-TOP is the glue layer that binds everything together, and provides signal routing, kind of like DE-TOP on A64. So the signal mux controls that were originally found in TCON0 and TVE0 were moved out.
The driver needs to know about that, but the graph about doesn't make much sense directly. Without looking at the manual, I understand it to likely be one mux between the mixers and TCONs, and one between the TCON-TVs and HDMI. Would it make more sense to just have the graph connections between the muxed components, and remove TCON-TOP from it, like we had in the past? A phandle could be used to reference the TCON-TOP for mux controls, in addition to the clocks and resets.
For TVE, we would need something to represent each of the output pins, so the device tree can actually describe what kind of signal, be it each component of RGB/YUV or composite video, is wanted on each pin, if any. This is also needed on the A20 for the Cubietruck, so we can describe which pins are tied to the VGA connector, and which one does R, G, or B.
I guess we'll see how the DT maintainers feel about this, but my impression is that the OF graph should model the flow of data between the devices. If there's a mux somewhere, then the data is definitely going through it, and as such it should be part of the graph.
I concur, but I spent few days thinking how to represent this sanely in graph, but I didn't find any good solution. I'll represent here my idea and please tell your opinion before I start implementing it.
First, let me be clear that mixer0 and mixer1 don't have same capabilities (different number of planes, mixer0 supports writeback, mixer1 does not, etc.). Thus, it does matter which mixer is connected to which TCON/encoder. mixer0 is meant to be connected to main display and mixer1 to auxiliary. That obviously depends on end system.
So, TCON TOP has 3 muxes, which have to be represented in graph. Two of them are for mixer/TCON relationship (each of them 1 input and 4 possible outputs) and one for TV TCON/HDMI pair selection (2 possible inputs, 1 output).
According to current practice in sun4i-drm driver, graph has to have port 0, representing input and port 1, representing output. This would mean that graph looks something like that:
tcon_top: tcon-top@1c70000 { compatible = "allwinner,sun8i-r40-tcon-top"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_top_in: port@0 { #address-cells = <1>; #size-cells = <0>; reg = <0>; tcon_top_in_mixer0: endpoint@0 { reg = <0>; remote-endpoint = <&mixer0_out_tcon_top>; }; tcon_top_in_mixer1: endpoint@1 { reg = <1>; remote-endpoint = <&mixer1_out_tcon_top>; }; tcon_top_in_tcon_tv: endpoint@2 { reg = <2>; // here is HDMI input connection, part of board DTS remote-endpoint = <board specific phandle to TV TCON output>; }; }; tcon_top_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; tcon_top_out_tcon0: endpoint@0 { reg = <0>; // here is mixer0 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_tcon1: endpoint@1 { reg = <1>; // here is mixer1 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_hdmi: endpoint@2 { reg = <2>; remote-endpoint = <&hdmi_in_tcon_top>; }; }; };};
IIRC, each port is supposed to be one route for the data, so we would have multiple ports, one for the mixers in input and for the tcon in output, and one for the TCON in input and for the HDMI/TV in output. Rob might correct me here.
tcon_tv0: lcd-controller@1c73000 { compatible = "allwinner,sun8i-r40-tcon-tv-0"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_tv0_in: port@0 { reg = <0>; tcon_tv0_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>;Just curious, what would be there?
}; }; tcon_tv0_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; // endpoints to TV TOP and TCON TOP HDMI input ... }; };};
tcon_tv1: lcd-controller@1c74000 { compatible = "allwinner,sun8i-r40-tcon-tv-1"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_tv1_in: port@0 { reg = <0>; tcon_tv1_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>; }; }; tcon_tv1_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; // endpoints to TV TOP and TCON TOP HDMI input ... }; };};
tcon_lcd0 and tcon_lcd1 would have similar connections, except that for outputs would be LCD or LVDS panels or MIPI DSI encoder.
Please note that each TCON (there are 4 of them) would need to have unique compatible and have HW index stored in quirks data. It would be used by TCON TOP driver for configuring muxes.
Can't we use the port/endpoint ID instead? If the mux is the only thing that changes, the compatible has no reason to. It's the same IP, and the only thing that changes is something that is not part of that IP.
I agree. Endpoint IDs should provide that information. I'm still not sure How to encode multiple in/out mux groups in a device node though.
ChenYu
On Fri, Jun 01, 2018 at 09:19:43AM -0700, Chen-Yu Tsai wrote:
On Fri, Jun 1, 2018 at 8:29 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
On Thu, May 31, 2018 at 07:54:08PM +0200, Jernej Škrabec wrote:
Dne četrtek, 31. maj 2018 ob 11:21:33 CEST je Maxime Ripard napisal(a):
On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote:
> > > + if (tcon->quirks->needs_tcon_top) { > > > + struct device_node *np; > > > + > > > + np = of_parse_phandle(dev->of_node, "allwinner,tcon-top", > > > 0); > > > + if (np) { > > > + struct platform_device *pdev; > > > + > > > + pdev = of_find_device_by_node(np); > > > + if (pdev) > > > + tcon->tcon_top = > > > platform_get_drvdata(pdev); > > > + of_node_put(np); > > > + > > > + if (!tcon->tcon_top) > > > + return -EPROBE_DEFER; > > > + } > > > + } > > > + > > > > I might have missed it, but I've not seen the bindings additions for > > that property. This shouldn't really be done that way anyway, instead > > of using a direct phandle, you should be using the of-graph, with the > > TCON-top sitting where it belongs in the flow of data. > > Just to answer to the first question, it did describe it in "[PATCH > 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline". > > As why I designed it that way - HW representation could be described > that way> >> > (ASCII art makes sense when fixed width font is used to view it): > / LCD0/LVDS0 > > / TCON-LCD0 > > | \ MIPI DSI > > mixer0 | > > \ / TCON-LCD1 - LCD1/LVDS1 > > TCON-TOP > > / \ TCON-TV0 - TVE0/RGB > > mixer1 | \ > > | TCON-TOP - HDMI > | > | / > > \ TCON-TV1 - TVE1/RGB > > This is a bit simplified, since there is also TVE-TOP, which is > responsible > for sharing 4 DACs between both TVE encoders. You can have two TV outs > (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It even > seems that you can arbitrarly choose which DAC is responsible for > which signal, so there is a ton of possible end combinations, but I'm > not 100% sure. > > Even though I wrote TCON-TOP twice, this is same unit in HW. R40 manual > suggest more possibilities, although some of them seem wrong, like RGB > feeding from LCD TCON. That is confirmed to be wrong when checking BSP > code. > > Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, TVE1 and > LCD1 for pin muxing, although I'm not sure why is that needed at all, > since according to R40 datasheet, TVE0 and TVE1 pins are dedicated and > not on PORT D and PORT H, respectively, as TCON-TOP documentation > suggest. However, HSYNC and PSYNC lines might be shared between TVE > (when it works in RGB mode) and LCD. But that is just my guess since > I'm not really familiar with RGB and LCD interfaces. > > I'm really not sure what would be the best representation in OF-graph. > Can you suggest one?
Rob might disagree on this one, but I don't see anything wrong with having loops in the graph. If the TCON-TOP can be both the input and output of the TCONs, then so be it, and have it described that way in the graph.
The code is already able to filter out nodes that have already been added to the list of devices we need to wait for in the component framework, so that should work as well.
And we'd need to describe TVE-TOP as well, even though we don't have a driver for it yet. That will simplify the backward compatibility later on.
I'm getting the feeling that TCON-TOP / TVE-TOP is the glue layer that binds everything together, and provides signal routing, kind of like DE-TOP on A64. So the signal mux controls that were originally found in TCON0 and TVE0 were moved out.
The driver needs to know about that, but the graph about doesn't make much sense directly. Without looking at the manual, I understand it to likely be one mux between the mixers and TCONs, and one between the TCON-TVs and HDMI. Would it make more sense to just have the graph connections between the muxed components, and remove TCON-TOP from it, like we had in the past? A phandle could be used to reference the TCON-TOP for mux controls, in addition to the clocks and resets.
For TVE, we would need something to represent each of the output pins, so the device tree can actually describe what kind of signal, be it each component of RGB/YUV or composite video, is wanted on each pin, if any. This is also needed on the A20 for the Cubietruck, so we can describe which pins are tied to the VGA connector, and which one does R, G, or B.
I guess we'll see how the DT maintainers feel about this, but my impression is that the OF graph should model the flow of data between the devices. If there's a mux somewhere, then the data is definitely going through it, and as such it should be part of the graph.
I concur, but I spent few days thinking how to represent this sanely in graph, but I didn't find any good solution. I'll represent here my idea and please tell your opinion before I start implementing it.
First, let me be clear that mixer0 and mixer1 don't have same capabilities (different number of planes, mixer0 supports writeback, mixer1 does not, etc.). Thus, it does matter which mixer is connected to which TCON/encoder. mixer0 is meant to be connected to main display and mixer1 to auxiliary. That obviously depends on end system.
So, TCON TOP has 3 muxes, which have to be represented in graph. Two of them are for mixer/TCON relationship (each of them 1 input and 4 possible outputs) and one for TV TCON/HDMI pair selection (2 possible inputs, 1 output).
According to current practice in sun4i-drm driver, graph has to have port 0, representing input and port 1, representing output. This would mean that graph looks something like that:
tcon_top: tcon-top@1c70000 { compatible = "allwinner,sun8i-r40-tcon-top"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_top_in: port@0 { #address-cells = <1>; #size-cells = <0>; reg = <0>; tcon_top_in_mixer0: endpoint@0 { reg = <0>; remote-endpoint = <&mixer0_out_tcon_top>; }; tcon_top_in_mixer1: endpoint@1 { reg = <1>; remote-endpoint = <&mixer1_out_tcon_top>; }; tcon_top_in_tcon_tv: endpoint@2 { reg = <2>; // here is HDMI input connection, part of board DTS remote-endpoint = <board specific phandle to TV TCON output>; }; }; tcon_top_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; tcon_top_out_tcon0: endpoint@0 { reg = <0>; // here is mixer0 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_tcon1: endpoint@1 { reg = <1>; // here is mixer1 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_hdmi: endpoint@2 { reg = <2>; remote-endpoint = <&hdmi_in_tcon_top>; }; }; };};
IIRC, each port is supposed to be one route for the data, so we would have multiple ports, one for the mixers in input and for the tcon in output, and one for the TCON in input and for the HDMI/TV in output. Rob might correct me here.
tcon_tv0: lcd-controller@1c73000 { compatible = "allwinner,sun8i-r40-tcon-tv-0"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_tv0_in: port@0 { reg = <0>; tcon_tv0_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>;Just curious, what would be there?
}; }; tcon_tv0_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; // endpoints to TV TOP and TCON TOP HDMI input ... }; };};
tcon_tv1: lcd-controller@1c74000 { compatible = "allwinner,sun8i-r40-tcon-tv-1"; ... ports { #address-cells = <1>; #size-cells = <0>;
tcon_tv1_in: port@0 { reg = <0>; tcon_tv1_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>; }; }; tcon_tv1_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; // endpoints to TV TOP and TCON TOP HDMI input ... }; };};
tcon_lcd0 and tcon_lcd1 would have similar connections, except that for outputs would be LCD or LVDS panels or MIPI DSI encoder.
Please note that each TCON (there are 4 of them) would need to have unique compatible and have HW index stored in quirks data. It would be used by TCON TOP driver for configuring muxes.
Can't we use the port/endpoint ID instead? If the mux is the only thing that changes, the compatible has no reason to. It's the same IP, and the only thing that changes is something that is not part of that IP.
I agree. Endpoint IDs should provide that information. I'm still not sure How to encode multiple in/out mux groups in a device node though.
I guess we can do that through different ports?
Maxime
Dne ponedeljek, 04. junij 2018 ob 13:50:34 CEST je Maxime Ripard napisal(a):
On Fri, Jun 01, 2018 at 09:19:43AM -0700, Chen-Yu Tsai wrote:
On Fri, Jun 1, 2018 at 8:29 AM, Maxime Ripard maxime.ripard@bootlin.com
wrote:
On Thu, May 31, 2018 at 07:54:08PM +0200, Jernej Škrabec wrote:
Dne četrtek, 31. maj 2018 ob 11:21:33 CEST je Maxime Ripard napisal(a):
On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote:
>> > > + if (tcon->quirks->needs_tcon_top) { >> > > + struct device_node *np; >> > > + >> > > + np = of_parse_phandle(dev->of_node, >> > > "allwinner,tcon-top", >> > > 0); >> > > + if (np) { >> > > + struct platform_device *pdev; >> > > + >> > > + pdev = of_find_device_by_node(np); >> > > + if (pdev) >> > > + tcon->tcon_top = >> > > platform_get_drvdata(pdev); >> > > + of_node_put(np); >> > > + >> > > + if (!tcon->tcon_top) >> > > + return -EPROBE_DEFER; >> > > + } >> > > + } >> > > + >> > >> > I might have missed it, but I've not seen the bindings >> > additions for >> > that property. This shouldn't really be done that way anyway, >> > instead >> > of using a direct phandle, you should be using the of-graph, >> > with the >> > TCON-top sitting where it belongs in the flow of data. >> >> Just to answer to the first question, it did describe it in >> "[PATCH >> 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline". >> >> As why I designed it that way - HW representation could be >> described >> that way> >> >> >> (ASCII art makes sense when fixed width font is used to view
it):
>> / LCD0/LVDS0 >> >> / TCON-LCD0 >> >> | \ MIPI DSI >> >> mixer0 | >> >> \ / TCON-LCD1 - LCD1/LVDS1 >> >> TCON-TOP >> >> / \ TCON-TV0 - TVE0/RGB >> >> mixer1 | \ >> >> | TCON-TOP - HDMI >> | >> | / >> >> \ TCON-TV1 - TVE1/RGB >> >> This is a bit simplified, since there is also TVE-TOP, which is >> responsible >> for sharing 4 DACs between both TVE encoders. You can have two >> TV outs >> (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It >> even >> seems that you can arbitrarly choose which DAC is responsible >> for >> which signal, so there is a ton of possible end combinations, >> but I'm >> not 100% sure. >> >> Even though I wrote TCON-TOP twice, this is same unit in HW. R40 >> manual >> suggest more possibilities, although some of them seem wrong, >> like RGB >> feeding from LCD TCON. That is confirmed to be wrong when >> checking BSP >> code. >> >> Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, >> TVE1 and >> LCD1 for pin muxing, although I'm not sure why is that needed at >> all, >> since according to R40 datasheet, TVE0 and TVE1 pins are >> dedicated and >> not on PORT D and PORT H, respectively, as TCON-TOP >> documentation >> suggest. However, HSYNC and PSYNC lines might be shared between >> TVE >> (when it works in RGB mode) and LCD. But that is just my guess >> since >> I'm not really familiar with RGB and LCD interfaces. >> >> I'm really not sure what would be the best representation in >> OF-graph. >> Can you suggest one? > > Rob might disagree on this one, but I don't see anything wrong > with > having loops in the graph. If the TCON-TOP can be both the input > and > output of the TCONs, then so be it, and have it described that > way in > the graph. > > The code is already able to filter out nodes that have already > been > added to the list of devices we need to wait for in the component > framework, so that should work as well. > > And we'd need to describe TVE-TOP as well, even though we don't > have a > driver for it yet. That will simplify the backward compatibility > later > on.
I'm getting the feeling that TCON-TOP / TVE-TOP is the glue layer that binds everything together, and provides signal routing, kind of like DE-TOP on A64. So the signal mux controls that were originally found in TCON0 and TVE0 were moved out.
The driver needs to know about that, but the graph about doesn't make much sense directly. Without looking at the manual, I understand it to likely be one mux between the mixers and TCONs, and one between the TCON-TVs and HDMI. Would it make more sense to just have the graph connections between the muxed components, and remove TCON-TOP from it, like we had in the past? A phandle could be used to reference the TCON-TOP for mux controls, in addition to the clocks and resets.
For TVE, we would need something to represent each of the output pins, so the device tree can actually describe what kind of signal, be it each component of RGB/YUV or composite video, is wanted on each pin, if any. This is also needed on the A20 for the Cubietruck, so we can describe which pins are tied to the VGA connector, and which one does R, G, or B.
I guess we'll see how the DT maintainers feel about this, but my impression is that the OF graph should model the flow of data between the devices. If there's a mux somewhere, then the data is definitely going through it, and as such it should be part of the graph.
I concur, but I spent few days thinking how to represent this sanely in graph, but I didn't find any good solution. I'll represent here my idea and please tell your opinion before I start implementing it.
First, let me be clear that mixer0 and mixer1 don't have same capabilities (different number of planes, mixer0 supports writeback, mixer1 does not, etc.). Thus, it does matter which mixer is connected to which TCON/encoder. mixer0 is meant to be connected to main display and mixer1 to auxiliary. That obviously depends on end system.
So, TCON TOP has 3 muxes, which have to be represented in graph. Two of them are for mixer/TCON relationship (each of them 1 input and 4 possible outputs) and one for TV TCON/HDMI pair selection (2 possible inputs, 1 output).
According to current practice in sun4i-drm driver, graph has to have port 0, representing input and port 1, representing output. This would mean that graph looks something like that:
tcon_top: tcon-top@1c70000 {
compatible = "allwinner,sun8i-r40-tcon-top"; ... ports { #address-cells = <1>; #size-cells = <0>; tcon_top_in: port@0 { #address-cells = <1>; #size-cells = <0>; reg = <0>; tcon_top_in_mixer0: endpoint@0 { reg = <0>; remote-endpoint = <&mixer0_out_tcon_top>; }; tcon_top_in_mixer1: endpoint@1 { reg = <1>; remote-endpoint = <&mixer1_out_tcon_top>; }; tcon_top_in_tcon_tv: endpoint@2 { reg = <2>; // here is HDMI input connection, part of board DTS remote-endpoint = <board specific phandle to TV TCON output>; }; }; tcon_top_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; tcon_top_out_tcon0: endpoint@0 { reg = <0>; // here is mixer0 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_tcon1: endpoint@1 { reg = <1>; // here is mixer1 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_hdmi: endpoint@2 { reg = <2>; remote-endpoint = <&hdmi_in_tcon_top>; }; }; };};
IIRC, each port is supposed to be one route for the data, so we would have multiple ports, one for the mixers in input and for the tcon in output, and one for the TCON in input and for the HDMI/TV in output. Rob might correct me here.
Ok, that seems more clean approach. I'll have to extend graph traversing algorithm in sun4i_drv.c, but that's no problem.
Just to be clear, you have in mind 3 pairs of ports (0/1 for mixer0 mux, 2/3 for mixer1 and 4/5 for HDMI input), right? That way each mux is represented with one pair of ports, even numbered for input and odd numbered for output.
tcon_tv0: lcd-controller@1c73000 {
compatible = "allwinner,sun8i-r40-tcon-tv-0"; ... ports { #address-cells = <1>; #size-cells = <0>; tcon_tv0_in: port@0 { reg = <0>; tcon_tv0_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>;Just curious, what would be there?
Either phandle to tcon_top_out_tcon0 or tcon_top_out_tcon1.
}; }; tcon_tv0_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; // endpoints to TV TOP and TCON TOP HDMI input ... }; };};
tcon_tv1: lcd-controller@1c74000 {
compatible = "allwinner,sun8i-r40-tcon-tv-1"; ... ports { #address-cells = <1>; #size-cells = <0>; tcon_tv1_in: port@0 { reg = <0>; tcon_tv1_in_tcon_top: endpoint { // endpoint depends on board, part of board DTS remote-endpoint = <phandle to one of tcon_top_out_tcon>; }; }; tcon_tv1_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; // endpoints to TV TOP and TCON TOP HDMI input ... }; };};
tcon_lcd0 and tcon_lcd1 would have similar connections, except that for outputs would be LCD or LVDS panels or MIPI DSI encoder.
Please note that each TCON (there are 4 of them) would need to have unique compatible and have HW index stored in quirks data. It would be used by TCON TOP driver for configuring muxes.
Can't we use the port/endpoint ID instead? If the mux is the only thing that changes, the compatible has no reason to. It's the same IP, and the only thing that changes is something that is not part of that IP.
I agree. Endpoint IDs should provide that information. I'm still not sure How to encode multiple in/out mux groups in a device node though.
I guess we can do that through different ports?
Ok, I'll try to do something with "reg" property.
Best regards, Jernej
On Mon, Jun 04, 2018 at 05:09:56PM +0200, Jernej Škrabec wrote:
Dne ponedeljek, 04. junij 2018 ob 13:50:34 CEST je Maxime Ripard napisal(a):
On Fri, Jun 01, 2018 at 09:19:43AM -0700, Chen-Yu Tsai wrote:
On Fri, Jun 1, 2018 at 8:29 AM, Maxime Ripard maxime.ripard@bootlin.com
wrote:
On Thu, May 31, 2018 at 07:54:08PM +0200, Jernej Škrabec wrote:
Dne četrtek, 31. maj 2018 ob 11:21:33 CEST je Maxime Ripard napisal(a):
On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote: > >> > > + if (tcon->quirks->needs_tcon_top) { > >> > > + struct device_node *np; > >> > > + > >> > > + np = of_parse_phandle(dev->of_node, > >> > > "allwinner,tcon-top", > >> > > 0); > >> > > + if (np) { > >> > > + struct platform_device *pdev; > >> > > + > >> > > + pdev = of_find_device_by_node(np); > >> > > + if (pdev) > >> > > + tcon->tcon_top = > >> > > platform_get_drvdata(pdev); > >> > > + of_node_put(np); > >> > > + > >> > > + if (!tcon->tcon_top) > >> > > + return -EPROBE_DEFER; > >> > > + } > >> > > + } > >> > > + > >> > > >> > I might have missed it, but I've not seen the bindings > >> > additions for > >> > that property. This shouldn't really be done that way anyway, > >> > instead > >> > of using a direct phandle, you should be using the of-graph, > >> > with the > >> > TCON-top sitting where it belongs in the flow of data. > >> > >> Just to answer to the first question, it did describe it in > >> "[PATCH > >> 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline". > >> > >> As why I designed it that way - HW representation could be > >> described > >> that way> >> > >> > >> (ASCII art makes sense when fixed width font is used to view
it):
> >> / LCD0/LVDS0 > >> > >> / TCON-LCD0 > >> > >> | \ MIPI DSI > >> > >> mixer0 | > >> > >> \ / TCON-LCD1 - LCD1/LVDS1 > >> > >> TCON-TOP > >> > >> / \ TCON-TV0 - TVE0/RGB > >> > >> mixer1 | \ > >> > >> | TCON-TOP - HDMI > >> | > >> | / > >> > >> \ TCON-TV1 - TVE1/RGB > >> > >> This is a bit simplified, since there is also TVE-TOP, which is > >> responsible > >> for sharing 4 DACs between both TVE encoders. You can have two > >> TV outs > >> (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It > >> even > >> seems that you can arbitrarly choose which DAC is responsible > >> for > >> which signal, so there is a ton of possible end combinations, > >> but I'm > >> not 100% sure. > >> > >> Even though I wrote TCON-TOP twice, this is same unit in HW. R40 > >> manual > >> suggest more possibilities, although some of them seem wrong, > >> like RGB > >> feeding from LCD TCON. That is confirmed to be wrong when > >> checking BSP > >> code. > >> > >> Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, > >> TVE1 and > >> LCD1 for pin muxing, although I'm not sure why is that needed at > >> all, > >> since according to R40 datasheet, TVE0 and TVE1 pins are > >> dedicated and > >> not on PORT D and PORT H, respectively, as TCON-TOP > >> documentation > >> suggest. However, HSYNC and PSYNC lines might be shared between > >> TVE > >> (when it works in RGB mode) and LCD. But that is just my guess > >> since > >> I'm not really familiar with RGB and LCD interfaces. > >> > >> I'm really not sure what would be the best representation in > >> OF-graph. > >> Can you suggest one? > > > > Rob might disagree on this one, but I don't see anything wrong > > with > > having loops in the graph. If the TCON-TOP can be both the input > > and > > output of the TCONs, then so be it, and have it described that > > way in > > the graph. > > > > The code is already able to filter out nodes that have already > > been > > added to the list of devices we need to wait for in the component > > framework, so that should work as well. > > > > And we'd need to describe TVE-TOP as well, even though we don't > > have a > > driver for it yet. That will simplify the backward compatibility > > later > > on. > > I'm getting the feeling that TCON-TOP / TVE-TOP is the glue layer > that > binds everything together, and provides signal routing, kind of > like > DE-TOP on A64. So the signal mux controls that were originally > found > in TCON0 and TVE0 were moved out. > > The driver needs to know about that, but the graph about doesn't > make > much sense directly. Without looking at the manual, I understand it > to > likely be one mux between the mixers and TCONs, and one between the > TCON-TVs and HDMI. Would it make more sense to just have the graph > connections between the muxed components, and remove TCON-TOP from > it, like we had in the past? A phandle could be used to reference > the TCON-TOP for mux controls, in addition to the clocks and > resets. > > For TVE, we would need something to represent each of the output > pins, > so the device tree can actually describe what kind of signal, be it > each component of RGB/YUV or composite video, is wanted on each > pin, > if any. This is also needed on the A20 for the Cubietruck, so we > can > describe which pins are tied to the VGA connector, and which one > does > R, G, or B.
I guess we'll see how the DT maintainers feel about this, but my impression is that the OF graph should model the flow of data between the devices. If there's a mux somewhere, then the data is definitely going through it, and as such it should be part of the graph.
I concur, but I spent few days thinking how to represent this sanely in graph, but I didn't find any good solution. I'll represent here my idea and please tell your opinion before I start implementing it.
First, let me be clear that mixer0 and mixer1 don't have same capabilities (different number of planes, mixer0 supports writeback, mixer1 does not, etc.). Thus, it does matter which mixer is connected to which TCON/encoder. mixer0 is meant to be connected to main display and mixer1 to auxiliary. That obviously depends on end system.
So, TCON TOP has 3 muxes, which have to be represented in graph. Two of them are for mixer/TCON relationship (each of them 1 input and 4 possible outputs) and one for TV TCON/HDMI pair selection (2 possible inputs, 1 output).
According to current practice in sun4i-drm driver, graph has to have port 0, representing input and port 1, representing output. This would mean that graph looks something like that:
tcon_top: tcon-top@1c70000 {
compatible = "allwinner,sun8i-r40-tcon-top"; ... ports { #address-cells = <1>; #size-cells = <0>; tcon_top_in: port@0 { #address-cells = <1>; #size-cells = <0>; reg = <0>; tcon_top_in_mixer0: endpoint@0 { reg = <0>; remote-endpoint = <&mixer0_out_tcon_top>; }; tcon_top_in_mixer1: endpoint@1 { reg = <1>; remote-endpoint = <&mixer1_out_tcon_top>; }; tcon_top_in_tcon_tv: endpoint@2 { reg = <2>; // here is HDMI input connection, part of board DTS remote-endpoint = <board specific phandle to TV TCON output>; }; }; tcon_top_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; tcon_top_out_tcon0: endpoint@0 { reg = <0>; // here is mixer0 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_tcon1: endpoint@1 { reg = <1>; // here is mixer1 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_hdmi: endpoint@2 { reg = <2>; remote-endpoint = <&hdmi_in_tcon_top>; }; }; };};
IIRC, each port is supposed to be one route for the data, so we would have multiple ports, one for the mixers in input and for the tcon in output, and one for the TCON in input and for the HDMI/TV in output. Rob might correct me here.
Ok, that seems more clean approach. I'll have to extend graph traversing algorithm in sun4i_drv.c, but that's no problem.
Just to be clear, you have in mind 3 pairs of ports (0/1 for mixer0 mux, 2/3 for mixer1 and 4/5 for HDMI input), right? That way each mux is represented with one pair of ports, even numbered for input and odd numbered for output.
Yep, unless Rob feels otherwise.
Maxime
Dne ponedeljek, 04. junij 2018 ob 18:23:57 CEST je Maxime Ripard napisal(a):
On Mon, Jun 04, 2018 at 05:09:56PM +0200, Jernej Škrabec wrote:
Dne ponedeljek, 04. junij 2018 ob 13:50:34 CEST je Maxime Ripard
napisal(a):
On Fri, Jun 01, 2018 at 09:19:43AM -0700, Chen-Yu Tsai wrote:
On Fri, Jun 1, 2018 at 8:29 AM, Maxime Ripard maxime.ripard@bootlin.com
wrote:
On Thu, May 31, 2018 at 07:54:08PM +0200, Jernej Škrabec wrote:
Dne četrtek, 31. maj 2018 ob 11:21:33 CEST je Maxime Ripard
napisal(a):
> On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote: > > >> > > + if (tcon->quirks->needs_tcon_top) { > > >> > > + struct device_node *np; > > >> > > + > > >> > > + np = of_parse_phandle(dev->of_node, > > >> > > "allwinner,tcon-top", > > >> > > 0); > > >> > > + if (np) { > > >> > > + struct platform_device *pdev; > > >> > > + > > >> > > + pdev = of_find_device_by_node(np); > > >> > > + if (pdev) > > >> > > + tcon->tcon_top = > > >> > > platform_get_drvdata(pdev); > > >> > > + of_node_put(np); > > >> > > + > > >> > > + if (!tcon->tcon_top) > > >> > > + return -EPROBE_DEFER; > > >> > > + } > > >> > > + } > > >> > > + > > >> > > > >> > I might have missed it, but I've not seen the bindings > > >> > additions for > > >> > that property. This shouldn't really be done that way > > >> > anyway, > > >> > instead > > >> > of using a direct phandle, you should be using the > > >> > of-graph, > > >> > with the > > >> > TCON-top sitting where it belongs in the flow of data. > > >> > > >> Just to answer to the first question, it did describe it in > > >> "[PATCH > > >> 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI > > >> pipeline". > > >> > > >> As why I designed it that way - HW representation could be > > >> described > > >> that way> >> > > >> > > >> (ASCII art makes sense when fixed width font is used to view
it):
> > >> / LCD0/LVDS0 > > >> > > >> / TCON-LCD0 > > >> > > >> | \ MIPI DSI > > >> > > >> mixer0 | > > >> > > >> \ / TCON-LCD1 - LCD1/LVDS1 > > >> > > >> TCON-TOP > > >> > > >> / \ TCON-TV0 - TVE0/RGB > > >> > > >> mixer1 | \ > > >> > > >> | TCON-TOP - HDMI > > >> | > > >> | / > > >> > > >> \ TCON-TV1 - TVE1/RGB > > >> > > >> This is a bit simplified, since there is also TVE-TOP, which > > >> is > > >> responsible > > >> for sharing 4 DACs between both TVE encoders. You can have > > >> two > > >> TV outs > > >> (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. > > >> It > > >> even > > >> seems that you can arbitrarly choose which DAC is > > >> responsible > > >> for > > >> which signal, so there is a ton of possible end > > >> combinations, > > >> but I'm > > >> not 100% sure. > > >> > > >> Even though I wrote TCON-TOP twice, this is same unit in HW. > > >> R40 > > >> manual > > >> suggest more possibilities, although some of them seem > > >> wrong, > > >> like RGB > > >> feeding from LCD TCON. That is confirmed to be wrong when > > >> checking BSP > > >> code. > > >> > > >> Additionally, TCON-TOP comes in the middle of TVE0 and LCD0, > > >> TVE1 and > > >> LCD1 for pin muxing, although I'm not sure why is that > > >> needed at > > >> all, > > >> since according to R40 datasheet, TVE0 and TVE1 pins are > > >> dedicated and > > >> not on PORT D and PORT H, respectively, as TCON-TOP > > >> documentation > > >> suggest. However, HSYNC and PSYNC lines might be shared > > >> between > > >> TVE > > >> (when it works in RGB mode) and LCD. But that is just my > > >> guess > > >> since > > >> I'm not really familiar with RGB and LCD interfaces. > > >> > > >> I'm really not sure what would be the best representation in > > >> OF-graph. > > >> Can you suggest one? > > > > > > Rob might disagree on this one, but I don't see anything > > > wrong > > > with > > > having loops in the graph. If the TCON-TOP can be both the > > > input > > > and > > > output of the TCONs, then so be it, and have it described > > > that > > > way in > > > the graph. > > > > > > The code is already able to filter out nodes that have > > > already > > > been > > > added to the list of devices we need to wait for in the > > > component > > > framework, so that should work as well. > > > > > > And we'd need to describe TVE-TOP as well, even though we > > > don't > > > have a > > > driver for it yet. That will simplify the backward > > > compatibility > > > later > > > on. > > > > I'm getting the feeling that TCON-TOP / TVE-TOP is the glue > > layer > > that > > binds everything together, and provides signal routing, kind of > > like > > DE-TOP on A64. So the signal mux controls that were originally > > found > > in TCON0 and TVE0 were moved out. > > > > The driver needs to know about that, but the graph about > > doesn't > > make > > much sense directly. Without looking at the manual, I > > understand it > > to > > likely be one mux between the mixers and TCONs, and one between > > the > > TCON-TVs and HDMI. Would it make more sense to just have the > > graph > > connections between the muxed components, and remove TCON-TOP > > from > > it, like we had in the past? A phandle could be used to > > reference > > the TCON-TOP for mux controls, in addition to the clocks and > > resets. > > > > For TVE, we would need something to represent each of the > > output > > pins, > > so the device tree can actually describe what kind of signal, > > be it > > each component of RGB/YUV or composite video, is wanted on each > > pin, > > if any. This is also needed on the A20 for the Cubietruck, so > > we > > can > > describe which pins are tied to the VGA connector, and which > > one > > does > > R, G, or B. > > I guess we'll see how the DT maintainers feel about this, but my > impression is that the OF graph should model the flow of data > between > the devices. If there's a mux somewhere, then the data is > definitely > going through it, and as such it should be part of the graph.
I concur, but I spent few days thinking how to represent this sanely in graph, but I didn't find any good solution. I'll represent here my idea and please tell your opinion before I start implementing it.
First, let me be clear that mixer0 and mixer1 don't have same capabilities (different number of planes, mixer0 supports writeback, mixer1 does not, etc.). Thus, it does matter which mixer is connected to which TCON/encoder. mixer0 is meant to be connected to main display and mixer1 to auxiliary. That obviously depends on end system.
So, TCON TOP has 3 muxes, which have to be represented in graph. Two of them are for mixer/TCON relationship (each of them 1 input and 4 possible outputs) and one for TV TCON/HDMI pair selection (2 possible inputs, 1 output).
According to current practice in sun4i-drm driver, graph has to have port 0, representing input and port 1, representing output. This would mean that graph looks something like that:
tcon_top: tcon-top@1c70000 {
compatible = "allwinner,sun8i-r40-tcon-top"; ... ports { #address-cells = <1>; #size-cells = <0>; tcon_top_in: port@0 { #address-cells = <1>; #size-cells = <0>; reg = <0>; tcon_top_in_mixer0: endpoint@0 { reg = <0>; remote-endpoint = <&mixer0_out_tcon_top>; }; tcon_top_in_mixer1: endpoint@1 { reg = <1>; remote-endpoint = <&mixer1_out_tcon_top>; }; tcon_top_in_tcon_tv: endpoint@2 { reg = <2>; // here is HDMI input connection, part of board DTS remote-endpoint = <board specific phandle to TV TCON output>; }; }; tcon_top_out: port@1 { #address-cells = <1>; #size-cells = <0>; reg = <1>; tcon_top_out_tcon0: endpoint@0 { reg = <0>; // here is mixer0 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_tcon1: endpoint@1 { reg = <1>; // here is mixer1 output connection, part of board DTS remote-endpoint = <board specific phandle to TCON>; }; tcon_top_out_hdmi: endpoint@2 { reg = <2>; remote-endpoint = <&hdmi_in_tcon_top>; }; }; };};
IIRC, each port is supposed to be one route for the data, so we would have multiple ports, one for the mixers in input and for the tcon in output, and one for the TCON in input and for the HDMI/TV in output. Rob might correct me here.
Ok, that seems more clean approach. I'll have to extend graph traversing algorithm in sun4i_drv.c, but that's no problem.
Just to be clear, you have in mind 3 pairs of ports (0/1 for mixer0 mux, 2/3 for mixer1 and 4/5 for HDMI input), right? That way each mux is represented with one pair of ports, even numbered for input and odd numbered for output.
Yep, unless Rob feels otherwise.
I found an issue with this concept.
HDMI driver (sun8i_dw_hdmi.c) uses drm_of_find_possible_crtcs() to find connected crtcs (TCONs) to HDMI. This function assumes that crtc and encoder are directly connected through of_graph, but that is not the case with TCON TOP HDMI mux anymore. I could give TCON TOP node as an input to this function, but that won't work, since TCON TOP can have connections to other crtcs, not only that of HDMI and they will also be picked up by drm_of_find_possible_crtcs().
Any suggestion how to solve this nicely? I think creating my own version of drm_of_find_possible_crtcs() which considers that case is one way, but not very nice solution. Alternatively, we can fix possible_crtcs to BIT(0), since it always has only one input. This is done in meson_dw_hdmi.c for example.
Best regards, Jernej
Dne četrtek, 07. junij 2018 ob 00:30:24 CEST je Jernej Škrabec napisal(a):
Dne ponedeljek, 04. junij 2018 ob 18:23:57 CEST je Maxime Ripard napisal(a):
On Mon, Jun 04, 2018 at 05:09:56PM +0200, Jernej Škrabec wrote:
Dne ponedeljek, 04. junij 2018 ob 13:50:34 CEST je Maxime Ripard
napisal(a):
On Fri, Jun 01, 2018 at 09:19:43AM -0700, Chen-Yu Tsai wrote:
On Fri, Jun 1, 2018 at 8:29 AM, Maxime Ripard maxime.ripard@bootlin.com
wrote:
On Thu, May 31, 2018 at 07:54:08PM +0200, Jernej Škrabec wrote: > Dne četrtek, 31. maj 2018 ob 11:21:33 CEST je Maxime Ripard
napisal(a):
> > On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote: > > > >> > > + if (tcon->quirks->needs_tcon_top) { > > > >> > > + struct device_node *np; > > > >> > > + > > > >> > > + np = of_parse_phandle(dev->of_node, > > > >> > > "allwinner,tcon-top", > > > >> > > 0); > > > >> > > + if (np) { > > > >> > > + struct platform_device *pdev; > > > >> > > + > > > >> > > + pdev = of_find_device_by_node(np); > > > >> > > + if (pdev) > > > >> > > + tcon->tcon_top = > > > >> > > platform_get_drvdata(pdev); > > > >> > > + of_node_put(np); > > > >> > > + > > > >> > > + if (!tcon->tcon_top) > > > >> > > + return -EPROBE_DEFER; > > > >> > > + } > > > >> > > + } > > > >> > > + > > > >> > > > > >> > I might have missed it, but I've not seen the bindings > > > >> > additions for > > > >> > that property. This shouldn't really be done that way > > > >> > anyway, > > > >> > instead > > > >> > of using a direct phandle, you should be using the > > > >> > of-graph, > > > >> > with the > > > >> > TCON-top sitting where it belongs in the flow of data. > > > >> > > > >> Just to answer to the first question, it did describe it > > > >> in > > > >> "[PATCH > > > >> 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI > > > >> pipeline". > > > >> > > > >> As why I designed it that way - HW representation could be > > > >> described > > > >> that way> >> > > > >> > > > >> (ASCII art makes sense when fixed width font is used to > > > >> view
it):
> > > >> / LCD0/LVDS0 > > > >> > > > >> / TCON-LCD0 > > > >> > > > >> | \ MIPI DSI > > > >> > > > >> mixer0 | > > > >> > > > >> \ / TCON-LCD1 - LCD1/LVDS1 > > > >> > > > >> TCON-TOP > > > >> > > > >> / \ TCON-TV0 - TVE0/RGB > > > >> > > > >> mixer1 | \ > > > >> > > > >> | TCON-TOP - HDMI > > > >> | > > > >> | / > > > >> > > > >> \ TCON-TV1 - TVE1/RGB > > > >> > > > >> This is a bit simplified, since there is also TVE-TOP, > > > >> which > > > >> is > > > >> responsible > > > >> for sharing 4 DACs between both TVE encoders. You can have > > > >> two > > > >> TV outs > > > >> (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice > > > >> versa. > > > >> It > > > >> even > > > >> seems that you can arbitrarly choose which DAC is > > > >> responsible > > > >> for > > > >> which signal, so there is a ton of possible end > > > >> combinations, > > > >> but I'm > > > >> not 100% sure. > > > >> > > > >> Even though I wrote TCON-TOP twice, this is same unit in > > > >> HW. > > > >> R40 > > > >> manual > > > >> suggest more possibilities, although some of them seem > > > >> wrong, > > > >> like RGB > > > >> feeding from LCD TCON. That is confirmed to be wrong when > > > >> checking BSP > > > >> code. > > > >> > > > >> Additionally, TCON-TOP comes in the middle of TVE0 and > > > >> LCD0, > > > >> TVE1 and > > > >> LCD1 for pin muxing, although I'm not sure why is that > > > >> needed at > > > >> all, > > > >> since according to R40 datasheet, TVE0 and TVE1 pins are > > > >> dedicated and > > > >> not on PORT D and PORT H, respectively, as TCON-TOP > > > >> documentation > > > >> suggest. However, HSYNC and PSYNC lines might be shared > > > >> between > > > >> TVE > > > >> (when it works in RGB mode) and LCD. But that is just my > > > >> guess > > > >> since > > > >> I'm not really familiar with RGB and LCD interfaces. > > > >> > > > >> I'm really not sure what would be the best representation > > > >> in > > > >> OF-graph. > > > >> Can you suggest one? > > > > > > > > Rob might disagree on this one, but I don't see anything > > > > wrong > > > > with > > > > having loops in the graph. If the TCON-TOP can be both the > > > > input > > > > and > > > > output of the TCONs, then so be it, and have it described > > > > that > > > > way in > > > > the graph. > > > > > > > > The code is already able to filter out nodes that have > > > > already > > > > been > > > > added to the list of devices we need to wait for in the > > > > component > > > > framework, so that should work as well. > > > > > > > > And we'd need to describe TVE-TOP as well, even though we > > > > don't > > > > have a > > > > driver for it yet. That will simplify the backward > > > > compatibility > > > > later > > > > on. > > > > > > I'm getting the feeling that TCON-TOP / TVE-TOP is the glue > > > layer > > > that > > > binds everything together, and provides signal routing, kind > > > of > > > like > > > DE-TOP on A64. So the signal mux controls that were > > > originally > > > found > > > in TCON0 and TVE0 were moved out. > > > > > > The driver needs to know about that, but the graph about > > > doesn't > > > make > > > much sense directly. Without looking at the manual, I > > > understand it > > > to > > > likely be one mux between the mixers and TCONs, and one > > > between > > > the > > > TCON-TVs and HDMI. Would it make more sense to just have the > > > graph > > > connections between the muxed components, and remove TCON-TOP > > > from > > > it, like we had in the past? A phandle could be used to > > > reference > > > the TCON-TOP for mux controls, in addition to the clocks and > > > resets. > > > > > > For TVE, we would need something to represent each of the > > > output > > > pins, > > > so the device tree can actually describe what kind of signal, > > > be it > > > each component of RGB/YUV or composite video, is wanted on > > > each > > > pin, > > > if any. This is also needed on the A20 for the Cubietruck, so > > > we > > > can > > > describe which pins are tied to the VGA connector, and which > > > one > > > does > > > R, G, or B. > > > > I guess we'll see how the DT maintainers feel about this, but > > my > > impression is that the OF graph should model the flow of data > > between > > the devices. If there's a mux somewhere, then the data is > > definitely > > going through it, and as such it should be part of the graph. > > I concur, but I spent few days thinking how to represent this > sanely in > graph, but I didn't find any good solution. I'll represent here > my > idea and please tell your opinion before I start implementing it. > > First, let me be clear that mixer0 and mixer1 don't have same > capabilities > (different number of planes, mixer0 supports writeback, mixer1 > does > not, > etc.). Thus, it does matter which mixer is connected to which > TCON/encoder. > mixer0 is meant to be connected to main display and mixer1 to > auxiliary. That obviously depends on end system. > > So, TCON TOP has 3 muxes, which have to be represented in graph. > Two of > them are for mixer/TCON relationship (each of them 1 input and 4 > possible outputs) and one for TV TCON/HDMI pair selection (2 > possible > inputs, 1 output). > > According to current practice in sun4i-drm driver, graph has to > have > port 0, representing input and port 1, representing output. This > would > mean that graph looks something like that: > > tcon_top: tcon-top@1c70000 { > > compatible = "allwinner,sun8i-r40-tcon-top"; > ... > ports { > > #address-cells = <1>; > #size-cells = <0>; > > tcon_top_in: port@0 { > > #address-cells = <1>; > #size-cells = <0>; > reg = <0>; > > tcon_top_in_mixer0: endpoint@0 { > > reg = <0>; > remote-endpoint = > <&mixer0_out_tcon_top>; > > }; > > tcon_top_in_mixer1: endpoint@1 { > > reg = <1>; > remote-endpoint = > <&mixer1_out_tcon_top>; > > }; > > tcon_top_in_tcon_tv: endpoint@2 { > > reg = <2>; > // here is HDMI input connection, > part of > board DTS > remote-endpoint = <board specific > phandle > to TV TCON output>; > > }; > > }; > > tcon_top_out: port@1 { > > #address-cells = <1>; > #size-cells = <0>; > reg = <1>; > > tcon_top_out_tcon0: endpoint@0 { > > reg = <0>; > // here is mixer0 output > connection, > part > of board DTS > remote-endpoint = <board specific > phandle > to TCON>; > > }; > > tcon_top_out_tcon1: endpoint@1 { > > reg = <1>; > // here is mixer1 output > connection, > part > of board DTS > remote-endpoint = <board specific > phandle > to TCON>; > > }; > > tcon_top_out_hdmi: endpoint@2 { > > reg = <2>; > remote-endpoint = > <&hdmi_in_tcon_top>; > > }; > > }; > > }; > > };
IIRC, each port is supposed to be one route for the data, so we would have multiple ports, one for the mixers in input and for the tcon in output, and one for the TCON in input and for the HDMI/TV in output. Rob might correct me here.
Ok, that seems more clean approach. I'll have to extend graph traversing algorithm in sun4i_drv.c, but that's no problem.
Just to be clear, you have in mind 3 pairs of ports (0/1 for mixer0 mux, 2/3 for mixer1 and 4/5 for HDMI input), right? That way each mux is represented with one pair of ports, even numbered for input and odd numbered for output.
Yep, unless Rob feels otherwise.
I found an issue with this concept.
HDMI driver (sun8i_dw_hdmi.c) uses drm_of_find_possible_crtcs() to find connected crtcs (TCONs) to HDMI. This function assumes that crtc and encoder are directly connected through of_graph, but that is not the case with TCON TOP HDMI mux anymore. I could give TCON TOP node as an input to this function, but that won't work, since TCON TOP can have connections to other crtcs, not only that of HDMI and they will also be picked up by drm_of_find_possible_crtcs().
Any suggestion how to solve this nicely? I think creating my own version of drm_of_find_possible_crtcs() which considers that case is one way, but not very nice solution. Alternatively, we can fix possible_crtcs to BIT(0),
This doesn't seem like a good solution, since it doesn't work in dual head system. I'll take first approach.
Best regards, Jernej
since it always has only one input. This is done in meson_dw_hdmi.c for example.
Missing compatibles and descriptions needed to implement R40 HDMI pipeline are added.
For mixers only compatibles are added.
TCON description is expanded with R40 TV TCON compatibles. If the SoC has TCON TOP unit, phandle to that unit has to be specified. Additional clock has to be specified if SoC has TCON TOP and TCON is TV TCON.
New compatible is added for DWC HDMI PHY, which has additional clock specified.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- .../bindings/display/sunxi/sun4i-drm.txt | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt index a099957ab62a..634276f713e8 100644 --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt @@ -101,6 +101,7 @@ DWC HDMI PHY
Required properties: - compatible: value must be one of: + * allwinner,sun50i-a64-hdmi-phy * allwinner,sun8i-a83t-hdmi-phy * allwinner,sun8i-h3-hdmi-phy - reg: base address and size of memory-mapped region @@ -111,8 +112,9 @@ Required properties: - resets: phandle to the reset controller driving the PHY - reset-names: must be "phy"
-H3 HDMI PHY requires additional clock: +H3 and A64 HDMI PHY requires additional clocks: - pll-0: parent of phy clock + - pll-1: second possible phy clock parent (A64 only)
TV Encoder ---------- @@ -145,6 +147,8 @@ Required properties: * allwinner,sun8i-a33-tcon * allwinner,sun8i-a83t-tcon-lcd * allwinner,sun8i-a83t-tcon-tv + * allwinner,sun8i-r40-tcon-tv-0 + * allwinner,sun8i-r40-tcon-tv-1 * allwinner,sun8i-v3s-tcon * allwinner,sun9i-a80-tcon-lcd * allwinner,sun9i-a80-tcon-tv @@ -179,7 +183,7 @@ For TCONs with channel 0, there is one more clock required: For TCONs with channel 1, there is one more clock required: - 'tcon-ch1': The clock driving the TCON channel 1
-When TCON support LVDS (all TCONs except TV TCON on A83T and those found +When TCON support LVDS (all TCONs except TV TCONs on A83T, R40 and those found in A13, H3, H5 and V3s SoCs), you need one more reset line: - 'lvds': The reset line driving the LVDS logic
@@ -187,6 +191,12 @@ And on the A23, A31, A31s and A33, you need one more clock line: - 'lvds-alt': An alternative clock source, separate from the TCON channel 0 clock, that can be used to drive the LVDS clock
+If SoC has TCON TOP, like R40, TCON has to have phandle to TCON TOP: + - 'allwinner,tcon-top': Phandle to TCON TOP unit + +TV TCONs which have phandle to TCON TOP need one more clock: + - 'tcon-top': TV TCON gate found in TCON TOP unit + TCON TOP --------
@@ -330,6 +340,8 @@ Required properties: * allwinner,sun8i-a83t-de2-mixer-0 * allwinner,sun8i-a83t-de2-mixer-1 * allwinner,sun8i-h3-de2-mixer-0 + * allwinner,sun8i-r40-de2-mixer-0 + * allwinner,sun8i-r40-de2-mixer-1 * allwinner,sun8i-v3s-de2-mixer - reg: base address and size of the memory-mapped region. - clocks: phandles to the clocks feeding the mixer
Hi Jernej,
On Sun, May 20, 2018 at 4:31 AM, Jernej Skrabec jernej.skrabec@siol.net wrote:
Missing compatibles and descriptions needed to implement R40 HDMI pipeline are added.
For mixers only compatibles are added.
TCON description is expanded with R40 TV TCON compatibles. If the SoC has TCON TOP unit, phandle to that unit has to be specified. Additional clock has to be specified if SoC has TCON TOP and TCON is TV TCON.
New compatible is added for DWC HDMI PHY, which has additional clock specified.
There's a bunch of A64 related stuff mixed in here, is the R40 compatible with the A64's parts? If so, you should probably mention that in the commit log.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
.../bindings/display/sunxi/sun4i-drm.txt | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt index a099957ab62a..634276f713e8 100644 --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt @@ -111,8 +112,9 @@ Required properties:
- resets: phandle to the reset controller driving the PHY
- reset-names: must be "phy"
-H3 HDMI PHY requires additional clock: +H3 and A64 HDMI PHY requires additional clocks:
- pll-0: parent of phy clock
- pll-1: second possible phy clock parent (A64 only)
Maybe split this into two:
H3 HDMI PHY ... - pll-0: ...
A64 HDMI PHY ... - pll-0: ... - pll-1: ...
At the moment a quick reading implies that H3 needs pll-1.
Thanks,
When TCON sets up TCON TOP, it needs to know mixer index. Here we do that by setting engine ID to number provided in mixer index quirk.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/sun4i/sun8i_mixer.c | 4 ++-- drivers/gpu/drm/sun4i/sun8i_mixer.h | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 126899d6f0d3..36d90c76317a 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -353,13 +353,13 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master, dev_set_drvdata(dev, mixer); mixer->engine.ops = &sun8i_engine_ops; mixer->engine.node = dev->of_node; - /* The ID of the mixer currently doesn't matter */ - mixer->engine.id = -1;
mixer->cfg = of_device_get_match_data(dev); if (!mixer->cfg) return -EINVAL;
+ mixer->engine.id = mixer->cfg->index; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); regs = devm_ioremap_resource(dev, res); if (IS_ERR(regs)) diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h index f34e70c42adf..aeda6e9a7627 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h @@ -123,6 +123,7 @@ struct de2_fmt_info { * are invalid. * @mod_rate: module clock rate that needs to be set in order to have * a functional block. + * @index: mixer index, needed to properly set TCON TOP */ struct sun8i_mixer_cfg { int vi_num; @@ -130,6 +131,7 @@ struct sun8i_mixer_cfg { int scaler_mask; int ccsc; unsigned long mod_rate; + int index; };
struct sun8i_mixer {
Both mixers have similar capabilities as others SoCs with DE2.
First mixer has 1 VI and 3 UI planes and supports HW scaling on all planes.
Second mixer has 1 VI and 1 UI planes and also supports HW scaling on all planes.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/sun4i/sun8i_mixer.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 36d90c76317a..78aa878e305c 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -500,6 +500,24 @@ static const struct sun8i_mixer_cfg sun8i_h3_mixer0_cfg = { .vi_num = 1, };
+static const struct sun8i_mixer_cfg sun8i_r40_mixer0_cfg = { + .ccsc = 0, + .index = 0, + .mod_rate = 297000000, + .scaler_mask = 0xf, + .ui_num = 3, + .vi_num = 1, +}; + +static const struct sun8i_mixer_cfg sun8i_r40_mixer1_cfg = { + .ccsc = 1, + .index = 1, + .mod_rate = 297000000, + .scaler_mask = 0x3, + .ui_num = 1, + .vi_num = 1, +}; + static const struct sun8i_mixer_cfg sun8i_v3s_mixer_cfg = { .vi_num = 2, .ui_num = 1, @@ -521,6 +539,14 @@ static const struct of_device_id sun8i_mixer_of_table[] = { .compatible = "allwinner,sun8i-h3-de2-mixer-0", .data = &sun8i_h3_mixer0_cfg, }, + { + .compatible = "allwinner,sun8i-r40-de2-mixer-0", + .data = &sun8i_r40_mixer0_cfg, + }, + { + .compatible = "allwinner,sun8i-r40-de2-mixer-1", + .data = &sun8i_r40_mixer1_cfg, + }, { .compatible = "allwinner,sun8i-v3s-de2-mixer", .data = &sun8i_v3s_mixer_cfg,
R40 display pipeline has a lot of possible configurations. HDMI can be connected to 2 different TCONs (out of 4) and mixers can be connected to any TCON. All this must be configured in TCON TOP.
Along with definition of TCON capabilities also add mux callback, which can configure this complex pipeline.
For now, only TCON TV is supported.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 39 ++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index e0c562ce1c22..81b9551e4f78 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -1274,6 +1274,31 @@ static int sun6i_tcon_set_mux(struct sun4i_tcon *tcon, return 0; }
+static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon, + const struct drm_encoder *encoder, + int index) +{ + if (encoder->encoder_type == DRM_MODE_ENCODER_TMDS) + sun8i_tcon_top_set_hdmi_src(tcon->tcon_top, index); + + sun8i_tcon_top_de_config(tcon->tcon_top, tcon->id, + tcon_type_tv, index); + + return 0; +} + +static int sun8i_r40_tcon_tv_set_mux_0(struct sun4i_tcon *tcon, + const struct drm_encoder *encoder) +{ + return sun8i_r40_tcon_tv_set_mux(tcon, encoder, 0); +} + +static int sun8i_r40_tcon_tv_set_mux_1(struct sun4i_tcon *tcon, + const struct drm_encoder *encoder) +{ + return sun8i_r40_tcon_tv_set_mux(tcon, encoder, 1); +} + static const struct sun4i_tcon_quirks sun4i_a10_quirks = { .has_channel_0 = true, .has_channel_1 = true, @@ -1321,6 +1346,18 @@ static const struct sun4i_tcon_quirks sun8i_a83t_tv_quirks = { .has_channel_1 = true, };
+static const struct sun4i_tcon_quirks sun8i_r40_tv_0_quirks = { + .has_channel_1 = true, + .needs_tcon_top = true, + .set_mux = sun8i_r40_tcon_tv_set_mux_0, +}; + +static const struct sun4i_tcon_quirks sun8i_r40_tv_1_quirks = { + .has_channel_1 = true, + .needs_tcon_top = true, + .set_mux = sun8i_r40_tcon_tv_set_mux_1, +}; + static const struct sun4i_tcon_quirks sun8i_v3s_quirks = { .has_channel_0 = true, }; @@ -1345,6 +1382,8 @@ const struct of_device_id sun4i_tcon_of_table[] = { { .compatible = "allwinner,sun8i-a33-tcon", .data = &sun8i_a33_quirks }, { .compatible = "allwinner,sun8i-a83t-tcon-lcd", .data = &sun8i_a83t_lcd_quirks }, { .compatible = "allwinner,sun8i-a83t-tcon-tv", .data = &sun8i_a83t_tv_quirks }, + { .compatible = "allwinner,sun8i-r40-tcon-tv-0", .data = &sun8i_r40_tv_0_quirks }, + { .compatible = "allwinner,sun8i-r40-tcon-tv-1", .data = &sun8i_r40_tv_1_quirks }, { .compatible = "allwinner,sun8i-v3s-tcon", .data = &sun8i_v3s_quirks }, { .compatible = "allwinner,sun9i-a80-tcon-lcd", .data = &sun9i_a80_tcon_lcd_quirks }, { .compatible = "allwinner,sun9i-a80-tcon-tv", .data = &sun9i_a80_tcon_tv_quirks },
Hi Jernej,
On Sun, May 20, 2018 at 4:31 AM, Jernej Skrabec jernej.skrabec@siol.net wrote:
R40 display pipeline has a lot of possible configurations. HDMI can be connected to 2 different TCONs (out of 4) and mixers can be connected to any TCON. All this must be configured in TCON TOP.
Along with definition of TCON capabilities also add mux callback, which can configure this complex pipeline.
For now, only TCON TV is supported.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun4i_tcon.c | 39 ++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index e0c562ce1c22..81b9551e4f78 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -1274,6 +1274,31 @@ static int sun6i_tcon_set_mux(struct sun4i_tcon *tcon, return 0; }
+static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon,
const struct drm_encoder *encoder,int index)+{
if (encoder->encoder_type == DRM_MODE_ENCODER_TMDS)sun8i_tcon_top_set_hdmi_src(tcon->tcon_top, index);sun8i_tcon_top_de_config(tcon->tcon_top, tcon->id,tcon_type_tv, index);return 0;+}
+static int sun8i_r40_tcon_tv_set_mux_0(struct sun4i_tcon *tcon,
const struct drm_encoder *encoder)+{
return sun8i_r40_tcon_tv_set_mux(tcon, encoder, 0);+}
+static int sun8i_r40_tcon_tv_set_mux_1(struct sun4i_tcon *tcon,
const struct drm_encoder *encoder)+{
return sun8i_r40_tcon_tv_set_mux(tcon, encoder, 1);+}
Are TCON-TOPs going to be a common thing in new SoCs from Allwinner? If so, maybe we should add an index to the TCON quirks and have a common TCON-TOP set_mux function.
Thanks,
Hi Jernej,
On Sun, May 20, 2018 at 11:57 AM, Julian Calaby julian.calaby@gmail.com wrote:
Hi Jernej,
On Sun, May 20, 2018 at 4:31 AM, Jernej Skrabec jernej.skrabec@siol.net wrote:
R40 display pipeline has a lot of possible configurations. HDMI can be connected to 2 different TCONs (out of 4) and mixers can be connected to any TCON. All this must be configured in TCON TOP.
Along with definition of TCON capabilities also add mux callback, which can configure this complex pipeline.
For now, only TCON TV is supported.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun4i_tcon.c | 39 ++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index e0c562ce1c22..81b9551e4f78 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -1274,6 +1274,31 @@ static int sun6i_tcon_set_mux(struct sun4i_tcon *tcon, return 0; }
+static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon,
const struct drm_encoder *encoder,int index)+{
if (encoder->encoder_type == DRM_MODE_ENCODER_TMDS)sun8i_tcon_top_set_hdmi_src(tcon->tcon_top, index);sun8i_tcon_top_de_config(tcon->tcon_top, tcon->id,tcon_type_tv, index);return 0;+}
+static int sun8i_r40_tcon_tv_set_mux_0(struct sun4i_tcon *tcon,
const struct drm_encoder *encoder)+{
return sun8i_r40_tcon_tv_set_mux(tcon, encoder, 0);+}
+static int sun8i_r40_tcon_tv_set_mux_1(struct sun4i_tcon *tcon,
const struct drm_encoder *encoder)+{
return sun8i_r40_tcon_tv_set_mux(tcon, encoder, 1);+}
Are TCON-TOPs going to be a common thing in new SoCs from Allwinner? If so, maybe we should add an index to the TCON quirks and have a common TCON-TOP set_mux function.
Actually, that only moves it up a level. Should it be a devicetree property?
Thanks,
Hi,
Dne nedelja, 20. maj 2018 ob 04:09:52 CEST je Julian Calaby napisal(a):
Hi Jernej,
On Sun, May 20, 2018 at 11:57 AM, Julian Calaby julian.calaby@gmail.com
wrote:
Hi Jernej,
On Sun, May 20, 2018 at 4:31 AM, Jernej Skrabec jernej.skrabec@siol.net
wrote:
R40 display pipeline has a lot of possible configurations. HDMI can be connected to 2 different TCONs (out of 4) and mixers can be connected to any TCON. All this must be configured in TCON TOP.
Along with definition of TCON capabilities also add mux callback, which can configure this complex pipeline.
For now, only TCON TV is supported.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun4i_tcon.c | 39 ++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index e0c562ce1c22..81b9551e4f78 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -1274,6 +1274,31 @@ static int sun6i_tcon_set_mux(struct sun4i_tcon *tcon,>> return 0;
}
+static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon,
const struct drm_encoder *encoder,int index)+{
if (encoder->encoder_type == DRM_MODE_ENCODER_TMDS)sun8i_tcon_top_set_hdmi_src(tcon->tcon_top, index);sun8i_tcon_top_de_config(tcon->tcon_top, tcon->id,tcon_type_tv, index);return 0;+}
+static int sun8i_r40_tcon_tv_set_mux_0(struct sun4i_tcon *tcon,
const struct drm_encoder *encoder)+{
return sun8i_r40_tcon_tv_set_mux(tcon, encoder, 0);+}
+static int sun8i_r40_tcon_tv_set_mux_1(struct sun4i_tcon *tcon,
const struct drm_encoder *encoder)+{
return sun8i_r40_tcon_tv_set_mux(tcon, encoder, 1);+}
Are TCON-TOPs going to be a common thing in new SoCs from Allwinner? If so, maybe we should add an index to the TCON quirks and have a common TCON-TOP set_mux function.
Actually, that only moves it up a level. Should it be a devicetree property?
TCON-TOP is besides R40 part of two newest Allwinner SoCs, H6 and A63. However, they have only one TV TCON and one LCD TCON, so indexes are not needed for them (always 0). This makes R40 somewhat special. I don't think it makes sense to expand everything just for one SoC.
Best regards, Jernej
Some DW HDMI PHYs like those found in A64 and R40 SoCs, can select between two clock parents.
Add code which reads second PLL from DT.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 2 ++ drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 22 ++++++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 79154f0f674a..801a17222762 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -147,6 +147,7 @@ struct sun8i_hdmi_phy;
struct sun8i_hdmi_phy_variant { bool has_phy_clk; + bool has_second_pll; void (*phy_init)(struct sun8i_hdmi_phy *phy); void (*phy_disable)(struct dw_hdmi *hdmi, struct sun8i_hdmi_phy *phy); @@ -160,6 +161,7 @@ struct sun8i_hdmi_phy { struct clk *clk_mod; struct clk *clk_phy; struct clk *clk_pll0; + struct clk *clk_pll1; unsigned int rcal; struct regmap *regs; struct reset_control *rst_phy; diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 5a52fc489a9d..deba47ed69d8 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -472,10 +472,19 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node) goto err_put_clk_mod; }
+ if (phy->variant->has_second_pll) { + phy->clk_pll1 = of_clk_get_by_name(node, "pll-1"); + if (IS_ERR(phy->clk_pll1)) { + dev_err(dev, "Could not get pll-1 clock\n"); + ret = PTR_ERR(phy->clk_pll1); + goto err_put_clk_pll0; + } + } + ret = sun8i_phy_clk_create(phy, dev); if (ret) { dev_err(dev, "Couldn't create the PHY clock\n"); - goto err_put_clk_pll0; + goto err_put_clk_pll1; } }
@@ -483,7 +492,7 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node) if (IS_ERR(phy->rst_phy)) { dev_err(dev, "Could not get phy reset control\n"); ret = PTR_ERR(phy->rst_phy); - goto err_put_clk_pll0; + goto err_put_clk_pll1; }
ret = reset_control_deassert(phy->rst_phy); @@ -514,9 +523,10 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node) reset_control_assert(phy->rst_phy); err_put_rst_phy: reset_control_put(phy->rst_phy); +err_put_clk_pll1: + clk_put(phy->clk_pll1); err_put_clk_pll0: - if (phy->variant->has_phy_clk) - clk_put(phy->clk_pll0); + clk_put(phy->clk_pll0); err_put_clk_mod: clk_put(phy->clk_mod); err_put_clk_bus: @@ -536,8 +546,8 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi)
reset_control_put(phy->rst_phy);
- if (phy->variant->has_phy_clk) - clk_put(phy->clk_pll0); + clk_put(phy->clk_pll0); + clk_put(phy->clk_pll1); clk_put(phy->clk_mod); clk_put(phy->clk_bus); }
Expand HDMI PHY clock driver to support second clock parent.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 6 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 29 ++++++- drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++++++++++++++------ 3 files changed, 98 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 801a17222762..aadbe0a10b0c 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -98,7 +98,8 @@ #define SUN8I_HDMI_PHY_PLL_CFG1_LDO2_EN BIT(29) #define SUN8I_HDMI_PHY_PLL_CFG1_LDO1_EN BIT(28) #define SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 BIT(27) -#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL BIT(26) +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK BIT(26) +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT 26 #define SUN8I_HDMI_PHY_PLL_CFG1_PLLEN BIT(25) #define SUN8I_HDMI_PHY_PLL_CFG1_LDO_VSET(x) ((x) << 22) #define SUN8I_HDMI_PHY_PLL_CFG1_UNKNOWN(x) ((x) << 20) @@ -190,6 +191,7 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi); void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); const struct dw_hdmi_phy_ops *sun8i_hdmi_phy_get_ops(void);
-int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev); +int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev, + bool second_parent);
#endif /* _SUN8I_DW_HDMI_H_ */ diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index deba47ed69d8..7a911f0a3ae3 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -183,7 +183,13 @@ static int sun8i_hdmi_phy_config_h3(struct dw_hdmi *hdmi, regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG, SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK, 0);
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, pll_cfg1_init); + /* + * NOTE: We have to be careful not to overwrite PHY parent + * clock selection bit and clock divider. + */ + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, + ~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, + pll_cfg1_init); regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, (u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK, pll_cfg2_init); @@ -352,6 +358,10 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy) SUN8I_HDMI_PHY_ANA_CFG3_SCLEN | SUN8I_HDMI_PHY_ANA_CFG3_SDAEN);
+ /* reset PLL clock configuration */ + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0); + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 0); + /* set HW control of CEC pins */ regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, 0);
@@ -481,18 +491,28 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node) } }
- ret = sun8i_phy_clk_create(phy, dev); + ret = sun8i_phy_clk_create(phy, dev, + phy->variant->has_second_pll); if (ret) { dev_err(dev, "Couldn't create the PHY clock\n"); goto err_put_clk_pll1; } + + /* + * Even though HDMI PHY clock doesn't have enable/disable + * handlers, we have to enable it. Otherwise it could happen + * that parent PLL is not enabled by clock framework in a + * highly unlikely event when parent PLL is used solely for + * HDMI PHY clock. + */ + clk_prepare_enable(phy->clk_phy); }
phy->rst_phy = of_reset_control_get_shared(node, "phy"); if (IS_ERR(phy->rst_phy)) { dev_err(dev, "Could not get phy reset control\n"); ret = PTR_ERR(phy->rst_phy); - goto err_put_clk_pll1; + goto err_disable_clk_phy; }
ret = reset_control_deassert(phy->rst_phy); @@ -523,6 +543,8 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node) reset_control_assert(phy->rst_phy); err_put_rst_phy: reset_control_put(phy->rst_phy); +err_disable_clk_phy: + clk_disable_unprepare(phy->clk_phy); err_put_clk_pll1: clk_put(phy->clk_pll1); err_put_clk_pll0: @@ -541,6 +563,7 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi)
clk_disable_unprepare(phy->clk_mod); clk_disable_unprepare(phy->clk_bus); + clk_disable_unprepare(phy->clk_phy);
reset_control_assert(phy->rst_phy);
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c index faea449812f8..a4d31fe3abff 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c @@ -22,35 +22,45 @@ static int sun8i_phy_clk_determine_rate(struct clk_hw *hw, { unsigned long rate = req->rate; unsigned long best_rate = 0; + struct clk_hw *best_parent = NULL; struct clk_hw *parent; int best_div = 1; - int i; + int i, p;
- parent = clk_hw_get_parent(hw); - - for (i = 1; i <= 16; i++) { - unsigned long ideal = rate * i; - unsigned long rounded; - - rounded = clk_hw_round_rate(parent, ideal); + for (p = 0; p < clk_hw_get_num_parents(hw); p++) { + parent = clk_hw_get_parent_by_index(hw, p); + if (!parent) + continue;
- if (rounded == ideal) { - best_rate = rounded; - best_div = i; - break; + for (i = 1; i <= 16; i++) { + unsigned long ideal = rate * i; + unsigned long rounded; + + rounded = clk_hw_round_rate(parent, ideal); + + if (rounded == ideal) { + best_rate = rounded; + best_div = i; + best_parent = parent; + break; + } + + if (!best_rate || + abs(rate - rounded / i) < + abs(rate - best_rate / best_div)) { + best_rate = rounded; + best_div = i; + best_parent = parent; + } }
- if (!best_rate || - abs(rate - rounded / i) < - abs(rate - best_rate / best_div)) { - best_rate = rounded; - best_div = i; - } + if (best_rate / best_div == rate) + break; }
req->rate = best_rate / best_div; req->best_parent_rate = best_rate; - req->best_parent_hw = parent; + req->best_parent_hw = best_parent;
return 0; } @@ -95,22 +105,58 @@ static int sun8i_phy_clk_set_rate(struct clk_hw *hw, unsigned long rate, return 0; }
+static u8 sun8i_phy_clk_get_parent(struct clk_hw *hw) +{ + struct sun8i_phy_clk *priv = hw_to_phy_clk(hw); + u32 reg; + + regmap_read(priv->phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, ®); + reg = (reg & SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK) >> + SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT; + + return reg; +} + +static int sun8i_phy_clk_set_parent(struct clk_hw *hw, u8 index) +{ + struct sun8i_phy_clk *priv = hw_to_phy_clk(hw); + + if (index > 1) + return -EINVAL; + + regmap_update_bits(priv->phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, + SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, + index << SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT); + + return 0; +} + static const struct clk_ops sun8i_phy_clk_ops = { .determine_rate = sun8i_phy_clk_determine_rate, .recalc_rate = sun8i_phy_clk_recalc_rate, .set_rate = sun8i_phy_clk_set_rate, + + .get_parent = sun8i_phy_clk_get_parent, + .set_parent = sun8i_phy_clk_set_parent, };
-int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev) +int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev, + bool second_parent) { struct clk_init_data init; struct sun8i_phy_clk *priv; - const char *parents[1]; + const char *parents[2];
parents[0] = __clk_get_name(phy->clk_pll0); if (!parents[0]) return -ENODEV;
+ if (second_parent) { + parents[1] = __clk_get_name(phy->clk_pll1); + if (!parents[1]) + return -ENODEV; + } + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; @@ -118,7 +164,7 @@ int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev) init.name = "hdmi-phy-clk"; init.ops = &sun8i_phy_clk_ops; init.parent_names = parents; - init.num_parents = 1; + init.num_parents = second_parent ? 2 : 1; init.flags = CLK_SET_RATE_PARENT;
priv->phy = phy;
Hi Jernej,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next] [also build test WARNING on v4.17-rc6 next-20180517] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jernej-Skrabec/Add-support-for-R40-... base: git://people.freedesktop.org/~airlied/linux.git drm-next config: arm64-allmodconfig (attached as .config) compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm64
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h:12:0, from drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c:9: drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c: In function 'sun8i_hdmi_phy_config_h3':
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c:191:7: warning: large integer implicitly truncated to unsigned type [-Woverflow]
~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, ^ include/linux/regmap.h:76:36: note: in definition of macro 'regmap_update_bits' regmap_update_bits_base(map, reg, mask, val, NULL, false, false) ^~~~
vim +191 drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
8
9 #include "sun8i_dw_hdmi.h"
10 11 /* 12 * Address can be actually any value. Here is set to same value as 13 * it is set in BSP driver. 14 */ 15 #define I2C_ADDR 0x69 16 17 static int sun8i_hdmi_phy_config_a83t(struct dw_hdmi *hdmi, 18 struct sun8i_hdmi_phy *phy, 19 unsigned int clk_rate) 20 { 21 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, 22 SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN, 23 SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN); 24 25 /* power down */ 26 dw_hdmi_phy_gen2_txpwron(hdmi, 0); 27 dw_hdmi_phy_gen2_pddq(hdmi, 1); 28 29 dw_hdmi_phy_reset(hdmi); 30 31 dw_hdmi_phy_gen2_pddq(hdmi, 0); 32 33 dw_hdmi_phy_i2c_set_addr(hdmi, I2C_ADDR); 34 35 /* 36 * Values are taken from BSP HDMI driver. Although AW didn't 37 * release any documentation, explanation of this values can 38 * be found in i.MX 6Dual/6Quad Reference Manual. 39 */ 40 if (clk_rate <= 27000000) { 41 dw_hdmi_phy_i2c_write(hdmi, 0x01e0, 0x06); 42 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x15); 43 dw_hdmi_phy_i2c_write(hdmi, 0x08da, 0x10); 44 dw_hdmi_phy_i2c_write(hdmi, 0x0007, 0x19); 45 dw_hdmi_phy_i2c_write(hdmi, 0x0318, 0x0e); 46 dw_hdmi_phy_i2c_write(hdmi, 0x8009, 0x09); 47 } else if (clk_rate <= 74250000) { 48 dw_hdmi_phy_i2c_write(hdmi, 0x0540, 0x06); 49 dw_hdmi_phy_i2c_write(hdmi, 0x0005, 0x15); 50 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x10); 51 dw_hdmi_phy_i2c_write(hdmi, 0x0007, 0x19); 52 dw_hdmi_phy_i2c_write(hdmi, 0x02b5, 0x0e); 53 dw_hdmi_phy_i2c_write(hdmi, 0x8009, 0x09); 54 } else if (clk_rate <= 148500000) { 55 dw_hdmi_phy_i2c_write(hdmi, 0x04a0, 0x06); 56 dw_hdmi_phy_i2c_write(hdmi, 0x000a, 0x15); 57 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x10); 58 dw_hdmi_phy_i2c_write(hdmi, 0x0002, 0x19); 59 dw_hdmi_phy_i2c_write(hdmi, 0x0021, 0x0e); 60 dw_hdmi_phy_i2c_write(hdmi, 0x8029, 0x09); 61 } else { 62 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x06); 63 dw_hdmi_phy_i2c_write(hdmi, 0x000f, 0x15); 64 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x10); 65 dw_hdmi_phy_i2c_write(hdmi, 0x0002, 0x19); 66 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x0e); 67 dw_hdmi_phy_i2c_write(hdmi, 0x802b, 0x09); 68 } 69 70 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x1e); 71 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x13); 72 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x17); 73 74 dw_hdmi_phy_gen2_txpwron(hdmi, 1); 75 76 return 0; 77 } 78 79 static int sun8i_hdmi_phy_config_h3(struct dw_hdmi *hdmi, 80 struct sun8i_hdmi_phy *phy, 81 unsigned int clk_rate) 82 { 83 u32 pll_cfg1_init; 84 u32 pll_cfg2_init; 85 u32 ana_cfg1_end; 86 u32 ana_cfg2_init; 87 u32 ana_cfg3_init; 88 u32 b_offset = 0; 89 u32 val; 90 91 /* bandwidth / frequency independent settings */ 92 93 pll_cfg1_init = SUN8I_HDMI_PHY_PLL_CFG1_LDO2_EN | 94 SUN8I_HDMI_PHY_PLL_CFG1_LDO1_EN | 95 SUN8I_HDMI_PHY_PLL_CFG1_LDO_VSET(7) | 96 SUN8I_HDMI_PHY_PLL_CFG1_UNKNOWN(1) | 97 SUN8I_HDMI_PHY_PLL_CFG1_PLLDBEN | 98 SUN8I_HDMI_PHY_PLL_CFG1_CS | 99 SUN8I_HDMI_PHY_PLL_CFG1_CP_S(2) | 100 SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(63) | 101 SUN8I_HDMI_PHY_PLL_CFG1_BWS; 102 103 pll_cfg2_init = SUN8I_HDMI_PHY_PLL_CFG2_SV_H | 104 SUN8I_HDMI_PHY_PLL_CFG2_VCOGAIN_EN | 105 SUN8I_HDMI_PHY_PLL_CFG2_SDIV2; 106 107 ana_cfg1_end = SUN8I_HDMI_PHY_ANA_CFG1_REG_SVBH(1) | 108 SUN8I_HDMI_PHY_ANA_CFG1_AMP_OPT | 109 SUN8I_HDMI_PHY_ANA_CFG1_EMP_OPT | 110 SUN8I_HDMI_PHY_ANA_CFG1_AMPCK_OPT | 111 SUN8I_HDMI_PHY_ANA_CFG1_EMPCK_OPT | 112 SUN8I_HDMI_PHY_ANA_CFG1_ENRCAL | 113 SUN8I_HDMI_PHY_ANA_CFG1_ENCALOG | 114 SUN8I_HDMI_PHY_ANA_CFG1_REG_SCKTMDS | 115 SUN8I_HDMI_PHY_ANA_CFG1_TMDSCLK_EN | 116 SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK | 117 SUN8I_HDMI_PHY_ANA_CFG1_TXEN_ALL | 118 SUN8I_HDMI_PHY_ANA_CFG1_BIASEN_TMDSCLK | 119 SUN8I_HDMI_PHY_ANA_CFG1_BIASEN_TMDS2 | 120 SUN8I_HDMI_PHY_ANA_CFG1_BIASEN_TMDS1 | 121 SUN8I_HDMI_PHY_ANA_CFG1_BIASEN_TMDS0 | 122 SUN8I_HDMI_PHY_ANA_CFG1_ENP2S_TMDS2 | 123 SUN8I_HDMI_PHY_ANA_CFG1_ENP2S_TMDS1 | 124 SUN8I_HDMI_PHY_ANA_CFG1_ENP2S_TMDS0 | 125 SUN8I_HDMI_PHY_ANA_CFG1_CKEN | 126 SUN8I_HDMI_PHY_ANA_CFG1_LDOEN | 127 SUN8I_HDMI_PHY_ANA_CFG1_ENVBS | 128 SUN8I_HDMI_PHY_ANA_CFG1_ENBI; 129 130 ana_cfg2_init = SUN8I_HDMI_PHY_ANA_CFG2_M_EN | 131 SUN8I_HDMI_PHY_ANA_CFG2_REG_DENCK | 132 SUN8I_HDMI_PHY_ANA_CFG2_REG_DEN | 133 SUN8I_HDMI_PHY_ANA_CFG2_REG_CKSS(1) | 134 SUN8I_HDMI_PHY_ANA_CFG2_REG_CSMPS(1); 135 136 ana_cfg3_init = SUN8I_HDMI_PHY_ANA_CFG3_REG_WIRE(0x3e0) | 137 SUN8I_HDMI_PHY_ANA_CFG3_SDAEN | 138 SUN8I_HDMI_PHY_ANA_CFG3_SCLEN; 139 140 /* bandwidth / frequency dependent settings */ 141 if (clk_rate <= 27000000) { 142 pll_cfg1_init |= SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 | 143 SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(32); 144 pll_cfg2_init |= SUN8I_HDMI_PHY_PLL_CFG2_VCO_S(4) | 145 SUN8I_HDMI_PHY_PLL_CFG2_S(4); 146 ana_cfg1_end |= SUN8I_HDMI_PHY_ANA_CFG1_REG_CALSW; 147 ana_cfg2_init |= SUN8I_HDMI_PHY_ANA_CFG2_REG_SLV(4) | 148 SUN8I_HDMI_PHY_ANA_CFG2_REG_RESDI(phy->rcal); 149 ana_cfg3_init |= SUN8I_HDMI_PHY_ANA_CFG3_REG_AMPCK(3) | 150 SUN8I_HDMI_PHY_ANA_CFG3_REG_AMP(5); 151 } else if (clk_rate <= 74250000) { 152 pll_cfg1_init |= SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 | 153 SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(32); 154 pll_cfg2_init |= SUN8I_HDMI_PHY_PLL_CFG2_VCO_S(4) | 155 SUN8I_HDMI_PHY_PLL_CFG2_S(5); 156 ana_cfg1_end |= SUN8I_HDMI_PHY_ANA_CFG1_REG_CALSW; 157 ana_cfg2_init |= SUN8I_HDMI_PHY_ANA_CFG2_REG_SLV(4) | 158 SUN8I_HDMI_PHY_ANA_CFG2_REG_RESDI(phy->rcal); 159 ana_cfg3_init |= SUN8I_HDMI_PHY_ANA_CFG3_REG_AMPCK(5) | 160 SUN8I_HDMI_PHY_ANA_CFG3_REG_AMP(7); 161 } else if (clk_rate <= 148500000) { 162 pll_cfg1_init |= SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 | 163 SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(32); 164 pll_cfg2_init |= SUN8I_HDMI_PHY_PLL_CFG2_VCO_S(4) | 165 SUN8I_HDMI_PHY_PLL_CFG2_S(6); 166 ana_cfg2_init |= SUN8I_HDMI_PHY_ANA_CFG2_REG_BIGSWCK | 167 SUN8I_HDMI_PHY_ANA_CFG2_REG_BIGSW | 168 SUN8I_HDMI_PHY_ANA_CFG2_REG_SLV(2); 169 ana_cfg3_init |= SUN8I_HDMI_PHY_ANA_CFG3_REG_AMPCK(7) | 170 SUN8I_HDMI_PHY_ANA_CFG3_REG_AMP(9); 171 } else { 172 b_offset = 2; 173 pll_cfg1_init |= SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(63); 174 pll_cfg2_init |= SUN8I_HDMI_PHY_PLL_CFG2_VCO_S(6) | 175 SUN8I_HDMI_PHY_PLL_CFG2_S(7); 176 ana_cfg2_init |= SUN8I_HDMI_PHY_ANA_CFG2_REG_BIGSWCK | 177 SUN8I_HDMI_PHY_ANA_CFG2_REG_BIGSW | 178 SUN8I_HDMI_PHY_ANA_CFG2_REG_SLV(4); 179 ana_cfg3_init |= SUN8I_HDMI_PHY_ANA_CFG3_REG_AMPCK(9) | 180 SUN8I_HDMI_PHY_ANA_CFG3_REG_AMP(13); 181 } 182 183 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG, 184 SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK, 0); 185 186 /* 187 * NOTE: We have to be careful not to overwrite PHY parent 188 * clock selection bit and clock divider. 189 */ 190 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG,
191 ~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK,
192 pll_cfg1_init); 193 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 194 (u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK, 195 pll_cfg2_init); 196 usleep_range(10000, 15000); 197 regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG3_REG, 198 SUN8I_HDMI_PHY_PLL_CFG3_SOUT_DIV2); 199 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 200 SUN8I_HDMI_PHY_PLL_CFG1_PLLEN, 201 SUN8I_HDMI_PHY_PLL_CFG1_PLLEN); 202 msleep(100); 203 204 /* get B value */ 205 regmap_read(phy->regs, SUN8I_HDMI_PHY_ANA_STS_REG, &val); 206 val = (val & SUN8I_HDMI_PHY_ANA_STS_B_OUT_MSK) >> 207 SUN8I_HDMI_PHY_ANA_STS_B_OUT_SHIFT; 208 val = min(val + b_offset, (u32)0x3f); 209 210 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 211 SUN8I_HDMI_PHY_PLL_CFG1_REG_OD1 | 212 SUN8I_HDMI_PHY_PLL_CFG1_REG_OD, 213 SUN8I_HDMI_PHY_PLL_CFG1_REG_OD1 | 214 SUN8I_HDMI_PHY_PLL_CFG1_REG_OD); 215 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 216 SUN8I_HDMI_PHY_PLL_CFG1_B_IN_MSK, 217 val << SUN8I_HDMI_PHY_PLL_CFG1_B_IN_SHIFT); 218 msleep(100); 219 regmap_write(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG, ana_cfg1_end); 220 regmap_write(phy->regs, SUN8I_HDMI_PHY_ANA_CFG2_REG, ana_cfg2_init); 221 regmap_write(phy->regs, SUN8I_HDMI_PHY_ANA_CFG3_REG, ana_cfg3_init); 222 223 return 0; 224 } 225
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, May 19, 2018 at 08:31:24PM +0200, Jernej Skrabec wrote:
Expand HDMI PHY clock driver to support second clock parent.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 6 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 29 ++++++- drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++++++++++++++------ 3 files changed, 98 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 801a17222762..aadbe0a10b0c 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -98,7 +98,8 @@ #define SUN8I_HDMI_PHY_PLL_CFG1_LDO2_EN BIT(29) #define SUN8I_HDMI_PHY_PLL_CFG1_LDO1_EN BIT(28) #define SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 BIT(27) -#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL BIT(26) +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK BIT(26) +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT 26 #define SUN8I_HDMI_PHY_PLL_CFG1_PLLEN BIT(25) #define SUN8I_HDMI_PHY_PLL_CFG1_LDO_VSET(x) ((x) << 22) #define SUN8I_HDMI_PHY_PLL_CFG1_UNKNOWN(x) ((x) << 20) @@ -190,6 +191,7 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi); void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); const struct dw_hdmi_phy_ops *sun8i_hdmi_phy_get_ops(void);
-int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev); +int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev,
bool second_parent);#endif /* _SUN8I_DW_HDMI_H_ */ diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index deba47ed69d8..7a911f0a3ae3 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -183,7 +183,13 @@ static int sun8i_hdmi_phy_config_h3(struct dw_hdmi *hdmi, regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG, SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK, 0);
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, pll_cfg1_init);
- /*
* NOTE: We have to be careful not to overwrite PHY parent* clock selection bit and clock divider.*/- regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG,
~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK,pll_cfg1_init);
It feels like it belongs in a separate patch.
regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, (u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK, pll_cfg2_init); @@ -352,6 +358,10 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy) SUN8I_HDMI_PHY_ANA_CFG3_SCLEN | SUN8I_HDMI_PHY_ANA_CFG3_SDAEN);
- /* reset PLL clock configuration */
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0);
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 0);
Ditto,
/** Even though HDMI PHY clock doesn't have enable/disable* handlers, we have to enable it. Otherwise it could happen* that parent PLL is not enabled by clock framework in a* highly unlikely event when parent PLL is used solely for* HDMI PHY clock.*/clk_prepare_enable(phy->clk_phy);
The implementation of the clock doesn't really matter in our API usage. If we're using a clock, we have to call clk_prepare_enable. That's documented everywhere, and mentionning how the clock is implemented is an abstraction leakage and it's irrelevant. I'd simply remove the comment here.
And it should be in a separate patch as well.
Maxime
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:12:53 CEST je Maxime Ripard napisal(a):
On Sat, May 19, 2018 at 08:31:24PM +0200, Jernej Skrabec wrote:
Expand HDMI PHY clock driver to support second clock parent.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 6 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 29 ++++++- drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++++++++++++++------ 3 files changed, 98 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 801a17222762..aadbe0a10b0c 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -98,7 +98,8 @@
#define SUN8I_HDMI_PHY_PLL_CFG1_LDO2_EN BIT(29) #define SUN8I_HDMI_PHY_PLL_CFG1_LDO1_EN BIT(28) #define SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 BIT(27)
-#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL BIT(26) +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK BIT(26) +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT 26
#define SUN8I_HDMI_PHY_PLL_CFG1_PLLEN BIT(25) #define SUN8I_HDMI_PHY_PLL_CFG1_LDO_VSET(x) ((x) << 22) #define SUN8I_HDMI_PHY_PLL_CFG1_UNKNOWN(x) ((x) << 20)
@@ -190,6 +191,7 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi);
void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); const struct dw_hdmi_phy_ops *sun8i_hdmi_phy_get_ops(void);
-int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev); +int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev,
bool second_parent);#endif /* _SUN8I_DW_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index deba47ed69d8..7a911f0a3ae3 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -183,7 +183,13 @@ static int sun8i_hdmi_phy_config_h3(struct dw_hdmi *hdmi,> regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG,
SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK, 0);
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, pll_cfg1_init);
- /*
* NOTE: We have to be careful not to overwrite PHY parent* clock selection bit and clock divider.*/- regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG,
~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK,pll_cfg1_init);It feels like it belongs in a separate patch.
Why? clk_set_rate() on HDMI PHY clock is called before this function, so SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL bit is already set to correct value. Original code doesn't take this into account and sets it to 0 here in all cases, which obviously is not right.
If you insist on splitting this in new patch, it has to be applied before clock changes. It would also need to include "reset PLL clock configuration" chunk (next change in this patch), otherwise other SoCs with only one parent could get 1 there, which is obviously wrong. Note that I didn't really tested if default value of this bit is 0 or 1, but I wouldn't really like to depend on default values.
regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG,
(u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK, pll_cfg2_init);@@ -352,6 +358,10 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy)> SUN8I_HDMI_PHY_ANA_CFG3_SCLEN | SUN8I_HDMI_PHY_ANA_CFG3_SDAEN);
- /* reset PLL clock configuration */
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0);
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 0);
Ditto,
/** Even though HDMI PHY clock doesn't have enable/disable* handlers, we have to enable it. Otherwise it could happen* that parent PLL is not enabled by clock framework in a* highly unlikely event when parent PLL is used solely for* HDMI PHY clock.*/clk_prepare_enable(phy->clk_phy);The implementation of the clock doesn't really matter in our API usage. If we're using a clock, we have to call clk_prepare_enable. That's documented everywhere, and mentionning how the clock is implemented is an abstraction leakage and it's irrelevant. I'd simply remove the comment here.
And it should be in a separate patch as well.
OK. Should I add Fixes tag, since current code obviously is not by the specs? It could still get in 4.17...
Best regards, Jernej
On Mon, May 21, 2018 at 05:02:23PM +0200, Jernej Škrabec wrote:
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:12:53 CEST je Maxime Ripard napisal(a):
On Sat, May 19, 2018 at 08:31:24PM +0200, Jernej Skrabec wrote:
Expand HDMI PHY clock driver to support second clock parent.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 6 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 29 ++++++- drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++++++++++++++------ 3 files changed, 98 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 801a17222762..aadbe0a10b0c 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -98,7 +98,8 @@
#define SUN8I_HDMI_PHY_PLL_CFG1_LDO2_EN BIT(29) #define SUN8I_HDMI_PHY_PLL_CFG1_LDO1_EN BIT(28) #define SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 BIT(27)
-#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL BIT(26) +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK BIT(26) +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT 26
#define SUN8I_HDMI_PHY_PLL_CFG1_PLLEN BIT(25) #define SUN8I_HDMI_PHY_PLL_CFG1_LDO_VSET(x) ((x) << 22) #define SUN8I_HDMI_PHY_PLL_CFG1_UNKNOWN(x) ((x) << 20)
@@ -190,6 +191,7 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi);
void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); const struct dw_hdmi_phy_ops *sun8i_hdmi_phy_get_ops(void);
-int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev); +int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev,
bool second_parent);#endif /* _SUN8I_DW_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index deba47ed69d8..7a911f0a3ae3 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -183,7 +183,13 @@ static int sun8i_hdmi_phy_config_h3(struct dw_hdmi *hdmi,> regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG,
SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK, 0);
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, pll_cfg1_init);
- /*
* NOTE: We have to be careful not to overwrite PHY parent* clock selection bit and clock divider.*/- regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG,
~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK,pll_cfg1_init);It feels like it belongs in a separate patch.
Why? clk_set_rate() on HDMI PHY clock is called before this function, so SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL bit is already set to correct value. Original code doesn't take this into account and sets it to 0 here in all cases, which obviously is not right.
If you insist on splitting this in new patch, it has to be applied before clock changes. It would also need to include "reset PLL clock configuration" chunk (next change in this patch), otherwise other SoCs with only one parent could get 1 there, which is obviously wrong. Note that I didn't really tested if default value of this bit is 0 or 1, but I wouldn't really like to depend on default values.
I don't have a strong feeling about this, but to me, the fact that you don't want to overwrite the muxing bit because the clock might use it is sensible in itself, disregarding the fact that the clock *will* use it.
regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG,
(u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK, pll_cfg2_init);@@ -352,6 +358,10 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy)> SUN8I_HDMI_PHY_ANA_CFG3_SCLEN | SUN8I_HDMI_PHY_ANA_CFG3_SDAEN);
- /* reset PLL clock configuration */
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0);
- regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 0);
Ditto,
/** Even though HDMI PHY clock doesn't have enable/disable* handlers, we have to enable it. Otherwise it could happen* that parent PLL is not enabled by clock framework in a* highly unlikely event when parent PLL is used solely for* HDMI PHY clock.*/clk_prepare_enable(phy->clk_phy);The implementation of the clock doesn't really matter in our API usage. If we're using a clock, we have to call clk_prepare_enable. That's documented everywhere, and mentionning how the clock is implemented is an abstraction leakage and it's irrelevant. I'd simply remove the comment here.
And it should be in a separate patch as well.
OK. Should I add Fixes tag, since current code obviously is not by the specs? It could still get in 4.17...
Yep!
Maxime
PHY is the same as in H3, except it can switch between two clock parents.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 7a911f0a3ae3..0c9d67bb1007 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -396,6 +396,14 @@ static struct regmap_config sun8i_hdmi_phy_regmap_config = { .name = "phy" };
+static const struct sun8i_hdmi_phy_variant sun50i_a64_hdmi_phy = { + .has_phy_clk = true, + .has_second_pll = true, + .phy_init = &sun8i_hdmi_phy_init_h3, + .phy_disable = &sun8i_hdmi_phy_disable_h3, + .phy_config = &sun8i_hdmi_phy_config_h3, +}; + static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = { .phy_init = &sun8i_hdmi_phy_init_a83t, .phy_disable = &sun8i_hdmi_phy_disable_a83t, @@ -410,6 +418,10 @@ static const struct sun8i_hdmi_phy_variant sun8i_h3_hdmi_phy = { };
static const struct of_device_id sun8i_hdmi_phy_of_table[] = { + { + .compatible = "allwinner,sun50i-a64-hdmi-phy", + .data = &sun50i_a64_hdmi_phy, + }, { .compatible = "allwinner,sun8i-a83t-hdmi-phy", .data = &sun8i_a83t_hdmi_phy,
Add all entries needed for HDMI to function properly.
Since R40 has highly configurable pipeline, both mixers and both TCON TVs are added. Board specific DT should then connect them together to best fit the purpose of the board.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- arch/arm/boot/dts/sun8i-r40.dtsi | 166 +++++++++++++++++++++++++++++++ 1 file changed, 166 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi index 173dcc1652d2..6d5407964997 100644 --- a/arch/arm/boot/dts/sun8i-r40.dtsi +++ b/arch/arm/boot/dts/sun8i-r40.dtsi @@ -42,8 +42,11 @@ */
#include <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/clock/sun8i-de2.h> #include <dt-bindings/clock/sun8i-r40-ccu.h> +#include <dt-bindings/clock/sun8i-tcon-top.h> #include <dt-bindings/reset/sun8i-r40-ccu.h> +#include <dt-bindings/reset/sun8i-de2.h>
/ { #address-cells = <1>; @@ -99,12 +102,70 @@ }; };
+ de: display-engine { + compatible = "allwinner,sun8i-r40-display-engine", + "allwinner,sun8i-h3-display-engine"; + allwinner,pipelines = <&mixer0>, <&mixer1>; + status = "disabled"; + }; + soc { compatible = "simple-bus"; #address-cells = <1>; #size-cells = <1>; ranges;
+ display_clocks: clock@1000000 { + compatible = "allwinner,sun8i-r40-de2-clk", + "allwinner,sun8i-h3-de2-clk"; + reg = <0x01000000 0x100000>; + clocks = <&ccu CLK_DE>, + <&ccu CLK_BUS_DE>; + clock-names = "mod", + "bus"; + resets = <&ccu RST_BUS_DE>; + #clock-cells = <1>; + #reset-cells = <1>; + }; + + mixer0: mixer@1100000 { + compatible = "allwinner,sun8i-r40-de2-mixer-0"; + reg = <0x01100000 0x100000>; + clocks = <&display_clocks CLK_BUS_MIXER0>, + <&display_clocks CLK_MIXER0>; + clock-names = "bus", + "mod"; + resets = <&display_clocks RST_MIXER0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + mixer0_out: port@1 { + reg = <1>; + }; + }; + }; + + mixer1: mixer@1200000 { + compatible = "allwinner,sun8i-r40-de2-mixer-1"; + reg = <0x01200000 0x100000>; + clocks = <&display_clocks CLK_BUS_MIXER1>, + <&display_clocks CLK_MIXER1>; + clock-names = "bus", + "mod"; + resets = <&display_clocks RST_WB>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + mixer1_out: port@1 { + reg = <1>; + }; + }; + }; + nmi_intc: interrupt-controller@1c00030 { compatible = "allwinner,sun7i-a20-sc-nmi"; interrupt-controller; @@ -451,6 +512,70 @@ #size-cells = <0>; };
+ tcon_top: tcon-top@1c70000 { + compatible = "allwinner,sun8i-r40-tcon-top"; + reg = <0x01c70000 0x1000>; + clocks = <&ccu CLK_BUS_TCON_TOP>; + clock-names = "bus"; + resets = <&ccu RST_BUS_TCON_TOP>; + reset-names = "rst"; + #clock-cells = <1>; + }; + + tcon_tv0: lcd-controller@1c73000 { + compatible = "allwinner,sun8i-r40-tcon-tv-0"; + reg = <0x01c73000 0x1000>; + interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_TCON_TV0>, <&ccu CLK_TCON_TV0>, + <&tcon_top 1>; + clock-names = "ahb", "tcon-ch1", "tcon-top"; + resets = <&ccu RST_BUS_TCON_TV0>; + reset-names = "lcd"; + allwinner,tcon-top = <&tcon_top>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + tcon_tv0_in: port@0 { + reg = <0>; + }; + + tcon_tv0_out: port@1 { + #address-cells = <1>; + #size-cells = <0>; + reg = <1>; + }; + }; + }; + + tcon_tv1: lcd-controller@1c74000 { + compatible = "allwinner,sun8i-r40-tcon-tv-1"; + reg = <0x01c74000 0x1000>; + interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_TCON_TV1>, <&ccu CLK_TCON_TV1>, + <&tcon_top 2>; + clock-names = "ahb", "tcon-ch1", "tcon-top"; + resets = <&ccu RST_BUS_TCON_TV1>; + reset-names = "lcd"; + allwinner,tcon-top = <&tcon_top>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + tcon_tv1_in: port@0 { + reg = <0>; + }; + + tcon_tv1_out: port@1 { + #address-cells = <1>; + #size-cells = <0>; + reg = <1>; + }; + }; + }; + gic: interrupt-controller@1c81000 { compatible = "arm,gic-400"; reg = <0x01c81000 0x1000>, @@ -461,6 +586,47 @@ #interrupt-cells = <3>; interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; }; + + hdmi: hdmi@1ee0000 { + compatible = "allwinner,sun8i-r40-dw-hdmi", + "allwinner,sun8i-a83t-dw-hdmi"; + reg = <0x01ee0000 0x10000>; + reg-io-width = <1>; + interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_HDMI0>, <&ccu CLK_HDMI_SLOW>, + <&ccu CLK_HDMI>; + clock-names = "iahb", "isfr", "tmds"; + resets = <&ccu RST_BUS_HDMI1>; + reset-names = "ctrl"; + phys = <&hdmi_phy>; + phy-names = "hdmi-phy"; + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + hdmi_in: port@0 { + reg = <0>; + }; + + hdmi_out: port@1 { + reg = <1>; + }; + }; + }; + + hdmi_phy: hdmi-phy@1ef0000 { + compatible = "allwinner,sun8i-r40-hdmi-phy", + "allwinner,sun50i-a64-hdmi-phy"; + reg = <0x01ef0000 0x10000>; + clocks = <&ccu CLK_BUS_HDMI1>, <&ccu CLK_HDMI_SLOW>, + <&ccu 7>, <&ccu 16>; + clock-names = "bus", "mod", "pll-0", "pll-1"; + resets = <&ccu RST_BUS_HDMI0>; + reset-names = "phy"; + #phy-cells = <0>; + }; };
timer {
Since HDMI can be considered as main output, most capable mixer is connected to it (mixer0).
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- .../boot/dts/sun8i-r40-bananapi-m2-ultra.dts | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts b/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts index 27d9ccd0ef2f..66b492ad5847 100644 --- a/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts +++ b/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts @@ -58,6 +58,17 @@ stdout-path = "serial0:115200n8"; };
+ connector { + compatible = "hdmi-connector"; + type = "a"; + + port { + hdmi_con_in: endpoint { + remote-endpoint = <&hdmi_out_con>; + }; + }; + }; + leds { compatible = "gpio-leds";
@@ -93,6 +104,10 @@ }; };
+&de { + status = "okay"; +}; + &ehci1 { status = "okay"; }; @@ -101,6 +116,22 @@ status = "okay"; };
+&hdmi { + status = "okay"; +}; + +&hdmi_in { + hdmi_in_tcon_tv0: endpoint { + remote-endpoint = <&tcon_tv0_out_hdmi>; + }; +}; + +&hdmi_out { + hdmi_out_con: endpoint { + remote-endpoint = <&hdmi_con_in>; + }; +}; + &i2c0 { status = "okay";
@@ -161,6 +192,12 @@ regulator-name = "vcc-wifi"; };
+&mixer0_out { + mixer0_out_tcon_tv0: endpoint { + remote-endpoint = <&tcon_tv0_in_mixer0>; + }; +}; + &mmc0 { vmmc-supply = <®_dcdc1>; bus-width = <4>; @@ -195,6 +232,19 @@ status = "okay"; };
+&tcon_tv0_in { + tcon_tv0_in_mixer0: endpoint { + remote-endpoint = <&mixer0_out_tcon_tv0>; + }; +}; + +&tcon_tv0_out { + tcon_tv0_out_hdmi: endpoint@1 { + reg = <1>; + remote-endpoint = <&hdmi_in_tcon_tv0>; + }; +}; + &uart0 { pinctrl-names = "default"; pinctrl-0 = <&uart0_pb_pins>;
dri-devel@lists.freedesktop.org
-
Chen-Yu Tsai -
Jernej Skrabec -
Jernej Škrabec
-
Jernej Škrabec -
Julian Calaby -
kbuild test robot -
Maxime Ripard -
Rob Herring