[PATCH 00/11] drm/sun4i: Support two display pipelines

[PATCH 00/11] drm/sun4i: Support two display pipelines

[Chen-Yu Tsai]

Hi Maxime,

This is part 3 of my sun4i drm clean up series. In this part support
for 2 display pipelines is added, after some more code cleanups and
restructuring.

While this series enables the second display pipeline, there's no
usable output at the moment. For the A31, the second TCON's panel
interface uses the same pins as the Ethernet controller. However
Ethernet is used on most boards. We will have to wait for HDMI
support to actually use it.

Patch 1 fixes the TCON's clock and regmap initialization sequence,
splitting out the dot clock init part till after the regmap.

Patch 2 fixes a comment spotted while reviewing Maxime's HDMI patches.

Patch 3 makes the crtc init code use the tcon pointer embedded in the
crtc structure, instead of the sun4i_drv structure, to get the tcon's
output port node. This should have been a part of the last batch of
patches.

Patch 4 makes the tv encoder code get the tcon and backend pointers
from its attached crtc.

Patch 5 makes the crtc init function take tcon and backend pointers.

Patch 6 makes the layer init functions take a backend pointer.

Patch 7 adds a function to fetch a backend's ID from the device tree.

Patch 8 adds a function to fetch a TCON's ID from the device tree.

Patch 9 extends the sun4i drm driver to support 2 display pipelines.

Patch 10 adds device nodes for sun6i's second display pipeline.

Patch 11 enables sun6i's tcon0 by default.


Regards
ChenYu

Chen-Yu Tsai (11):
  drm/sun4i: Fix TCON clock and regmap initialization sequence
  drm/sun4i: Fix tcon channel 0 comment about backporch = backporch +
    hsync
  drm/sun4i: Use embedded tcon pointer to get the tcon's output port
    node
  drm/sun4i: tv: Get tcon and backend pointers from associated crtc
  drm/sun4i: Pass pointers for associated backend and tcon into crtc
    init
  drm/sun4i: Pass pointer for underlying backend into layer init
  drm/sun4i: Fetch backend ID from device tree
  drm/sun4i: Fetch TCON ID from device tree
  drm/sun4i: Support two display pipelines
  ARM: dts: sun6i: Add second display pipeline device nodes
  ARM: dts: sun6i: Enable tcon0 by default

 arch/arm/boot/dts/sun6i-a31-hummingbird.dts |   1 -
 arch/arm/boot/dts/sun6i-a31.dtsi            | 169 +++++++++++++++++++++++++++-
 drivers/gpu/drm/sun4i/sun4i_backend.c       |  53 ++++++++-
 drivers/gpu/drm/sun4i/sun4i_backend.h       |   2 +
 drivers/gpu/drm/sun4i/sun4i_crtc.c          |  13 ++-
 drivers/gpu/drm/sun4i/sun4i_crtc.h          |   4 +-
 drivers/gpu/drm/sun4i/sun4i_drv.c           |   2 +-
 drivers/gpu/drm/sun4i/sun4i_drv.h           |   6 +-
 drivers/gpu/drm/sun4i/sun4i_layer.c         |  13 +--
 drivers/gpu/drm/sun4i/sun4i_layer.h         |   3 +-
 drivers/gpu/drm/sun4i/sun4i_tcon.c          |  99 ++++++++++++++--
 drivers/gpu/drm/sun4i/sun4i_tcon.h          |   2 +
 drivers/gpu/drm/sun4i/sun4i_tv.c            |  19 ++--
 13 files changed, 344 insertions(+), 42 deletions(-)

-- 
2.11.0

[PATCH 01/11] drm/sun4i: Fix TCON clock and regmap initialization sequence

[Chen-Yu Tsai]

The TCON driver calls sun4i_tcon_init_regmap and sun4i_tcon_init_clocks
in its bind function. The former creates a regmap and writes to several
register to clear its configuration to a known default. The latter
initializes various clocks. This includes enabling the bus clock for
register access and creating the dotclock.

In order for the first step's writes to work, the bus clock must be
enabled which is done in the second step. but the dotclock's ops use
the regmap created in the first step.

Rearrange the function calls such that the clocks are initialized before
the regmap, and split out the dot clock creation to after the regmap is
initialized.

Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 505520baa585..ee51d7e77a6a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -335,12 +335,11 @@ static int sun4i_tcon_init_clocks(struct device *dev,
 		}
 	}
 
-	return sun4i_dclk_create(dev, tcon);
+	return 0;
 }
 
 static void sun4i_tcon_free_clocks(struct sun4i_tcon *tcon)
 {
-	sun4i_dclk_free(tcon);
 	clk_disable_unprepare(tcon->clk);
 }
 
@@ -505,22 +504,28 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
+	ret = sun4i_tcon_init_clocks(dev, tcon);
+	if (ret) {
+		dev_err(dev, "Couldn't init our TCON clocks\n");
+		goto err_assert_reset;
+	}
+
 	ret = sun4i_tcon_init_regmap(dev, tcon);
 	if (ret) {
 		dev_err(dev, "Couldn't init our TCON regmap\n");
-		goto err_assert_reset;
+		goto err_free_clocks;
 	}
 
-	ret = sun4i_tcon_init_clocks(dev, tcon);
+	ret = sun4i_dclk_create(dev, tcon);
 	if (ret) {
-		dev_err(dev, "Couldn't init our TCON clocks\n");
-		goto err_assert_reset;
+		dev_err(dev, "Couldn't create our TCON dot clock\n");
+		goto err_free_clocks;
 	}
 
 	ret = sun4i_tcon_init_irq(dev, tcon);
 	if (ret) {
 		dev_err(dev, "Couldn't init our TCON interrupts\n");
-		goto err_free_clocks;
+		goto err_free_dotclock;
 	}
 
 	tcon->crtc = sun4i_crtc_init(drm);
@@ -536,6 +541,8 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 
 	return 0;
 
+err_free_dotclock:
+	sun4i_dclk_free(tcon);
 err_free_clocks:
 	sun4i_tcon_free_clocks(tcon);
 err_assert_reset:
@@ -548,6 +555,7 @@ static void sun4i_tcon_unbind(struct device *dev, struct device *master,
 {
 	struct sun4i_tcon *tcon = dev_get_drvdata(dev);
 
+	sun4i_dclk_free(tcon);
 	sun4i_tcon_free_clocks(tcon);
 }
 
-- 
2.11.0

[PATCH 02/11] drm/sun4i: Fix tcon channel 0 comment about backporch = backporch + hsync

[Chen-Yu Tsai]

The backporch programmed into the tcon registers is actually the
backporch + hsync length from the display timings, as indicated in
the interface timing diagrams found in the user manual of the A31
and A33 SoCs.

The comments for channel 0 mistakenly describe the discrepancy as
TCON backporch = frontporch + hsync.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index ee51d7e77a6a..c52c482c8fd0 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -143,7 +143,7 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
 
 	/*
 	 * This is called a backporch in the register documentation,
-	 * but it really is the front porch + hsync
+	 * but it really is the back porch + hsync
 	 */
 	bp = mode->crtc_htotal - mode->crtc_hsync_start;
 	DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
@@ -156,7 +156,7 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
 
 	/*
 	 * This is called a backporch in the register documentation,
-	 * but it really is the front porch + hsync
+	 * but it really is the back porch + hsync
 	 */
 	bp = mode->crtc_vtotal - mode->crtc_vsync_start;
 	DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
-- 
2.11.0

[PATCH 03/11] drm/sun4i: Use embedded tcon pointer to get the tcon's output port node

[Chen-Yu Tsai]

A pointer to the underlying tcon of the crtc was added to the sun4i_crtc
structure in "drm/sun4i: Add backend and tcon pointers to sun4i_crtc".
However the crtc init function was still using the copy from sun4i_drv
to set drm_crtc.port. This was an oversight when the patches were
reordered.

Switch to using the embedded tcon pointer to get the tcon's ouptut port
and assign it to drm_crtc.port.

This makes it possible to remove the usage of sun4i_drv completely in
subsequent patches.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index 7bbcedff9f07..5323e3485988 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -183,7 +183,7 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm)
 	drm_crtc_helper_add(&scrtc->crtc, &sun4i_crtc_helper_funcs);
 
 	/* Set crtc.port to output port node of the tcon */
-	scrtc->crtc.port = of_graph_get_port_by_id(drv->tcon->dev->of_node,
+	scrtc->crtc.port = of_graph_get_port_by_id(scrtc->tcon->dev->of_node,
 						   1);
 
 	/* Set possible_crtcs to this crtc for overlay planes */
-- 
2.11.0

[PATCH 04/11] drm/sun4i: tv: Get tcon and backend pointers from associated crtc

[Chen-Yu Tsai]

The drm_encoder structure provides us with a pointer to the crtc
currently tied to the encoder. Subsequently we can extract the
tcon and backend pointers from our crtc structure, instead of
getting it directly from the sun4i_drv structure.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_tv.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c
index 32ed5fdf0c4d..49c49431a053 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
@@ -23,6 +23,7 @@
 #include <drm/drm_panel.h>
 
 #include "sun4i_backend.h"
+#include "sun4i_crtc.h"
 #include "sun4i_drv.h"
 #include "sun4i_tcon.h"
 
@@ -350,8 +351,9 @@ static int sun4i_tv_atomic_check(struct drm_encoder *encoder,
 static void sun4i_tv_disable(struct drm_encoder *encoder)
 {
 	struct sun4i_tv *tv = drm_encoder_to_sun4i_tv(encoder);
-	struct sun4i_drv *drv = tv->drv;
-	struct sun4i_tcon *tcon = drv->tcon;
+	struct sun4i_crtc *crtc = drm_crtc_to_sun4i_crtc(encoder->crtc);
+	struct sun4i_tcon *tcon = crtc->tcon;
+	struct sun4i_backend *backend = crtc->backend;
 
 	DRM_DEBUG_DRIVER("Disabling the TV Output\n");
 
@@ -360,18 +362,19 @@ static void sun4i_tv_disable(struct drm_encoder *encoder)
 	regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
 			   SUN4I_TVE_EN_ENABLE,
 			   0);
-	sun4i_backend_disable_color_correction(drv->backend);
+	sun4i_backend_disable_color_correction(backend);
 }
 
 static void sun4i_tv_enable(struct drm_encoder *encoder)
 {
 	struct sun4i_tv *tv = drm_encoder_to_sun4i_tv(encoder);
-	struct sun4i_drv *drv = tv->drv;
-	struct sun4i_tcon *tcon = drv->tcon;
+	struct sun4i_crtc *crtc = drm_crtc_to_sun4i_crtc(encoder->crtc);
+	struct sun4i_tcon *tcon = crtc->tcon;
+	struct sun4i_backend *backend = crtc->backend;
 
 	DRM_DEBUG_DRIVER("Enabling the TV Output\n");
 
-	sun4i_backend_apply_color_correction(drv->backend);
+	sun4i_backend_apply_color_correction(backend);
 
 	regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
 			   SUN4I_TVE_EN_ENABLE,
@@ -385,8 +388,8 @@ static void sun4i_tv_mode_set(struct drm_encoder *encoder,
 			      struct drm_display_mode *adjusted_mode)
 {
 	struct sun4i_tv *tv = drm_encoder_to_sun4i_tv(encoder);
-	struct sun4i_drv *drv = tv->drv;
-	struct sun4i_tcon *tcon = drv->tcon;
+	struct sun4i_crtc *crtc = drm_crtc_to_sun4i_crtc(encoder->crtc);
+	struct sun4i_tcon *tcon = crtc->tcon;
 	const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode);
 
 	sun4i_tcon1_mode_set(tcon, mode);
-- 
2.11.0

[PATCH 05/11] drm/sun4i: Pass pointers for associated backend and tcon into crtc init

[Chen-Yu Tsai]

sun4i_crtc controls the backend and tcon hardware blocks of the display
pipeline.

Pass pointers to the underlying devices into the crtc init function,
instead of trying to fetch them from the drm_device structure. This
avoids the headache of trying to figure out which devices the crtc
is actually associated with.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_crtc.c | 9 +++++----
 drivers/gpu/drm/sun4i/sun4i_crtc.h | 4 +++-
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index 5323e3485988..221e6d5ee970 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -134,9 +134,10 @@ static const struct drm_crtc_funcs sun4i_crtc_funcs = {
 	.disable_vblank		= sun4i_crtc_disable_vblank,
 };
 
-struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm)
+struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm,
+				   struct sun4i_backend *backend,
+				   struct sun4i_tcon *tcon)
 {
-	struct sun4i_drv *drv = drm->dev_private;
 	struct sun4i_crtc *scrtc;
 	struct drm_plane *primary = NULL, *cursor = NULL;
 	int ret, i;
@@ -144,8 +145,8 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm)
 	scrtc = devm_kzalloc(drm->dev, sizeof(*scrtc), GFP_KERNEL);
 	if (!scrtc)
 		return ERR_PTR(-ENOMEM);
