[PATCH v2 00/11] Support for H3 Composite Output support

[PATCH v2 00/11] Support for H3 Composite Output support

[Icenowy Zheng]

Allwinner H3 SoC features a TV Encoder like the one in Allwinner A13,
which can only output TV Composite signal.

The display pipeline of H3 is also special -- it has two mixers and
two TCONs, of which the connection can be swapped. The TCONs do not
have channel 0 (as they are all connected to internal bridges, TVE
and HDMI TX).

Add support for the display pipeline and the TVE in H3, in order to
make it possible to display something with mainline kernel with H3.

The image quality of TVE is bad, so HDMI is a better output -- this
patchset also prepared the mixers and TCONs for HDMI output, and
the HDMI controller driver is already done by Jernej Skrabec.

So if possible, please apply PATCH 1~5 and 8,9 as soon as possible,
so that Jernej can submit his HDMI patches.

Currently the jack detection feature of the TVE is still not so
clear -- so it's not implemented in this version. Thus the TV
output shouldn't be defaultly enabled now.

Icenowy Zheng (11):
  dt-bindings: update the binding for Allwinner H3 TVE support
  drm: sun4i: add support for H3 mixers
  drm: sun4i: ignore swapped mixer<->tcon connection for DE2
  drm: sun4i: add support for H3's TCON0/1
  drm: sun4i: add compatible for H3 display engine
  drm: sun4i: add color space correction support for DE2 mixer
  drm: sun4i: add support for the TV encoder in H3 SoC
  clk: sunxi-ng: allow CLK_DE to set CLK_PLL_DE for H3
  clk: sunxi-ng: export CLK_PLL_DE for H3
  ARM: sun8i: h3: add display engine pipeline for TVE
  [DO NOT MERGE] ARM: sun8i: h3: enable TV output on Orange Pi PC

 .../bindings/display/sunxi/sun4i-drm.txt           |  37 +++-
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts         |  12 ++
 arch/arm/boot/dts/sun8i-h3.dtsi                    | 186 +++++++++++++++++++++
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c                |   2 +-
 drivers/clk/sunxi-ng/ccu-sun8i-h3.h                |   3 +-
 drivers/gpu/drm/sun4i/sun4i_drv.c                  |  46 +++++
 drivers/gpu/drm/sun4i/sun4i_tcon.c                 | 113 ++++++++++---
 drivers/gpu/drm/sun4i/sun4i_tcon.h                 |   3 +
 drivers/gpu/drm/sun4i/sun4i_tv.c                   |  35 +++-
 drivers/gpu/drm/sun4i/sun8i_mixer.c                |  53 ++++++
 drivers/gpu/drm/sun4i/sun8i_mixer.h                |   6 +-
 include/dt-bindings/clock/sun8i-h3-ccu.h           |   2 +
 12 files changed, 463 insertions(+), 35 deletions(-)

-- 
2.12.2

[PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[Icenowy Zheng]

Allwinner H3 features a "DE2.0" and a TV Encoder.

Add device tree bindings for the following parts:
- H3 TCONs
- H3 Mixers
- The connection between H3 TCONs and H3 Mixers
- H3 TV Encoder
- H3 Display engine

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v2:
- Changed endpoint reg definition on TCONs and Mixers. Now the endpoint
  id is just the component id.
- Changed TVE and TCON1 binding -- now CLK_TVE is passed as TCON1 lcd-ch1.

 .../bindings/display/sunxi/sun4i-drm.txt           | 37 +++++++++++++++++++---
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
index b83e6018041d..7ad164cb7dcb 100644
--- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
+++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
@@ -49,7 +49,9 @@ The TV Encoder supports the composite and VGA output. It is one end of
 the pipeline.
 
 Required properties:
- - compatible: value should be "allwinner,sun4i-a10-tv-encoder".
+ - compatible: value must be either:
+    * allwinner,sun4i-a10-tv-encoder
+    * allwinner,sun8i-h3-tv-encoder
  - reg: base address and size of memory-mapped region
  - clocks: the clocks driving the TV encoder
  - resets: phandle to the reset controller driving the encoder
@@ -69,23 +71,27 @@ Required properties:
    * allwinner,sun6i-a31-tcon
    * allwinner,sun6i-a31s-tcon
    * allwinner,sun8i-a33-tcon
+   * allwinner,sun8i-h3-tcon
    * allwinner,sun8i-v3s-tcon
  - reg: base address and size of memory-mapped region
  - interrupts: interrupt associated to this IP
  - clocks: phandles to the clocks feeding the TCON. Three are needed:
    - 'ahb': the interface clocks
-   - 'tcon-ch0': The clock driving the TCON channel 0
  - resets: phandles to the reset controllers driving the encoder
    - "lcd": the reset line for the TCON channel 0
 
  - clock-names: the clock names mentioned above
  - reset-names: the reset names mentioned above
- - clock-output-names: Name of the pixel clock created
 
 - ports: A ports node with endpoint definitions as defined in
   Documentation/devicetree/bindings/media/video-interfaces.txt. The
   first port should be the input endpoint, the second one the output
 
+  In the situation of Display Engine 2.0 that the connection between
+  the mixer and the TCON can be swapped, the input should have two
+  endpoints. The first input endpoint should be connected to mixer0
+  and the second should be connected to mixer1.
+
   The output may have multiple endpoints. The TCON has two channels,
   usually with the first channel being used for the panels interfaces
   (RGB, LVDS, etc.), and the second being used for the outputs that
@@ -94,7 +100,23 @@ Required properties:
   channel the endpoint is associated to. If that property is not
   present, the endpoint number will be used as the channel number.
 
-On SoCs other than the A33 and V3s, there is one more clock required:
+For the following compatibles:
+   * allwinner,sun5i-a13-tcon
+   * allwinner,sun6i-a31-tcon
+   * allwinner,sun6i-a31s-tcon
+   * allwinner,sun8i-a33-tcon
+   * allwinner,sun8i-v3s-tcon
+there is one more clock and one more property required:
+ - clocks:
+   - 'tcon-ch0': The clock driving the TCON channel 0
+ - clock-output-names: Name of the pixel clock created
+
+For the following compatibles:
+   * allwinner,sun5i-a13-tcon
+   * allwinner,sun6i-a31-tcon
+   * allwinner,sun6i-a31s-tcon
+   * allwinner,sun8i-h3-tcon
+there is one more clock required:
    - 'tcon-ch1': The clock driving the TCON channel 1
 
 DRC
@@ -189,6 +211,8 @@ supported.
 Required properties:
   - compatible: value must be one of:
     * allwinner,sun8i-v3s-de2-mixer
+    * allwinner,sun8i-h3-de2-mixer0
+    * allwinner,sun8i-h3-de2-mixer1
   - reg: base address and size of the memory-mapped region.
   - clocks: phandles to the clocks feeding the mixer
     * bus: the mixer interface clock
@@ -200,6 +224,10 @@ Required properties:
   Documentation/devicetree/bindings/media/video-interfaces.txt. The
   first port should be the input endpoints, the second one the output
 
+  In the situation of Display Engine 2.0 that the connection between
+  the mixer and the TCON can be swapped, the output should have two
+  endpoints. The first should be connected to TCON0 and the second
+  should be connected to TCON1.
 
 Display Engine Pipeline
 -----------------------
@@ -215,6 +243,7 @@ Required properties:
     * allwinner,sun6i-a31-display-engine
     * allwinner,sun6i-a31s-display-engine
     * allwinner,sun8i-a33-display-engine
+    * allwinner,sun8i-h3-display-engine
     * allwinner,sun8i-v3s-display-engine
 
   - allwinner,pipelines: list of phandle to the display engine
-- 
2.12.2

[PATCH v2 02/11] drm: sun4i: add support for H3 mixers

[Icenowy Zheng]

From: Icenowy Zheng <icenowy@aosc.xyz>

Allwinner H3 SoC has two mixers, one has 1 VI channel and 3 UI channels,
and the other has 1 VI and 1 UI.

Add support for these two variants.

Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index cb193c5f1686..d658a3a8159a 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -390,11 +390,29 @@ static const struct sun8i_mixer_cfg sun8i_v3s_mixer_cfg = {
 	.ui_num = 1,
 };
 
+static const struct sun8i_mixer_cfg sun8i_h3_mixer0_cfg = {
+	.vi_num = 1,
+	.ui_num = 3,
+};
+
+static const struct sun8i_mixer_cfg sun8i_h3_mixer1_cfg = {
+	.vi_num = 1,
+	.ui_num = 1,
+};
+
 static const struct of_device_id sun8i_mixer_of_table[] = {
 	{
 		.compatible = "allwinner,sun8i-v3s-de2-mixer",
 		.data = &sun8i_v3s_mixer_cfg,
 	},
+	{
+		.compatible = "allwinner,sun8i-h3-de2-mixer0",
+		.data = &sun8i_h3_mixer0_cfg
+	},
+	{
+		.compatible = "allwinner,sun8i-h3-de2-mixer1",
+		.data = &sun8i_h3_mixer1_cfg
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sun8i_mixer_of_table);
-- 
2.12.2

[PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Icenowy Zheng]

Some SoC's DE2 has two mixers. Defaultly the mixer0 is connected to
tcon0 and mixer1 is connected to tcon1; however by setting a bit
the connection can be swapped.

As we now hardcode the default connection, ignore the bonus endpoint for
the mixer's output and the TCON's input, as they stands for the swapped
connection.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v2:
- Change to use new endpoint reg definition.

 drivers/gpu/drm/sun4i/sun4i_drv.c  | 45 ++++++++++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 61 ++++++++++++++++++++++++++++++++------
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 ++
 3 files changed, 99 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index f19100c91c2b..775eee82d8a9 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -178,6 +178,13 @@ static bool sun4i_drv_node_is_frontend(struct device_node *node)
 		of_device_is_compatible(node, "allwinner,sun8i-a33-display-frontend");
 }
 
+static bool sun4i_drv_node_is_swappable_de2_mixer(struct device_node *node)
+{
+	/* The V3s has only one mixer-tcon pair, so it's not listed here. */
+	return of_device_is_compatible(node, "allwinner,sun8i-h3-de2-mixer0") ||
+		of_device_is_compatible(node, "allwinner,sun8i-h3-de2-mixer1");
+}
+
 static bool sun4i_drv_node_is_tcon(struct device_node *node)
 {
 	return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon") ||
@@ -261,6 +268,44 @@ static int sun4i_drv_add_endpoints(struct device *dev,
 			}
 		}
 
+		/*
+		 * The second endpoint of the output of a swappable DE2 mixer
+		 * is the TCON after connection swapping.
+		 * Ignore it now, as we now hardcode mixer0->tcon0,
+		 * mixer1->tcon1 connection.
+		 */
+		if (sun4i_drv_node_is_swappable_de2_mixer(node)) {
+			struct device_node *remote_ep_node;
+			struct of_endpoint local_endpoint, remote_endpoint;
+
+			remote_ep_node = of_graph_get_remote_endpoint(ep);
+			if (!remote_ep_node) {
+				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
+				continue;
+			}
+
+			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
+				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
+				of_node_put(remote_ep_node);
+				continue;
+			}
+
+			if (of_graph_parse_endpoint(remote_ep_node,
+						    &remote_endpoint)) {
+				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
+				of_node_put(remote_ep_node);
+				continue;
+			}
+
+			if (local_endpoint.id != remote_endpoint.id) {
+				DRM_DEBUG_DRIVER("Endpoint is an unused connection for DE2 mixer... skipping\n");
+				of_node_put(remote_ep_node);
+				continue;
+			}
+
+			of_node_put(remote_ep_node);
+		}
+
 		/* Walk down our tree */
 		count += sun4i_drv_add_endpoints(dev, match, remote);
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index d9791292553e..568cea0e5f8f 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -464,7 +464,8 @@ static int sun4i_tcon_init_regmap(struct device *dev,
  * requested via the get_id function of the engine.
  */
 static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv *drv,
-						   struct device_node *node)
+						   struct device_node *node,
+						   bool skip_bonus_ep)
 {
 	struct device_node *port, *ep, *remote;
 	struct sunxi_engine *engine;
@@ -478,6 +479,42 @@ static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv *drv,
 		if (!remote)
 			continue;
 
+		if (skip_bonus_ep) {
+			struct device_node *remote_ep_node;
+			struct of_endpoint local_endpoint, remote_endpoint;
+
+			remote_ep_node = of_graph_get_remote_endpoint(ep);
+			if (!remote_ep_node) {
+				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
+				of_node_put(remote);
+				continue;
+			}
+
+			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
+				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
+				of_node_put(remote);
+				of_node_put(remote_ep_node);
+				continue;
+			}
+
+			if (of_graph_parse_endpoint(remote_ep_node,
+						    &remote_endpoint)) {
+				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
+				of_node_put(remote);
+				of_node_put(remote_ep_node);
+				continue;
+			}
+
+			if (local_endpoint.id != remote_endpoint.id) {
+				DRM_DEBUG_DRIVER("Skipping bonus mixer->TCON connection when searching engine\n");
+				of_node_put(remote);
+				of_node_put(remote_ep_node);
+				continue;
+			}
+
+			of_node_put(remote_ep_node);
+		}
+
 		/* does this node match any registered engines? */
 		list_for_each_entry(engine, &drv->engine_list, list) {
 			if (remote == engine->node) {
@@ -488,7 +525,7 @@ static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv *drv,
 		}
 
 		/* keep looking through upstream ports */
-		engine = sun4i_tcon_find_engine(drv, remote);
+		engine = sun4i_tcon_find_engine(drv, remote, skip_bonus_ep);
 		if (!IS_ERR(engine)) {
 			of_node_put(remote);
 			of_node_put(port);
@@ -508,21 +545,27 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	struct sun4i_tcon *tcon;
 	int ret;
 
-	engine = sun4i_tcon_find_engine(drv, dev->of_node);
-	if (IS_ERR(engine)) {
-		dev_err(dev, "Couldn't find matching engine\n");
-		return -EPROBE_DEFER;
-	}
-
 	tcon = devm_kzalloc(dev, sizeof(*tcon), GFP_KERNEL);
 	if (!tcon)
 		return -ENOMEM;
 	dev_set_drvdata(dev, tcon);
 	tcon->drm = drm;
 	tcon->dev = dev;
-	tcon->id = engine->id;
 	tcon->quirks = of_device_get_match_data(dev);
 
+	/*
+	 * As we keep the connection between DE2 mixer and TCON not swapped,
+	 * skip the bonus endpoints (which stand for swapped connection)
+	 * when finding the correspoing engine.
+	 */
+	engine = sun4i_tcon_find_engine(drv, dev->of_node,
+					tcon->quirks->swappable_input);
+	if (IS_ERR(engine)) {
+		dev_err(dev, "Couldn't find matching engine\n");
+		return -EPROBE_DEFER;
+	}
+	tcon->id = engine->id;
+
 	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 e3c50ecdcd04..c3e01c06e9a0 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -146,6 +146,8 @@
 struct sun4i_tcon_quirks {
 	bool	has_unknown_mux; /* sun5i has undocumented mux */
 	bool	has_channel_1;	/* a33 does not have channel 1 */
+	/* Some DE2 can swap the mixer<->TCON connection */
+	bool	swappable_input;
 };
 
 struct sun4i_tcon {
-- 
2.12.2

[PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[Icenowy Zheng]

From: Icenowy Zheng <icenowy@aosc.xyz>

Allwinner H3 has two special TCONs, both come without channel0. And the
TCON1 of H3 has no special clocks even for the channel1.

Add support for these kinds of TCON.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v2:
- Merged TCON0 and TCON1 quirks and compatibles.

 drivers/gpu/drm/sun4i/sun4i_tcon.c | 52 +++++++++++++++++++++++++-------------
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  1 +
 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 568cea0e5f8f..62ba4fc19f18 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -59,6 +59,7 @@ void sun4i_tcon_channel_disable(struct sun4i_tcon *tcon, int channel)
 
 	/* Disable the TCON's channel */
 	if (channel == 0) {
+		WARN_ON(!tcon->quirks->has_channel_0);
 		regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
 				   SUN4I_TCON0_CTL_TCON_ENABLE, 0);
 		clk_disable_unprepare(tcon->dclk);
@@ -78,6 +79,7 @@ void sun4i_tcon_channel_enable(struct sun4i_tcon *tcon, int channel)
 
 	/* Enable the TCON's channel */
 	if (channel == 0) {
+		WARN_ON(!tcon->quirks->has_channel_0);
 		regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
 				   SUN4I_TCON0_CTL_TCON_ENABLE,
 				   SUN4I_TCON0_CTL_TCON_ENABLE);
@@ -159,6 +161,7 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
 
 	/* Configure the dot clock */
 	clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
+	WARN_ON(!tcon->quirks->has_channel_0);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
@@ -366,10 +369,12 @@ static int sun4i_tcon_init_clocks(struct device *dev,
 	}
 	clk_prepare_enable(tcon->clk);
 
-	tcon->sclk0 = devm_clk_get(dev, "tcon-ch0");
-	if (IS_ERR(tcon->sclk0)) {
-		dev_err(dev, "Couldn't get the TCON channel 0 clock\n");
-		return PTR_ERR(tcon->sclk0);
+	if (tcon->quirks->has_channel_0) {
+		tcon->sclk0 = devm_clk_get(dev, "tcon-ch0");
+		if (IS_ERR(tcon->sclk0)) {
+			dev_err(dev, "Couldn't get the TCON channel 0 clock\n");
+			return PTR_ERR(tcon->sclk0);
+		}
 	}
 
 	if (tcon->quirks->has_channel_1) {
@@ -594,10 +599,12 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 		goto err_free_clocks;
 	}
 
-	ret = sun4i_dclk_create(dev, tcon);
-	if (ret) {
-		dev_err(dev, "Couldn't create our TCON dot clock\n");
-		goto err_free_clocks;
+	if (tcon->quirks->has_channel_0) {
+		ret = sun4i_dclk_create(dev, tcon);
+		if (ret) {
+			dev_err(dev, "Couldn't create our TCON dot clock\n");
+			goto err_free_clocks;
+		}
 	}
 
 	ret = sun4i_tcon_init_irq(dev, tcon);
@@ -622,7 +629,8 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	return 0;
 
 err_free_dotclock:
-	sun4i_dclk_free(tcon);
+	if (tcon->quirks->has_channel_0)
+		sun4i_dclk_free(tcon);
 err_free_clocks:
 	sun4i_tcon_free_clocks(tcon);
 err_assert_reset:
@@ -636,7 +644,9 @@ static void sun4i_tcon_unbind(struct device *dev, struct device *master,
 	struct sun4i_tcon *tcon = dev_get_drvdata(dev);
 
 	list_del(&tcon->list);
-	sun4i_dclk_free(tcon);
+
+	if (tcon->quirks->has_channel_0)
+		sun4i_dclk_free(tcon);
 	sun4i_tcon_free_clocks(tcon);
 }
 
@@ -667,24 +677,32 @@ static int sun4i_tcon_remove(struct platform_device *pdev)
 }
 
 static const struct sun4i_tcon_quirks sun5i_a13_quirks = {
-	.has_unknown_mux = true,
-	.has_channel_1	= true,
+	.has_unknown_mux	= true,
+	.has_channel_0		= true,
+	.has_channel_1		= true,
 };
 
 static const struct sun4i_tcon_quirks sun6i_a31_quirks = {
-	.has_channel_1	= true,
+	.has_channel_0		= true,
+	.has_channel_1		= true,
 };
 
 static const struct sun4i_tcon_quirks sun6i_a31s_quirks = {
-	.has_channel_1	= true,
+	.has_channel_0		= true,
+	.has_channel_1		= true,
 };
 
 static const struct sun4i_tcon_quirks sun8i_a33_quirks = {
-	/* nothing is supported */
+	.has_channel_0	= true,
 };
 
 static const struct sun4i_tcon_quirks sun8i_v3s_quirks = {
-	/* nothing is supported */
+	.has_channel_0	= true,
+};
+
+static const struct sun4i_tcon_quirks sun8i_h3_quirks = {
+	.has_channel_1		= true,
+	.swappable_input	= true,
 };
 
 static const struct of_device_id sun4i_tcon_of_table[] = {
@@ -693,7 +711,7 @@ static const struct of_device_id sun4i_tcon_of_table[] = {
 	{ .compatible = "allwinner,sun6i-a31s-tcon", .data = &sun6i_a31s_quirks },
 	{ .compatible = "allwinner,sun8i-a33-tcon", .data = &sun8i_a33_quirks },
 	{ .compatible = "allwinner,sun8i-v3s-tcon", .data = &sun8i_v3s_quirks },
-	{ }
+	{ .compatible = "allwinner,sun8i-h3-tcon", .data = &sun8i_h3_quirks },
 };
 MODULE_DEVICE_TABLE(of, sun4i_tcon_of_table);
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index c3e01c06e9a0..9c706a0bd478 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -145,6 +145,7 @@
 
 struct sun4i_tcon_quirks {
 	bool	has_unknown_mux; /* sun5i has undocumented mux */
+	bool	has_channel_0;	/* some A83T+ TCONs don't have channel 0*/
 	bool	has_channel_1;	/* a33 does not have channel 1 */
 	/* Some DE2 can swap the mixer<->TCON connection */
 	bool	swappable_input;
-- 
2.12.2

[PATCH v2 05/11] drm: sun4i: add compatible for H3 display engine

[Icenowy Zheng]

Add a compatible string for H3 display engine in sun4i_drv code.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 775eee82d8a9..2003507b41a6 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -355,6 +355,7 @@ static const struct of_device_id sun4i_drv_of_table[] = {
 	{ .compatible = "allwinner,sun6i-a31-display-engine" },
 	{ .compatible = "allwinner,sun6i-a31s-display-engine" },
 	{ .compatible = "allwinner,sun8i-a33-display-engine" },
+	{ .compatible = "allwinner,sun8i-h3-display-engine" },
 	{ .compatible = "allwinner,sun8i-v3s-display-engine" },
 	{ }
 };
-- 
2.12.2

[PATCH v2 06/11] drm: sun4i: add color space correction support for DE2 mixer

[Icenowy Zheng]

The DE2 mixer can do color space correction needed by TV Encoder with
its DCSC sub-engine.

Add support for it.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun8i_mixer.h |  6 +++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index d658a3a8159a..65f86641eca3 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -29,6 +29,14 @@
 #include "sun8i_layer.h"
 #include "sunxi_engine.h"
 
+static const u32 sun8i_rgb2yuv_coef[12] = {
+	0x00000107, 0x00000204, 0x00000064, 0x00004200,
+	0x00001f68, 0x00001ed6, 0x000001c2, 0x00020200,
+	0x000001c2, 0x00001e87, 0x00001fb7, 0x00020200,
+};
+
+static const u32 sun8i_rgb2yuv_dcsc_alpha = 0x00020200;
+
 static void sun8i_mixer_commit(struct sunxi_engine *engine)
 {
 	DRM_DEBUG_DRIVER("Committing changes\n");
@@ -37,6 +45,31 @@ static void sun8i_mixer_commit(struct sunxi_engine *engine)
 		     SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
 }
 
+static void sun8i_mixer_apply_color_correction(struct sunxi_engine *engine)
+{
+	int i;
+
+	DRM_DEBUG_DRIVER("Applying RGB to YUV color correction\n");
+
+	/* Set color correction */
+	regmap_write(engine->regs, SUN8I_MIXER_DCSC_EN, 1);
+
+	for (i = 0; i < 12; i++)
+		regmap_write(engine->regs, SUN8I_MIXER_DCSC_COEF_REG(i),
+			     sun8i_rgb2yuv_coef[i]);
+
+	regmap_write(engine->regs, SUN8I_MIXER_DCSC_COEF_ALPHA,
+		     sun8i_rgb2yuv_dcsc_alpha);
+}
+
+static void sun8i_mixer_disable_color_correction(struct sunxi_engine *engine)
+{
+	DRM_DEBUG_DRIVER("Disabling color correction\n");
+
+	/* Disable color correction */
+	regmap_write(engine->regs, SUN8I_MIXER_DCSC_EN, 0);
+}
+
 void sun8i_mixer_layer_enable(struct sun8i_mixer *mixer,
 				int layer, bool enable)
 {
@@ -229,6 +262,8 @@ int sun8i_mixer_update_layer_buffer(struct sun8i_mixer *mixer,
 static const struct sunxi_engine_ops sun8i_engine_ops = {
 	.commit		= sun8i_mixer_commit,
 	.layers_init	= sun8i_layers_init,
+	.apply_color_correction		= sun8i_mixer_apply_color_correction,
+	.disable_color_correction	= sun8i_mixer_disable_color_correction,
 };
 
 static struct regmap_config sun8i_mixer_regmap_config = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index 4785ac090b8c..d7f7513898b6 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -88,6 +88,11 @@
 #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_RGB888	(8 << 8)
 #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_DEF	(0xff << 24)
 
+/* The DCSC sub-engine is used to do color space conversation */
+#define SUN8I_MIXER_DCSC_EN			0xb0000
+#define SUN8I_MIXER_DCSC_COEF_REG(x)		(0xb0010 + 0x4 * x)
+#define SUN8I_MIXER_DCSC_COEF_ALPHA		0xb0040
+
 /*
  * These sub-engines are still unknown now, the EN registers are here only to
  * be used to disable these sub-engines.
@@ -102,7 +107,6 @@
 #define SUN8I_MIXER_PEAK_EN			0xa6000
 #define SUN8I_MIXER_ASE_EN			0xa8000
 #define SUN8I_MIXER_FCC_EN			0xaa000
-#define SUN8I_MIXER_DCSC_EN			0xb0000
 
 struct sun8i_mixer_cfg {
 	int		vi_num;
-- 
2.12.2

[PATCH v2 07/11] drm: sun4i: add support for the TV encoder in H3 SoC

[Icenowy Zheng]

Allwinner H3 features a TV encoder similar to the one in earlier SoCs,
but has a internal fixed clock divider that divides the TCON1 clock
(called TVE clock in datasheet) by 11.

Add support for it.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v2:
- Quirk part rewritten.

 drivers/gpu/drm/sun4i/sun4i_tv.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c
index 338b9e5bb2a3..b9ff6d5ea67a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
@@ -13,6 +13,7 @@
 #include <linux/clk.h>
 #include <linux/component.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
 
@@ -169,14 +170,21 @@ struct tv_mode {
 	const struct resync_parameters	*resync_params;
 };
 
+struct sun4i_tv_quirks {
+	int fixed_divider;
+};
+
 struct sun4i_tv {
 	struct drm_connector	connector;
 	struct drm_encoder	encoder;
 
 	struct clk		*clk;
+	struct clk		*mod_clk;
 	struct regmap		*regs;
 	struct reset_control	*reset;
 
+	const struct sun4i_tv_quirks *quirks;
+
 	struct sun4i_drv	*drv;
 };
 
@@ -391,6 +399,12 @@ static void sun4i_tv_mode_set(struct drm_encoder *encoder,
 	struct sun4i_tcon *tcon = crtc->tcon;
 	const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode);
 
+	if (tv->quirks->fixed_divider) {
+		DRM_DEBUG_DRIVER("Applying fixed divider %d on TVE clock\n",
+				 tv->quirks->fixed_divider);
+		mode->crtc_clock *= tv->quirks->fixed_divider;
+	}
+
 	sun4i_tcon1_mode_set(tcon, mode);
 	sun4i_tcon_set_mux(tcon, 1, encoder);
 
@@ -579,6 +593,10 @@ static int sun4i_tv_bind(struct device *dev, struct device *master,
 	tv->drv = drv;
 	dev_set_drvdata(dev, tv);
 
+	tv->quirks = of_device_get_match_data(dev);
+	if (!tv->quirks)
+		return -EINVAL;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	regs = devm_ioremap_resource(dev, res);
 	if (IS_ERR(regs)) {
@@ -684,8 +702,23 @@ static int sun4i_tv_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct sun4i_tv_quirks sun4i_a10_tv_quirks = {
+	/* Nothing special */
+};
+
+static const struct sun4i_tv_quirks sun8i_h3_tv_quirks = {
+	.fixed_divider = 16,
+};
+
 static const struct of_device_id sun4i_tv_of_table[] = {
-	{ .compatible = "allwinner,sun4i-a10-tv-encoder" },
+	{
+		.compatible = "allwinner,sun4i-a10-tv-encoder",
+		.data = &sun4i_a10_tv_quirks,
+	},
+	{
+		.compatible = "allwinner,sun8i-h3-tv-encoder",
+		.data = &sun8i_h3_tv_quirks,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sun4i_tv_of_table);
-- 
2.12.2

[PATCH v2 08/11] clk: sunxi-ng: allow CLK_DE to set CLK_PLL_DE for H3

[Icenowy Zheng]

Allwinner H3 features a PLL named CLK_PLL_DE, and a mod clock for the
"Display Engine 2.0" named CLK_DE. As the name indicated, the CLK_PLL_DE
is a PLL for CLK_DE.

Only CLK_DE and CLK_TVE have a parent of CLK_PLL_DE, and CLK_TVE is also
one part of the display clocks.

So allow CLK_DE to set CLK_PLL_DE (add CLK_SET_RATE_PARENT to it).

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index 62e4f0d2b2fc..b6a1636c2f6b 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -439,7 +439,7 @@ static SUNXI_CCU_GATE(dram_ts_clk,	"dram-ts",	"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 const char * const tcon_parents[] = { "pll-video" };
 static SUNXI_CCU_M_WITH_MUX_GATE(tcon_clk, "tcon", tcon_parents,
-- 
2.12.2

[PATCH v2 09/11] clk: sunxi-ng: export CLK_PLL_DE for H3

[Icenowy Zheng]

The CLK_PLL_DE is needed to be referenced in device tree for H3, for
both forcing the parent of PLL_DE.

So export it to the device tree binding header.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/clk/sunxi-ng/ccu-sun8i-h3.h      | 3 +--
 include/dt-bindings/clock/sun8i-h3-ccu.h | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.h b/drivers/clk/sunxi-ng/ccu-sun8i-h3.h
index 1b4baea37d81..add3a7c18212 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.h
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.h
@@ -35,9 +35,8 @@
 #define CLK_PLL_PERIPH0_2X	10
 #define CLK_PLL_GPU		11
 #define CLK_PLL_PERIPH1		12
-#define CLK_PLL_DE		13
 
-/* The CPUX clock is exported */
+/* The PLL_DE and CPUX clocks is exported */
 
 #define CLK_AXI			15
 #define CLK_AHB1		16
diff --git a/include/dt-bindings/clock/sun8i-h3-ccu.h b/include/dt-bindings/clock/sun8i-h3-ccu.h
index e139fe5c62ec..5345957a8c2e 100644
--- a/include/dt-bindings/clock/sun8i-h3-ccu.h
+++ b/include/dt-bindings/clock/sun8i-h3-ccu.h
@@ -45,6 +45,8 @@
 
 #define CLK_PLL_PERIPH0		9
 
+#define CLK_PLL_DE		13
+
 #define CLK_CPUX		14
 
 #define CLK_BUS_CE		20
-- 
2.12.2

[PATCH v2 10/11] ARM: sun8i: h3: add display engine pipeline for TVE

[Icenowy Zheng]

As we have already the support for the TV encoder on Allwinner H3, add
the display engine pipeline device tree nodes to its DTSI file.

The H5 pipeline has some differences and will be enabled later.

The currently-unused mixer0 and tcon0 are also needed, for the
completement of the pipeline.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v2:
- Changes according to new dt bindings.

 arch/arm/boot/dts/sun8i-h3.dtsi | 186 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 186 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index b36f9f423c39..d5f2f43c3fb4 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -41,6 +41,8 @@
  */
 
 #include "sunxi-h3-h5.dtsi"
+#include <dt-bindings/clock/sun8i-de2.h>
+#include <dt-bindings/reset/sun8i-de2.h>
 
 / {
 	cpus {
@@ -72,6 +74,190 @@
 		};
 	};
 
+	de: display-engine {
+		compatible = "allwinner,sun8i-h3-display-engine";
+		allwinner,pipelines = <&mixer0>,
+				      <&mixer1>;
+		status = "disabled";
+	};
+
+	soc {
+		display_clocks: clock@1000000 {
+			compatible = "allwinner,sun8i-a83t-de2-clk";
+			reg = <0x01000000 0x100000>;
+			clocks = <&ccu CLK_BUS_DE>,
+				 <&ccu CLK_DE>;
+			clock-names = "bus",
+				      "mod";
+			resets = <&ccu RST_BUS_DE>;
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			assigned-clocks = <&ccu CLK_DE>;
+			assigned-clock-parents = <&ccu CLK_PLL_DE>;
+			assigned-clock-rates = <432000000>;
+		};
+
+		mixer0: mixer@1100000 {
+			compatible = "allwinner,sun8i-h3-de2-mixer0";
+			reg = <0x01100000 0x100000>;
+			clocks = <&display_clocks CLK_BUS_MIXER0>,
+				 <&display_clocks CLK_MIXER0>;
+			clock-names = "bus",
+				      "mod";
+			resets = <&display_clocks RST_MIXER0>;
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				mixer0_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					mixer0_out_tcon0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&tcon0_in_mixer0>;
+					};
+
+					mixer0_out_tcon1: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&tcon1_in_mixer0>;
+					};
+				};
+			};
+		};
+
+		mixer1: mixer@1200000 {
+			compatible = "allwinner,sun8i-h3-de2-mixer1";
+			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 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					mixer1_out_tcon0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&tcon0_in_mixer1>;
+					};
+
+					mixer1_out_tcon1: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&tcon1_in_mixer1>;
+					};
+				};
+			};
+		};
+
+		tcon0: lcd-controller@1c0c000 {
+			compatible = "allwinner,sun8i-h3-tcon";
+			reg = <0x01c0c000 0x1000>;
+			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_TCON0>,
+				 <&ccu CLK_TCON0>;
+			clock-names = "ahb",
+				      "tcon-ch1";
+			resets = <&ccu RST_BUS_TCON0>;
+			reset-names = "lcd";
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				tcon0_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					tcon0_in_mixer0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&mixer0_out_tcon0>;
+					};
+
+					tcon0_in_mixer1: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&mixer1_out_tcon0>;
+					};
+				};
+			};
+		};
+
+		tcon1: lcd-controller@1c0d000 {
+			compatible = "allwinner,sun8i-h3-tcon";
+			reg = <0x01c0d000 0x1000>;
+			interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_TCON1>,
+				 <&ccu CLK_TVE>;
+			clock-names = "ahb",
+				      "tcon-ch1";
+			resets = <&ccu RST_BUS_TCON1>;
+			reset-names = "lcd";
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				tcon1_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					tcon1_in_mixer0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&mixer0_out_tcon1>;
+					};
+
+					tcon1_in_mixer1: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&mixer1_out_tcon1>;
+					};
+				};
+
+				tcon1_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					tcon1_out_tve0: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&tve0_in_tcon1>;
+					};
+				};
+			};
+		};
+
+		tve0: tv-encoder@1e00000 {
+			compatible = "allwinner,sun8i-h3-tv-encoder";
+			reg = <0x01e00000 0x1000>;
+			clocks = <&ccu CLK_BUS_TVE>;
+			resets = <&ccu RST_BUS_TVE>;
+			status = "disabled";
+
+			port {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				tve0_in_tcon1: endpoint@0 {
+					reg = <0>;
+					remote-endpoint = <&tcon1_out_tve0>;
+				};
+			};
+		};
+	};
+
 	timer {
 		compatible = "arm,armv7-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
-- 
2.12.2

[PATCH v2 11/11] [DO NOT MERGE] ARM: sun8i: h3: enable TV output on Orange Pi PC

[Icenowy Zheng]

Orange Pi PC features a 3.5mm jack with TV output in it.

Enable the TV output.

As it currently do not have jack detection feature, do not merge this
patch.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
index 998b60f8d295..a81d25e722e9 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
@@ -98,6 +98,10 @@
 	status = "okay";
 };
 