-	scrtc->backend = drv->backend;
-	scrtc->tcon = drv->tcon;
+	scrtc->backend = backend;
+	scrtc->tcon = tcon;
 
 	/* Create our layers */
 	scrtc->layers = sun4i_layers_init(drm);
diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.h b/drivers/gpu/drm/sun4i/sun4i_crtc.h
index cd0e633cce3a..230cb8f0d601 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.h
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.h
@@ -27,6 +27,8 @@ static inline struct sun4i_crtc *drm_crtc_to_sun4i_crtc(struct drm_crtc *crtc)
 	return container_of(crtc, struct sun4i_crtc, crtc);
 }
 
-struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm);
+struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm,
+				   struct sun4i_backend *backend,
+				   struct sun4i_tcon *tcon);
 
 #endif /* _SUN4I_CRTC_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c52c482c8fd0..3ced0b1cef6e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -528,7 +528,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 		goto err_free_dotclock;
 	}
 
-	tcon->crtc = sun4i_crtc_init(drm);
+	tcon->crtc = sun4i_crtc_init(drm, drv->backend, tcon);
 	if (IS_ERR(tcon->crtc)) {
 		dev_err(dev, "Couldn't create our CRTC\n");
 		ret = PTR_ERR(tcon->crtc);
-- 
2.11.0

[PATCH 06/11] drm/sun4i: Pass pointer for underlying backend into layer init

[Chen-Yu Tsai]

sun4i_layer only controls the backend hardware block of the display
pipeline.

Pass pointers to the underlying backend in the layer init function,
instead of trying to fetch it from the drm_device structure. This
avoids the headache of trying to figure out which device the layers
actually belong to.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_crtc.c  |  2 +-
 drivers/gpu/drm/sun4i/sun4i_layer.c | 13 ++++++-------
 drivers/gpu/drm/sun4i/sun4i_layer.h |  3 ++-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index 221e6d5ee970..3c876c3a356a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -149,7 +149,7 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm,
 	scrtc->tcon = tcon;
 
 	/* Create our layers */
-	scrtc->layers = sun4i_layers_init(drm);
+	scrtc->layers = sun4i_layers_init(drm, scrtc->backend);
 	if (IS_ERR(scrtc->layers)) {
 		dev_err(drm->dev, "Couldn't create the planes\n");
 		return NULL;
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 6feaf85a5942..f26bde5b9117 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -16,7 +16,6 @@
 #include <drm/drmP.h>
 
 #include "sun4i_backend.h"
-#include "sun4i_drv.h"
 #include "sun4i_layer.h"
 
 struct sun4i_plane_desc {
@@ -102,9 +101,9 @@ static const struct sun4i_plane_desc sun4i_backend_planes[] = {
 };
 
 static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
+						struct sun4i_backend *backend,
 						const struct sun4i_plane_desc *plane)
 {
-	struct sun4i_drv *drv = drm->dev_private;
 	struct sun4i_layer *layer;
 	int ret;
 
@@ -124,14 +123,14 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
 
 	drm_plane_helper_add(&layer->plane,
 			     &sun4i_backend_layer_helper_funcs);
-	layer->backend = drv->backend;
+	layer->backend = backend;
 
 	return layer;
 }
 
-struct sun4i_layer **sun4i_layers_init(struct drm_device *drm)
+struct sun4i_layer **sun4i_layers_init(struct drm_device *drm,
+				       struct sun4i_backend *backend)
 {
-	struct sun4i_drv *drv = drm->dev_private;
 	struct sun4i_layer **layers;
 	int i;
 
@@ -165,7 +164,7 @@ struct sun4i_layer **sun4i_layers_init(struct drm_device *drm)
 		const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i];
 		struct sun4i_layer *layer;
 
-		layer = sun4i_layer_init_one(drm, plane);
+		layer = sun4i_layer_init_one(drm, backend, plane);
 		if (IS_ERR(layer)) {
 			dev_err(drm->dev, "Couldn't initialize %s plane\n",
 				i ? "overlay" : "primary");
@@ -174,7 +173,7 @@ struct sun4i_layer **sun4i_layers_init(struct drm_device *drm)
 
 		DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n",
 				 i ? "overlay" : "primary", plane->pipe);
-		regmap_update_bits(drv->backend->regs, SUN4I_BACKEND_ATTCTL_REG0(i),
+		regmap_update_bits(backend->regs, SUN4I_BACKEND_ATTCTL_REG0(i),
 				   SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK,
 				   SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(plane->pipe));
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.h b/drivers/gpu/drm/sun4i/sun4i_layer.h
index a97e376bae17..4be1f0919df2 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.h
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.h
@@ -26,6 +26,7 @@ plane_to_sun4i_layer(struct drm_plane *plane)
 	return container_of(plane, struct sun4i_layer, plane);
 }
 
-struct sun4i_layer **sun4i_layers_init(struct drm_device *drm);
+struct sun4i_layer **sun4i_layers_init(struct drm_device *drm,
+				       struct sun4i_backend *backend);
 
 #endif /* _SUN4I_LAYER_H_ */
-- 
2.11.0

[PATCH 07/11] drm/sun4i: Fetch backend ID from device tree

[Chen-Yu Tsai]

Some Allwinner SoCs have 2 display pipelines, as in 2 of each
components, including the frontend, backend, TCON, and any other
extras.

As the backend and TCON are always paired together and form the CRTC,
we need to know which backend or TCON we are currently probing, so we
can pair them when initializing the CRTC.

This patch figures out the backend's ID from the device tree and stores
it in the backend's data structure. It does this by looking at the "reg"
property of any remote endpoints connected to the backend's input port.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_backend.c | 44 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun4i_backend.h |  2 ++
 2 files changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index d660741ba475..f3c92d54c8e4 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -19,6 +19,7 @@
 #include <drm/drm_plane_helper.h>
 
 #include <linux/component.h>
+#include <linux/of_graph.h>
 #include <linux/reset.h>
 
 #include "sun4i_backend.h"
@@ -288,6 +289,45 @@ static int sun4i_backend_free_sat(struct device *dev) {
 	return 0;
 }
 
+/*
+ * The display backend can take video output from the display frontend, or
+ * the display enhancement unit on the A80, as input for one it its layers.
+ * This relationship within the display pipeline is encoded in the device
+ * tree with of_graph, and we use it here to figure out which backend, if
+ * there are 2 or more, we are currently probing. The number would be in
+ * the "reg" property of the upstream output port endpoint.
+ */
+static int sun4i_backend_of_get_id(struct device_node *node)
+{
+	struct device_node *port, *ep;
+	int ret = -EINVAL;
+
+	/* input is port 0 */
+	port = of_graph_get_port_by_id(node, 0);
+	if (!port)
+		return -EINVAL;
+
+	/* try finding an upstream endpoint */
+	for_each_available_child_of_node(port, ep) {
+		struct device_node *remote;
+		u32 reg;
+
+		remote = of_parse_phandle(ep, "remote-endpoint", 0);
+		if (!remote)
+			continue;
+
+		ret = of_property_read_u32(remote, "reg", &reg);
+		if (ret)
+			continue;
+
+		ret = reg;
+	}
+
+	of_node_put(port);
+
+	return ret;
+}
+
 static struct regmap_config sun4i_backend_regmap_config = {
 	.reg_bits	= 32,
 	.val_bits	= 32,
@@ -312,6 +352,10 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
 	dev_set_drvdata(dev, backend);
 	drv->backend = backend;
 
+	backend->id = sun4i_backend_of_get_id(dev->of_node);
+	if (backend->id < 0)
+		return backend->id;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	regs = devm_ioremap_resource(dev, res);
 	if (IS_ERR(regs))
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h
index 83e63cc702b4..d9a7df55c63f 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.h
@@ -149,6 +149,8 @@ struct sun4i_backend {
 
 	struct clk		*sat_clk;
 	struct reset_control	*sat_reset;
+
+	int			id;
 };
 
 void sun4i_backend_apply_color_correction(struct sun4i_backend *backend);
-- 
2.11.0

[PATCH 08/11] drm/sun4i: Fetch TCON ID from device tree

[Chen-Yu Tsai]

Some Allwinner SoCs have 2 display pipelines, as in 2 of each
components, including the frontend, backend, TCON, and any other
extras.

As the backend and TCON are always paired together and form the CRTC,
we need to know which backend or TCON we are currently probing, so we
can pair them when initializing the CRTC.

This patch figures out the TCON's ID from the device tree and stores
it in the TCON's data structure. It does this by looking at the "reg"
property of any remote endpoints connected to the TCON's output port,
except LCD panels or external bridges on the RGB interface.

If none are found, it assumes the system only has 1 TCON.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 60 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 ++
 2 files changed, 62 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 3ced0b1cef6e..b774c9a50c55 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -471,6 +471,55 @@ struct drm_bridge *sun4i_tcon_find_bridge(struct device_node *node)
 	return of_drm_find_bridge(remote) ?: ERR_PTR(-EPROBE_DEFER);
 }
 
+/*
+ * The tcon can output video to an tv or hdmi encoder. When there are 2
+ * tcons, on older SoCs with DE 1.0, either tcon can be muxed to the
+ * encoder.
+ *
+ * This relationship within the display pipeline is encoded in the device
+ * tree with of_graph. Here we use it to figure out which tcon, if there
+ * are 2 or more, we are currently probing. The number would be in the
+ * "reg" property of the downstream input port endpoint.
+ */
+static int sun4i_tcon_of_get_id(struct device_node *node)
+{
+	struct device_node *port, *ep;
+	int ret = -EINVAL;
+
+	/* output is port 1 */
+	port = of_graph_get_port_by_id(node, 1);
+	if (!port)
+		return -EINVAL;
+
+	/* try finding a downstream endpoint */
+	for_each_available_child_of_node(port, ep) {
+		struct device_node *remote;
+		u32 reg;
+
+		ret = of_property_read_u32(ep, "reg", &reg);
+		if (ret)
+			continue;
+
+		/* Skip endpoint 0, the LCD panel interface */
+		if (!reg)
+			continue;
+
+		remote = of_parse_phandle(ep, "remote-endpoint", 0);
+		if (!remote)
+			continue;
+
+		ret = of_property_read_u32(remote, "reg", &reg);
+		if (ret)
+			continue;
+
+		ret = reg;
+	}
+
+	of_node_put(port);
+
+	return ret;
+}
+
 static int sun4i_tcon_bind(struct device *dev, struct device *master,
 			   void *data)
 {
@@ -488,6 +537,17 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	tcon->dev = dev;
 	tcon->quirks = of_device_get_match_data(dev);
 
+	/* This can fail if the DT does not have any downstream encoders. */
+	tcon->id = sun4i_tcon_of_get_id(dev->of_node);
+	if (tcon->id < 0) {
+		/*
+		 * TODO We currently support only 1 TCON, so we can
+		 * safely set this to 0. This should be revisited
+		 * when we add support for multiple pipelines.
+		 */
+		tcon->id = 0;
+	}
+
 	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 f636343a935d..28b8da1e6c18 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -172,6 +172,8 @@ struct sun4i_tcon {
 
 	/* Associated crtc */
 	struct sun4i_crtc		*crtc;
+
+	int				id;
 };
 
 struct drm_bridge *sun4i_tcon_find_bridge(struct device_node *node);
-- 
2.11.0

[PATCH 09/11] drm/sun4i: Support two display pipelines

[Chen-Yu Tsai]

Some Allwinner SoCs have two display pipelines (frontend -> backend ->
tcon).

Previously we only supported one pipeline. This patch extends the
current driver to support two. It extends the tcon and backend pointers
in sun4i_drv into arrays, and makes the related bind functions store
the pointer into said arrays based on the id fetched from the device
tree. In the case of the tcons, it falls back to a first come order
if no encoders that can be used for differentiating the tcons are
defined. The driver's depth-first traversal of the of graph, coupled
with the increasing address ordering of the of graph endpoints, and
the fact that tcon0 should always be enabled for the tcon/encoder
mux to be accessible, means that tcon1 would always come after tcon0.

Assignment of the device structure into sun4i_drv is moved to the end
of the bind function, when all possible error checks have passed.

This patch also drops a trailing 0 in one of the backend probe messages.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_backend.c |  9 +++++++--
 drivers/gpu/drm/sun4i/sun4i_drv.c     |  2 +-
 drivers/gpu/drm/sun4i/sun4i_drv.h     |  6 ++++--
 drivers/gpu/drm/sun4i/sun4i_tcon.c    | 25 +++++++++++++++++--------
 4 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index f3c92d54c8e4..8d22efd5a9cc 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -350,12 +350,15 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
 	if (!backend)
 		return -ENOMEM;
 	dev_set_drvdata(dev, backend);
-	drv->backend = backend;
 
 	backend->id = sun4i_backend_of_get_id(dev->of_node);
 	if (backend->id < 0)
 		return backend->id;
 
+	/* We only support SUN4I_DRM_MAX_PIPELINES number of backends */
+	if (backend->id >= SUN4I_DRM_MAX_PIPELINES)
+		return -EINVAL;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	regs = devm_ioremap_resource(dev, res);
 	if (IS_ERR(regs))
@@ -364,7 +367,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
 	backend->regs = devm_regmap_init_mmio(dev, regs,
 					      &sun4i_backend_regmap_config);
 	if (IS_ERR(backend->regs)) {
-		dev_err(dev, "Couldn't create the backend0 regmap\n");
+		dev_err(dev, "Couldn't create the backend regmap\n");
 		return PTR_ERR(backend->regs);
 	}
 
@@ -413,6 +416,8 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
 		}
 	}
 
+	drv->backend[backend->id] = backend;
+
 	/* Reset the registers */
 	for (i = 0x800; i < 0x1000; i += 4)
 		regmap_write(backend->regs, i, 0);
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 767bbadcc85d..c15ecb8343d7 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -271,7 +271,7 @@ static int sun4i_drv_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	int i, count = 0;
 
-	for (i = 0;; i++) {
+	for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++) {
 		struct device_node *pipeline = of_parse_phandle(np,
 								"allwinner,pipelines",
 								i);
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
index 5df50126ff52..ec1c08af47e1 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.h
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
@@ -16,9 +16,11 @@
 #include <linux/clk.h>
 #include <linux/regmap.h>
 
+#define SUN4I_DRM_MAX_PIPELINES		2
+
 struct sun4i_drv {
-	struct sun4i_backend	*backend;
-	struct sun4i_tcon	*tcon;
+	struct sun4i_backend	*backend[SUN4I_DRM_MAX_PIPELINES];
+	struct sun4i_tcon	*tcon[SUN4I_DRM_MAX_PIPELINES];
 
 	struct drm_fbdev_cma	*fbdev;
 };
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index b774c9a50c55..7749c3133f38 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -532,7 +532,6 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	if (!tcon)
 		return -ENOMEM;
 	dev_set_drvdata(dev, tcon);
-	drv->tcon = tcon;
 	tcon->drm = drm;
 	tcon->dev = dev;
 	tcon->quirks = of_device_get_match_data(dev);
@@ -540,14 +539,22 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	/* This can fail if the DT does not have any downstream encoders. */
 	tcon->id = sun4i_tcon_of_get_id(dev->of_node);
 	if (tcon->id < 0) {
-		/*
-		 * TODO We currently support only 1 TCON, so we can
-		 * safely set this to 0. This should be revisited
-		 * when we add support for multiple pipelines.
-		 */
-		tcon->id = 0;
+		int i;
+
+		/* find the first empty tcon in sun4i_drv */
+		for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++)
+			if (!drv->tcon[i])
+				tcon->id = i;
+
+		/* bail out if that failed */
+		if (tcon->id < 0)
+			return tcon->id;
 	}
 
+	/* We only support SUN4I_DRM_MAX_PIPELINES number of tcons */
+	if (tcon->id >= SUN4I_DRM_MAX_PIPELINES)
+		return -EINVAL;
+
 	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");
@@ -588,7 +595,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 		goto err_free_dotclock;
 	}
 
-	tcon->crtc = sun4i_crtc_init(drm, drv->backend, tcon);
+	tcon->crtc = sun4i_crtc_init(drm, drv->backend[tcon->id], tcon);
 	if (IS_ERR(tcon->crtc)) {
 		dev_err(dev, "Couldn't create our CRTC\n");
 		ret = PTR_ERR(tcon->crtc);
@@ -599,6 +606,8 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	if (ret < 0)
 		goto err_free_clocks;
 
+	drv->tcon[tcon->id] = tcon;
+
 	return 0;
 
 err_free_dotclock:
-- 
2.11.0

[PATCH 10/11] ARM: dts: sun6i: Add second display pipeline device nodes

[Chen-Yu Tsai]

The Allwinner A31/A31s SoCs have 2 display pipelines, as in 2 display
frontends, backends, and tcons each. The relationship between the
backends and tcons are 1:1, but the frontends can feed either backend.

Add device nodes and of graph nodes describing this relationship.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 168 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 167 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index a4b96184cac1..40f020824ccc 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -233,7 +233,7 @@
 
 	de: display-engine {
 		compatible = "allwinner,sun6i-a31-display-engine";
-		allwinner,pipelines = <&fe0>;
+		allwinner,pipelines = <&fe0>, <&fe1>;
 		status = "disabled";
 	};
 
@@ -290,6 +290,43 @@
 			};
 		};
 
+		tcon1: lcd-controller@01c0d000 {
+			compatible = "allwinner,sun6i-a31-tcon";
+			reg = <0x01c0d000 0x1000>;
+			interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+			resets = <&ccu RST_AHB1_LCD1>;
+			reset-names = "lcd";
+			clocks = <&ccu CLK_AHB1_LCD1>,
+				 <&ccu CLK_LCD1_CH0>,
+				 <&ccu CLK_LCD1_CH1>;
+			clock-names = "ahb",
+				      "tcon-ch0",
+				      "tcon-ch1";
+			clock-output-names = "tcon1-pixel-clock";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				tcon1_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					tcon1_in_drc1: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&drc1_out_tcon1>;
+					};
+				};
+
+				tcon1_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+				};
+			};
+		};
+
 		mmc0: mmc@01c0f000 {
 			compatible = "allwinner,sun7i-a20-mmc";
 			reg = <0x01c0f000 0x1000>;
@@ -897,6 +934,130 @@
 						reg = <0>;
 						remote-endpoint = <&be0_in_fe0>;
 					};
+
+					fe0_out_be1: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&be1_in_fe0>;
+					};
+				};
+			};
+		};
+
+		fe1: display-frontend@01e20000 {
+			compatible = "allwinner,sun6i-a31-display-frontend";
+			reg = <0x01e20000 0x20000>;
+			interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_AHB1_FE1>, <&ccu CLK_FE1>,
+				 <&ccu CLK_DRAM_FE1>;
+			clock-names = "ahb", "mod",
+				      "ram";
+			resets = <&ccu RST_AHB1_FE1>;
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				fe1_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					fe1_out_be0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&be0_in_fe1>;
+					};
+
+					fe1_out_be1: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&be1_in_fe1>;
+					};
+				};
+			};
+		};
+
+		be1: display-backend@01e40000 {
+			compatible = "allwinner,sun6i-a31-display-backend";
+			reg = <0x01e40000 0x10000>;
+			interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_AHB1_BE1>, <&ccu CLK_BE1>,
+				 <&ccu CLK_DRAM_BE1>;
+			clock-names = "ahb", "mod",
+				      "ram";
+			resets = <&ccu RST_AHB1_BE1>;
+
+			assigned-clocks = <&ccu CLK_BE1>;
+			assigned-clock-rates = <300000000>;
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				be1_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					be1_in_fe0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&fe0_out_be1>;
+					};
+
+					be1_in_fe1: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&fe1_out_be1>;
+					};
+				};
+
+				be1_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					be1_out_drc1: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&drc1_in_be1>;
+					};
+				};
+			};
+		};
+
+		drc1: drc@01e50000 {
+			compatible = "allwinner,sun6i-a31-drc";
+			reg = <0x01e50000 0x10000>;
+			interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_AHB1_DRC1>, <&ccu CLK_IEP_DRC1>,
+				 <&ccu CLK_DRAM_DRC1>;
+			clock-names = "ahb", "mod",
+				      "ram";
+			resets = <&ccu RST_AHB1_DRC1>;
+
+			assigned-clocks = <&ccu CLK_IEP_DRC1>;
+			assigned-clock-rates = <300000000>;
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				drc1_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					drc1_in_be1: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&be1_out_drc1>;
+					};
+				};
+
+				drc1_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					drc1_out_tcon1: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&tcon1_in_drc1>;
+					};
 				};
 			};
 		};
@@ -927,6 +1088,11 @@
 						reg = <0>;
 						remote-endpoint = <&fe0_out_be0>;
 					};