+&de {
+	status = "okay";
+};
+
 &ehci0 {
 	status = "okay";
 };
@@ -177,6 +181,14 @@
 	status = "okay";
 };
 
+&tcon1 {
+	status = "okay";
+};
+
+&tve0 {
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_a>;
-- 
2.12.2

Re: [PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[Jernej Škrabec]

Hi,

Dne nedelja, 04. junij 2017 ob 18:01:42 CEST je Icenowy Zheng napisal(a):
> From: Icenowy Zheng <icenowy@aosc.xyz>
> 
> Allwinner H3 has two special TCONs, both come without channel0. And the
> TCON1 of H3 has no special clocks even for the channel1.
> 
> Add support for these kinds of TCON.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
> Changes in v2:
> - Merged TCON0 and TCON1 quirks and compatibles.
> 
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 52
> +++++++++++++++++++++++++------------- drivers/gpu/drm/sun4i/sun4i_tcon.h |
>  1 +
>  2 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 568cea0e5f8f..62ba4fc19f18
> 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -59,6 +59,7 @@ void sun4i_tcon_channel_disable(struct sun4i_tcon *tcon,
> int channel)
> 
>  	/* Disable the TCON's channel */
>  	if (channel == 0) {
> +		WARN_ON(!tcon->quirks->has_channel_0);
>  		regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
>  				   SUN4I_TCON0_CTL_TCON_ENABLE, 0);
>  		clk_disable_unprepare(tcon->dclk);
> @@ -78,6 +79,7 @@ void sun4i_tcon_channel_enable(struct sun4i_tcon *tcon,
> int channel)
> 
>  	/* Enable the TCON's channel */
>  	if (channel == 0) {
> +		WARN_ON(!tcon->quirks->has_channel_0);
>  		regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
>  				   SUN4I_TCON0_CTL_TCON_ENABLE,
>  				   SUN4I_TCON0_CTL_TCON_ENABLE);
> @@ -159,6 +161,7 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
> 
>  	/* Configure the dot clock */
>  	clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
> +	WARN_ON(!tcon->quirks->has_channel_0);
> 
>  	/* Adjust clock delay */
>  	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
> @@ -366,10 +369,12 @@ static int sun4i_tcon_init_clocks(struct device *dev,
>  	}
>  	clk_prepare_enable(tcon->clk);
> 
> -	tcon->sclk0 = devm_clk_get(dev, "tcon-ch0");
> -	if (IS_ERR(tcon->sclk0)) {
> -		dev_err(dev, "Couldn't get the TCON channel 0 clock\n");
> -		return PTR_ERR(tcon->sclk0);
> +	if (tcon->quirks->has_channel_0) {
> +		tcon->sclk0 = devm_clk_get(dev, "tcon-ch0");
> +		if (IS_ERR(tcon->sclk0)) {
> +			dev_err(dev, "Couldn't get the TCON channel 0 clock\n");
> +			return PTR_ERR(tcon->sclk0);
> +		}
>  	}
> 
>  	if (tcon->quirks->has_channel_1) {
> @@ -594,10 +599,12 @@ static int sun4i_tcon_bind(struct device *dev, struct
> device *master, goto err_free_clocks;
>  	}
> 
> -	ret = sun4i_dclk_create(dev, tcon);
> -	if (ret) {
> -		dev_err(dev, "Couldn't create our TCON dot clock\n");
> -		goto err_free_clocks;
> +	if (tcon->quirks->has_channel_0) {
> +		ret = sun4i_dclk_create(dev, tcon);
> +		if (ret) {
> +			dev_err(dev, "Couldn't create our TCON dot clock\n");
> +			goto err_free_clocks;
> +		}
>  	}
> 
>  	ret = sun4i_tcon_init_irq(dev, tcon);
> @@ -622,7 +629,8 @@ static int sun4i_tcon_bind(struct device *dev, struct
> device *master, return 0;
> 
>  err_free_dotclock:
> -	sun4i_dclk_free(tcon);
> +	if (tcon->quirks->has_channel_0)
> +		sun4i_dclk_free(tcon);
>  err_free_clocks:
>  	sun4i_tcon_free_clocks(tcon);
>  err_assert_reset:
> @@ -636,7 +644,9 @@ static void sun4i_tcon_unbind(struct device *dev, struct
> device *master, struct sun4i_tcon *tcon = dev_get_drvdata(dev);
> 
>  	list_del(&tcon->list);
> -	sun4i_dclk_free(tcon);
> +
> +	if (tcon->quirks->has_channel_0)
> +		sun4i_dclk_free(tcon);
>  	sun4i_tcon_free_clocks(tcon);
>  }
> 
> @@ -667,24 +677,32 @@ static int sun4i_tcon_remove(struct platform_device
> *pdev) }
> 
>  static const struct sun4i_tcon_quirks sun5i_a13_quirks = {
> -	.has_unknown_mux = true,
> -	.has_channel_1	= true,
> +	.has_unknown_mux	= true,
> +	.has_channel_0		= true,
> +	.has_channel_1		= true,
>  };
> 
>  static const struct sun4i_tcon_quirks sun6i_a31_quirks = {
> -	.has_channel_1	= true,
> +	.has_channel_0		= true,
> +	.has_channel_1		= true,
>  };
> 
>  static const struct sun4i_tcon_quirks sun6i_a31s_quirks = {
> -	.has_channel_1	= true,
> +	.has_channel_0		= true,
> +	.has_channel_1		= true,
>  };
> 
>  static const struct sun4i_tcon_quirks sun8i_a33_quirks = {
> -	/* nothing is supported */
> +	.has_channel_0	= true,
>  };
> 
>  static const struct sun4i_tcon_quirks sun8i_v3s_quirks = {
> -	/* nothing is supported */
> +	.has_channel_0	= true,
> +};
> +
> +static const struct sun4i_tcon_quirks sun8i_h3_quirks = {
> +	.has_channel_1		= true,
> +	.swappable_input	= true,
>  };
> 
>  static const struct of_device_id sun4i_tcon_of_table[] = {
> @@ -693,7 +711,7 @@ static const struct of_device_id sun4i_tcon_of_table[] =
> { { .compatible = "allwinner,sun6i-a31s-tcon", .data = &sun6i_a31s_quirks
> }, { .compatible = "allwinner,sun8i-a33-tcon", .data = &sun8i_a33_quirks },
> { .compatible = "allwinner,sun8i-v3s-tcon", .data = &sun8i_v3s_quirks },
> -	{ }
> +	{ .compatible = "allwinner,sun8i-h3-tcon", .data = &sun8i_h3_quirks },

I think you need to leave empty entry as a sentinel.

>  };
>  MODULE_DEVICE_TABLE(of, sun4i_tcon_of_table);
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h
> b/drivers/gpu/drm/sun4i/sun4i_tcon.h index c3e01c06e9a0..9c706a0bd478
> 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> @@ -145,6 +145,7 @@
> 
>  struct sun4i_tcon_quirks {
>  	bool	has_unknown_mux; /* sun5i has undocumented mux */
> +	bool	has_channel_0;	/* some A83T+ TCONs don't have channel 0*/
>  	bool	has_channel_1;	/* a33 does not have channel 1 */
>  	/* Some DE2 can swap the mixer<->TCON connection */
>  	bool	swappable_input;
> --
> 2.12.2

You missed sun4i_rgb_init() function which should also be guarded with "if 
(tcon->quirks->has_channel_0)".

You should also expand function sun4i_drv_node_is_tcon() at sun4i_drv.c with 
new entries, but I'm not sure if this fits in this patch.

Best regards,
Jernej

Re: [PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[Icenowy Zheng]

于 2017年6月5日 GMT+08:00 上午2:46:24, "Jernej Škrabec" <jernej.skrabec@siol.net> 写到:
>Hi,
>
>Dne nedelja, 04. junij 2017 ob 18:01:42 CEST je Icenowy Zheng
>napisal(a):
>> From: Icenowy Zheng <icenowy@aosc.xyz>
>> 
>> Allwinner H3 has two special TCONs, both come without channel0. And
>the
>> TCON1 of H3 has no special clocks even for the channel1.
>> 
>> Add support for these kinds of TCON.
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>> Changes in v2:
>> - Merged TCON0 and TCON1 quirks and compatibles.
>> 
>>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 52
>> +++++++++++++++++++++++++-------------
>drivers/gpu/drm/sun4i/sun4i_tcon.h |
>>  1 +
>>  2 files changed, 36 insertions(+), 17 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 568cea0e5f8f..62ba4fc19f18
>> 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -59,6 +59,7 @@ void sun4i_tcon_channel_disable(struct sun4i_tcon
>*tcon,
>> int channel)
>> 
>>  	/* Disable the TCON's channel */
>>  	if (channel == 0) {
>> +		WARN_ON(!tcon->quirks->has_channel_0);
>>  		regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
>>  				   SUN4I_TCON0_CTL_TCON_ENABLE, 0);
>>  		clk_disable_unprepare(tcon->dclk);
>> @@ -78,6 +79,7 @@ void sun4i_tcon_channel_enable(struct sun4i_tcon
>*tcon,
>> int channel)
>> 
>>  	/* Enable the TCON's channel */
>>  	if (channel == 0) {
>> +		WARN_ON(!tcon->quirks->has_channel_0);
>>  		regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
>>  				   SUN4I_TCON0_CTL_TCON_ENABLE,
>>  				   SUN4I_TCON0_CTL_TCON_ENABLE);
>> @@ -159,6 +161,7 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon
>*tcon,
>> 
>>  	/* Configure the dot clock */
>>  	clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
>> +	WARN_ON(!tcon->quirks->has_channel_0);
>> 
>>  	/* Adjust clock delay */
>>  	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
>> @@ -366,10 +369,12 @@ static int sun4i_tcon_init_clocks(struct device
>*dev,
>>  	}
>>  	clk_prepare_enable(tcon->clk);
>> 
>> -	tcon->sclk0 = devm_clk_get(dev, "tcon-ch0");
>> -	if (IS_ERR(tcon->sclk0)) {
>> -		dev_err(dev, "Couldn't get the TCON channel 0 clock\n");
>> -		return PTR_ERR(tcon->sclk0);
>> +	if (tcon->quirks->has_channel_0) {
>> +		tcon->sclk0 = devm_clk_get(dev, "tcon-ch0");
>> +		if (IS_ERR(tcon->sclk0)) {
>> +			dev_err(dev, "Couldn't get the TCON channel 0 clock\n");
>> +			return PTR_ERR(tcon->sclk0);
>> +		}
>>  	}
>> 
>>  	if (tcon->quirks->has_channel_1) {
>> @@ -594,10 +599,12 @@ static int sun4i_tcon_bind(struct device *dev,
>struct
>> device *master, goto err_free_clocks;
>>  	}
>> 
>> -	ret = sun4i_dclk_create(dev, tcon);
>> -	if (ret) {
>> -		dev_err(dev, "Couldn't create our TCON dot clock\n");
>> -		goto err_free_clocks;
>> +	if (tcon->quirks->has_channel_0) {
>> +		ret = sun4i_dclk_create(dev, tcon);
>> +		if (ret) {
>> +			dev_err(dev, "Couldn't create our TCON dot clock\n");
>> +			goto err_free_clocks;
>> +		}
>>  	}
>> 
>>  	ret = sun4i_tcon_init_irq(dev, tcon);
>> @@ -622,7 +629,8 @@ static int sun4i_tcon_bind(struct device *dev,
>struct
>> device *master, return 0;
>> 
>>  err_free_dotclock:
>> -	sun4i_dclk_free(tcon);
>> +	if (tcon->quirks->has_channel_0)
>> +		sun4i_dclk_free(tcon);
>>  err_free_clocks:
>>  	sun4i_tcon_free_clocks(tcon);
>>  err_assert_reset:
>> @@ -636,7 +644,9 @@ static void sun4i_tcon_unbind(struct device *dev,
>struct
>> device *master, struct sun4i_tcon *tcon = dev_get_drvdata(dev);
>> 
>>  	list_del(&tcon->list);
>> -	sun4i_dclk_free(tcon);
>> +
>> +	if (tcon->quirks->has_channel_0)
>> +		sun4i_dclk_free(tcon);
>>  	sun4i_tcon_free_clocks(tcon);
>>  }
>> 
>> @@ -667,24 +677,32 @@ static int sun4i_tcon_remove(struct
>platform_device
>> *pdev) }
>> 
>>  static const struct sun4i_tcon_quirks sun5i_a13_quirks = {
>> -	.has_unknown_mux = true,
>> -	.has_channel_1	= true,
>> +	.has_unknown_mux	= true,
>> +	.has_channel_0		= true,
>> +	.has_channel_1		= true,
>>  };
>> 
>>  static const struct sun4i_tcon_quirks sun6i_a31_quirks = {
>> -	.has_channel_1	= true,
>> +	.has_channel_0		= true,
>> +	.has_channel_1		= true,
>>  };
>> 
>>  static const struct sun4i_tcon_quirks sun6i_a31s_quirks = {
>> -	.has_channel_1	= true,
>> +	.has_channel_0		= true,
>> +	.has_channel_1		= true,
>>  };
>> 
>>  static const struct sun4i_tcon_quirks sun8i_a33_quirks = {
>> -	/* nothing is supported */
>> +	.has_channel_0	= true,
>>  };
>> 
>>  static const struct sun4i_tcon_quirks sun8i_v3s_quirks = {
>> -	/* nothing is supported */
>> +	.has_channel_0	= true,
>> +};
>> +
>> +static const struct sun4i_tcon_quirks sun8i_h3_quirks = {
>> +	.has_channel_1		= true,
>> +	.swappable_input	= true,
>>  };
>> 
>>  static const struct of_device_id sun4i_tcon_of_table[] = {
>> @@ -693,7 +711,7 @@ static const struct of_device_id
>sun4i_tcon_of_table[] =
>> { { .compatible = "allwinner,sun6i-a31s-tcon", .data =
>&sun6i_a31s_quirks
>> }, { .compatible = "allwinner,sun8i-a33-tcon", .data =
>&sun8i_a33_quirks },
>> { .compatible = "allwinner,sun8i-v3s-tcon", .data = &sun8i_v3s_quirks
>},
>> -	{ }
>> +	{ .compatible = "allwinner,sun8i-h3-tcon", .data = &sun8i_h3_quirks
>},
>
>I think you need to leave empty entry as a sentinel.

Sorry.

>
>>  };
>>  MODULE_DEVICE_TABLE(of, sun4i_tcon_of_table);
>> 
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h
>> b/drivers/gpu/drm/sun4i/sun4i_tcon.h index c3e01c06e9a0..9c706a0bd478
>> 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
>> @@ -145,6 +145,7 @@
>> 
>>  struct sun4i_tcon_quirks {
>>  	bool	has_unknown_mux; /* sun5i has undocumented mux */
>> +	bool	has_channel_0;	/* some A83T+ TCONs don't have channel 0*/
>>  	bool	has_channel_1;	/* a33 does not have channel 1 */
>>  	/* Some DE2 can swap the mixer<->TCON connection */
>>  	bool	swappable_input;
>> --
>> 2.12.2
>
>You missed sun4i_rgb_init() function which should also be guarded with
>"if 
>(tcon->quirks->has_channel_0)".
>

Yes...

>You should also expand function sun4i_drv_node_is_tcon() at sun4i_drv.c
>with 
>new entries, but I'm not sure if this fits in this patch.

Instead I think it should be renamed to something like
"sun4i_drv_node_is_tcon_with_ch0".

>
>Best regards,
>Jernej

Re: [PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2107 bytes --]

Hi Icenowy,

[auto build test ERROR on next-20170602]
[cannot apply to mripard/sunxi/for-next robh/for-next clk/clk-next v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.12-rc3]
[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/Icenowy-Zheng/Support-for-H3-Composite-Output-support/20170605-003002
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/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/sun4i-tcon: struct of_device_id is 196 bytes.  The last of 6 is:
   0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x61 0x6c 0x6c 0x77 0x69 0x6e 0x6e 0x65 0x72 0x2c 0x73 0x75 0x6e 0x38 0x69 0x2d 0x68 0x33 0x2d 0x74 0x63 0x6f 0x6e 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0xa0 0x04 0x00 0x00 
>> FATAL: drivers/gpu/drm/sun4i/sun4i-tcon: struct of_device_id is not terminated with a NULL entry!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41415 bytes --]

Re: [PATCH v2 00/11] Support for H3 Composite Output support

[icenowy]

在 2017-06-05 00:01，Icenowy Zheng 写道：
> Allwinner H3 SoC features a TV Encoder like the one in Allwinner A13,
> which can only output TV Composite signal.
> 
> The display pipeline of H3 is also special -- it has two mixers and
> two TCONs, of which the connection can be swapped. The TCONs do not
> have channel 0 (as they are all connected to internal bridges, TVE
> and HDMI TX).
> 
> Add support for the display pipeline and the TVE in H3, in order to
> make it possible to display something with mainline kernel with H3.
> 
> The image quality of TVE is bad, so HDMI is a better output -- this
> patchset also prepared the mixers and TCONs for HDMI output, and
> the HDMI controller driver is already done by Jernej Skrabec.
> 
> So if possible, please apply PATCH 1~5 and 8,9 as soon as possible,
> so that Jernej can submit his HDMI patches.
> 
> Currently the jack detection feature of the TVE is still not so
> clear -- so it's not implemented in this version. Thus the TV
> output shouldn't be defaultly enabled now.
> 
> Icenowy Zheng (11):
>   dt-bindings: update the binding for Allwinner H3 TVE support
>   drm: sun4i: add support for H3 mixers
>   drm: sun4i: ignore swapped mixer<->tcon connection for DE2
>   drm: sun4i: add support for H3's TCON0/1
>   drm: sun4i: add compatible for H3 display engine
>   drm: sun4i: add color space correction support for DE2 mixer
>   drm: sun4i: add support for the TV encoder in H3 SoC
>   clk: sunxi-ng: allow CLK_DE to set CLK_PLL_DE for H3
>   clk: sunxi-ng: export CLK_PLL_DE for H3
>   ARM: sun8i: h3: add display engine pipeline for TVE
>   [DO NOT MERGE] ARM: sun8i: h3: enable TV output on Orange Pi PC

Maxime, could you pick some patches that you think appliable in this
series? (especially PATCH 1, the binding patch)

I think they can be useful.

Thanks!

> 
>  .../bindings/display/sunxi/sun4i-drm.txt           |  37 +++-
>  arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts         |  12 ++
>  arch/arm/boot/dts/sun8i-h3.dtsi                    | 186 
> +++++++++++++++++++++
>  drivers/clk/sunxi-ng/ccu-sun8i-h3.c                |   2 +-
>  drivers/clk/sunxi-ng/ccu-sun8i-h3.h                |   3 +-
>  drivers/gpu/drm/sun4i/sun4i_drv.c                  |  46 +++++
>  drivers/gpu/drm/sun4i/sun4i_tcon.c                 | 113 ++++++++++---
>  drivers/gpu/drm/sun4i/sun4i_tcon.h                 |   3 +
>  drivers/gpu/drm/sun4i/sun4i_tv.c                   |  35 +++-
>  drivers/gpu/drm/sun4i/sun8i_mixer.c                |  53 ++++++
>  drivers/gpu/drm/sun4i/sun8i_mixer.h                |   6 +-
>  include/dt-bindings/clock/sun8i-h3-ccu.h           |   2 +
>  12 files changed, 463 insertions(+), 35 deletions(-)

Re: [PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4894 bytes --]

On Mon, Jun 05, 2017 at 12:01:39AM +0800, Icenowy Zheng wrote:
> Allwinner H3 features a "DE2.0" and a TV Encoder.
> 
> Add device tree bindings for the following parts:
> - H3 TCONs
> - H3 Mixers
> - The connection between H3 TCONs and H3 Mixers
> - H3 TV Encoder
> - H3 Display engine
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
> Changes in v2:
> - Changed endpoint reg definition on TCONs and Mixers. Now the endpoint
>   id is just the component id.
> - Changed TVE and TCON1 binding -- now CLK_TVE is passed as TCON1 lcd-ch1.
> 
>  .../bindings/display/sunxi/sun4i-drm.txt           | 37 +++++++++++++++++++---
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> index b83e6018041d..7ad164cb7dcb 100644
> --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> @@ -49,7 +49,9 @@ The TV Encoder supports the composite and VGA output. It is one end of
>  the pipeline.
>  
>  Required properties:
> - - compatible: value should be "allwinner,sun4i-a10-tv-encoder".
> + - compatible: value must be either:
> +    * allwinner,sun4i-a10-tv-encoder
> +    * allwinner,sun8i-h3-tv-encoder
>   - reg: base address and size of memory-mapped region
>   - clocks: the clocks driving the TV encoder
>   - resets: phandle to the reset controller driving the encoder
> @@ -69,23 +71,27 @@ Required properties:
>     * allwinner,sun6i-a31-tcon
>     * allwinner,sun6i-a31s-tcon
>     * allwinner,sun8i-a33-tcon
> +   * allwinner,sun8i-h3-tcon
>     * allwinner,sun8i-v3s-tcon
>   - reg: base address and size of memory-mapped region
>   - interrupts: interrupt associated to this IP
>   - clocks: phandles to the clocks feeding the TCON. Three are needed:
>     - 'ahb': the interface clocks
> -   - 'tcon-ch0': The clock driving the TCON channel 0
>   - resets: phandles to the reset controllers driving the encoder
>     - "lcd": the reset line for the TCON channel 0
>  
>   - clock-names: the clock names mentioned above
>   - reset-names: the reset names mentioned above
> - - clock-output-names: Name of the pixel clock created
>  
>  - ports: A ports node with endpoint definitions as defined in
>    Documentation/devicetree/bindings/media/video-interfaces.txt. The
>    first port should be the input endpoint, the second one the output
>  
> +  In the situation of Display Engine 2.0 that the connection between
> +  the mixer and the TCON can be swapped, the input should have two
> +  endpoints. The first input endpoint should be connected to mixer0
> +  and the second should be connected to mixer1.
> +

AFAIK, this is also the case for dual-pipelines on DE1 (for example on
the A31). And this is already covered by:

"
For the input port of all components up to the TCON in the display
pipeline, if there are multiple components, the local endpoint IDs
must correspond to the index of the upstream block. For example, if
the remote endpoint is Frontend 1, then the local endpoint ID must
be 1.
"

>    The output may have multiple endpoints. The TCON has two channels,
>    usually with the first channel being used for the panels interfaces
>    (RGB, LVDS, etc.), and the second being used for the outputs that
> @@ -94,7 +100,23 @@ Required properties:
>    channel the endpoint is associated to. If that property is not
>    present, the endpoint number will be used as the channel number.
>  
> -On SoCs other than the A33 and V3s, there is one more clock required:
> +For the following compatibles:
> +   * allwinner,sun5i-a13-tcon
> +   * allwinner,sun6i-a31-tcon
> +   * allwinner,sun6i-a31s-tcon
> +   * allwinner,sun8i-a33-tcon
> +   * allwinner,sun8i-v3s-tcon
> +there is one more clock and one more property required:
> + - clocks:
> +   - 'tcon-ch0': The clock driving the TCON channel 0
> + - clock-output-names: Name of the pixel clock created
> +
> +For the following compatibles:
> +   * allwinner,sun5i-a13-tcon
> +   * allwinner,sun6i-a31-tcon
> +   * allwinner,sun6i-a31s-tcon
> +   * allwinner,sun8i-h3-tcon
> +there is one more clock required:
>     - 'tcon-ch1': The clock driving the TCON channel 1
>  
>  DRC
> @@ -189,6 +211,8 @@ supported.
>  Required properties:
>    - compatible: value must be one of:
>      * allwinner,sun8i-v3s-de2-mixer
> +    * allwinner,sun8i-h3-de2-mixer0
> +    * allwinner,sun8i-h3-de2-mixer1

Again, please explain why we need to have different compatibles
here. If it's only about the number of planes, this should be dealt
with a property, not a compatible.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[Icenowy Zheng]

于 2017年6月7日 GMT+08:00 下午4:45:44, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Mon, Jun 05, 2017 at 12:01:39AM +0800, Icenowy Zheng wrote:
>> Allwinner H3 features a "DE2.0" and a TV Encoder.
>> 
>> Add device tree bindings for the following parts:
>> - H3 TCONs
>> - H3 Mixers
>> - The connection between H3 TCONs and H3 Mixers
>> - H3 TV Encoder
>> - H3 Display engine
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>> Changes in v2:
>> - Changed endpoint reg definition on TCONs and Mixers. Now the
>endpoint
>>   id is just the component id.
>> - Changed TVE and TCON1 binding -- now CLK_TVE is passed as TCON1
>lcd-ch1.
>> 
>>  .../bindings/display/sunxi/sun4i-drm.txt           | 37
>+++++++++++++++++++---
>>  1 file changed, 33 insertions(+), 4 deletions(-)
>> 
>> diff --git
>a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
>b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
>> index b83e6018041d..7ad164cb7dcb 100644
>> --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
>> +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
>> @@ -49,7 +49,9 @@ The TV Encoder supports the composite and VGA
>output. It is one end of
>>  the pipeline.
>>  
>>  Required properties:
>> - - compatible: value should be "allwinner,sun4i-a10-tv-encoder".
>> + - compatible: value must be either:
>> +    * allwinner,sun4i-a10-tv-encoder
>> +    * allwinner,sun8i-h3-tv-encoder
>>   - reg: base address and size of memory-mapped region
>>   - clocks: the clocks driving the TV encoder
>>   - resets: phandle to the reset controller driving the encoder
>> @@ -69,23 +71,27 @@ Required properties:
>>     * allwinner,sun6i-a31-tcon
>>     * allwinner,sun6i-a31s-tcon
>>     * allwinner,sun8i-a33-tcon
>> +   * allwinner,sun8i-h3-tcon
>>     * allwinner,sun8i-v3s-tcon
>>   - reg: base address and size of memory-mapped region
>>   - interrupts: interrupt associated to this IP
>>   - clocks: phandles to the clocks feeding the TCON. Three are
>needed:
>>     - 'ahb': the interface clocks
>> -   - 'tcon-ch0': The clock driving the TCON channel 0
>>   - resets: phandles to the reset controllers driving the encoder
>>     - "lcd": the reset line for the TCON channel 0
>>  
>>   - clock-names: the clock names mentioned above
>>   - reset-names: the reset names mentioned above
>> - - clock-output-names: Name of the pixel clock created
>>  
>>  - ports: A ports node with endpoint definitions as defined in
>>    Documentation/devicetree/bindings/media/video-interfaces.txt. The
>>    first port should be the input endpoint, the second one the output
>>  
>> +  In the situation of Display Engine 2.0 that the connection between
>> +  the mixer and the TCON can be swapped, the input should have two
>> +  endpoints. The first input endpoint should be connected to mixer0
>> +  and the second should be connected to mixer1.
>> +
>
>AFAIK, this is also the case for dual-pipelines on DE1 (for example on
>the A31). And this is already covered by:
>
>"
>For the input port of all components up to the TCON in the display
>pipeline, if there are multiple components, the local endpoint IDs
>must correspond to the index of the upstream block. For example, if
>the remote endpoint is Frontend 1, then the local endpoint ID must
>be 1.
>"

If you think it can fully cover this situation, I'm glad to remove
this paragraph, although I think overall paragraphs are easy
to be ignored.

>
>>    The output may have multiple endpoints. The TCON has two channels,
>>    usually with the first channel being used for the panels
>interfaces
>>    (RGB, LVDS, etc.), and the second being used for the outputs that
>> @@ -94,7 +100,23 @@ Required properties:
>>    channel the endpoint is associated to. If that property is not
>>    present, the endpoint number will be used as the channel number.
>>  
>> -On SoCs other than the A33 and V3s, there is one more clock
>required:
>> +For the following compatibles:
>> +   * allwinner,sun5i-a13-tcon
>> +   * allwinner,sun6i-a31-tcon
>> +   * allwinner,sun6i-a31s-tcon
>> +   * allwinner,sun8i-a33-tcon
>> +   * allwinner,sun8i-v3s-tcon
>> +there is one more clock and one more property required:
>> + - clocks:
>> +   - 'tcon-ch0': The clock driving the TCON channel 0
>> + - clock-output-names: Name of the pixel clock created
>> +
>> +For the following compatibles:
>> +   * allwinner,sun5i-a13-tcon
>> +   * allwinner,sun6i-a31-tcon
>> +   * allwinner,sun6i-a31s-tcon
>> +   * allwinner,sun8i-h3-tcon
>> +there is one more clock required:
>>     - 'tcon-ch1': The clock driving the TCON channel 1
>>  
>>  DRC
>> @@ -189,6 +211,8 @@ supported.
>>  Required properties:
>>    - compatible: value must be one of:
>>      * allwinner,sun8i-v3s-de2-mixer
>> +    * allwinner,sun8i-h3-de2-mixer0
>> +    * allwinner,sun8i-h3-de2-mixer1
>
>Again, please explain why we need to have different compatibles
>here. If it's only about the number of planes, this should be dealt
>with a property, not a compatible.

Only mixer0 has "VEP" and write-back support, at least on H3.

>
>Maxime

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6209 bytes --]

On Mon, Jun 05, 2017 at 12:01:41AM +0800, Icenowy Zheng wrote:
> Some SoC's DE2 has two mixers. Defaultly the mixer0 is connected to
> tcon0 and mixer1 is connected to tcon1; however by setting a bit
> the connection can be swapped.
> 
> As we now hardcode the default connection, ignore the bonus endpoint for
> the mixer's output and the TCON's input, as they stands for the swapped
> connection.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>

So, I'm still not quite sure what this patch exactly is supposed to be
doing.

You mention that the routing between the mixers and tcons can be
changed, and that we need to ignore the TCON input...

> ---
> Changes in v2:
> - Change to use new endpoint reg definition.
> 
>  drivers/gpu/drm/sun4i/sun4i_drv.c  | 45 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 61 ++++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 ++
>  3 files changed, 99 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index f19100c91c2b..775eee82d8a9 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -178,6 +178,13 @@ static bool sun4i_drv_node_is_frontend(struct device_node *node)
>  		of_device_is_compatible(node, "allwinner,sun8i-a33-display-frontend");
>  }
>  
> +static bool sun4i_drv_node_is_swappable_de2_mixer(struct device_node *node)
> +{
> +	/* The V3s has only one mixer-tcon pair, so it's not listed here. */
> +	return of_device_is_compatible(node, "allwinner,sun8i-h3-de2-mixer0") ||
> +		of_device_is_compatible(node, "allwinner,sun8i-h3-de2-mixer1");
> +}
> +
>  static bool sun4i_drv_node_is_tcon(struct device_node *node)
>  {
>  	return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon") ||
> @@ -261,6 +268,44 @@ static int sun4i_drv_add_endpoints(struct device *dev,
>  			}
>  		}
>  
> +		/*
> +		 * The second endpoint of the output of a swappable DE2 mixer
> +		 * is the TCON after connection swapping.
> +		 * Ignore it now, as we now hardcode mixer0->tcon0,
> +		 * mixer1->tcon1 connection.
> +		 */
> +		if (sun4i_drv_node_is_swappable_de2_mixer(node)) {
> +			struct device_node *remote_ep_node;
> +			struct of_endpoint local_endpoint, remote_endpoint;
> +
> +			remote_ep_node = of_graph_get_remote_endpoint(ep);
> +			if (!remote_ep_node) {
> +				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
> +				continue;
> +			}
> +
> +			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
> +				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			if (of_graph_parse_endpoint(remote_ep_node,
> +						    &remote_endpoint)) {
> +				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			if (local_endpoint.id != remote_endpoint.id) {
> +				DRM_DEBUG_DRIVER("Endpoint is an unused connection for DE2 mixer... skipping\n");
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			of_node_put(remote_ep_node);
> +		}
> +
>  		/* Walk down our tree */
>  		count += sun4i_drv_add_endpoints(dev, match, remote);

... yet this is not parsing the input at all, but only the output nodes.


> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index d9791292553e..568cea0e5f8f 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -464,7 +464,8 @@ static int sun4i_tcon_init_regmap(struct device *dev,
>   * requested via the get_id function of the engine.
>   */
>  static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv *drv,
> -						   struct device_node *node)
> +						   struct device_node *node,
> +						   bool skip_bonus_ep)
>  {
>  	struct device_node *port, *ep, *remote;
>  	struct sunxi_engine *engine;
> @@ -478,6 +479,42 @@ static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv *drv,
>  		if (!remote)
>  			continue;
>  
> +		if (skip_bonus_ep) {
> +			struct device_node *remote_ep_node;
> +			struct of_endpoint local_endpoint, remote_endpoint;
> +
> +			remote_ep_node = of_graph_get_remote_endpoint(ep);
> +			if (!remote_ep_node) {
> +				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
> +				of_node_put(remote);
> +				continue;
> +			}
> +
> +			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
> +				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
> +				of_node_put(remote);
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			if (of_graph_parse_endpoint(remote_ep_node,
> +						    &remote_endpoint)) {
> +				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
> +				of_node_put(remote);
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			if (local_endpoint.id != remote_endpoint.id) {
> +				DRM_DEBUG_DRIVER("Skipping bonus mixer->TCON connection when searching engine\n");
> +				of_node_put(remote);
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			of_node_put(remote_ep_node);
> +		}
> +

I have no idea what this is supposed to be doing either.

I might be wrong, but I really feel like there's a big mismatch
between your commit log, and what you actually implement.

In your commit log, you should state:

A) What is the current behaviour
B) Why that is a problem
C) How do you address it

And you don't.

However, after discussing it with Chen-Yu, it seems like you're trying
to have all the mixers probed before the TCONs. If that is so, there's
nothing specific to the H3 here, and we also have the same issue on
dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
the easiest solution would be to move from a DFS algorithm to walk
down the graph to a BFS one.

That way, we would add all mixers first, then the TCONs, then the
encoders, and the component framework will probe them in order.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 07/11] drm: sun4i: add support for the TV encoder in H3 SoC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

On Mon, Jun 05, 2017 at 12:01:45AM +0800, Icenowy Zheng wrote:
> Allwinner H3 features a TV encoder similar to the one in earlier SoCs,
> but has a internal fixed clock divider that divides the TCON1 clock
> (called TVE clock in datasheet) by 11.
> 
> Add support for it.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
> Changes in v2:
> - Quirk part rewritten.
> 
>  drivers/gpu/drm/sun4i/sun4i_tv.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c
> index 338b9e5bb2a3..b9ff6d5ea67a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
> @@ -13,6 +13,7 @@
>  #include <linux/clk.h>
>  #include <linux/component.h>
>  #include <linux/of_address.h>
> +#include <linux/of_device.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  
> @@ -169,14 +170,21 @@ struct tv_mode {
>  	const struct resync_parameters	*resync_params;
>  };
>  
> +struct sun4i_tv_quirks {
> +	int fixed_divider;
> +};
> +
>  struct sun4i_tv {
>  	struct drm_connector	connector;
>  	struct drm_encoder	encoder;
>  
>  	struct clk		*clk;
> +	struct clk		*mod_clk;
>  	struct regmap		*regs;
>  	struct reset_control	*reset;
>  
> +	const struct sun4i_tv_quirks *quirks;
> +
>  	struct sun4i_drv	*drv;
>  };
>  
> @@ -391,6 +399,12 @@ static void sun4i_tv_mode_set(struct drm_encoder *encoder,
>  	struct sun4i_tcon *tcon = crtc->tcon;
>  	const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode);
>  
> +	if (tv->quirks->fixed_divider) {
> +		DRM_DEBUG_DRIVER("Applying fixed divider %d on TVE clock\n",
> +				 tv->quirks->fixed_divider);
> +		mode->crtc_clock *= tv->quirks->fixed_divider;
> +	}
> +

You're not allowed to change the mode in mode_set, and you shouldn't
even change it. The output pixel clock is still 27MHz.

You should implement that using the states, as we discussed already.

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 10/11] ARM: sun8i: h3: add display engine pipeline for TVE

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

On Mon, Jun 05, 2017 at 12:01:48AM +0800, Icenowy Zheng wrote:
> +	soc {
> +		display_clocks: clock@1000000 {
> +			compatible = "allwinner,sun8i-a83t-de2-clk";
> +			reg = <0x01000000 0x100000>;
> +			clocks = <&ccu CLK_BUS_DE>,
> +				 <&ccu CLK_DE>;
> +			clock-names = "bus",
> +				      "mod";
> +			resets = <&ccu RST_BUS_DE>;
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			assigned-clocks = <&ccu CLK_DE>;
> +			assigned-clock-parents = <&ccu CLK_PLL_DE>;
> +			assigned-clock-rates = <432000000>;
> +		};

We discussed that already a few times, but there's no reason to do
so. If you need a downstream clock at a particular rate, call
clk_set_rate on it, period.

Whether its parent will be coming from PLL_DE or some other more
appriopriate clock is not relevant and doesn't make any difference.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 588 bytes --]

On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote:
> >You should also expand function sun4i_drv_node_is_tcon() at sun4i_drv.c
> >with 
> >new entries, but I'm not sure if this fits in this patch.
> 
> Instead I think it should be renamed to something like
> "sun4i_drv_node_is_tcon_with_ch0".

I'm not sure, or at least, it shouldn't make any difference, since
TCON without a channel 0 will not have an endpoint 0, so this will be
dealt with already.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Icenowy Zheng]

于 2017年6月7日 GMT+08:00 下午5:35:12, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Mon, Jun 05, 2017 at 12:01:41AM +0800, Icenowy Zheng wrote:
>> Some SoC's DE2 has two mixers. Defaultly the mixer0 is connected to
>> tcon0 and mixer1 is connected to tcon1; however by setting a bit
>> the connection can be swapped.
>> 
>> As we now hardcode the default connection, ignore the bonus endpoint
>for
>> the mixer's output and the TCON's input, as they stands for the
>swapped
>> connection.
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>
>So, I'm still not quite sure what this patch exactly is supposed to be
>doing.
>
>You mention that the routing between the mixers and tcons can be
>changed, and that we need to ignore the TCON input...

Yes, from the TCON perspective, the connection from
mixer0 to TCON1 and from mixer 1 to TCON0 should be
ignored. And from the mixer perspective, the connections should
also be omitted when binding in sun4i_drv.c.

>
>> ---
>> Changes in v2:
>> - Change to use new endpoint reg definition.
>> 
>>  drivers/gpu/drm/sun4i/sun4i_drv.c  | 45 ++++++++++++++++++++++++++++
>>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 61
>++++++++++++++++++++++++++++++++------
>>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 ++
>>  3 files changed, 99 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c
>b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> index f19100c91c2b..775eee82d8a9 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> @@ -178,6 +178,13 @@ static bool sun4i_drv_node_is_frontend(struct
>device_node *node)
>>  		of_device_is_compatible(node,
>"allwinner,sun8i-a33-display-frontend");
>>  }
>>  
>> +static bool sun4i_drv_node_is_swappable_de2_mixer(struct device_node
>*node)
>> +{
>> +	/* The V3s has only one mixer-tcon pair, so it's not listed here.
>*/
>> +	return of_device_is_compatible(node,
>"allwinner,sun8i-h3-de2-mixer0") ||
>> +		of_device_is_compatible(node, "allwinner,sun8i-h3-de2-mixer1");
>> +}
>> +
>>  static bool sun4i_drv_node_is_tcon(struct device_node *node)
>>  {
>>  	return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon") ||
>> @@ -261,6 +268,44 @@ static int sun4i_drv_add_endpoints(struct device
>*dev,
>>  			}
>>  		}
>>  
>> +		/*
>> +		 * The second endpoint of the output of a swappable DE2 mixer
>> +		 * is the TCON after connection swapping.
>> +		 * Ignore it now, as we now hardcode mixer0->tcon0,
>> +		 * mixer1->tcon1 connection.
>> +		 */
>> +		if (sun4i_drv_node_is_swappable_de2_mixer(node)) {
>> +			struct device_node *remote_ep_node;
>> +			struct of_endpoint local_endpoint, remote_endpoint;
>> +
>> +			remote_ep_node = of_graph_get_remote_endpoint(ep);
>> +			if (!remote_ep_node) {
>> +				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
>> +				continue;
>> +			}
>> +
>> +			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
>> +				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			if (of_graph_parse_endpoint(remote_ep_node,
>> +						    &remote_endpoint)) {
>> +				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			if (local_endpoint.id != remote_endpoint.id) {
>> +				DRM_DEBUG_DRIVER("Endpoint is an unused connection for DE2
>mixer... skipping\n");
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			of_node_put(remote_ep_node);
>> +		}
>> +
>>  		/* Walk down our tree */
>>  		count += sun4i_drv_add_endpoints(dev, match, remote);
>
>... yet this is not parsing the input at all, but only the output
>nodes.

Yes.

Mixers will have two output endpoints, but we will only use
the same id one and ignore the swapped one.

So does the situation of TCON input.

>
>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index d9791292553e..568cea0e5f8f 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -464,7 +464,8 @@ static int sun4i_tcon_init_regmap(struct device
>*dev,
>>   * requested via the get_id function of the engine.
>>   */
>>  static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv
>*drv,
>> -						   struct device_node *node)
>> +						   struct device_node *node,
>> +						   bool skip_bonus_ep)
>>  {
>>  	struct device_node *port, *ep, *remote;
>>  	struct sunxi_engine *engine;
>> @@ -478,6 +479,42 @@ static struct sunxi_engine
>*sun4i_tcon_find_engine(struct sun4i_drv *drv,
>>  		if (!remote)
>>  			continue;
>>  
>> +		if (skip_bonus_ep) {
>> +			struct device_node *remote_ep_node;
>> +			struct of_endpoint local_endpoint, remote_endpoint;
>> +
>> +			remote_ep_node = of_graph_get_remote_endpoint(ep);
>> +			if (!remote_ep_node) {
>> +				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
>> +				of_node_put(remote);
>> +				continue;
>> +			}
>> +
>> +			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
>> +				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
>> +				of_node_put(remote);
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			if (of_graph_parse_endpoint(remote_ep_node,
>> +						    &remote_endpoint)) {
>> +				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
>> +				of_node_put(remote);
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			if (local_endpoint.id != remote_endpoint.id) {
>> +				DRM_DEBUG_DRIVER("Skipping bonus mixer->TCON connection when
>searching engine\n");
>> +				of_node_put(remote);
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			of_node_put(remote_ep_node);
>> +		}
>> +
>
>I have no idea what this is supposed to be doing either.
>
>I might be wrong, but I really feel like there's a big mismatch
>between your commit log, and what you actually implement.
>
>In your commit log, you should state:
>
>A) What is the current behaviour
>B) Why that is a problem
>C) How do you address it
>
>And you don't.
>
>However, after discussing it with Chen-Yu, it seems like you're trying
>to have all the mixers probed before the TCONs. If that is so, there's
>nothing specific to the H3 here, and we also have the same issue on
>dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
>the easiest solution would be to move from a DFS algorithm to walk
>down the graph to a BFS one.
>
>That way, we would add all mixers first, then the TCONs, then the
>encoders, and the component framework will probe them in order.

No. I said that they're swappable, however, I don't
want to implement the swap now, but hardcode 0-0 1-1
connection. However, as you and Chen-Yu said, device tree
should reflect the real hardware, there will be bonus endpoints
for the swapped connection.

What I want to do is to ignore the bonus connection, in order to
prevent them from confusing the code.

If you just change the bind sequence, I think it cannot be
prevented that wrong connections will be bound.

>
>Maxime

Re: [PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[Icenowy Zheng]

于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote:
>> >You should also expand function sun4i_drv_node_is_tcon() at
>sun4i_drv.c
>> >with 
>> >new entries, but I'm not sure if this fits in this patch.
>> 
>> Instead I think it should be renamed to something like
>> "sun4i_drv_node_is_tcon_with_ch0".
>
>I'm not sure, or at least, it shouldn't make any difference, since
>TCON without a channel 0 will not have an endpoint 0, so this will be
>dealt with already.

But that will prevent new coders from add CH1-less TCON
compatibles to this function.

>
>Maxime

Re: [PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[Icenowy Zheng]

于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote:
>> >You should also expand function sun4i_drv_node_is_tcon() at
>sun4i_drv.c
>> >with 
>> >new entries, but I'm not sure if this fits in this patch.
>> 
>> Instead I think it should be renamed to something like
>> "sun4i_drv_node_is_tcon_with_ch0".
>
>I'm not sure, or at least, it shouldn't make any difference, since
>TCON without a channel 0 will not have an endpoint 0, so this will be
>dealt with already.

But that will prevent new coders from add CH1-less TCON
compatibles to this function.

>
>Maxime

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Icenowy Zheng]

于 2017年6月7日 GMT+08:00 下午5:35:12, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Mon, Jun 05, 2017 at 12:01:41AM +0800, Icenowy Zheng wrote:
>> Some SoC's DE2 has two mixers. Defaultly the mixer0 is connected to
>> tcon0 and mixer1 is connected to tcon1; however by setting a bit
>> the connection can be swapped.
>> 
>> As we now hardcode the default connection, ignore the bonus endpoint
>for
>> the mixer's output and the TCON's input, as they stands for the
>swapped
>> connection.
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>
>So, I'm still not quite sure what this patch exactly is supposed to be
>doing.
>
>You mention that the routing between the mixers and tcons can be
>changed, and that we need to ignore the TCON input...

Yes, from the TCON perspective, the connection from
mixer0 to TCON1 and from mixer 1 to TCON0 should be
ignored. And from the mixer perspective, the connections should
also be omitted when binding in sun4i_drv.c.

>
>> ---
>> Changes in v2:
>> - Change to use new endpoint reg definition.
>> 
>>  drivers/gpu/drm/sun4i/sun4i_drv.c  | 45 ++++++++++++++++++++++++++++
>>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 61
>++++++++++++++++++++++++++++++++------
>>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 ++
>>  3 files changed, 99 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c
>b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> index f19100c91c2b..775eee82d8a9 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> @@ -178,6 +178,13 @@ static bool sun4i_drv_node_is_frontend(struct
>device_node *node)
>>  		of_device_is_compatible(node,
>"allwinner,sun8i-a33-display-frontend");
>>  }
>>  
>> +static bool sun4i_drv_node_is_swappable_de2_mixer(struct device_node
>*node)
>> +{
>> +	/* The V3s has only one mixer-tcon pair, so it's not listed here.
>*/
>> +	return of_device_is_compatible(node,
>"allwinner,sun8i-h3-de2-mixer0") ||
>> +		of_device_is_compatible(node, "allwinner,sun8i-h3-de2-mixer1");
>> +}
>> +
>>  static bool sun4i_drv_node_is_tcon(struct device_node *node)
>>  {
>>  	return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon") ||
>> @@ -261,6 +268,44 @@ static int sun4i_drv_add_endpoints(struct device
>*dev,
>>  			}
>>  		}
>>  
>> +		/*
>> +		 * The second endpoint of the output of a swappable DE2 mixer
>> +		 * is the TCON after connection swapping.
>> +		 * Ignore it now, as we now hardcode mixer0->tcon0,
>> +		 * mixer1->tcon1 connection.
>> +		 */
>> +		if (sun4i_drv_node_is_swappable_de2_mixer(node)) {
>> +			struct device_node *remote_ep_node;
>> +			struct of_endpoint local_endpoint, remote_endpoint;
>> +
>> +			remote_ep_node = of_graph_get_remote_endpoint(ep);
>> +			if (!remote_ep_node) {
>> +				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
>> +				continue;
>> +			}
>> +
>> +			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
>> +				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			if (of_graph_parse_endpoint(remote_ep_node,
>> +						    &remote_endpoint)) {
>> +				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			if (local_endpoint.id != remote_endpoint.id) {
>> +				DRM_DEBUG_DRIVER("Endpoint is an unused connection for DE2
>mixer... skipping\n");
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			of_node_put(remote_ep_node);
>> +		}
>> +
>>  		/* Walk down our tree */
>>  		count += sun4i_drv_add_endpoints(dev, match, remote);
>
>... yet this is not parsing the input at all, but only the output
>nodes.

Yes.

Mixers will have two output endpoints, but we will only use
the same id one and ignore the swapped one.

So does the situation of TCON input.

>
>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index d9791292553e..568cea0e5f8f 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -464,7 +464,8 @@ static int sun4i_tcon_init_regmap(struct device
>*dev,
>>   * requested via the get_id function of the engine.
>>   */
>>  static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv
>*drv,
>> -						   struct device_node *node)
>> +						   struct device_node *node,
>> +						   bool skip_bonus_ep)
>>  {
>>  	struct device_node *port, *ep, *remote;
>>  	struct sunxi_engine *engine;
>> @@ -478,6 +479,42 @@ static struct sunxi_engine
>*sun4i_tcon_find_engine(struct sun4i_drv *drv,
>>  		if (!remote)
>>  			continue;
>>  
>> +		if (skip_bonus_ep) {
>> +			struct device_node *remote_ep_node;
>> +			struct of_endpoint local_endpoint, remote_endpoint;
>> +
>> +			remote_ep_node = of_graph_get_remote_endpoint(ep);
>> +			if (!remote_ep_node) {
>> +				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
>> +				of_node_put(remote);
>> +				continue;
>> +			}
>> +
>> +			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
>> +				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
>> +				of_node_put(remote);
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			if (of_graph_parse_endpoint(remote_ep_node,
>> +						    &remote_endpoint)) {
>> +				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
>> +				of_node_put(remote);
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			if (local_endpoint.id != remote_endpoint.id) {
>> +				DRM_DEBUG_DRIVER("Skipping bonus mixer->TCON connection when
>searching engine\n");
>> +				of_node_put(remote);
>> +				of_node_put(remote_ep_node);
>> +				continue;
>> +			}
>> +
>> +			of_node_put(remote_ep_node);
>> +		}
>> +
>
>I have no idea what this is supposed to be doing either.
>
>I might be wrong, but I really feel like there's a big mismatch
>between your commit log, and what you actually implement.
>
>In your commit log, you should state:
>
>A) What is the current behaviour
>B) Why that is a problem
>C) How do you address it
>
>And you don't.
>
>However, after discussing it with Chen-Yu, it seems like you're trying
>to have all the mixers probed before the TCONs. If that is so, there's
>nothing specific to the H3 here, and we also have the same issue on
>dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
>the easiest solution would be to move from a DFS algorithm to walk
>down the graph to a BFS one.
>
>That way, we would add all mixers first, then the TCONs, then the
>encoders, and the component framework will probe them in order.

No. I said that they're swappable, however, I don't
want to implement the swap now, but hardcode 0-0 1-1
connection. However, as you and Chen-Yu said, device tree
should reflect the real hardware, there will be bonus endpoints
for the swapped connection.

What I want to do is to ignore the bonus connection, in order to
prevent them from confusing the code.

If you just change the bind sequence, I think it cannot be
prevented that wrong connections will be bound.

>
>Maxime

Re: [PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On Wed, Jun 07, 2017 at 05:44:56PM +0800, Icenowy Zheng wrote:
> 于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote:
> >> >You should also expand function sun4i_drv_node_is_tcon() at
> >sun4i_drv.c
> >> >with 
> >> >new entries, but I'm not sure if this fits in this patch.
> >> 
> >> Instead I think it should be renamed to something like
> >> "sun4i_drv_node_is_tcon_with_ch0".
> >
> >I'm not sure, or at least, it shouldn't make any difference, since
> >TCON without a channel 0 will not have an endpoint 0, so this will be
> >dealt with already.
> 
> But that will prevent new coders from add CH1-less TCON
> compatibles to this function.

Why? We already have such TCONs (like the A33's, or V3S') in that
function.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[Icenowy Zheng]

于 2017年6月7日 GMT+08:00 下午10:19:57, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Wed, Jun 07, 2017 at 05:44:56PM +0800, Icenowy Zheng wrote:
>> 于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard
><maxime.ripard@free-electrons.com> 写到:
>> >On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote:
>> >> >You should also expand function sun4i_drv_node_is_tcon() at
>> >sun4i_drv.c
>> >> >with 
>> >> >new entries, but I'm not sure if this fits in this patch.
>> >> 
>> >> Instead I think it should be renamed to something like
>> >> "sun4i_drv_node_is_tcon_with_ch0".
>> >
>> >I'm not sure, or at least, it shouldn't make any difference, since
>> >TCON without a channel 0 will not have an endpoint 0, so this will
>be
>> >dealt with already.
>> 
>> But that will prevent new coders from add CH1-less TCON
>> compatibles to this function.
>
>Why? We already have such TCONs (like the A33's, or V3S') in that
>function.

Sorry, CH0-less.

>
>Maxime

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2413 bytes --]

On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
> >I have no idea what this is supposed to be doing either.
> >
> >I might be wrong, but I really feel like there's a big mismatch
> >between your commit log, and what you actually implement.
> >
> >In your commit log, you should state:
> >
> >A) What is the current behaviour
> >B) Why that is a problem
> >C) How do you address it
> >
> >And you don't.
> >
> >However, after discussing it with Chen-Yu, it seems like you're trying
> >to have all the mixers probed before the TCONs. If that is so, there's
> >nothing specific to the H3 here, and we also have the same issue on
> >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
> >the easiest solution would be to move from a DFS algorithm to walk
> >down the graph to a BFS one.
> >
> >That way, we would add all mixers first, then the TCONs, then the
> >encoders, and the component framework will probe them in order.
> 
> No. I said that they're swappable, however, I don't want to
> implement the swap now, but hardcode 0-0 1-1 connection.

We're on the same page, it's definitely not what I was mentionning
here. This would require a significant rework, and the usecase is
still unclear for now.

> However, as you and Chen-Yu said, device tree should reflect the
> real hardware, there will be bonus endpoints for the swapped
> connection.

If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
tcon 0, then yes, we're going to need it.

> What I want to do is to ignore the bonus connection, in order to
> prevent them from confusing the code.
> 
> If you just change the bind sequence, I think it cannot be
> prevented that wrong connections will be bound.

This is where I don't follow you anymore. The component framework
doesn't list connections but devices. The swapped connections do not
matter here, we have the same set of devices: mixer0, mixer1, tcon0
and tcon1.

The thing that does change with your patch is that before, the binding
sequence would have been mixer0, tcon0, tcon1, mixer1. With your
patch, it's mixer0, tcon0, mixer1, tcon1.

So, again, stating what issue you were seeing before making this patch
would be very helpful to see what you're trying to do / fix.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Jernej Škrabec]

Hi!

Dne sreda, 07. junij 2017 ob 16:38:27 CEST je Maxime Ripard napisal(a):
> On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
> > >I have no idea what this is supposed to be doing either.
> > >
> > >I might be wrong, but I really feel like there's a big mismatch
> > >between your commit log, and what you actually implement.
> > >
> > >In your commit log, you should state:
> > >
> > >A) What is the current behaviour
> > >B) Why that is a problem
> > >C) How do you address it
> > >
> > >And you don't.
> > >
> > >However, after discussing it with Chen-Yu, it seems like you're trying
> > >to have all the mixers probed before the TCONs. If that is so, there's
> > >nothing specific to the H3 here, and we also have the same issue on
> > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
> > >the easiest solution would be to move from a DFS algorithm to walk
> > >down the graph to a BFS one.
> > >
> > >That way, we would add all mixers first, then the TCONs, then the
> > >encoders, and the component framework will probe them in order.
> > 
> > No. I said that they're swappable, however, I don't want to
> > implement the swap now, but hardcode 0-0 1-1 connection.
> 
> We're on the same page, it's definitely not what I was mentionning
> here. This would require a significant rework, and the usecase is
> still unclear for now.
> 
> > However, as you and Chen-Yu said, device tree should reflect the
> > real hardware, there will be bonus endpoints for the swapped
> > connection.
> 
> If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
> tcon 0, then yes, we're going to need it.
> 
> > What I want to do is to ignore the bonus connection, in order to
> > prevent them from confusing the code.
> > 
> > If you just change the bind sequence, I think it cannot be
> > prevented that wrong connections will be bound.
> 
> This is where I don't follow you anymore. The component framework
> doesn't list connections but devices. The swapped connections do not
> matter here, we have the same set of devices: mixer0, mixer1, tcon0
> and tcon1.
> 
> The thing that does change with your patch is that before, the binding
> sequence would have been mixer0, tcon0, tcon1, mixer1. With your
> patch, it's mixer0, tcon0, mixer1, tcon1.
> 
> So, again, stating what issue you were seeing before making this patch
> would be very helpful to see what you're trying to do / fix.

If I understand correctly, she wants to make sure that DT has mixer0 - tcon0 
and mixer1 - tcon1 relationship and discard mixed relationship. I also believe 
that this is just temporary measure until mixed relationship is supported by 
the driver.

Maybe we could just leave this out for now and define only one endpoint in DT 
for TVE instead of two? Vast majority of users will have 0-0 and 1-1 
relationship, since there is not much point connecting HDMI with less capable 
mixer than TVE.

Best regards,
Jernej

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[icenowy]

在 2017-06-07 22:38，Maxime Ripard 写道：
> On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
>> >I have no idea what this is supposed to be doing either.
>> >
>> >I might be wrong, but I really feel like there's a big mismatch
>> >between your commit log, and what you actually implement.
>> >
>> >In your commit log, you should state:
>> >
>> >A) What is the current behaviour
>> >B) Why that is a problem
>> >C) How do you address it
>> >
>> >And you don't.
>> >
>> >However, after discussing it with Chen-Yu, it seems like you're trying
>> >to have all the mixers probed before the TCONs. If that is so, there's
>> >nothing specific to the H3 here, and we also have the same issue on
>> >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
>> >the easiest solution would be to move from a DFS algorithm to walk
>> >down the graph to a BFS one.
>> >
>> >That way, we would add all mixers first, then the TCONs, then the
>> >encoders, and the component framework will probe them in order.
>> 
>> No. I said that they're swappable, however, I don't want to
>> implement the swap now, but hardcode 0-0 1-1 connection.
> 
> We're on the same page, it's definitely not what I was mentionning
> here. This would require a significant rework, and the usecase is
> still unclear for now.
> 
>> However, as you and Chen-Yu said, device tree should reflect the
>> real hardware, there will be bonus endpoints for the swapped
>> connection.
> 
> If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
> tcon 0, then yes, we're going to need it.
> 
>> What I want to do is to ignore the bonus connection, in order to
>> prevent them from confusing the code.
>> 
>> If you just change the bind sequence, I think it cannot be
>> prevented that wrong connections will be bound.
> 
> This is where I don't follow you anymore. The component framework
> doesn't list connections but devices. The swapped connections do not
> matter here, we have the same set of devices: mixer0, mixer1, tcon0
> and tcon1.
> 
> The thing that does change with your patch is that before, the binding
> sequence would have been mixer0, tcon0, tcon1, mixer1. With your
> patch, it's mixer0, tcon0, mixer1, tcon1.
> 
> So, again, stating what issue you were seeing before making this patch
> would be very helpful to see what you're trying to do / fix.

So maybe I can drop the forward search (searching output) code, and keep
only the backward search (search input) code in TCON?

Forward search code is only used when binding, but backward search is 
used
for TCON to find connected mixer.

> 
> Maxime
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3703 bytes --]

Hi Jernej,

On Wed, Jun 07, 2017 at 08:15:12PM +0200, Jernej Škrabec wrote:
> Hi!
> 
> Dne sreda, 07. junij 2017 ob 16:38:27 CEST je Maxime Ripard napisal(a):
> > On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
> > > >I have no idea what this is supposed to be doing either.
> > > >
> > > >I might be wrong, but I really feel like there's a big mismatch
> > > >between your commit log, and what you actually implement.
> > > >
> > > >In your commit log, you should state:
> > > >
> > > >A) What is the current behaviour
> > > >B) Why that is a problem
> > > >C) How do you address it
> > > >
> > > >And you don't.
> > > >
> > > >However, after discussing it with Chen-Yu, it seems like you're trying
> > > >to have all the mixers probed before the TCONs. If that is so, there's
> > > >nothing specific to the H3 here, and we also have the same issue on
> > > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
> > > >the easiest solution would be to move from a DFS algorithm to walk
> > > >down the graph to a BFS one.
> > > >
> > > >That way, we would add all mixers first, then the TCONs, then the
> > > >encoders, and the component framework will probe them in order.
> > > 
> > > No. I said that they're swappable, however, I don't want to
> > > implement the swap now, but hardcode 0-0 1-1 connection.
> > 
> > We're on the same page, it's definitely not what I was mentionning
> > here. This would require a significant rework, and the usecase is
> > still unclear for now.
> > 
> > > However, as you and Chen-Yu said, device tree should reflect the
> > > real hardware, there will be bonus endpoints for the swapped
> > > connection.
> > 
> > If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
> > tcon 0, then yes, we're going to need it.
> > 
> > > What I want to do is to ignore the bonus connection, in order to
> > > prevent them from confusing the code.
> > > 
> > > If you just change the bind sequence, I think it cannot be
> > > prevented that wrong connections will be bound.
> > 
> > This is where I don't follow you anymore. The component framework
> > doesn't list connections but devices. The swapped connections do not
> > matter here, we have the same set of devices: mixer0, mixer1, tcon0
> > and tcon1.
> > 
> > The thing that does change with your patch is that before, the binding
> > sequence would have been mixer0, tcon0, tcon1, mixer1. With your
> > patch, it's mixer0, tcon0, mixer1, tcon1.
> > 
> > So, again, stating what issue you were seeing before making this patch
> > would be very helpful to see what you're trying to do / fix.
> 
> If I understand correctly, she wants to make sure that DT has mixer0 - tcon0 
> and mixer1 - tcon1 relationship and discard mixed relationship. I also believe 
> that this is just temporary measure until mixed relationship is supported by 
> the driver.

yeah, but all the component stuff doesn't care about the relationships
themselves, it just cares about the set of devices, and we use those
relationships to build that set. In this case, even with the swapped
connections, the set of devices remains the same, which is what
puzzles me.

> Maybe we could just leave this out for now and define only one endpoint in DT 
> for TVE instead of two? Vast majority of users will have 0-0 and 1-1 
> relationship, since there is not much point connecting HDMI with less capable 
> mixer than TVE.

Yeah, but unfortunately, we have to have a perfect representation from
day one now...

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3262 bytes --]

On Thu, Jun 08, 2017 at 01:01:53PM +0800, icenowy@aosc.io wrote:
> 在 2017-06-07 22:38，Maxime Ripard 写道：
> > On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
> > > >I have no idea what this is supposed to be doing either.
> > > >
> > > >I might be wrong, but I really feel like there's a big mismatch
> > > >between your commit log, and what you actually implement.
> > > >
> > > >In your commit log, you should state:
> > > >
> > > >A) What is the current behaviour
> > > >B) Why that is a problem
> > > >C) How do you address it
> > > >
> > > >And you don't.
> > > >
> > > >However, after discussing it with Chen-Yu, it seems like you're trying
> > > >to have all the mixers probed before the TCONs. If that is so, there's
> > > >nothing specific to the H3 here, and we also have the same issue on
> > > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
> > > >the easiest solution would be to move from a DFS algorithm to walk
> > > >down the graph to a BFS one.
> > > >
> > > >That way, we would add all mixers first, then the TCONs, then the
> > > >encoders, and the component framework will probe them in order.
> > > 
> > > No. I said that they're swappable, however, I don't want to
> > > implement the swap now, but hardcode 0-0 1-1 connection.
> > 
> > We're on the same page, it's definitely not what I was mentionning
> > here. This would require a significant rework, and the usecase is
> > still unclear for now.
> > 
> > > However, as you and Chen-Yu said, device tree should reflect the
> > > real hardware, there will be bonus endpoints for the swapped
> > > connection.
> > 
> > If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
> > tcon 0, then yes, we're going to need it.
> > 
> > > What I want to do is to ignore the bonus connection, in order to
> > > prevent them from confusing the code.
> > > 
> > > If you just change the bind sequence, I think it cannot be
> > > prevented that wrong connections will be bound.
> > 
> > This is where I don't follow you anymore. The component framework
> > doesn't list connections but devices. The swapped connections do not
> > matter here, we have the same set of devices: mixer0, mixer1, tcon0
> > and tcon1.
> > 
> > The thing that does change with your patch is that before, the binding
> > sequence would have been mixer0, tcon0, tcon1, mixer1. With your
> > patch, it's mixer0, tcon0, mixer1, tcon1.
> > 
> > So, again, stating what issue you were seeing before making this patch
> > would be very helpful to see what you're trying to do / fix.
> 
> So maybe I can drop the forward search (searching output) code, and keep
> only the backward search (search input) code in TCON?
> 
> Forward search code is only used when binding, but backward search is used
> for TCON to find connected mixer.

It is hard to talk about a solution, when it's not clear what the
issue is.

So please state 
> > > >A) What is the current behaviour
> > > >B) Why that is a problem
> > > >C) How do you address it

We'll talk about a solution once this is done.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 04/11] drm: sun4i: add support for H3's TCON0/1

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

On Wed, Jun 07, 2017 at 10:21:02PM +0800, Icenowy Zheng wrote:
> 
> 
> 于 2017年6月7日 GMT+08:00 下午10:19:57, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >On Wed, Jun 07, 2017 at 05:44:56PM +0800, Icenowy Zheng wrote:
> >> 于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard
> ><maxime.ripard@free-electrons.com> 写到:
> >> >On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote:
> >> >> >You should also expand function sun4i_drv_node_is_tcon() at
> >> >sun4i_drv.c
> >> >> >with 
> >> >> >new entries, but I'm not sure if this fits in this patch.
> >> >> 
> >> >> Instead I think it should be renamed to something like
> >> >> "sun4i_drv_node_is_tcon_with_ch0".
> >> >
> >> >I'm not sure, or at least, it shouldn't make any difference, since
> >> >TCON without a channel 0 will not have an endpoint 0, so this will
> >be
> >> >dealt with already.
> >> 
> >> But that will prevent new coders from add CH1-less TCON
> >> compatibles to this function.
> >
> >Why? We already have such TCONs (like the A33's, or V3S') in that
> >function.
> 
> Sorry, CH0-less.