+
+					be0_in_fe1: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&fe1_out_be0>;
+					};
 				};
 
 				be0_out: port@1 {
-- 
2.11.0

[PATCH 11/11] ARM: dts: sun6i: Enable tcon0 by default

[Chen-Yu Tsai]

tcon0 contains a muxing register used to mux tcon output to downstream
hdmi or mipi dsi encoders. tcon0 must be available for the mux to be
configured.

Whether the display subsystem is enabled or not is now solely controlled
by the display-engine node.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 1 -
 arch/arm/boot/dts/sun6i-a31.dtsi            | 1 -
 2 files changed, 2 deletions(-)

diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts
index f094eeb6c499..fb237ce60198 100644
--- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts
+++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts
@@ -320,7 +320,6 @@
 &tcon0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&lcd0_rgb888_pins>;
-	status = "okay";
 };
 
 &tcon0_out {
diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 40f020824ccc..0ddcffab594e 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -265,7 +265,6 @@
 				      "tcon-ch0",
 				      "tcon-ch1";
 			clock-output-names = "tcon0-pixel-clock";
-			status = "disabled";
 
 			ports {
 				#address-cells = <1>;
-- 
2.11.0

Re: [PATCH 00/11] drm/sun4i: Support two display pipelines

[Maxime Ripard]

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

On Thu, Mar 09, 2017 at 06:05:23PM +0800, Chen-Yu Tsai wrote:
> Hi Maxime,
> 
> This is part 3 of my sun4i drm clean up series. In this part support
> for 2 display pipelines is added, after some more code cleanups and
> restructuring.
> 
> While this series enables the second display pipeline, there's no
> usable output at the moment. For the A31, the second TCON's panel
> interface uses the same pins as the Ethernet controller. However
> Ethernet is used on most boards. We will have to wait for HDMI
> support to actually use it.
> 
> Patch 1 fixes the TCON's clock and regmap initialization sequence,
> splitting out the dot clock init part till after the regmap.
> 
> Patch 2 fixes a comment spotted while reviewing Maxime's HDMI patches.
> 
> Patch 3 makes the crtc init code use the tcon pointer embedded in the
> crtc structure, instead of the sun4i_drv structure, to get the tcon's
> output port node. This should have been a part of the last batch of
> patches.
> 
> Patch 4 makes the tv encoder code get the tcon and backend pointers
> from its attached crtc.
> 
> Patch 5 makes the crtc init function take tcon and backend pointers.
> 
> Patch 6 makes the layer init functions take a backend pointer.

Applied the patches up to this one, see my comments for the rest of
the patches.

Thanks!
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 09/11] drm/sun4i: Support two display pipelines

[Maxime Ripard]

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

Hi,

On Thu, Mar 09, 2017 at 06:05:32PM +0800, Chen-Yu Tsai wrote:
> Some Allwinner SoCs have two display pipelines (frontend -> backend ->
> tcon).
> 
> Previously we only supported one pipeline. This patch extends the
> current driver to support two. It extends the tcon and backend pointers
> in sun4i_drv into arrays, and makes the related bind functions store
> the pointer into said arrays based on the id fetched from the device
> tree. In the case of the tcons, it falls back to a first come order
> if no encoders that can be used for differentiating the tcons are
> defined. The driver's depth-first traversal of the of graph, coupled
> with the increasing address ordering of the of graph endpoints, and
> the fact that tcon0 should always be enabled for the tcon/encoder
> mux to be accessible, means that tcon1 would always come after tcon0.
> 
> Assignment of the device structure into sun4i_drv is moved to the end
> of the bind function, when all possible error checks have passed.
> 
> This patch also drops a trailing 0 in one of the backend probe messages.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/gpu/drm/sun4i/sun4i_backend.c |  9 +++++++--
>  drivers/gpu/drm/sun4i/sun4i_drv.c     |  2 +-
>  drivers/gpu/drm/sun4i/sun4i_drv.h     |  6 ++++--
>  drivers/gpu/drm/sun4i/sun4i_tcon.c    | 25 +++++++++++++++++--------
>  4 files changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index f3c92d54c8e4..8d22efd5a9cc 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -350,12 +350,15 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>  	if (!backend)
>  		return -ENOMEM;
>  	dev_set_drvdata(dev, backend);
> -	drv->backend = backend;
>  
>  	backend->id = sun4i_backend_of_get_id(dev->of_node);
>  	if (backend->id < 0)
>  		return backend->id;
>  
> +	/* We only support SUN4I_DRM_MAX_PIPELINES number of backends */
> +	if (backend->id >= SUN4I_DRM_MAX_PIPELINES)
> +		return -EINVAL;
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	regs = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(regs))
> @@ -364,7 +367,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>  	backend->regs = devm_regmap_init_mmio(dev, regs,
>  					      &sun4i_backend_regmap_config);
>  	if (IS_ERR(backend->regs)) {
> -		dev_err(dev, "Couldn't create the backend0 regmap\n");
> +		dev_err(dev, "Couldn't create the backend regmap\n");
>  		return PTR_ERR(backend->regs);
>  	}
>  
> @@ -413,6 +416,8 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>  		}
>  	}
>  
> +	drv->backend[backend->id] = backend;
> +
>  	/* Reset the registers */
>  	for (i = 0x800; i < 0x1000; i += 4)
>  		regmap_write(backend->regs, i, 0);
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index 767bbadcc85d..c15ecb8343d7 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -271,7 +271,7 @@ static int sun4i_drv_probe(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	int i, count = 0;
>  
> -	for (i = 0;; i++) {
> +	for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++) {
>  		struct device_node *pipeline = of_parse_phandle(np,
>  								"allwinner,pipelines",
>  								i);
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
> index 5df50126ff52..ec1c08af47e1 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
> @@ -16,9 +16,11 @@
>  #include <linux/clk.h>
>  #include <linux/regmap.h>
>  
> +#define SUN4I_DRM_MAX_PIPELINES		2
> +
>  struct sun4i_drv {
> -	struct sun4i_backend	*backend;
> -	struct sun4i_tcon	*tcon;
> +	struct sun4i_backend	*backend[SUN4I_DRM_MAX_PIPELINES];
> +	struct sun4i_tcon	*tcon[SUN4I_DRM_MAX_PIPELINES];
>  
>  	struct drm_fbdev_cma	*fbdev;
>  };
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index b774c9a50c55..7749c3133f38 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -532,7 +532,6 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>  	if (!tcon)
>  		return -ENOMEM;
>  	dev_set_drvdata(dev, tcon);
> -	drv->tcon = tcon;
>  	tcon->drm = drm;
>  	tcon->dev = dev;
>  	tcon->quirks = of_device_get_match_data(dev);
> @@ -540,14 +539,22 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>  	/* This can fail if the DT does not have any downstream encoders. */
>  	tcon->id = sun4i_tcon_of_get_id(dev->of_node);
>  	if (tcon->id < 0) {
> -		/*
> -		 * TODO We currently support only 1 TCON, so we can
> -		 * safely set this to 0. This should be revisited
> -		 * when we add support for multiple pipelines.
> -		 */
> -		tcon->id = 0;
> +		int i;
> +
> +		/* find the first empty tcon in sun4i_drv */
> +		for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++)
> +			if (!drv->tcon[i])
> +				tcon->id = i;
> +
> +		/* bail out if that failed */
> +		if (tcon->id < 0)
> +			return tcon->id;
>  	}
>  
> +	/* We only support SUN4I_DRM_MAX_PIPELINES number of tcons */
> +	if (tcon->id >= SUN4I_DRM_MAX_PIPELINES)
> +		return -EINVAL;
> +
>  	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");
> @@ -588,7 +595,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>  		goto err_free_dotclock;
>  	}
>  
> -	tcon->crtc = sun4i_crtc_init(drm, drv->backend, tcon);
> +	tcon->crtc = sun4i_crtc_init(drm, drv->backend[tcon->id], tcon);

I'm not a big fan of those IDs. The heuristic seems to be a bit
fragile since we really never enforced any order in our bindings.

You seem to use it for two things:
 - to match a TCON to its backend in our code
 - to not step on each others' toes when registering the backends/tcons

I think the second could be easily addressed using a linked list, and
the first one by storing the of_node. Then we just need to follow the
OF graph to our input of_node, and then iterate through our registered
backend list to find the one with the same of_node.

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 09/11] drm/sun4i: Support two display pipelines

[Chen-Yu Tsai]

On Thu, Mar 9, 2017 at 6:36 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Thu, Mar 09, 2017 at 06:05:32PM +0800, Chen-Yu Tsai wrote:
>> Some Allwinner SoCs have two display pipelines (frontend -> backend ->
>> tcon).
>>
>> Previously we only supported one pipeline. This patch extends the
>> current driver to support two. It extends the tcon and backend pointers
>> in sun4i_drv into arrays, and makes the related bind functions store
>> the pointer into said arrays based on the id fetched from the device
>> tree. In the case of the tcons, it falls back to a first come order
>> if no encoders that can be used for differentiating the tcons are
>> defined. The driver's depth-first traversal of the of graph, coupled
>> with the increasing address ordering of the of graph endpoints, and
>> the fact that tcon0 should always be enabled for the tcon/encoder
>> mux to be accessible, means that tcon1 would always come after tcon0.
>>
>> Assignment of the device structure into sun4i_drv is moved to the end
>> of the bind function, when all possible error checks have passed.
>>
>> This patch also drops a trailing 0 in one of the backend probe messages.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>  drivers/gpu/drm/sun4i/sun4i_backend.c |  9 +++++++--
>>  drivers/gpu/drm/sun4i/sun4i_drv.c     |  2 +-
>>  drivers/gpu/drm/sun4i/sun4i_drv.h     |  6 ++++--
>>  drivers/gpu/drm/sun4i/sun4i_tcon.c    | 25 +++++++++++++++++--------
>>  4 files changed, 29 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> index f3c92d54c8e4..8d22efd5a9cc 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> @@ -350,12 +350,15 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>>       if (!backend)
>>               return -ENOMEM;
>>       dev_set_drvdata(dev, backend);
>> -     drv->backend = backend;
>>
>>       backend->id = sun4i_backend_of_get_id(dev->of_node);
>>       if (backend->id < 0)
>>               return backend->id;
>>
>> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of backends */
>> +     if (backend->id >= SUN4I_DRM_MAX_PIPELINES)
>> +             return -EINVAL;
>> +
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       regs = devm_ioremap_resource(dev, res);
>>       if (IS_ERR(regs))
>> @@ -364,7 +367,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>>       backend->regs = devm_regmap_init_mmio(dev, regs,
>>                                             &sun4i_backend_regmap_config);
>>       if (IS_ERR(backend->regs)) {
>> -             dev_err(dev, "Couldn't create the backend0 regmap\n");
>> +             dev_err(dev, "Couldn't create the backend regmap\n");
>>               return PTR_ERR(backend->regs);
>>       }
>>
>> @@ -413,6 +416,8 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>>               }
>>       }
>>
>> +     drv->backend[backend->id] = backend;
>> +
>>       /* Reset the registers */
>>       for (i = 0x800; i < 0x1000; i += 4)
>>               regmap_write(backend->regs, i, 0);
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> index 767bbadcc85d..c15ecb8343d7 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> @@ -271,7 +271,7 @@ static int sun4i_drv_probe(struct platform_device *pdev)
>>       struct device_node *np = pdev->dev.of_node;
>>       int i, count = 0;
>>
>> -     for (i = 0;; i++) {
>> +     for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++) {
>>               struct device_node *pipeline = of_parse_phandle(np,
>>                                                               "allwinner,pipelines",
>>                                                               i);
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
>> index 5df50126ff52..ec1c08af47e1 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
>> @@ -16,9 +16,11 @@
>>  #include <linux/clk.h>
>>  #include <linux/regmap.h>
>>
>> +#define SUN4I_DRM_MAX_PIPELINES              2
>> +
>>  struct sun4i_drv {
>> -     struct sun4i_backend    *backend;
>> -     struct sun4i_tcon       *tcon;
>> +     struct sun4i_backend    *backend[SUN4I_DRM_MAX_PIPELINES];
>> +     struct sun4i_tcon       *tcon[SUN4I_DRM_MAX_PIPELINES];
>>
>>       struct drm_fbdev_cma    *fbdev;
>>  };
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index b774c9a50c55..7749c3133f38 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -532,7 +532,6 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>>       if (!tcon)
>>               return -ENOMEM;
>>       dev_set_drvdata(dev, tcon);
>> -     drv->tcon = tcon;
>>       tcon->drm = drm;
>>       tcon->dev = dev;
>>       tcon->quirks = of_device_get_match_data(dev);
>> @@ -540,14 +539,22 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>>       /* This can fail if the DT does not have any downstream encoders. */
>>       tcon->id = sun4i_tcon_of_get_id(dev->of_node);
>>       if (tcon->id < 0) {
>> -             /*
>> -              * TODO We currently support only 1 TCON, so we can
>> -              * safely set this to 0. This should be revisited
>> -              * when we add support for multiple pipelines.
>> -              */
>> -             tcon->id = 0;
>> +             int i;
>> +
>> +             /* find the first empty tcon in sun4i_drv */
>> +             for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++)
>> +                     if (!drv->tcon[i])
>> +                             tcon->id = i;
>> +
>> +             /* bail out if that failed */
>> +             if (tcon->id < 0)
>> +                     return tcon->id;
>>       }
>>
>> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of tcons */
>> +     if (tcon->id >= SUN4I_DRM_MAX_PIPELINES)
>> +             return -EINVAL;
>> +
>>       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");
>> @@ -588,7 +595,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>>               goto err_free_dotclock;
>>       }
>>
>> -     tcon->crtc = sun4i_crtc_init(drm, drv->backend, tcon);
>> +     tcon->crtc = sun4i_crtc_init(drm, drv->backend[tcon->id], tcon);
>
> I'm not a big fan of those IDs. The heuristic seems to be a bit
> fragile since we really never enforced any order in our bindings.

Yes. I seem to have forgotten that bit which I had intended to add.
The endpoint IDs would ideally match the actual mux values used.

On the TCON side that's not doable, so we might have to just require
them being in an increasing order.

> You seem to use it for two things:
>  - to match a TCON to its backend in our code
>  - to not step on each others' toes when registering the backends/tcons
>
> I think the second could be easily addressed using a linked list, and
> the first one by storing the of_node. Then we just need to follow the
> OF graph to our input of_node, and then iterate through our registered
> backend list to find the one with the same of_node.

Yes that would work for the above purposes.

For getting the first TCON to access the mux registers, it kind of falls
short. We also need something like this for the TV encoders on A10/A20.
They too have a mux, which seems to be in the first TV encoder. This
controls how the 8 DACs are mapped to the 4 external pins.

Regards
ChenYu

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

Re: [PATCH 09/11] drm/sun4i: Support two display pipelines

[Maxime Ripard]

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

On Thu, Mar 09, 2017 at 07:20:30PM +0800, Chen-Yu Tsai wrote:
> On Thu, Mar 9, 2017 at 6:36 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > On Thu, Mar 09, 2017 at 06:05:32PM +0800, Chen-Yu Tsai wrote:
> >> Some Allwinner SoCs have two display pipelines (frontend -> backend ->
> >> tcon).
> >>
> >> Previously we only supported one pipeline. This patch extends the
> >> current driver to support two. It extends the tcon and backend pointers
> >> in sun4i_drv into arrays, and makes the related bind functions store
> >> the pointer into said arrays based on the id fetched from the device
> >> tree. In the case of the tcons, it falls back to a first come order
> >> if no encoders that can be used for differentiating the tcons are
> >> defined. The driver's depth-first traversal of the of graph, coupled
> >> with the increasing address ordering of the of graph endpoints, and
> >> the fact that tcon0 should always be enabled for the tcon/encoder
> >> mux to be accessible, means that tcon1 would always come after tcon0.
> >>
> >> Assignment of the device structure into sun4i_drv is moved to the end
> >> of the bind function, when all possible error checks have passed.
> >>
> >> This patch also drops a trailing 0 in one of the backend probe messages.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >> ---
> >>  drivers/gpu/drm/sun4i/sun4i_backend.c |  9 +++++++--
> >>  drivers/gpu/drm/sun4i/sun4i_drv.c     |  2 +-
> >>  drivers/gpu/drm/sun4i/sun4i_drv.h     |  6 ++++--
> >>  drivers/gpu/drm/sun4i/sun4i_tcon.c    | 25 +++++++++++++++++--------
> >>  4 files changed, 29 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> >> index f3c92d54c8e4..8d22efd5a9cc 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> >> @@ -350,12 +350,15 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
> >>       if (!backend)
> >>               return -ENOMEM;
> >>       dev_set_drvdata(dev, backend);
> >> -     drv->backend = backend;
> >>
> >>       backend->id = sun4i_backend_of_get_id(dev->of_node);
> >>       if (backend->id < 0)
> >>               return backend->id;
> >>
> >> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of backends */
> >> +     if (backend->id >= SUN4I_DRM_MAX_PIPELINES)
> >> +             return -EINVAL;
> >> +
> >>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>       regs = devm_ioremap_resource(dev, res);
> >>       if (IS_ERR(regs))
> >> @@ -364,7 +367,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
> >>       backend->regs = devm_regmap_init_mmio(dev, regs,
> >>                                             &sun4i_backend_regmap_config);
> >>       if (IS_ERR(backend->regs)) {
> >> -             dev_err(dev, "Couldn't create the backend0 regmap\n");
> >> +             dev_err(dev, "Couldn't create the backend regmap\n");
> >>               return PTR_ERR(backend->regs);
> >>       }
> >>
> >> @@ -413,6 +416,8 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
> >>               }
> >>       }
> >>
> >> +     drv->backend[backend->id] = backend;
> >> +
> >>       /* Reset the registers */
> >>       for (i = 0x800; i < 0x1000; i += 4)
> >>               regmap_write(backend->regs, i, 0);
> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> >> index 767bbadcc85d..c15ecb8343d7 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> >> @@ -271,7 +271,7 @@ static int sun4i_drv_probe(struct platform_device *pdev)
> >>       struct device_node *np = pdev->dev.of_node;
> >>       int i, count = 0;
> >>
> >> -     for (i = 0;; i++) {
> >> +     for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++) {
> >>               struct device_node *pipeline = of_parse_phandle(np,
> >>                                                               "allwinner,pipelines",
> >>                                                               i);
> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
> >> index 5df50126ff52..ec1c08af47e1 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
> >> @@ -16,9 +16,11 @@
> >>  #include <linux/clk.h>
> >>  #include <linux/regmap.h>
> >>
> >> +#define SUN4I_DRM_MAX_PIPELINES              2
> >> +
> >>  struct sun4i_drv {
> >> -     struct sun4i_backend    *backend;
> >> -     struct sun4i_tcon       *tcon;
> >> +     struct sun4i_backend    *backend[SUN4I_DRM_MAX_PIPELINES];
> >> +     struct sun4i_tcon       *tcon[SUN4I_DRM_MAX_PIPELINES];
> >>
> >>       struct drm_fbdev_cma    *fbdev;
> >>  };
> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> index b774c9a50c55..7749c3133f38 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> @@ -532,7 +532,6 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
> >>       if (!tcon)
> >>               return -ENOMEM;
> >>       dev_set_drvdata(dev, tcon);
> >> -     drv->tcon = tcon;
> >>       tcon->drm = drm;
> >>       tcon->dev = dev;
> >>       tcon->quirks = of_device_get_match_data(dev);
> >> @@ -540,14 +539,22 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
> >>       /* This can fail if the DT does not have any downstream encoders. */
> >>       tcon->id = sun4i_tcon_of_get_id(dev->of_node);
> >>       if (tcon->id < 0) {
> >> -             /*
> >> -              * TODO We currently support only 1 TCON, so we can
> >> -              * safely set this to 0. This should be revisited
> >> -              * when we add support for multiple pipelines.
> >> -              */
> >> -             tcon->id = 0;
> >> +             int i;
> >> +
> >> +             /* find the first empty tcon in sun4i_drv */
> >> +             for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++)
> >> +                     if (!drv->tcon[i])
> >> +                             tcon->id = i;
> >> +
> >> +             /* bail out if that failed */
> >> +             if (tcon->id < 0)
> >> +                     return tcon->id;
> >>       }
> >>
> >> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of tcons */
> >> +     if (tcon->id >= SUN4I_DRM_MAX_PIPELINES)
> >> +             return -EINVAL;
> >> +
> >>       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");
> >> @@ -588,7 +595,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
> >>               goto err_free_dotclock;
> >>       }
> >>
> >> -     tcon->crtc = sun4i_crtc_init(drm, drv->backend, tcon);
> >> +     tcon->crtc = sun4i_crtc_init(drm, drv->backend[tcon->id], tcon);
> >
> > I'm not a big fan of those IDs. The heuristic seems to be a bit
> > fragile since we really never enforced any order in our bindings.
> 
> Yes. I seem to have forgotten that bit which I had intended to add.
> The endpoint IDs would ideally match the actual mux values used.

That works for me, but the binding documentation would need to be
amended.

> On the TCON side that's not doable, so we might have to just require
> them being in an increasing order.

What are you planning to use those IDs on for the TCON?

> 
> > You seem to use it for two things:
> >  - to match a TCON to its backend in our code
> >  - to not step on each others' toes when registering the backends/tcons
> >
> > I think the second could be easily addressed using a linked list, and
> > the first one by storing the of_node. Then we just need to follow the
> > OF graph to our input of_node, and then iterate through our registered
> > backend list to find the one with the same of_node.
> 
> Yes that would work for the above purposes.
> 
> For getting the first TCON to access the mux registers, it kind of falls
> short. We also need something like this for the TV encoders on A10/A20.
> They too have a mux, which seems to be in the first TV encoder. This
> controls how the 8 DACs are mapped to the 4 external pins.

How are those pins muxed between the two? Can't we just create a
connector that would be usable for both encoders?

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 09/11] drm/sun4i: Support two display pipelines

[Chen-Yu Tsai]

Hi,