That's not really an issue I think, since the endpoint 0 will not be
there on those TCONs, the code will bail out.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

On Wed, Jun 07, 2017 at 04:48:50PM +0800, Icenowy Zheng wrote:
> >> @@ -189,6 +211,8 @@ supported.
> >>  Required properties:
> >>    - compatible: value must be one of:
> >>      * allwinner,sun8i-v3s-de2-mixer
> >> +    * allwinner,sun8i-h3-de2-mixer0
> >> +    * allwinner,sun8i-h3-de2-mixer1
> >
> >Again, please explain why we need to have different compatibles
> >here. If it's only about the number of planes, this should be dealt
> >with a property, not a compatible.
> 
> Only mixer0 has "VEP" and write-back support, at least on H3.

What is that VEP? writeback support can also be expressed by a
property.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [linux-sunxi] Re: [PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[Icenowy Zheng]

于 2017年6月10日 GMT+08:00 上午12:49:15, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Wed, Jun 07, 2017 at 04:48:50PM +0800, Icenowy Zheng wrote:
>> >> @@ -189,6 +211,8 @@ supported.
>> >>  Required properties:
>> >>    - compatible: value must be one of:
>> >>      * allwinner,sun8i-v3s-de2-mixer
>> >> +    * allwinner,sun8i-h3-de2-mixer0
>> >> +    * allwinner,sun8i-h3-de2-mixer1
>> >
>> >Again, please explain why we need to have different compatibles
>> >here. If it's only about the number of planes, this should be dealt
>> >with a property, not a compatible.
>> 
>> Only mixer0 has "VEP" and write-back support, at least on H3.
>
>What is that VEP? writeback support can also be expressed by a
>property.

I don't know what VEP is...

But we are really facing different cores, like sun50i-a64-mmc
and sun50i-a64-emmc.

>
>Maxime

Re: [linux-sunxi] Re: [PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[Jernej Škrabec]

Hi!

Dne petek, 09. junij 2017 ob 18:51:02 CEST je Icenowy Zheng napisal(a):
> 于 2017年6月10日 GMT+08:00 上午12:49:15, Maxime Ripard <maxime.ripard@free-
electrons.com> 写到:
> >On Wed, Jun 07, 2017 at 04:48:50PM +0800, Icenowy Zheng wrote:
> >> >> @@ -189,6 +211,8 @@ supported.
> >> >> 
> >> >>  Required properties:
> >> >>    - compatible: value must be one of:
> >> >>      * allwinner,sun8i-v3s-de2-mixer
> >> >> 
> >> >> +    * allwinner,sun8i-h3-de2-mixer0
> >> >> +    * allwinner,sun8i-h3-de2-mixer1
> >> >
> >> >Again, please explain why we need to have different compatibles
> >> >here. If it's only about the number of planes, this should be dealt
> >> >with a property, not a compatible.
> >> 
> >> Only mixer0 has "VEP" and write-back support, at least on H3.
> >
> >What is that VEP? writeback support can also be expressed by a
> >property.
> 
> I don't know what VEP is...

VEP is probably Video Enhancement Processor (?). Although I'm not sure if that 
is really mixer specific. Icenowy, can you explain where did you get this info? 

Best regards,
Jernej

> 
> But we are really facing different cores, like sun50i-a64-mmc
> and sun50i-a64-emmc.
> 
> >Maxime
> 
> --
> You received this message because you are subscribed to the Google Groups
> "linux-sunxi" group. To unsubscribe from this group and stop receiving
> emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[icenowy]

在 2017-06-09 22:46，Maxime Ripard 写道：
> On Thu, Jun 08, 2017 at 01:01:53PM +0800, icenowy@aosc.io wrote:
>> 在 2017-06-07 22:38，Maxime Ripard 写道：
>> > On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
>> > > >I have no idea what this is supposed to be doing either.
>> > > >
>> > > >I might be wrong, but I really feel like there's a big mismatch
>> > > >between your commit log, and what you actually implement.
>> > > >
>> > > >In your commit log, you should state:
>> > > >
>> > > >A) What is the current behaviour
>> > > >B) Why that is a problem
>> > > >C) How do you address it
>> > > >
>> > > >And you don't.
>> > > >
>> > > >However, after discussing it with Chen-Yu, it seems like you're trying
>> > > >to have all the mixers probed before the TCONs. If that is so, there's
>> > > >nothing specific to the H3 here, and we also have the same issue on
>> > > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
>> > > >the easiest solution would be to move from a DFS algorithm to walk
>> > > >down the graph to a BFS one.
>> > > >
>> > > >That way, we would add all mixers first, then the TCONs, then the
>> > > >encoders, and the component framework will probe them in order.
>> > >
>> > > No. I said that they're swappable, however, I don't want to
>> > > implement the swap now, but hardcode 0-0 1-1 connection.
>> >
>> > We're on the same page, it's definitely not what I was mentionning
>> > here. This would require a significant rework, and the usecase is
>> > still unclear for now.
>> >
>> > > However, as you and Chen-Yu said, device tree should reflect the
>> > > real hardware, there will be bonus endpoints for the swapped
>> > > connection.
>> >
>> > If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
>> > tcon 0, then yes, we're going to need it.
>> >
>> > > What I want to do is to ignore the bonus connection, in order to
>> > > prevent them from confusing the code.
>> > >
>> > > If you just change the bind sequence, I think it cannot be
>> > > prevented that wrong connections will be bound.
>> >
>> > This is where I don't follow you anymore. The component framework
>> > doesn't list connections but devices. The swapped connections do not
>> > matter here, we have the same set of devices: mixer0, mixer1, tcon0
>> > and tcon1.
>> >
>> > The thing that does change with your patch is that before, the binding
>> > sequence would have been mixer0, tcon0, tcon1, mixer1. With your
>> > patch, it's mixer0, tcon0, mixer1, tcon1.
>> >
>> > So, again, stating what issue you were seeing before making this patch
>> > would be very helpful to see what you're trying to do / fix.
>> 
>> So maybe I can drop the forward search (searching output) code, and 
>> keep
>> only the backward search (search input) code in TCON?
>> 
>> Forward search code is only used when binding, but backward search is 
>> used
>> for TCON to find connected mixer.
> 
> It is hard to talk about a solution, when it's not clear what the
> issue is.
> 
> So please state
>> > > >A) What is the current behaviour
>> > > >B) Why that is a problem
>> > > >C) How do you address it
> 
> We'll talk about a solution once this is done.

(All those things are based on the assumption that mixer0, mixer1, tcon0
and tcon1 are all enabled in DT. If one group of mixer-tcon pair is 
fully
disabled in DT it will behave properly.)

For the backward search:

A) The current behaviour is to take the first engine found, which will 
be
wrong in the situation of tcon1 if mixer0 and mixer1 are both enabled:
mixer0 is taken for tcon1 instead of mixer1.

B) It takes mixer0 as it matches the first endpoint of tcon0's input.

C) It's a logic failure in the backward search, as it only considered
the DE1 situation, in which TCONs will only have one engine as input.

For the bind process:

A) The current behaviour is to try to bind all output endpoints of the
engine, during binding all outputs of mixer0, these will happen:
   1. tcon1 is bound with mixer0 as its engine if backward searching
   is not fixed.
   2. tcon1 fails to be bound as its engine is not yet bound when
   backward searching works properly, then sun4i_drv will refuse
   to continue as a component is not properly bound.
B) The binding process in sun4i_drv will bind a component that is not
really an working output of the forward component, but only exists in
the endpoint list as a theortically possible output (in fact not an
real output).
C) I tested with this patch's sun4i_drv_node_is_swappable_de2_mixer
function masked (always return false), and then the multiple
mixer+tcon situations don't work properly.