On Thu, Mar 9, 2017 at 10:40 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Mar 09, 2017 at 07:20:30PM +0800, Chen-Yu Tsai wrote:
>> On Thu, Mar 9, 2017 at 6:36 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > Hi,
>> >
>> > On Thu, Mar 09, 2017 at 06:05:32PM +0800, Chen-Yu Tsai wrote:
>> >> Some Allwinner SoCs have two display pipelines (frontend -> backend ->
>> >> tcon).
>> >>
>> >> Previously we only supported one pipeline. This patch extends the
>> >> current driver to support two. It extends the tcon and backend pointers
>> >> in sun4i_drv into arrays, and makes the related bind functions store
>> >> the pointer into said arrays based on the id fetched from the device
>> >> tree. In the case of the tcons, it falls back to a first come order
>> >> if no encoders that can be used for differentiating the tcons are
>> >> defined. The driver's depth-first traversal of the of graph, coupled
>> >> with the increasing address ordering of the of graph endpoints, and
>> >> the fact that tcon0 should always be enabled for the tcon/encoder
>> >> mux to be accessible, means that tcon1 would always come after tcon0.
>> >>
>> >> Assignment of the device structure into sun4i_drv is moved to the end
>> >> of the bind function, when all possible error checks have passed.
>> >>
>> >> This patch also drops a trailing 0 in one of the backend probe messages.
>> >>
>> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> >> ---
>> >>  drivers/gpu/drm/sun4i/sun4i_backend.c |  9 +++++++--
>> >>  drivers/gpu/drm/sun4i/sun4i_drv.c     |  2 +-
>> >>  drivers/gpu/drm/sun4i/sun4i_drv.h     |  6 ++++--
>> >>  drivers/gpu/drm/sun4i/sun4i_tcon.c    | 25 +++++++++++++++++--------
>> >>  4 files changed, 29 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> >> index f3c92d54c8e4..8d22efd5a9cc 100644
>> >> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
>> >> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> >> @@ -350,12 +350,15 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>> >>       if (!backend)
>> >>               return -ENOMEM;
>> >>       dev_set_drvdata(dev, backend);
>> >> -     drv->backend = backend;
>> >>
>> >>       backend->id = sun4i_backend_of_get_id(dev->of_node);
>> >>       if (backend->id < 0)
>> >>               return backend->id;
>> >>
>> >> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of backends */
>> >> +     if (backend->id >= SUN4I_DRM_MAX_PIPELINES)
>> >> +             return -EINVAL;
>> >> +
>> >>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >>       regs = devm_ioremap_resource(dev, res);
>> >>       if (IS_ERR(regs))
>> >> @@ -364,7 +367,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>> >>       backend->regs = devm_regmap_init_mmio(dev, regs,
>> >>                                             &sun4i_backend_regmap_config);
>> >>       if (IS_ERR(backend->regs)) {
>> >> -             dev_err(dev, "Couldn't create the backend0 regmap\n");
>> >> +             dev_err(dev, "Couldn't create the backend regmap\n");
>> >>               return PTR_ERR(backend->regs);
>> >>       }
>> >>
>> >> @@ -413,6 +416,8 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>> >>               }
>> >>       }
>> >>
>> >> +     drv->backend[backend->id] = backend;
>> >> +
>> >>       /* Reset the registers */
>> >>       for (i = 0x800; i < 0x1000; i += 4)
>> >>               regmap_write(backend->regs, i, 0);
>> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> >> index 767bbadcc85d..c15ecb8343d7 100644
>> >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
>> >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> >> @@ -271,7 +271,7 @@ static int sun4i_drv_probe(struct platform_device *pdev)
>> >>       struct device_node *np = pdev->dev.of_node;
>> >>       int i, count = 0;
>> >>
>> >> -     for (i = 0;; i++) {
>> >> +     for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++) {
>> >>               struct device_node *pipeline = of_parse_phandle(np,
>> >>                                                               "allwinner,pipelines",
>> >>                                                               i);
>> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
>> >> index 5df50126ff52..ec1c08af47e1 100644
>> >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
>> >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
>> >> @@ -16,9 +16,11 @@
>> >>  #include <linux/clk.h>
>> >>  #include <linux/regmap.h>
>> >>
>> >> +#define SUN4I_DRM_MAX_PIPELINES              2
>> >> +
>> >>  struct sun4i_drv {
>> >> -     struct sun4i_backend    *backend;
>> >> -     struct sun4i_tcon       *tcon;
>> >> +     struct sun4i_backend    *backend[SUN4I_DRM_MAX_PIPELINES];
>> >> +     struct sun4i_tcon       *tcon[SUN4I_DRM_MAX_PIPELINES];
>> >>
>> >>       struct drm_fbdev_cma    *fbdev;
>> >>  };
>> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> >> index b774c9a50c55..7749c3133f38 100644
>> >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> >> @@ -532,7 +532,6 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>> >>       if (!tcon)
>> >>               return -ENOMEM;
>> >>       dev_set_drvdata(dev, tcon);
>> >> -     drv->tcon = tcon;
>> >>       tcon->drm = drm;
>> >>       tcon->dev = dev;
>> >>       tcon->quirks = of_device_get_match_data(dev);
>> >> @@ -540,14 +539,22 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>> >>       /* This can fail if the DT does not have any downstream encoders. */
>> >>       tcon->id = sun4i_tcon_of_get_id(dev->of_node);
>> >>       if (tcon->id < 0) {
>> >> -             /*
>> >> -              * TODO We currently support only 1 TCON, so we can
>> >> -              * safely set this to 0. This should be revisited
>> >> -              * when we add support for multiple pipelines.
>> >> -              */
>> >> -             tcon->id = 0;
>> >> +             int i;
>> >> +
>> >> +             /* find the first empty tcon in sun4i_drv */
>> >> +             for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++)
>> >> +                     if (!drv->tcon[i])
>> >> +                             tcon->id = i;
>> >> +
>> >> +             /* bail out if that failed */
>> >> +             if (tcon->id < 0)
>> >> +                     return tcon->id;
>> >>       }
>> >>
>> >> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of tcons */
>> >> +     if (tcon->id >= SUN4I_DRM_MAX_PIPELINES)
>> >> +             return -EINVAL;
>> >> +
>> >>       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");
>> >> @@ -588,7 +595,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>> >>               goto err_free_dotclock;
>> >>       }
>> >>
>> >> -     tcon->crtc = sun4i_crtc_init(drm, drv->backend, tcon);
>> >> +     tcon->crtc = sun4i_crtc_init(drm, drv->backend[tcon->id], tcon);
>> >
>> > I'm not a big fan of those IDs. The heuristic seems to be a bit
>> > fragile since we really never enforced any order in our bindings.
>>
>> Yes. I seem to have forgotten that bit which I had intended to add.
>> The endpoint IDs would ideally match the actual mux values used.
>
> That works for me, but the binding documentation would need to be
> amended.
>
>> On the TCON side that's not doable, so we might have to just require
>> them being in an increasing order.
>
> What are you planning to use those IDs on for the TCON?

This reply sat in my draft box for way too long.

As mentioned, the IDs on the TCON side are for the IDing the TV encoders.
I think having the IDs for the same type of encoder be increasing, such
that the endpoint for TV encoder 1 has a higher ID than the one for TV
encoder 0 should be enough. There aren't any other instances where we
have 2 or more of the same type. Or we could just add some kind of index
property to the TV encoder node. This is for the A20 by the way.

>>
>> > You seem to use it for two things:
>> >  - to match a TCON to its backend in our code
>> >  - to not step on each others' toes when registering the backends/tcons
>> >
>> > I think the second could be easily addressed using a linked list, and
>> > the first one by storing the of_node. Then we just need to follow the
>> > OF graph to our input of_node, and then iterate through our registered
>> > backend list to find the one with the same of_node.
>>
>> Yes that would work for the above purposes.
>>
>> For getting the first TCON to access the mux registers, it kind of falls
>> short. We also need something like this for the TV encoders on A10/A20.
>> They too have a mux, which seems to be in the first TV encoder. This
>> controls how the 8 DACs are mapped to the 4 external pins.
>
> How are those pins muxed between the two? Can't we just create a
> connector that would be usable for both encoders?

They are freely muxable. However if you want VGA output, you need 3 pins
for RGB from the same TV encoder, plus H/V sync from it's upstream TCON.
And you have one pin left that you can use for composite, which would
likely be fed from the other TCON, as you need interlaced YUV data,
which is like the opposite of VGA.

Regards
ChenYu

Re: [PATCH 09/11] drm/sun4i: Support two display pipelines

[Maxime Ripard]

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

Hi Chen-Yu,

On Sat, Apr 08, 2017 at 01:30:55AM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Thu, Mar 9, 2017 at 10:40 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Thu, Mar 09, 2017 at 07:20:30PM +0800, Chen-Yu Tsai wrote:
> >> On Thu, Mar 9, 2017 at 6:36 PM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > Hi,
> >> >
> >> > On Thu, Mar 09, 2017 at 06:05:32PM +0800, Chen-Yu Tsai wrote:
> >> >> Some Allwinner SoCs have two display pipelines (frontend -> backend ->
> >> >> tcon).
> >> >>
> >> >> Previously we only supported one pipeline. This patch extends the
> >> >> current driver to support two. It extends the tcon and backend pointers
> >> >> in sun4i_drv into arrays, and makes the related bind functions store
> >> >> the pointer into said arrays based on the id fetched from the device
> >> >> tree. In the case of the tcons, it falls back to a first come order
> >> >> if no encoders that can be used for differentiating the tcons are
> >> >> defined. The driver's depth-first traversal of the of graph, coupled
> >> >> with the increasing address ordering of the of graph endpoints, and
> >> >> the fact that tcon0 should always be enabled for the tcon/encoder
> >> >> mux to be accessible, means that tcon1 would always come after tcon0.
> >> >>
> >> >> Assignment of the device structure into sun4i_drv is moved to the end
> >> >> of the bind function, when all possible error checks have passed.
> >> >>
> >> >> This patch also drops a trailing 0 in one of the backend probe messages.
> >> >>
> >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >> >> ---
> >> >>  drivers/gpu/drm/sun4i/sun4i_backend.c |  9 +++++++--
> >> >>  drivers/gpu/drm/sun4i/sun4i_drv.c     |  2 +-
> >> >>  drivers/gpu/drm/sun4i/sun4i_drv.h     |  6 ++++--
> >> >>  drivers/gpu/drm/sun4i/sun4i_tcon.c    | 25 +++++++++++++++++--------
> >> >>  4 files changed, 29 insertions(+), 13 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> >> >> index f3c92d54c8e4..8d22efd5a9cc 100644
> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> >> >> @@ -350,12 +350,15 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
> >> >>       if (!backend)
> >> >>               return -ENOMEM;
> >> >>       dev_set_drvdata(dev, backend);
> >> >> -     drv->backend = backend;
> >> >>
> >> >>       backend->id = sun4i_backend_of_get_id(dev->of_node);
> >> >>       if (backend->id < 0)
> >> >>               return backend->id;
> >> >>
> >> >> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of backends */
> >> >> +     if (backend->id >= SUN4I_DRM_MAX_PIPELINES)
> >> >> +             return -EINVAL;
> >> >> +
> >> >>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> >>       regs = devm_ioremap_resource(dev, res);
> >> >>       if (IS_ERR(regs))
> >> >> @@ -364,7 +367,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
> >> >>       backend->regs = devm_regmap_init_mmio(dev, regs,
> >> >>                                             &sun4i_backend_regmap_config);
> >> >>       if (IS_ERR(backend->regs)) {
> >> >> -             dev_err(dev, "Couldn't create the backend0 regmap\n");
> >> >> +             dev_err(dev, "Couldn't create the backend regmap\n");
> >> >>               return PTR_ERR(backend->regs);
> >> >>       }
> >> >>
> >> >> @@ -413,6 +416,8 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
> >> >>               }
> >> >>       }
> >> >>
> >> >> +     drv->backend[backend->id] = backend;
> >> >> +
> >> >>       /* Reset the registers */
> >> >>       for (i = 0x800; i < 0x1000; i += 4)
> >> >>               regmap_write(backend->regs, i, 0);
> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> >> >> index 767bbadcc85d..c15ecb8343d7 100644
> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> >> >> @@ -271,7 +271,7 @@ static int sun4i_drv_probe(struct platform_device *pdev)
> >> >>       struct device_node *np = pdev->dev.of_node;
> >> >>       int i, count = 0;
> >> >>
> >> >> -     for (i = 0;; i++) {
> >> >> +     for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++) {
> >> >>               struct device_node *pipeline = of_parse_phandle(np,
> >> >>                                                               "allwinner,pipelines",
> >> >>                                                               i);
> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
> >> >> index 5df50126ff52..ec1c08af47e1 100644
> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
> >> >> @@ -16,9 +16,11 @@
> >> >>  #include <linux/clk.h>
> >> >>  #include <linux/regmap.h>
> >> >>
> >> >> +#define SUN4I_DRM_MAX_PIPELINES              2
> >> >> +
> >> >>  struct sun4i_drv {
> >> >> -     struct sun4i_backend    *backend;
> >> >> -     struct sun4i_tcon       *tcon;
> >> >> +     struct sun4i_backend    *backend[SUN4I_DRM_MAX_PIPELINES];
> >> >> +     struct sun4i_tcon       *tcon[SUN4I_DRM_MAX_PIPELINES];
> >> >>
> >> >>       struct drm_fbdev_cma    *fbdev;
> >> >>  };
> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> >> index b774c9a50c55..7749c3133f38 100644
> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> >> @@ -532,7 +532,6 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
> >> >>       if (!tcon)
> >> >>               return -ENOMEM;
> >> >>       dev_set_drvdata(dev, tcon);
> >> >> -     drv->tcon = tcon;
> >> >>       tcon->drm = drm;
> >> >>       tcon->dev = dev;
> >> >>       tcon->quirks = of_device_get_match_data(dev);
> >> >> @@ -540,14 +539,22 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
> >> >>       /* This can fail if the DT does not have any downstream encoders. */
> >> >>       tcon->id = sun4i_tcon_of_get_id(dev->of_node);
> >> >>       if (tcon->id < 0) {
> >> >> -             /*
> >> >> -              * TODO We currently support only 1 TCON, so we can
> >> >> -              * safely set this to 0. This should be revisited
> >> >> -              * when we add support for multiple pipelines.
> >> >> -              */
> >> >> -             tcon->id = 0;
> >> >> +             int i;
> >> >> +
> >> >> +             /* find the first empty tcon in sun4i_drv */
> >> >> +             for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++)
> >> >> +                     if (!drv->tcon[i])
> >> >> +                             tcon->id = i;
> >> >> +
> >> >> +             /* bail out if that failed */
> >> >> +             if (tcon->id < 0)
> >> >> +                     return tcon->id;
> >> >>       }
> >> >>
> >> >> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of tcons */
> >> >> +     if (tcon->id >= SUN4I_DRM_MAX_PIPELINES)
> >> >> +             return -EINVAL;
> >> >> +
> >> >>       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");
> >> >> @@ -588,7 +595,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
> >> >>               goto err_free_dotclock;
> >> >>       }
> >> >>
> >> >> -     tcon->crtc = sun4i_crtc_init(drm, drv->backend, tcon);
> >> >> +     tcon->crtc = sun4i_crtc_init(drm, drv->backend[tcon->id], tcon);
> >> >
> >> > I'm not a big fan of those IDs. The heuristic seems to be a bit
> >> > fragile since we really never enforced any order in our bindings.
> >>
> >> Yes. I seem to have forgotten that bit which I had intended to add.
> >> The endpoint IDs would ideally match the actual mux values used.
> >
> > That works for me, but the binding documentation would need to be
> > amended.
> >
> >> On the TCON side that's not doable, so we might have to just require
> >> them being in an increasing order.
> >
> > What are you planning to use those IDs on for the TCON?
> 
> This reply sat in my draft box for way too long.
> 
> As mentioned, the IDs on the TCON side are for the IDing the TV encoders.
> I think having the IDs for the same type of encoder be increasing, such
> that the endpoint for TV encoder 1 has a higher ID than the one for TV
> encoder 0 should be enough. There aren't any other instances where we
> have 2 or more of the same type. Or we could just add some kind of index
> property to the TV encoder node. This is for the A20 by the way.

Sorry if I'm missing a bit of context here, but I still don't get
*why* you would need to ID them. So far, your explanation seems to
have been "so that we can ID them", which is pretty circular to me :)

> >> > You seem to use it for two things:
> >> >  - to match a TCON to its backend in our code
> >> >  - to not step on each others' toes when registering the backends/tcons
> >> >
> >> > I think the second could be easily addressed using a linked list, and
> >> > the first one by storing the of_node. Then we just need to follow the
> >> > OF graph to our input of_node, and then iterate through our registered
> >> > backend list to find the one with the same of_node.
> >>
> >> Yes that would work for the above purposes.
> >>
> >> For getting the first TCON to access the mux registers, it kind of falls
> >> short. We also need something like this for the TV encoders on A10/A20.
> >> They too have a mux, which seems to be in the first TV encoder. This
> >> controls how the 8 DACs are mapped to the 4 external pins.
> >
> > How are those pins muxed between the two? Can't we just create a
> > connector that would be usable for both encoders?
> 
> They are freely muxable. However if you want VGA output, you need 3 pins
> for RGB from the same TV encoder, plus H/V sync from it's upstream TCON.

(you don't really need hsync and vsync on VGA, those are optional signals)

> And you have one pin left that you can use for composite, which would
> likely be fed from the other TCON, as you need interlaced YUV data,
> which is like the opposite of VGA.

I'm not sure how these muxers should be implemented in DRM. One
trivial way would be to have a small pinctrl driver in the TCON driver
to deal with that, and switch states in both TV encoders at runtime
(if needed, statically otherwise).

Or maybe we can do something simpler, I don't know.

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 09/11] drm/sun4i: Support two display pipelines

[Chen-Yu Tsai]

On Tue, Apr 18, 2017 at 5:57 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Chen-Yu,
>
> On Sat, Apr 08, 2017 at 01:30:55AM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Thu, Mar 9, 2017 at 10:40 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Thu, Mar 09, 2017 at 07:20:30PM +0800, Chen-Yu Tsai wrote:
>> >> On Thu, Mar 9, 2017 at 6:36 PM, Maxime Ripard
>> >> <maxime.ripard@free-electrons.com> wrote:
>> >> > Hi,
>> >> >
>> >> > On Thu, Mar 09, 2017 at 06:05:32PM +0800, Chen-Yu Tsai wrote:
>> >> >> Some Allwinner SoCs have two display pipelines (frontend -> backend ->
>> >> >> tcon).
>> >> >>
>> >> >> Previously we only supported one pipeline. This patch extends the
>> >> >> current driver to support two. It extends the tcon and backend pointers
>> >> >> in sun4i_drv into arrays, and makes the related bind functions store
>> >> >> the pointer into said arrays based on the id fetched from the device
>> >> >> tree. In the case of the tcons, it falls back to a first come order
>> >> >> if no encoders that can be used for differentiating the tcons are
>> >> >> defined. The driver's depth-first traversal of the of graph, coupled
>> >> >> with the increasing address ordering of the of graph endpoints, and
>> >> >> the fact that tcon0 should always be enabled for the tcon/encoder
>> >> >> mux to be accessible, means that tcon1 would always come after tcon0.
>> >> >>
>> >> >> Assignment of the device structure into sun4i_drv is moved to the end
>> >> >> of the bind function, when all possible error checks have passed.
>> >> >>
>> >> >> This patch also drops a trailing 0 in one of the backend probe messages.
>> >> >>
>> >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> >> >> ---
>> >> >>  drivers/gpu/drm/sun4i/sun4i_backend.c |  9 +++++++--
>> >> >>  drivers/gpu/drm/sun4i/sun4i_drv.c     |  2 +-
>> >> >>  drivers/gpu/drm/sun4i/sun4i_drv.h     |  6 ++++--
>> >> >>  drivers/gpu/drm/sun4i/sun4i_tcon.c    | 25 +++++++++++++++++--------
>> >> >>  4 files changed, 29 insertions(+), 13 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> >> >> index f3c92d54c8e4..8d22efd5a9cc 100644
>> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
>> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> >> >> @@ -350,12 +350,15 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>> >> >>       if (!backend)
>> >> >>               return -ENOMEM;
>> >> >>       dev_set_drvdata(dev, backend);
>> >> >> -     drv->backend = backend;
>> >> >>
>> >> >>       backend->id = sun4i_backend_of_get_id(dev->of_node);
>> >> >>       if (backend->id < 0)
>> >> >>               return backend->id;
>> >> >>
>> >> >> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of backends */
>> >> >> +     if (backend->id >= SUN4I_DRM_MAX_PIPELINES)
>> >> >> +             return -EINVAL;
>> >> >> +
>> >> >>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> >>       regs = devm_ioremap_resource(dev, res);
>> >> >>       if (IS_ERR(regs))
>> >> >> @@ -364,7 +367,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>> >> >>       backend->regs = devm_regmap_init_mmio(dev, regs,
>> >> >>                                             &sun4i_backend_regmap_config);
>> >> >>       if (IS_ERR(backend->regs)) {
>> >> >> -             dev_err(dev, "Couldn't create the backend0 regmap\n");
>> >> >> +             dev_err(dev, "Couldn't create the backend regmap\n");
>> >> >>               return PTR_ERR(backend->regs);
>> >> >>       }
>> >> >>
>> >> >> @@ -413,6 +416,8 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>> >> >>               }
>> >> >>       }
>> >> >>
>> >> >> +     drv->backend[backend->id] = backend;
>> >> >> +
>> >> >>       /* Reset the registers */
>> >> >>       for (i = 0x800; i < 0x1000; i += 4)
>> >> >>               regmap_write(backend->regs, i, 0);
>> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> >> >> index 767bbadcc85d..c15ecb8343d7 100644
>> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
>> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> >> >> @@ -271,7 +271,7 @@ static int sun4i_drv_probe(struct platform_device *pdev)
>> >> >>       struct device_node *np = pdev->dev.of_node;
>> >> >>       int i, count = 0;
>> >> >>
>> >> >> -     for (i = 0;; i++) {
>> >> >> +     for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++) {
>> >> >>               struct device_node *pipeline = of_parse_phandle(np,
>> >> >>                                                               "allwinner,pipelines",
>> >> >>                                                               i);
>> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
>> >> >> index 5df50126ff52..ec1c08af47e1 100644
>> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
>> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
>> >> >> @@ -16,9 +16,11 @@
>> >> >>  #include <linux/clk.h>
>> >> >>  #include <linux/regmap.h>
>> >> >>
>> >> >> +#define SUN4I_DRM_MAX_PIPELINES              2
>> >> >> +
>> >> >>  struct sun4i_drv {
>> >> >> -     struct sun4i_backend    *backend;
>> >> >> -     struct sun4i_tcon       *tcon;
>> >> >> +     struct sun4i_backend    *backend[SUN4I_DRM_MAX_PIPELINES];
>> >> >> +     struct sun4i_tcon       *tcon[SUN4I_DRM_MAX_PIPELINES];
>> >> >>
>> >> >>       struct drm_fbdev_cma    *fbdev;
>> >> >>  };
>> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> >> >> index b774c9a50c55..7749c3133f38 100644
>> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> >> >> @@ -532,7 +532,6 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>> >> >>       if (!tcon)
>> >> >>               return -ENOMEM;
>> >> >>       dev_set_drvdata(dev, tcon);
>> >> >> -     drv->tcon = tcon;
>> >> >>       tcon->drm = drm;
>> >> >>       tcon->dev = dev;
>> >> >>       tcon->quirks = of_device_get_match_data(dev);
>> >> >> @@ -540,14 +539,22 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>> >> >>       /* This can fail if the DT does not have any downstream encoders. */
>> >> >>       tcon->id = sun4i_tcon_of_get_id(dev->of_node);
>> >> >>       if (tcon->id < 0) {
>> >> >> -             /*
>> >> >> -              * TODO We currently support only 1 TCON, so we can
>> >> >> -              * safely set this to 0. This should be revisited
>> >> >> -              * when we add support for multiple pipelines.
>> >> >> -              */
>> >> >> -             tcon->id = 0;
>> >> >> +             int i;
>> >> >> +
>> >> >> +             /* find the first empty tcon in sun4i_drv */
>> >> >> +             for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++)
>> >> >> +                     if (!drv->tcon[i])
>> >> >> +                             tcon->id = i;
>> >> >> +
>> >> >> +             /* bail out if that failed */
>> >> >> +             if (tcon->id < 0)
>> >> >> +                     return tcon->id;
>> >> >>       }
>> >> >>
>> >> >> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of tcons */
>> >> >> +     if (tcon->id >= SUN4I_DRM_MAX_PIPELINES)
>> >> >> +             return -EINVAL;
>> >> >> +
>> >> >>       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");
>> >> >> @@ -588,7 +595,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>> >> >>               goto err_free_dotclock;
>> >> >>       }
>> >> >>
>> >> >> -     tcon->crtc = sun4i_crtc_init(drm, drv->backend, tcon);
>> >> >> +     tcon->crtc = sun4i_crtc_init(drm, drv->backend[tcon->id], tcon);
>> >> >
>> >> > I'm not a big fan of those IDs. The heuristic seems to be a bit
>> >> > fragile since we really never enforced any order in our bindings.
>> >>
>> >> Yes. I seem to have forgotten that bit which I had intended to add.
>> >> The endpoint IDs would ideally match the actual mux values used.
>> >
>> > That works for me, but the binding documentation would need to be
>> > amended.
>> >
>> >> On the TCON side that's not doable, so we might have to just require
>> >> them being in an increasing order.
>> >
>> > What are you planning to use those IDs on for the TCON?
>>
>> This reply sat in my draft box for way too long.
>>
>> As mentioned, the IDs on the TCON side are for the IDing the TV encoders.
>> I think having the IDs for the same type of encoder be increasing, such
>> that the endpoint for TV encoder 1 has a higher ID than the one for TV
>> encoder 0 should be enough. There aren't any other instances where we
>> have 2 or more of the same type. Or we could just add some kind of index
>> property to the TV encoder node. This is for the A20 by the way.
>
> Sorry if I'm missing a bit of context here, but I still don't get
> *why* you would need to ID them. So far, your explanation seems to
> have been "so that we can ID them", which is pretty circular to me :)