P.S. I think the BFS solution is really a dirty hack -- although we
bind components, not connections, we should decide the next component
to bind according to the connections -- not really connected
components shouldn't be bound.

For example, if we enabled mixer0, tcon0 and tcon1, tcon1 shouldn't
be bound at all. However in BFS situation tcon1 will also be bound
and then fail to be bound if the backward engine searching is fixed.

> 
> Maxime

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[icenowy]

在 2017-06-10 22:57，icenowy@aosc.io 写道：
> 在 2017-06-09 22:46，Maxime Ripard 写道：
>> On Thu, Jun 08, 2017 at 01:01:53PM +0800, icenowy@aosc.io wrote:
>>> 在 2017-06-07 22:38，Maxime Ripard 写道：
>>> > On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
>>> > > >I have no idea what this is supposed to be doing either.
>>> > > >
>>> > > >I might be wrong, but I really feel like there's a big mismatch
>>> > > >between your commit log, and what you actually implement.
>>> > > >
>>> > > >In your commit log, you should state:
>>> > > >
>>> > > >A) What is the current behaviour
>>> > > >B) Why that is a problem
>>> > > >C) How do you address it
>>> > > >
>>> > > >And you don't.
>>> > > >
>>> > > >However, after discussing it with Chen-Yu, it seems like you're trying
>>> > > >to have all the mixers probed before the TCONs. If that is so, there's
>>> > > >nothing specific to the H3 here, and we also have the same issue on
>>> > > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
>>> > > >the easiest solution would be to move from a DFS algorithm to walk
>>> > > >down the graph to a BFS one.
>>> > > >
>>> > > >That way, we would add all mixers first, then the TCONs, then the
>>> > > >encoders, and the component framework will probe them in order.
>>> > >
>>> > > No. I said that they're swappable, however, I don't want to
>>> > > implement the swap now, but hardcode 0-0 1-1 connection.
>>> >
>>> > We're on the same page, it's definitely not what I was mentionning
>>> > here. This would require a significant rework, and the usecase is
>>> > still unclear for now.
>>> >
>>> > > However, as you and Chen-Yu said, device tree should reflect the
>>> > > real hardware, there will be bonus endpoints for the swapped
>>> > > connection.
>>> >
>>> > If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
>>> > tcon 0, then yes, we're going to need it.
>>> >
>>> > > What I want to do is to ignore the bonus connection, in order to
>>> > > prevent them from confusing the code.
>>> > >
>>> > > If you just change the bind sequence, I think it cannot be
>>> > > prevented that wrong connections will be bound.
>>> >
>>> > This is where I don't follow you anymore. The component framework
>>> > doesn't list connections but devices. The swapped connections do not
>>> > matter here, we have the same set of devices: mixer0, mixer1, tcon0
>>> > and tcon1.
>>> >
>>> > The thing that does change with your patch is that before, the binding
>>> > sequence would have been mixer0, tcon0, tcon1, mixer1. With your
>>> > patch, it's mixer0, tcon0, mixer1, tcon1.
>>> >
>>> > So, again, stating what issue you were seeing before making this patch
>>> > would be very helpful to see what you're trying to do / fix.
>>> 
>>> So maybe I can drop the forward search (searching output) code, and 
>>> keep
>>> only the backward search (search input) code in TCON?
>>> 
>>> Forward search code is only used when binding, but backward search is 
>>> used
>>> for TCON to find connected mixer.
>> 
>> It is hard to talk about a solution, when it's not clear what the
>> issue is.
>> 
>> So please state
>>> > > >A) What is the current behaviour
>>> > > >B) Why that is a problem
>>> > > >C) How do you address it
>> 
>> We'll talk about a solution once this is done.
> 
> (All those things are based on the assumption that mixer0, mixer1, 
> tcon0
> and tcon1 are all enabled in DT. If one group of mixer-tcon pair is 
> fully
> disabled in DT it will behave properly.)

So there's a temporary workaround -- only enable one pipeline and 
disable
the unused mixer and tcon totally.

It's shown to work with this commit reverted in my local TVE branch. 
(The
swappable_input value is also deleted from H3 TCON's quirks)

> 
> For the backward search:
> 
> A) The current behaviour is to take the first engine found, which will 
> be
> wrong in the situation of tcon1 if mixer0 and mixer1 are both enabled:
> mixer0 is taken for tcon1 instead of mixer1.
> 
> B) It takes mixer0 as it matches the first endpoint of tcon0's input.
> 
> C) It's a logic failure in the backward search, as it only considered
> the DE1 situation, in which TCONs will only have one engine as input.
> 
> For the bind process:
> 
> A) The current behaviour is to try to bind all output endpoints of the
> engine, during binding all outputs of mixer0, these will happen:
>   1. tcon1 is bound with mixer0 as its engine if backward searching
>   is not fixed.
>   2. tcon1 fails to be bound as its engine is not yet bound when
>   backward searching works properly, then sun4i_drv will refuse
>   to continue as a component is not properly bound.
> B) The binding process in sun4i_drv will bind a component that is not
> really an working output of the forward component, but only exists in
> the endpoint list as a theortically possible output (in fact not an
> real output).
> C) I tested with this patch's sun4i_drv_node_is_swappable_de2_mixer
> function masked (always return false), and then the multiple
> mixer+tcon situations don't work properly.
> 
> P.S. I think the BFS solution is really a dirty hack -- although we
> bind components, not connections, we should decide the next component
> to bind according to the connections -- not really connected
> components shouldn't be bound.
> 
> For example, if we enabled mixer0, tcon0 and tcon1, tcon1 shouldn't
> be bound at all. However in BFS situation tcon1 will also be bound
> and then fail to be bound if the backward engine searching is fixed.
> 
>> 
>> Maxime
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [linux-sunxi] Re: [PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[icenowy]

在 2017-06-10 05:24，Jernej Škrabec 写道：
> Hi!
> 
> Dne petek, 09. junij 2017 ob 18:51:02 CEST je Icenowy Zheng napisal(a):
>> 于 2017年6月10日 GMT+08:00 上午12:49:15, Maxime Ripard <maxime.ripard@free-
> electrons.com> 写到:
>> >On Wed, Jun 07, 2017 at 04:48:50PM +0800, Icenowy Zheng wrote:
>> >> >> @@ -189,6 +211,8 @@ supported.
>> >> >>
>> >> >>  Required properties:
>> >> >>    - compatible: value must be one of:
>> >> >>      * allwinner,sun8i-v3s-de2-mixer
>> >> >>
>> >> >> +    * allwinner,sun8i-h3-de2-mixer0
>> >> >> +    * allwinner,sun8i-h3-de2-mixer1
>> >> >
>> >> >Again, please explain why we need to have different compatibles
>> >> >here. If it's only about the number of planes, this should be dealt
>> >> >with a property, not a compatible.
>> >>
>> >> Only mixer0 has "VEP" and write-back support, at least on H3.
>> >
>> >What is that VEP? writeback support can also be expressed by a
>> >property.
>> 
>> I don't know what VEP is...
> 
> VEP is probably Video Enhancement Processor (?). Although I'm not sure 
> if that
> is really mixer specific. Icenowy, can you explain where did you get 
> this info?

In 
lichee/linux-3.4/drivers/video/sunxi/disp2/disp/de/lowlevel_sun8iw7/de_feat.c:

static const int de_is_support_vep[] = {
	/* DISP0 CH0 */
	1,
	/* DISP0 CH1 */
	0,
	/* DISP0 CH2 */
	0,
	/* DISP0 CH3 */
	0,
	/* DISP1 CH0 */
	0,
	/* DISP1 CH1 */
	0,
};

> 
> Best regards,
> Jernej
> 
>> 
>> But we are really facing different cores, like sun50i-a64-mmc
>> and sun50i-a64-emmc.
>> 
>> >Maxime
>> 
>> --
>> You received this message because you are subscribed to the Google 
>> Groups
>> "linux-sunxi" group. To unsubscribe from this group and stop receiving
>> emails from it, send an email to 
>> linux-sunxi+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 07/11] drm: sun4i: add support for the TV encoder in H3 SoC

[icenowy]

在 2017-06-07 17:38，Maxime Ripard 写道：
> On Mon, Jun 05, 2017 at 12:01:45AM +0800, Icenowy Zheng wrote:
>> Allwinner H3 features a TV encoder similar to the one in earlier SoCs,
>> but has a internal fixed clock divider that divides the TCON1 clock
>> (called TVE clock in datasheet) by 11.
>> 
>> Add support for it.
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>> Changes in v2:
>> - Quirk part rewritten.
>> 
>>  drivers/gpu/drm/sun4i/sun4i_tv.c | 35 
>> ++++++++++++++++++++++++++++++++++-
>>  1 file changed, 34 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c 
>> b/drivers/gpu/drm/sun4i/sun4i_tv.c
>> index 338b9e5bb2a3..b9ff6d5ea67a 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tv.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/clk.h>
>>  #include <linux/component.h>
>>  #include <linux/of_address.h>
>> +#include <linux/of_device.h>
>>  #include <linux/regmap.h>
>>  #include <linux/reset.h>
>> 
>> @@ -169,14 +170,21 @@ struct tv_mode {
>>  	const struct resync_parameters	*resync_params;
>>  };
>> 
>> +struct sun4i_tv_quirks {
>> +	int fixed_divider;
>> +};
>> +
>>  struct sun4i_tv {
>>  	struct drm_connector	connector;
>>  	struct drm_encoder	encoder;
>> 
>>  	struct clk		*clk;
>> +	struct clk		*mod_clk;
>>  	struct regmap		*regs;
>>  	struct reset_control	*reset;
>> 
>> +	const struct sun4i_tv_quirks *quirks;
>> +
>>  	struct sun4i_drv	*drv;
>>  };
>> 
>> @@ -391,6 +399,12 @@ static void sun4i_tv_mode_set(struct drm_encoder 
>> *encoder,
>>  	struct sun4i_tcon *tcon = crtc->tcon;
>>  	const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode);
>> 
>> +	if (tv->quirks->fixed_divider) {
>> +		DRM_DEBUG_DRIVER("Applying fixed divider %d on TVE clock\n",
>> +				 tv->quirks->fixed_divider);
>> +		mode->crtc_clock *= tv->quirks->fixed_divider;
>> +	}
>> +
> 
> You're not allowed to change the mode in mode_set, and you shouldn't
> even change it. The output pixel clock is still 27MHz.
> 
> You should implement that using the states, as we discussed already.

After reading the comments at include/drm/drm_modeset_helper_vtables.h,
I think the atomic_check function is allowed to directly change
the adjust_mode of crtc_state.

And according to other comments at include/drm/drm_modes.h, the
crtc_clock in adjust_mode should be the clock to be really feed
to the hardware.

So I think I can just change this in atomic_check.

However, the mode_set function of sun4i_tv seems to be not
regarding adjusted_mode and still use original mode.

So my current design is:
- Multiply adjusted_mode in atomic_check.
- Feed adjust_mode to TCON code in mode_set function.

Is this proper?

(From my own understanding to the comments I think so.)

> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [linux-sunxi] Re: [PATCH v2 10/11] ARM: sun8i: h3: add display engine pipeline for TVE

[icenowy]

在 2017-06-07 17:42，Maxime Ripard 写道：
> On Mon, Jun 05, 2017 at 12:01:48AM +0800, Icenowy Zheng wrote:
>> +	soc {
>> +		display_clocks: clock@1000000 {
>> +			compatible = "allwinner,sun8i-a83t-de2-clk";
>> +			reg = <0x01000000 0x100000>;
>> +			clocks = <&ccu CLK_BUS_DE>,
>> +				 <&ccu CLK_DE>;
>> +			clock-names = "bus",
>> +				      "mod";
>> +			resets = <&ccu RST_BUS_DE>;
>> +			#clock-cells = <1>;
>> +			#reset-cells = <1>;
>> +			assigned-clocks = <&ccu CLK_DE>;
>> +			assigned-clock-parents = <&ccu CLK_PLL_DE>;
>> +			assigned-clock-rates = <432000000>;
>> +		};
> 
> We discussed that already a few times, but there's no reason to do
> so. If you need a downstream clock at a particular rate, call
> clk_set_rate on it, period.
> 
> Whether its parent will be coming from PLL_DE or some other more
> appriopriate clock is not relevant and doesn't make any difference.

The clock framework is not so smart to deal with these infomations:
- CLK_PLL_PERIPH should always be 600MHz
- CLK_TVE should always be 216MHz
- CLK_DE (in fact CLK_MIXER{0,1}) should be larger than 300MHz (for 4K)

So we have to specify CLK_DE to be 432MHz, and then it will set
CLK_PLL_DE to this value, then the CLK_TVE can be set to 216MHz with
divider 2.

For DE there's no a real hardware block clock requirement, set it to
> =300MHz is for 4K output support.

> 
> Maxime
> 
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [linux-sunxi] Re: [PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]

On Sat, Jun 10, 2017 at 12:51:02AM +0800, Icenowy Zheng wrote:
> 
> 
> 于 2017年6月10日 GMT+08:00 上午12:49:15, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >On Wed, Jun 07, 2017 at 04:48:50PM +0800, Icenowy Zheng wrote:
> >> >> @@ -189,6 +211,8 @@ supported.
> >> >>  Required properties:
> >> >>    - compatible: value must be one of:
> >> >>      * allwinner,sun8i-v3s-de2-mixer
> >> >> +    * allwinner,sun8i-h3-de2-mixer0
> >> >> +    * allwinner,sun8i-h3-de2-mixer1
> >> >
> >> >Again, please explain why we need to have different compatibles
> >> >here. If it's only about the number of planes, this should be dealt
> >> >with a property, not a compatible.
> >> 
> >> Only mixer0 has "VEP" and write-back support, at least on H3.
> >
> >What is that VEP? writeback support can also be expressed by a
> >property.
> 
> I don't know what VEP is...

Ok. Let's forget about it at the moment then, especially if it's
optional, we can add support for it later.

> But we are really facing different cores, like sun50i-a64-mmc
> and sun50i-a64-emmc.

Not really. The eMMC and MMC controllers are *very* different. They
behave (slightly) differently, the eMMC controller has more signals
(8-bit bus, data strobe), can run at a higher frequency. You cannot
have exactly the same driver (or rather code path) to handle
both. Therefore the need for some other compatible.

This is not the case for the mixer here. The thing that differs
between instances is not behaviour but data. And we provide most of
the data through DT.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 07/11] drm: sun4i: add support for the TV encoder in H3 SoC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3487 bytes --]

On Sun, Jun 11, 2017 at 02:43:42PM +0800, icenowy@aosc.io wrote:
> 在 2017-06-07 17:38，Maxime Ripard 写道：
> > On Mon, Jun 05, 2017 at 12:01:45AM +0800, Icenowy Zheng wrote:
> > > Allwinner H3 features a TV encoder similar to the one in earlier SoCs,
> > > but has a internal fixed clock divider that divides the TCON1 clock
> > > (called TVE clock in datasheet) by 11.
> > > 
> > > Add support for it.
> > > 
> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > > ---
> > > Changes in v2:
> > > - Quirk part rewritten.
> > > 
> > >  drivers/gpu/drm/sun4i/sun4i_tv.c | 35
> > > ++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c
> > > b/drivers/gpu/drm/sun4i/sun4i_tv.c
> > > index 338b9e5bb2a3..b9ff6d5ea67a 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_tv.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
> > > @@ -13,6 +13,7 @@
> > >  #include <linux/clk.h>
> > >  #include <linux/component.h>
> > >  #include <linux/of_address.h>
> > > +#include <linux/of_device.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/reset.h>
> > > 
> > > @@ -169,14 +170,21 @@ struct tv_mode {
> > >  	const struct resync_parameters	*resync_params;
> > >  };
> > > 
> > > +struct sun4i_tv_quirks {
> > > +	int fixed_divider;
> > > +};
> > > +
> > >  struct sun4i_tv {
> > >  	struct drm_connector	connector;
> > >  	struct drm_encoder	encoder;
> > > 
> > >  	struct clk		*clk;
> > > +	struct clk		*mod_clk;
> > >  	struct regmap		*regs;
> > >  	struct reset_control	*reset;
> > > 
> > > +	const struct sun4i_tv_quirks *quirks;
> > > +
> > >  	struct sun4i_drv	*drv;
> > >  };
> > > 
> > > @@ -391,6 +399,12 @@ static void sun4i_tv_mode_set(struct
> > > drm_encoder *encoder,
> > >  	struct sun4i_tcon *tcon = crtc->tcon;
> > >  	const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode);
> > > 
> > > +	if (tv->quirks->fixed_divider) {
> > > +		DRM_DEBUG_DRIVER("Applying fixed divider %d on TVE clock\n",
> > > +				 tv->quirks->fixed_divider);
> > > +		mode->crtc_clock *= tv->quirks->fixed_divider;
> > > +	}
> > > +
> > 
> > You're not allowed to change the mode in mode_set, and you shouldn't
> > even change it. The output pixel clock is still 27MHz.
> > 
> > You should implement that using the states, as we discussed already.
> 
> After reading the comments at include/drm/drm_modeset_helper_vtables.h,
> I think the atomic_check function is allowed to directly change
> the adjust_mode of crtc_state.
> 
> And according to other comments at include/drm/drm_modes.h, the
> crtc_clock in adjust_mode should be the clock to be really feed
> to the hardware.
> 
> So I think I can just change this in atomic_check.
> 
> However, the mode_set function of sun4i_tv seems to be not
> regarding adjusted_mode and still use original mode.
> 
> So my current design is:
> - Multiply adjusted_mode in atomic_check.
> - Feed adjust_mode to TCON code in mode_set function.
> 
> Is this proper?
> 
> (From my own understanding to the comments I think so.)

No, it's not. The pixel clock in the mode is the pixel clock output
physically by the connector. You want to use it for an intermediate
clock in your pipeline.

This is not the same clock, and not the same frequency.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [linux-sunxi] Re: [PATCH v2 10/11] ARM: sun8i: h3: add display engine pipeline for TVE

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

On Sun, Jun 11, 2017 at 02:58:47PM +0800, icenowy@aosc.io wrote:
> 在 2017-06-07 17:42，Maxime Ripard 写道：
> > On Mon, Jun 05, 2017 at 12:01:48AM +0800, Icenowy Zheng wrote:
> > > +	soc {
> > > +		display_clocks: clock@1000000 {
> > > +			compatible = "allwinner,sun8i-a83t-de2-clk";
> > > +			reg = <0x01000000 0x100000>;
> > > +			clocks = <&ccu CLK_BUS_DE>,
> > > +				 <&ccu CLK_DE>;
> > > +			clock-names = "bus",
> > > +				      "mod";
> > > +			resets = <&ccu RST_BUS_DE>;
> > > +			#clock-cells = <1>;
> > > +			#reset-cells = <1>;
> > > +			assigned-clocks = <&ccu CLK_DE>;
> > > +			assigned-clock-parents = <&ccu CLK_PLL_DE>;
> > > +			assigned-clock-rates = <432000000>;
> > > +		};
> > 
> > We discussed that already a few times, but there's no reason to do
> > so. If you need a downstream clock at a particular rate, call
> > clk_set_rate on it, period.
> > 
> > Whether its parent will be coming from PLL_DE or some other more
> > appriopriate clock is not relevant and doesn't make any difference.
> 
> The clock framework is not so smart to deal with these infomations:
> - CLK_PLL_PERIPH should always be 600MHz
> - CLK_TVE should always be 216MHz
> - CLK_DE (in fact CLK_MIXER{0,1}) should be larger than 300MHz (for 4K)

None of what you're doing guarantees what you state above, so I'm not
really sure what your point is.

> So we have to specify CLK_DE to be 432MHz, and then it will set
> CLK_PLL_DE to this value, then the CLK_TVE can be set to 216MHz with
> divider 2.

Yes, but it works by accident. Any clock change somewhere in the same
clock-tree might break whatever you have set in the DT.

Hence why you want to do it within the clock framework and your
driver, not here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6745 bytes --]

On Sat, Jun 10, 2017 at 10:57:28PM +0800, icenowy@aosc.io wrote:
> 在 2017-06-09 22:46，Maxime Ripard 写道：
> > On Thu, Jun 08, 2017 at 01:01:53PM +0800, icenowy@aosc.io wrote:
> > > 在 2017-06-07 22:38，Maxime Ripard 写道：
> > > > On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
> > > > > >I have no idea what this is supposed to be doing either.
> > > > > >
> > > > > >I might be wrong, but I really feel like there's a big mismatch
> > > > > >between your commit log, and what you actually implement.
> > > > > >
> > > > > >In your commit log, you should state:
> > > > > >
> > > > > >A) What is the current behaviour
> > > > > >B) Why that is a problem
> > > > > >C) How do you address it
> > > > > >
> > > > > >And you don't.
> > > > > >
> > > > > >However, after discussing it with Chen-Yu, it seems like you're trying
> > > > > >to have all the mixers probed before the TCONs. If that is so, there's
> > > > > >nothing specific to the H3 here, and we also have the same issue on
> > > > > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
> > > > > >the easiest solution would be to move from a DFS algorithm to walk
> > > > > >down the graph to a BFS one.
> > > > > >
> > > > > >That way, we would add all mixers first, then the TCONs, then the
> > > > > >encoders, and the component framework will probe them in order.
> > > > >
> > > > > No. I said that they're swappable, however, I don't want to
> > > > > implement the swap now, but hardcode 0-0 1-1 connection.
> > > >
> > > > We're on the same page, it's definitely not what I was mentionning
> > > > here. This would require a significant rework, and the usecase is
> > > > still unclear for now.
> > > >
> > > > > However, as you and Chen-Yu said, device tree should reflect the
> > > > > real hardware, there will be bonus endpoints for the swapped
> > > > > connection.
> > > >
> > > > If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
> > > > tcon 0, then yes, we're going to need it.
> > > >
> > > > > What I want to do is to ignore the bonus connection, in order to
> > > > > prevent them from confusing the code.
> > > > >
> > > > > If you just change the bind sequence, I think it cannot be
> > > > > prevented that wrong connections will be bound.
> > > >
> > > > This is where I don't follow you anymore. The component framework
> > > > doesn't list connections but devices. The swapped connections do not
> > > > matter here, we have the same set of devices: mixer0, mixer1, tcon0
> > > > and tcon1.
> > > >
> > > > The thing that does change with your patch is that before, the binding
> > > > sequence would have been mixer0, tcon0, tcon1, mixer1. With your
> > > > patch, it's mixer0, tcon0, mixer1, tcon1.
> > > >
> > > > So, again, stating what issue you were seeing before making this patch
> > > > would be very helpful to see what you're trying to do / fix.
> > > 
> > > So maybe I can drop the forward search (searching output) code, and
> > > keep
> > > only the backward search (search input) code in TCON?
> > > 
> > > Forward search code is only used when binding, but backward search
> > > is used
> > > for TCON to find connected mixer.
> > 
> > It is hard to talk about a solution, when it's not clear what the
> > issue is.
> > 
> > So please state
> > > > > >A) What is the current behaviour
> > > > > >B) Why that is a problem
> > > > > >C) How do you address it
> > 
> > We'll talk about a solution once this is done.
> 
> (All those things are based on the assumption that mixer0, mixer1, tcon0
> and tcon1 are all enabled in DT. If one group of mixer-tcon pair is fully
> disabled in DT it will behave properly.)
> 
> For the backward search:
> 
> A) The current behaviour is to take the first engine found, which will be
> wrong in the situation of tcon1 if mixer0 and mixer1 are both enabled:
> mixer0 is taken for tcon1 instead of mixer1.
> 
> B) It takes mixer0 as it matches the first endpoint of tcon0's input.
> 
> C) It's a logic failure in the backward search, as it only considered
> the DE1 situation, in which TCONs will only have one engine as input.

That's not true. DE1's can output to several TCONs (or rather, TCONs
can select multiple engines as their input). The A31 for example is in
this case.

I think what Chen-Yu did so far is that he only enables one engine and
TCON for now. That will leave us some time to rework and improve
things gradually. It would probably be easier (and faster) for you too.

> For the bind process:
> 
> A) The current behaviour is to try to bind all output endpoints of the
> engine, during binding all outputs of mixer0, these will happen:
>   1. tcon1 is bound with mixer0 as its engine if backward searching
>   is not fixed.
>   2. tcon1 fails to be bound as its engine is not yet bound when
>   backward searching works properly, then sun4i_drv will refuse
>   to continue as a component is not properly bound.

So this is the ordering issue I was mentionning earlier. The way to
fix this is to use BFS instead of DFS when building the component
list.

> B) The binding process in sun4i_drv will bind a component that is not
> really an working output of the forward component, but only exists in
> the endpoint list as a theortically possible output (in fact not an
> real output).

I'm not sure what you mean by that. Isn't the tcon1 a real output for
mixer0?

> C) I tested with this patch's sun4i_drv_node_is_swappable_de2_mixer
> function masked (always return false), and then the multiple
> mixer+tcon situations don't work properly.
> 
> P.S. I think the BFS solution is really a dirty hack -- although we
> bind components, not connections, we should decide the next component
> to bind according to the connections -- not really connected
> components shouldn't be bound.

This isn't really about whether you follow connections or not, in both
cases you do. It's how you follow those connections that matters, and
it does make sense to follow them stage by stage in our
pipeline. ie. something that would bind all the engines, then all the
TCONs, then all the encoders.

All of them would have connections between them.

> For example, if we enabled mixer0, tcon0 and tcon1, tcon1 shouldn't
> be bound at all. However in BFS situation tcon1 will also be bound
> and then fail to be bound if the backward engine searching is fixed.

Short term view: we shouldn't be in that case in the first place.
Long term view: there's no reason it shouldn't work.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4321 bytes --]

On Sat, Jun 10, 2017 at 11:16:35PM +0800, icenowy@aosc.io wrote:
> 在 2017-06-10 22:57，icenowy@aosc.io 写道：
> > 在 2017-06-09 22:46，Maxime Ripard 写道：
> > > On Thu, Jun 08, 2017 at 01:01:53PM +0800, icenowy@aosc.io wrote:
> > > > 在 2017-06-07 22:38，Maxime Ripard 写道：
> > > > > On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
> > > > > > >I have no idea what this is supposed to be doing either.
> > > > > > >
> > > > > > >I might be wrong, but I really feel like there's a big mismatch
> > > > > > >between your commit log, and what you actually implement.
> > > > > > >
> > > > > > >In your commit log, you should state:
> > > > > > >
> > > > > > >A) What is the current behaviour
> > > > > > >B) Why that is a problem
> > > > > > >C) How do you address it
> > > > > > >
> > > > > > >And you don't.
> > > > > > >
> > > > > > >However, after discussing it with Chen-Yu, it seems like you're trying
> > > > > > >to have all the mixers probed before the TCONs. If that is so, there's
> > > > > > >nothing specific to the H3 here, and we also have the same issue on
> > > > > > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
> > > > > > >the easiest solution would be to move from a DFS algorithm to walk
> > > > > > >down the graph to a BFS one.
> > > > > > >
> > > > > > >That way, we would add all mixers first, then the TCONs, then the
> > > > > > >encoders, and the component framework will probe them in order.
> > > > > >
> > > > > > No. I said that they're swappable, however, I don't want to
> > > > > > implement the swap now, but hardcode 0-0 1-1 connection.
> > > > >
> > > > > We're on the same page, it's definitely not what I was mentionning
> > > > > here. This would require a significant rework, and the usecase is
> > > > > still unclear for now.
> > > > >
> > > > > > However, as you and Chen-Yu said, device tree should reflect the
> > > > > > real hardware, there will be bonus endpoints for the swapped
> > > > > > connection.
> > > > >
> > > > > If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
> > > > > tcon 0, then yes, we're going to need it.
> > > > >
> > > > > > What I want to do is to ignore the bonus connection, in order to
> > > > > > prevent them from confusing the code.
> > > > > >
> > > > > > If you just change the bind sequence, I think it cannot be
> > > > > > prevented that wrong connections will be bound.
> > > > >
> > > > > This is where I don't follow you anymore. The component framework
> > > > > doesn't list connections but devices. The swapped connections do not
> > > > > matter here, we have the same set of devices: mixer0, mixer1, tcon0
> > > > > and tcon1.
> > > > >
> > > > > The thing that does change with your patch is that before, the binding
> > > > > sequence would have been mixer0, tcon0, tcon1, mixer1. With your
> > > > > patch, it's mixer0, tcon0, mixer1, tcon1.
> > > > >
> > > > > So, again, stating what issue you were seeing before making this patch
> > > > > would be very helpful to see what you're trying to do / fix.
> > > > 
> > > > So maybe I can drop the forward search (searching output) code,
> > > > and keep
> > > > only the backward search (search input) code in TCON?
> > > > 
> > > > Forward search code is only used when binding, but backward
> > > > search is used
> > > > for TCON to find connected mixer.
> > > 
> > > It is hard to talk about a solution, when it's not clear what the
> > > issue is.
> > > 
> > > So please state
> > > > > > >A) What is the current behaviour
> > > > > > >B) Why that is a problem
> > > > > > >C) How do you address it
> > > 
> > > We'll talk about a solution once this is done.
> > 
> > (All those things are based on the assumption that mixer0, mixer1, tcon0
> > and tcon1 are all enabled in DT. If one group of mixer-tcon pair is
> > fully
> > disabled in DT it will behave properly.)
> 
> So there's a temporary workaround -- only enable one pipeline and disable
> the unused mixer and tcon totally.