The mux controls for the TCON output path to the TV/HDMI/MIPI encoders,
and the TV encoder outputs, reside in the address space of the first TCON
and TV encoder, respectively. That's why we need to ID them, to access
the mux controls in the right block to setup the display pipeline.

>> >> > You seem to use it for two things:
>> >> >  - to match a TCON to its backend in our code
>> >> >  - to not step on each others' toes when registering the backends/tcons
>> >> >
>> >> > I think the second could be easily addressed using a linked list, and
>> >> > the first one by storing the of_node. Then we just need to follow the
>> >> > OF graph to our input of_node, and then iterate through our registered
>> >> > backend list to find the one with the same of_node.
>> >>
>> >> Yes that would work for the above purposes.
>> >>
>> >> For getting the first TCON to access the mux registers, it kind of falls
>> >> short. We also need something like this for the TV encoders on A10/A20.
>> >> They too have a mux, which seems to be in the first TV encoder. This
>> >> controls how the 8 DACs are mapped to the 4 external pins.
>> >
>> > How are those pins muxed between the two? Can't we just create a
>> > connector that would be usable for both encoders?
>>
>> They are freely muxable. However if you want VGA output, you need 3 pins
>> for RGB from the same TV encoder, plus H/V sync from it's upstream TCON.
>
> (you don't really need hsync and vsync on VGA, those are optional signals)

Really? I think you are confusing VGA with the broader "component RGB video".
AFAIK most monitors need separate sync signals.

See https://en.wikipedia.org/wiki/Component_video#RGB_analog_component_video

>> And you have one pin left that you can use for composite, which would
>> likely be fed from the other TCON, as you need interlaced YUV data,
>> which is like the opposite of VGA.
>
> I'm not sure how these muxers should be implemented in DRM. One
> trivial way would be to have a small pinctrl driver in the TCON driver
> to deal with that, and switch states in both TV encoders at runtime
> (if needed, statically otherwise).
>
> Or maybe we can do something simpler, I don't know.

Pinctrl or pinctrl like seems to be a good solution. The lines are routed
on the board so it doesn't make sense to have something runtime configurable.
We just need to know which pins are in the same group, and what kind of
signals they each expect.

ChenYu

Re: [PATCH 09/11] drm/sun4i: Support two display pipelines

[Maxime Ripard]

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


Hi,

On Tue, Apr 18, 2017 at 06:10:26PM +0800, Chen-Yu Tsai wrote:
> >> >> >> @@ -540,14 +539,22 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
> >> >> >>       /* This can fail if the DT does not have any downstream encoders. */
> >> >> >>       tcon->id = sun4i_tcon_of_get_id(dev->of_node);
> >> >> >>       if (tcon->id < 0) {
> >> >> >> -             /*
> >> >> >> -              * TODO We currently support only 1 TCON, so we can
> >> >> >> -              * safely set this to 0. This should be revisited
> >> >> >> -              * when we add support for multiple pipelines.
> >> >> >> -              */
> >> >> >> -             tcon->id = 0;
> >> >> >> +             int i;
> >> >> >> +
> >> >> >> +             /* find the first empty tcon in sun4i_drv */
> >> >> >> +             for (i = 0; i < SUN4I_DRM_MAX_PIPELINES; i++)
> >> >> >> +                     if (!drv->tcon[i])
> >> >> >> +                             tcon->id = i;
> >> >> >> +
> >> >> >> +             /* bail out if that failed */
> >> >> >> +             if (tcon->id < 0)
> >> >> >> +                     return tcon->id;
> >> >> >>       }
> >> >> >>
> >> >> >> +     /* We only support SUN4I_DRM_MAX_PIPELINES number of tcons */
> >> >> >> +     if (tcon->id >= SUN4I_DRM_MAX_PIPELINES)
> >> >> >> +             return -EINVAL;
> >> >> >> +
> >> >> >>       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");
> >> >> >> @@ -588,7 +595,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
> >> >> >>               goto err_free_dotclock;
> >> >> >>       }
> >> >> >>
> >> >> >> -     tcon->crtc = sun4i_crtc_init(drm, drv->backend, tcon);
> >> >> >> +     tcon->crtc = sun4i_crtc_init(drm, drv->backend[tcon->id], tcon);
> >> >> >
> >> >> > I'm not a big fan of those IDs. The heuristic seems to be a bit
> >> >> > fragile since we really never enforced any order in our bindings.
> >> >>
> >> >> Yes. I seem to have forgotten that bit which I had intended to add.
> >> >> The endpoint IDs would ideally match the actual mux values used.
> >> >
> >> > That works for me, but the binding documentation would need to be
> >> > amended.
> >> >
> >> >> On the TCON side that's not doable, so we might have to just require
> >> >> them being in an increasing order.
> >> >
> >> > What are you planning to use those IDs on for the TCON?
> >>
> >> This reply sat in my draft box for way too long.
> >>
> >> As mentioned, the IDs on the TCON side are for the IDing the TV encoders.
> >> I think having the IDs for the same type of encoder be increasing, such
> >> that the endpoint for TV encoder 1 has a higher ID than the one for TV
> >> encoder 0 should be enough. There aren't any other instances where we
> >> have 2 or more of the same type. Or we could just add some kind of index
> >> property to the TV encoder node. This is for the A20 by the way.
> >
> > Sorry if I'm missing a bit of context here, but I still don't get
> > *why* you would need to ID them. So far, your explanation seems to
> > have been "so that we can ID them", which is pretty circular to me :)
> 
> The mux controls for the TCON output path to the TV/HDMI/MIPI encoders,
> and the TV encoder outputs, reside in the address space of the first TCON
> and TV encoder, respectively. That's why we need to ID them, to access
> the mux controls in the right block to setup the display pipeline.

Cant' we just do that based on the endpoint ID? Especially if the only
thing we really need to tell is which TCON, we basically just have to
loop around until we find a TCON, and use that one (and the same
applies for the TV encoder).

This needs to be documented properly, but I don't see anything wrong
with that

> >> >> > You seem to use it for two things:
> >> >> >  - to match a TCON to its backend in our code
> >> >> >  - to not step on each others' toes when registering the backends/tcons
> >> >> >
> >> >> > I think the second could be easily addressed using a linked list, and
> >> >> > the first one by storing the of_node. Then we just need to follow the
> >> >> > OF graph to our input of_node, and then iterate through our registered
> >> >> > backend list to find the one with the same of_node.
> >> >>
> >> >> Yes that would work for the above purposes.
> >> >>
> >> >> For getting the first TCON to access the mux registers, it kind of falls
> >> >> short. We also need something like this for the TV encoders on A10/A20.
> >> >> They too have a mux, which seems to be in the first TV encoder. This
> >> >> controls how the 8 DACs are mapped to the 4 external pins.
> >> >
> >> > How are those pins muxed between the two? Can't we just create a
> >> > connector that would be usable for both encoders?
> >>
> >> They are freely muxable. However if you want VGA output, you need 3 pins
> >> for RGB from the same TV encoder, plus H/V sync from it's upstream TCON.
> >
> > (you don't really need hsync and vsync on VGA, those are optional signals)
> 
> Really? I think you are confusing VGA with the broader "component RGB video".
> AFAIK most monitors need separate sync signals.
> 
> See https://en.wikipedia.org/wiki/Component_video#RGB_analog_component_video

Hmm, Wikipedia seem to contradict itself :)
https://en.wikipedia.org/wiki/VGA_connector

In the video signal box... but yeah, it seems like it's needed in VGA.

> >> And you have one pin left that you can use for composite, which would
> >> likely be fed from the other TCON, as you need interlaced YUV data,
> >> which is like the opposite of VGA.
> >
> > I'm not sure how these muxers should be implemented in DRM. One
> > trivial way would be to have a small pinctrl driver in the TCON driver
> > to deal with that, and switch states in both TV encoders at runtime
> > (if needed, statically otherwise).
> >
> > Or maybe we can do something simpler, I don't know.
> 
> Pinctrl or pinctrl like seems to be a good solution. The lines are routed
> on the board so it doesn't make sense to have something runtime configurable.
> We just need to know which pins are in the same group, and what kind of
> signals they each expect.

I discussed this with Boris yesterday, and he also pointed out that
there is another solution that might work. The global DRM device also
has an atomic_check method we could use to check that we aren't using
an impossible combination (2 VGA outputs for example), and assign the
pins properly between the two blocks that need them.

This would be easier to implement than a pinctrl driver.

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 --]