As a short-term workaround, while we rework the code to enable both
pipelines, it definitely works for me.

Maxim

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 07/11] drm: sun4i: add support for the TV encoder in H3 SoC

[Icenowy Zheng]

于 2017年6月13日 GMT+08:00 下午3:44:32, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Sun, Jun 11, 2017 at 02:43:42PM +0800, icenowy@aosc.io wrote:
>> 在 2017-06-07 17:38，Maxime Ripard 写道：
>> > On Mon, Jun 05, 2017 at 12:01:45AM +0800, Icenowy Zheng wrote:
>> > > Allwinner H3 features a TV encoder similar to the one in earlier
>SoCs,
>> > > but has a internal fixed clock divider that divides the TCON1
>clock
>> > > (called TVE clock in datasheet) by 11.
>> > > 
>> > > Add support for it.
>> > > 
>> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> > > ---
>> > > Changes in v2:
>> > > - Quirk part rewritten.
>> > > 
>> > >  drivers/gpu/drm/sun4i/sun4i_tv.c | 35
>> > > ++++++++++++++++++++++++++++++++++-
>> > >  1 file changed, 34 insertions(+), 1 deletion(-)
>> > > 
>> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c
>> > > b/drivers/gpu/drm/sun4i/sun4i_tv.c
>> > > index 338b9e5bb2a3..b9ff6d5ea67a 100644
>> > > --- a/drivers/gpu/drm/sun4i/sun4i_tv.c
>> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
>> > > @@ -13,6 +13,7 @@
>> > >  #include <linux/clk.h>
>> > >  #include <linux/component.h>
>> > >  #include <linux/of_address.h>
>> > > +#include <linux/of_device.h>
>> > >  #include <linux/regmap.h>
>> > >  #include <linux/reset.h>
>> > > 
>> > > @@ -169,14 +170,21 @@ struct tv_mode {
>> > >  	const struct resync_parameters	*resync_params;
>> > >  };
>> > > 
>> > > +struct sun4i_tv_quirks {
>> > > +	int fixed_divider;
>> > > +};
>> > > +
>> > >  struct sun4i_tv {
>> > >  	struct drm_connector	connector;
>> > >  	struct drm_encoder	encoder;
>> > > 
>> > >  	struct clk		*clk;
>> > > +	struct clk		*mod_clk;
>> > >  	struct regmap		*regs;
>> > >  	struct reset_control	*reset;
>> > > 
>> > > +	const struct sun4i_tv_quirks *quirks;
>> > > +
>> > >  	struct sun4i_drv	*drv;
>> > >  };
>> > > 
>> > > @@ -391,6 +399,12 @@ static void sun4i_tv_mode_set(struct
>> > > drm_encoder *encoder,
>> > >  	struct sun4i_tcon *tcon = crtc->tcon;
>> > >  	const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode);
>> > > 
>> > > +	if (tv->quirks->fixed_divider) {
>> > > +		DRM_DEBUG_DRIVER("Applying fixed divider %d on TVE clock\n",
>> > > +				 tv->quirks->fixed_divider);
>> > > +		mode->crtc_clock *= tv->quirks->fixed_divider;
>> > > +	}
>> > > +
>> > 
>> > You're not allowed to change the mode in mode_set, and you
>shouldn't
>> > even change it. The output pixel clock is still 27MHz.
>> > 
>> > You should implement that using the states, as we discussed
>already.
>> 
>> After reading the comments at
>include/drm/drm_modeset_helper_vtables.h,
>> I think the atomic_check function is allowed to directly change
>> the adjust_mode of crtc_state.
>> 
>> And according to other comments at include/drm/drm_modes.h, the
>> crtc_clock in adjust_mode should be the clock to be really feed
>> to the hardware.
>> 
>> So I think I can just change this in atomic_check.
>> 
>> However, the mode_set function of sun4i_tv seems to be not
>> regarding adjusted_mode and still use original mode.
>> 
>> So my current design is:
>> - Multiply adjusted_mode in atomic_check.
>> - Feed adjust_mode to TCON code in mode_set function.
>> 
>> Is this proper?
>> 
>> (From my own understanding to the comments I think so.)
>
>No, it's not. The pixel clock in the mode is the pixel clock output
>physically by the connector. You want to use it for an intermediate
>clock in your pipeline.
>
>This is not the same clock, and not the same frequency.

So should a multiplier parameter be passed via
tcon1_mode_set function?

>
>Maxime

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Chen-Yu Tsai]

On Tue, Jun 13, 2017 at 5:43 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Sat, Jun 10, 2017 at 10:57:28PM +0800, icenowy@aosc.io wrote:
>> 在 2017-06-09 22:46，Maxime Ripard 写道：
>> > On Thu, Jun 08, 2017 at 01:01:53PM +0800, icenowy@aosc.io wrote:
>> > > 在 2017-06-07 22:38，Maxime Ripard 写道：
>> > > > On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote:
>> > > > > >I have no idea what this is supposed to be doing either.
>> > > > > >
>> > > > > >I might be wrong, but I really feel like there's a big mismatch
>> > > > > >between your commit log, and what you actually implement.
>> > > > > >
>> > > > > >In your commit log, you should state:
>> > > > > >
>> > > > > >A) What is the current behaviour
>> > > > > >B) Why that is a problem
>> > > > > >C) How do you address it
>> > > > > >
>> > > > > >And you don't.
>> > > > > >
>> > > > > >However, after discussing it with Chen-Yu, it seems like you're trying
>> > > > > >to have all the mixers probed before the TCONs. If that is so, there's
>> > > > > >nothing specific to the H3 here, and we also have the same issue on
>> > > > > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
>> > > > > >the easiest solution would be to move from a DFS algorithm to walk
>> > > > > >down the graph to a BFS one.
>> > > > > >
>> > > > > >That way, we would add all mixers first, then the TCONs, then the
>> > > > > >encoders, and the component framework will probe them in order.
>> > > > >
>> > > > > No. I said that they're swappable, however, I don't want to
>> > > > > implement the swap now, but hardcode 0-0 1-1 connection.
>> > > >
>> > > > We're on the same page, it's definitely not what I was mentionning
>> > > > here. This would require a significant rework, and the usecase is
>> > > > still unclear for now.
>> > > >
>> > > > > However, as you and Chen-Yu said, device tree should reflect the
>> > > > > real hardware, there will be bonus endpoints for the swapped
>> > > > > connection.
>> > > >
>> > > > If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to
>> > > > tcon 0, then yes, we're going to need it.
>> > > >
>> > > > > What I want to do is to ignore the bonus connection, in order to
>> > > > > prevent them from confusing the code.
>> > > > >
>> > > > > If you just change the bind sequence, I think it cannot be
>> > > > > prevented that wrong connections will be bound.
>> > > >
>> > > > This is where I don't follow you anymore. The component framework
>> > > > doesn't list connections but devices. The swapped connections do not
>> > > > matter here, we have the same set of devices: mixer0, mixer1, tcon0
>> > > > and tcon1.
>> > > >
>> > > > The thing that does change with your patch is that before, the binding
>> > > > sequence would have been mixer0, tcon0, tcon1, mixer1. With your
>> > > > patch, it's mixer0, tcon0, mixer1, tcon1.
>> > > >
>> > > > So, again, stating what issue you were seeing before making this patch
>> > > > would be very helpful to see what you're trying to do / fix.
>> > >
>> > > So maybe I can drop the forward search (searching output) code, and
>> > > keep
>> > > only the backward search (search input) code in TCON?
>> > >
>> > > Forward search code is only used when binding, but backward search
>> > > is used
>> > > for TCON to find connected mixer.
>> >
>> > It is hard to talk about a solution, when it's not clear what the
>> > issue is.
>> >
>> > So please state
>> > > > > >A) What is the current behaviour
>> > > > > >B) Why that is a problem
>> > > > > >C) How do you address it
>> >
>> > We'll talk about a solution once this is done.
>>
>> (All those things are based on the assumption that mixer0, mixer1, tcon0
>> and tcon1 are all enabled in DT. If one group of mixer-tcon pair is fully
>> disabled in DT it will behave properly.)
>>
>> For the backward search:
>>
>> A) The current behaviour is to take the first engine found, which will be
>> wrong in the situation of tcon1 if mixer0 and mixer1 are both enabled:
>> mixer0 is taken for tcon1 instead of mixer1.
>>
>> B) It takes mixer0 as it matches the first endpoint of tcon0's input.
>>
>> C) It's a logic failure in the backward search, as it only considered
>> the DE1 situation, in which TCONs will only have one engine as input.

Then you should fix it so it works with DE2. For DE2 it would be something
like this:

Mixer probe
-----------
You have two choices on how to get its ID: a) Look downstream to TCONs,
and get the reg value from the remote endpoint, or b) get the display-engine
node, and check the index of the phandle that points to itself (the mixer).
A is easier, and it is why I asked you to follow the connection ID rule in
the device tree binding.

TCON probe
----------
Similar to A) for the mixers, look upstream and get the reg value from the
remote endpoint.

Afterwards you would have correct IDs on both sides, then it's just a matter
of going through the list of mixers and TCONs and pairing them up.


Mixer0  0 ---- 0  TCON0
        1      1 <- This would be the remote endpoint reg value for mixer1.
         \    /
          \  /
           \/
           /\
          /  \
         /    \
        0      0 <- This would be the remote endpoint reg value for mixer0.
Mixer1  1 ---- 1  TCON1

Hope this makes things clearer.

>
> That's not true. DE1's can output to several TCONs (or rather, TCONs
> can select multiple engines as their input). The A31 for example is in
> this case.

Actually that's not true. The TCON is bound to the backend. I don't see
any controls for muxing that. So the TCON-backend search routine is very
simple for DE1. The frontends are free to feed either backend though.

> I think what Chen-Yu did so far is that he only enables one engine and
> TCON for now. That will leave us some time to rework and improve
> things gradually. It would probably be easier (and faster) for you too.

My code deals with muxes between the TCON and the downstream encoders (HDMI/TV).

You need something similar to deal with the mux between the mixers and TCONs.
My code doesn't fit DE2, because the case hadn't been added yet.

>> For the bind process:
>>
>> A) The current behaviour is to try to bind all output endpoints of the
>> engine, during binding all outputs of mixer0, these will happen:
>>   1. tcon1 is bound with mixer0 as its engine if backward searching
>>   is not fixed.
>>   2. tcon1 fails to be bound as its engine is not yet bound when
>>   backward searching works properly, then sun4i_drv will refuse
>>   to continue as a component is not properly bound.
>
> So this is the ordering issue I was mentionning earlier. The way to
> fix this is to use BFS instead of DFS when building the component
> list.
>
>> B) The binding process in sun4i_drv will bind a component that is not
>> really an working output of the forward component, but only exists in
>> the endpoint list as a theortically possible output (in fact not an
>> real output).
>
> I'm not sure what you mean by that. Isn't the tcon1 a real output for
> mixer0?
>
>> C) I tested with this patch's sun4i_drv_node_is_swappable_de2_mixer
>> function masked (always return false), and then the multiple
>> mixer+tcon situations don't work properly.
>>
>> P.S. I think the BFS solution is really a dirty hack -- although we
>> bind components, not connections, we should decide the next component
>> to bind according to the connections -- not really connected
>> components shouldn't be bound.
>
> This isn't really about whether you follow connections or not, in both
> cases you do. It's how you follow those connections that matters, and
> it does make sense to follow them stage by stage in our
> pipeline. ie. something that would bind all the engines, then all the
> TCONs, then all the encoders.
>
> All of them would have connections between them.
>
>> For example, if we enabled mixer0, tcon0 and tcon1, tcon1 shouldn't
>> be bound at all. However in BFS situation tcon1 will also be bound
>> and then fail to be bound if the backward engine searching is fixed.
>
> Short term view: we shouldn't be in that case in the first place.
> Long term view: there's no reason it shouldn't work.

Maybe I missed something? TCONs and everything before them should always
be enabled. There's no reason not to. This is especially true for TCON0
which holds the mux register on some SoCs.

About Maxime's long term view: there's no reason we can't just silently
ignore a component if its supposed companion is missing, like a TCON
missing its backend, or the other way around. The current code already
covers the latter case: since the CRTC (and by extension, all DRM/KMS
functionality related to that pipeline) is created in the TCON bind
function, if the TCON is not enabled, the backend just idles.

ChenYu

Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

Hi,

On Tue, Jun 13, 2017 at 06:05:57PM +0800, Chen-Yu Tsai wrote:
> > That's not true. DE1's can output to several TCONs (or rather, TCONs
> > can select multiple engines as their input). The A31 for example is in
> > this case.
> 
> Actually that's not true. The TCON is bound to the backend. I don't see
> any controls for muxing that. So the TCON-backend search routine is very
> simple for DE1. The frontends are free to feed either backend though.

I think it does, look at the lower bits of TCON0_CTL_REG and
TCON1_CTL_REG in the A31 datasheet. It clearly seems used to control
from which DE you fetch your data from, and you have both options.

> >> For example, if we enabled mixer0, tcon0 and tcon1, tcon1 shouldn't
> >> be bound at all. However in BFS situation tcon1 will also be bound
> >> and then fail to be bound if the backward engine searching is fixed.
> >
> > Short term view: we shouldn't be in that case in the first place.
> > Long term view: there's no reason it shouldn't work.
> 
> Maybe I missed something? TCONs and everything before them should always
> be enabled. There's no reason not to. This is especially true for TCON0
> which holds the mux register on some SoCs.

If we're not able to use anything connected to TCON1, disabling it
still seems like a good stop-gap measure.

> About Maxime's long term view: there's no reason we can't just silently
> ignore a component if its supposed companion is missing, like a TCON
> missing its backend, or the other way around.

It's a bit more complicated than that. TCON0 is probably mandatory,
and even if TCON1 is missing, we can entirely route around it in
hardware, and I think it's the case for all the stages in our
pipelines. There's no reason we could not operate that way.

But this is clearly something that we should aim for, not something
that needs to be done today.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 07/11] drm: sun4i: add support for the TV encoder in H3 SoC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4172 bytes --]

On Tue, Jun 13, 2017 at 05:51:42PM +0800, Icenowy Zheng wrote:
> 
> 
> 于 2017年6月13日 GMT+08:00 下午3:44:32, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >On Sun, Jun 11, 2017 at 02:43:42PM +0800, icenowy@aosc.io wrote:
> >> 在 2017-06-07 17:38，Maxime Ripard 写道：
> >> > On Mon, Jun 05, 2017 at 12:01:45AM +0800, Icenowy Zheng wrote:
> >> > > Allwinner H3 features a TV encoder similar to the one in earlier
> >SoCs,
> >> > > but has a internal fixed clock divider that divides the TCON1
> >clock
> >> > > (called TVE clock in datasheet) by 11.
> >> > > 
> >> > > Add support for it.
> >> > > 
> >> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >> > > ---
> >> > > Changes in v2:
> >> > > - Quirk part rewritten.
> >> > > 
> >> > >  drivers/gpu/drm/sun4i/sun4i_tv.c | 35
> >> > > ++++++++++++++++++++++++++++++++++-
> >> > >  1 file changed, 34 insertions(+), 1 deletion(-)
> >> > > 
> >> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c
> >> > > b/drivers/gpu/drm/sun4i/sun4i_tv.c
> >> > > index 338b9e5bb2a3..b9ff6d5ea67a 100644
> >> > > --- a/drivers/gpu/drm/sun4i/sun4i_tv.c
> >> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
> >> > > @@ -13,6 +13,7 @@
> >> > >  #include <linux/clk.h>
> >> > >  #include <linux/component.h>
> >> > >  #include <linux/of_address.h>
> >> > > +#include <linux/of_device.h>
> >> > >  #include <linux/regmap.h>
> >> > >  #include <linux/reset.h>
> >> > > 
> >> > > @@ -169,14 +170,21 @@ struct tv_mode {
> >> > >  	const struct resync_parameters	*resync_params;
> >> > >  };
> >> > > 
> >> > > +struct sun4i_tv_quirks {
> >> > > +	int fixed_divider;
> >> > > +};
> >> > > +
> >> > >  struct sun4i_tv {
> >> > >  	struct drm_connector	connector;
> >> > >  	struct drm_encoder	encoder;
> >> > > 
> >> > >  	struct clk		*clk;
> >> > > +	struct clk		*mod_clk;
> >> > >  	struct regmap		*regs;
> >> > >  	struct reset_control	*reset;
> >> > > 
> >> > > +	const struct sun4i_tv_quirks *quirks;
> >> > > +
> >> > >  	struct sun4i_drv	*drv;
> >> > >  };
> >> > > 
> >> > > @@ -391,6 +399,12 @@ static void sun4i_tv_mode_set(struct
> >> > > drm_encoder *encoder,
> >> > >  	struct sun4i_tcon *tcon = crtc->tcon;
> >> > >  	const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode);
> >> > > 
> >> > > +	if (tv->quirks->fixed_divider) {
> >> > > +		DRM_DEBUG_DRIVER("Applying fixed divider %d on TVE clock\n",
> >> > > +				 tv->quirks->fixed_divider);
> >> > > +		mode->crtc_clock *= tv->quirks->fixed_divider;
> >> > > +	}
> >> > > +
> >> > 
> >> > You're not allowed to change the mode in mode_set, and you
> >shouldn't
> >> > even change it. The output pixel clock is still 27MHz.
> >> > 
> >> > You should implement that using the states, as we discussed
> >already.
> >> 
> >> After reading the comments at
> >include/drm/drm_modeset_helper_vtables.h,
> >> I think the atomic_check function is allowed to directly change
> >> the adjust_mode of crtc_state.
> >> 
> >> And according to other comments at include/drm/drm_modes.h, the
> >> crtc_clock in adjust_mode should be the clock to be really feed
> >> to the hardware.
> >> 
> >> So I think I can just change this in atomic_check.
> >> 
> >> However, the mode_set function of sun4i_tv seems to be not
> >> regarding adjusted_mode and still use original mode.
> >> 
> >> So my current design is:
> >> - Multiply adjusted_mode in atomic_check.
> >> - Feed adjust_mode to TCON code in mode_set function.
> >> 
> >> Is this proper?
> >> 
> >> (From my own understanding to the comments I think so.)
> >
> >No, it's not. The pixel clock in the mode is the pixel clock output
> >physically by the connector. You want to use it for an intermediate
> >clock in your pipeline.
> >
> >This is not the same clock, and not the same frequency.
> 
> So should a multiplier parameter be passed via
> tcon1_mode_set function?

No, and I've explained a couple of times already what you needed to
do. Please read again the previous messages.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]