[PATCH v3 00/10] R-Car DU: LVDS dual-link mode support

[PATCH v3 00/10] R-Car DU: LVDS dual-link mode support

[Laurent Pinchart]

Hello everybody,

This patch series implements support for LVDS dual-link mode in the
R-Car DU and R-Car LVDS encoder drivers, and well as in the thc63lvd1024
LVDS decoder driver.

LVDS dual-link is a mode of operation where two individual LVDS links
are operated together to carry even- and odd-numbered pixels separately.
This doubles the possible bandwidth of the video transmission. Both the
transmitter and the receiver need to support this mode of operation.

The R-Car D3 and E3 SoCs include two independent LVDS encoders that can
be grouped together to operate in dual-link mode. When used separately,
the LVDS encoders are connected to two different CRTCs and transmit
independent video streams. When used in dual-link mode, the first LVDS
encoder is connected to the first CRTC, and split even- and odd-numbered
pixels. It transmits half of the pixels on its LVDS output, and sends
the other half to the second LVDS encoder for transmittion over the
second LVDS link. The second LVDS encoder thus operates under control of
the first one, and isn't connected directly to a CRTC.

On the receiving side, the THC63LVD1024 LVDS-to-parallel bridge has two
LVDS inputs and two parallel outputs. It can operate in four different
modes:

- Single-in, single-out: The first LVDS input receives the video stream,
  and the bridge outputs it on the first parallel output. The second
  LVDS input and the second parallel output are not used.

- Single-in, dual-out: The first LVDS input receives the video stream,
  and the bridge splits even- and odd-numbered pixels and outputs them
  on the first and second parallel outputs. The second LVDS input is not
  used.

- Dual-in, single-out: The two LVDS inputs are used in dual-link mode,
  and the bridge combines the even- and odd-numbered pixels and outputs
  them on the first parallel output. The second parallel output is not
  used.

- Dual-in, dual-out: The two LVDS inputs are used in dual-link mode,
  and the bridge outputs the even- and odd-numbered pixels on the first
  parallel output.

The operating mode is selected by two input pins of the bridge, which
are connected to DIP switches on the development boards I use. The mode
is thus fixed from a Linux point of view.

Patch 01/10 adds a new dual_link boolen field to the drm_bridge_timings
structure to let bridges report their LVDS mode of operation. Patch
02/10 clarifies the THC63LVD1024 DT bindings to document dual-link
operation, and patch 03/10 implements dual-link support in the
thc64lvd1024 bridge driver by setting the drm_bridge_timings dual_link
field according to the mode selected through DT.

Patch 04/10 extends the R-Car LVDS DT bindings to specify the companion
LVDS encoder for dual-link operation. Patches 05/10 then performs a
small cleanup in the LVDS encoder driver. Patch 06/10 implements
dual-link support in the LVDS encoder driver, which involves retrieving
the operation mode from the LVDS receiver, locating the companion LVDS
encoder, and configuring both encoders when dual-link operation is
desired. The API towards the DU driver is also extended to report the
mode of operation.

Patch 07/10 implements dual-link mode support in the DU driver. There is
no specific configuration to be performed there, as dual-link is fully
implemented in the LVDS encoder driver, but the DU driver has to skip
creation of the DRM encoder and connector related to the second LVDS
encoder when dual-link is used, as the second LVDS encoder operates as a
slave of the first one, transparently from a CRTC (and thus userspace)
perspective.

Patch 08/10 specifies the companion LVDS encoder in the D3 and E3 DT
bindings. This by itself doesn't enable dual-link mode, the LVDS0
encoder is still connected to the HDMI output through LVDS receiver, and
the LVDS1 encoder is not used. Patches 09/10 and 10/10, not intended to
be merged, enable dual-link operation for the D3 and E3 boards for
testing and require flipping DIP switches on the boards.

The patches are based on top of my drm/du/next branch, and are available
for convenience at

        git://linuxtv.org/pinchartl/media.git drm/du/lvds/dual-link

They have been tested successfully on the D3 Draak board. I expect them
to work on E3 as well, but I don't have access to an Ebisu board to test
this.

Laurent Pinchart (10):
  drm: bridge: Add dual_link field to the drm_bridge_timings structure
  dt-bindings: display: bridge: thc63lvd1024: Document dual-link
    operation
  drm: bridge: thc63: Report input bus mode through bridge timings
  dt-bindings: display: renesas: lvds: Add renesas,companion property
  drm: rcar-du: lvds: Remove LVDS double-enable checks
  drm: rcar-du: lvds: Add support for dual-link mode
  drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode
  arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1
  [HACK] arm64: dts: renesas: draak: Enable LVDS dual-link operation
  [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation

 .../bindings/display/bridge/renesas,lvds.txt  |   7 +
 .../display/bridge/thine,thc63lvd1024.txt     |   6 +
 .../arm64/boot/dts/renesas/r8a77990-ebisu.dts |  24 +++-
 arch/arm64/boot/dts/renesas/r8a77990.dtsi     |   2 +
 .../arm64/boot/dts/renesas/r8a77995-draak.dts |  24 +++-
 arch/arm64/boot/dts/renesas/r8a77995.dtsi     |   2 +
 drivers/gpu/drm/bridge/thc63lvd1024.c         |  54 ++++++--
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c     |  12 ++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c         |   2 +-
 drivers/gpu/drm/rcar-du/rcar_lvds.c           | 126 +++++++++++++-----
 drivers/gpu/drm/rcar-du/rcar_lvds.h           |   5 +
 include/drm/drm_bridge.h                      |   8 ++
 12 files changed, 214 insertions(+), 58 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v3 01/10] drm: bridge: Add dual_link field to the drm_bridge_timings structure

[Laurent Pinchart]

Extend the drm_bridge_timings structure with a new dual_link field to
indicate that the bridge's input bus carries data on two separate
physical links. The first use case is LVDS dual-link mode where even-
and odd-numbered pixels are transferred on separate LVDS links.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/drm/drm_bridge.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index d4428913a4e1..aea1fcfd92a7 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -265,6 +265,14 @@ struct drm_bridge_timings {
 	 * input signal after the clock edge.
 	 */
 	u32 hold_time_ps;
+	/**
+	 * @dual_link:
+	 *
+	 * True if the bus operates in dual-link mode. The exact meaning is
+	 * dependent on the bus type. For LVDS buses, this indicates that even-
+	 * and odd-numbered pixels are received on separate links.
+	 */
+	bool dual_link;
 };
 
 /**
-- 
Regards,

Laurent Pinchart

[PATCH v3 02/10] dt-bindings: display: bridge: thc63lvd1024: Document dual-link operation

[Laurent Pinchart]

The THC63LVD1024 LVDS decoder can operate in two modes, single-link or
dual-link. In dual-link mode both input ports are used to carry even-
and odd-numbered pixels separately. Document this in the DT bindings,
along with the related rules governing port and usage.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/display/bridge/thine,thc63lvd1024.txt          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
index 37f0c04d5a28..d17d1e5820d7 100644
--- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
+++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
@@ -28,6 +28,12 @@ Optional video port nodes:
 - port@1: Second LVDS input port
 - port@3: Second digital CMOS/TTL parallel output
 
+The device can operate in single-link mode or dual-link mode. In single-link
+mode, all pixels are received on port@0, and port@1 shall not contain any
+endpoint. In dual-link mode, even-numbered pixels are received on port@0 and
+odd-numbered pixels on port@1, and both port@0 and port@1 shall contain
+endpoints.
+
 Example:
 --------
 
-- 
Regards,

Laurent Pinchart

[PATCH v3 03/10] drm: bridge: thc63: Report input bus mode through bridge timings

[Laurent Pinchart]

Set a drm_bridge_timings in the drm_bridge, and use it to report the
input bus mode (single-link or dual-link). The other fields of the
timings structure are kept to 0 as they do not apply to LVDS buses.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Changes since v1:

- Ignore disabled remote device
---
 drivers/gpu/drm/bridge/thc63lvd1024.c | 54 +++++++++++++++++++++------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
index b083a740565c..709dd28b43d6 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -31,6 +31,8 @@ struct thc63_dev {
 
 	struct drm_bridge bridge;
 	struct drm_bridge *next;
+
+	struct drm_bridge_timings timings;
 };
 
 static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
@@ -48,15 +50,28 @@ static int thc63_attach(struct drm_bridge *bridge)
 static enum drm_mode_status thc63_mode_valid(struct drm_bridge *bridge,
 					const struct drm_display_mode *mode)
 {
+	struct thc63_dev *thc63 = to_thc63(bridge);
+	unsigned int min_freq;
+	unsigned int max_freq;
+
 	/*
-	 * The THC63LVD1024 clock frequency range is 8 to 135 MHz in single-in
-	 * mode. Note that the limits are different in dual-in, single-out mode,
-	 * and will need to be adjusted accordingly.
+	 * The THC63LVD1024 pixel rate range is 8 to 135 MHz in all modes but
+	 * dual-in, single-out where it is 40 to 150 MHz. As dual-in, dual-out
+	 * isn't supported by the driver yet, simply derive the limits from the
+	 * input mode.
 	 */
-	if (mode->clock < 8000)
+	if (thc63->timings.dual_link) {
+		min_freq = 40000;
+		max_freq = 150000;
+	} else {
+		min_freq = 8000;
+		max_freq = 135000;
+	}
+
+	if (mode->clock < min_freq)
 		return MODE_CLOCK_LOW;
 
-	if (mode->clock > 135000)
+	if (mode->clock > max_freq)
 		return MODE_CLOCK_HIGH;
 
 	return MODE_OK;
@@ -101,19 +116,19 @@ static const struct drm_bridge_funcs thc63_bridge_func = {
 
 static int thc63_parse_dt(struct thc63_dev *thc63)
 {
-	struct device_node *thc63_out;
+	struct device_node *endpoint;
 	struct device_node *remote;
 
-	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
-						  THC63_RGB_OUT0, -1);
-	if (!thc63_out) {
+	endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
+						 THC63_RGB_OUT0, -1);
+	if (!endpoint) {
 		dev_err(thc63->dev, "Missing endpoint in port@%u\n",
 			THC63_RGB_OUT0);
 		return -ENODEV;
 	}
 
-	remote = of_graph_get_remote_port_parent(thc63_out);
-	of_node_put(thc63_out);
+	remote = of_graph_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
 	if (!remote) {
 		dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
 			THC63_RGB_OUT0);
@@ -132,6 +147,22 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
 	if (!thc63->next)
 		return -EPROBE_DEFER;
 
+	endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
+						 THC63_LVDS_IN1, -1);
+	if (endpoint) {
+		remote = of_graph_get_remote_port_parent(endpoint);
+		of_node_put(endpoint);
+
+		if (remote) {
+			if (of_device_is_available(remote))
+				thc63->timings.dual_link = true;
+			of_node_put(remote);
+		}
+	}
+
+	dev_dbg(thc63->dev, "operating in %s-link mode\n",
+		thc63->timings.dual_link ? "dual" : "single");
+
 	return 0;
 }
 
@@ -188,6 +219,7 @@ static int thc63_probe(struct platform_device *pdev)
 	thc63->bridge.driver_private = thc63;
 	thc63->bridge.of_node = pdev->dev.of_node;
 	thc63->bridge.funcs = &thc63_bridge_func;
+	thc63->bridge.timings = &thc63->timings;
 
 	drm_bridge_add(&thc63->bridge);
 
-- 
Regards,

Laurent Pinchart

[PATCH v3 04/10] dt-bindings: display: renesas: lvds: Add renesas,companion property

[Laurent Pinchart]

Add a new optional renesas,companion property to point to the companion
LVDS encoder. This is used to support dual-link operation where the main
LVDS encoder splits even-numbered and odd-numbered pixels between the
two LVDS encoders.

The new property doesn't control the mode of operation, it only
describes the relationship between the master and companion LVDS
encoders.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Changes since v2:

- Clarify when the companion property is required or not allowed

Changes since v1:

- Fixed typo
---
 .../devicetree/bindings/display/bridge/renesas,lvds.txt    | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
index 900a884ad9f5..2d24bd8cbec5 100644
--- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
+++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
@@ -45,6 +45,13 @@ OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
 
 Each port shall have a single endpoint.
 
+Optional properties:
+
+- renesas,companion : phandle to the companion LVDS encoder. This property is
+  mandatory for the first LVDS encoder on D3 and E3 SoCs, and shall point to
+  the second encoder to be used as a companion in dual-link mode. It shall not
+  be set for any other LVDS encoder.
+
 
 Example:
 
-- 
Regards,

Laurent Pinchart

[PATCH v3 05/10] drm: rcar-du: lvds: Remove LVDS double-enable checks

[Laurent Pinchart]

The DRM core and DU driver guarantee that the LVDS bridge will not be
double-enabled or double-disabled. Remove the corresponding unnecessary
checks.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 620b51aab291..a331f0c32187 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -63,7 +63,6 @@ struct rcar_lvds {
 		struct clk *extal;		/* External clock */
 		struct clk *dotclkin[2];	/* External DU clocks */
 	} clocks;
-	bool enabled;
 
 	struct drm_display_mode display_mode;
 	enum rcar_lvds_mode mode;
@@ -368,15 +367,12 @@ int rcar_lvds_clk_enable(struct drm_bridge *bridge, unsigned long freq)
 
 	dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq);
 
-	WARN_ON(lvds->enabled);
-
 	ret = clk_prepare_enable(lvds->clocks.mod);
 	if (ret < 0)
 		return ret;
 
 	__rcar_lvds_pll_setup_d3_e3(lvds, freq, true);
 
-	lvds->enabled = true;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(rcar_lvds_clk_enable);
@@ -390,13 +386,9 @@ void rcar_lvds_clk_disable(struct drm_bridge *bridge)
 
 	dev_dbg(lvds->dev, "disabling LVDS PLL\n");
 
-	WARN_ON(!lvds->enabled);
-
 	rcar_lvds_write(lvds, LVDPLLCR, 0);
 
 	clk_disable_unprepare(lvds->clocks.mod);
-
-	lvds->enabled = false;
 }
 EXPORT_SYMBOL_GPL(rcar_lvds_clk_disable);
 
@@ -417,8 +409,6 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
 	u32 lvdcr0;
 	int ret;
 
-	WARN_ON(lvds->enabled);
-
 	ret = clk_prepare_enable(lvds->clocks.mod);
 	if (ret < 0)
 		return;
@@ -507,16 +497,12 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
 		drm_panel_prepare(lvds->panel);
 		drm_panel_enable(lvds->panel);
 	}
-
-	lvds->enabled = true;
 }
 
 static void rcar_lvds_disable(struct drm_bridge *bridge)
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
 
-	WARN_ON(!lvds->enabled);
-
 	if (lvds->panel) {
 		drm_panel_disable(lvds->panel);
 		drm_panel_unprepare(lvds->panel);
@@ -527,8 +513,6 @@ static void rcar_lvds_disable(struct drm_bridge *bridge)
 	rcar_lvds_write(lvds, LVDPLLCR, 0);
 
 	clk_disable_unprepare(lvds->clocks.mod);
-
-	lvds->enabled = false;
 }
 
 static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
@@ -592,8 +576,6 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
 
-	WARN_ON(lvds->enabled);
-
 	lvds->display_mode = *adjusted_mode;
 
 	rcar_lvds_get_lvds_mode(lvds);
@@ -793,7 +775,6 @@ static int rcar_lvds_probe(struct platform_device *pdev)
 
 	lvds->dev = &pdev->dev;
 	lvds->info = of_device_get_match_data(&pdev->dev);
-	lvds->enabled = false;
 
 	ret = rcar_lvds_parse_dt(lvds);
 	if (ret < 0)
-- 
Regards,

Laurent Pinchart

[PATCH v3 06/10] drm: rcar-du: lvds: Add support for dual-link mode

[Laurent Pinchart]

In dual-link mode the LVDS0 encoder transmits even-numbered pixels, and
sends odd-numbered pixels to the LVDS1 encoder for transmission on a
separate link.

To implement support for this mode of operation, determine if the LVDS
connection operates in dual-link mode by querying the next device in the
pipeline, locate the companion encoder, and control it directly through
its bridge operations.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Changes since v2:

- Fail probe if the companion controller can't be found or is invalid
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 107 ++++++++++++++++++++++++----
 drivers/gpu/drm/rcar-du/rcar_lvds.h |   5 ++
 2 files changed, 99 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index a331f0c32187..d090191e858e 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -66,6 +66,9 @@ struct rcar_lvds {
 
 	struct drm_display_mode display_mode;
 	enum rcar_lvds_mode mode;
+
+	struct drm_bridge *companion;
+	bool dual_link;
 };
 
 #define bridge_to_rcar_lvds(bridge) \
@@ -400,11 +403,6 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
 	const struct drm_display_mode *mode = &lvds->display_mode;
-	/*
-	 * FIXME: We should really retrieve the CRTC through the state, but how
-	 * do we get a state pointer?
-	 */
-	struct drm_crtc *crtc = lvds->bridge.encoder->crtc;
 	u32 lvdhcr;
 	u32 lvdcr0;
 	int ret;
@@ -413,6 +411,10 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
 	if (ret < 0)
 		return;
 
+	/* Enable the companion LVDS encoder in dual-link mode. */
+	if (lvds->dual_link && lvds->companion)
+		lvds->companion->funcs->enable(lvds->companion);
+
 	/*
 	 * Hardcode the channels and control signals routing for now.
 	 *
@@ -435,17 +437,33 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
 	rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
 
 	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
-		/* Disable dual-link mode. */
-		rcar_lvds_write(lvds, LVDSTRIPE, 0);
+		/*
+		 * Configure vertical stripe based on the mode of operation of
+		 * the connected device.
+		 */
+		rcar_lvds_write(lvds, LVDSTRIPE,
+				lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
 	}
 
-	/* PLL clock configuration. */
-	lvds->info->pll_setup(lvds, mode->clock * 1000);
+	/*
+	 * PLL clock configuration on all instances but the companion in
+	 * dual-link mode.
+	 */
+	if (!lvds->dual_link || lvds->companion)
+		lvds->info->pll_setup(lvds, mode->clock * 1000);
 
 	/* Set the LVDS mode and select the input. */
 	lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
-	if (drm_crtc_index(crtc) == 2)
-		lvdcr0 |= LVDCR0_DUSEL;
+
+	if (lvds->bridge.encoder) {
+		/*
+		 * FIXME: We should really retrieve the CRTC through the state,
+		 * but how do we get a state pointer?
+		 */
+		if (drm_crtc_index(lvds->bridge.encoder->crtc) == 2)
+			lvdcr0 |= LVDCR0_DUSEL;
+	}
+
 	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
 
 	/* Turn all the channels on. */
@@ -512,6 +530,10 @@ static void rcar_lvds_disable(struct drm_bridge *bridge)
 	rcar_lvds_write(lvds, LVDCR1, 0);
 	rcar_lvds_write(lvds, LVDPLLCR, 0);
 
+	/* Disable the companion LVDS encoder in dual-link mode. */
+	if (lvds->dual_link && lvds->companion)
+		lvds->companion->funcs->disable(lvds->companion);
+
 	clk_disable_unprepare(lvds->clocks.mod);
 }
 
@@ -628,10 +650,57 @@ static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
 	.mode_set = rcar_lvds_mode_set,
 };
 
+bool rcar_lvds_dual_link(struct drm_bridge *bridge)
+{
+	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
+
+	return lvds->dual_link;
+}
+EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
+
 /* -----------------------------------------------------------------------------
  * Probe & Remove
  */
 
+static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
+{
+	const struct of_device_id *match;
+	struct device_node *companion;
+	struct device *dev = lvds->dev;
+	int ret = 0;
+
+	/* Locate the companion LVDS encoder for dual-link operation, if any. */
+	companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
+	if (!companion) {
+		dev_err(dev, "Companion LVDS encoder not found\n");
+		return -ENXIO;
+	}
+
+	/*
+	 * Sanity check: the companion encoder must have the same compatible
+	 * string.
+	 */
+	match = of_match_device(dev->driver->of_match_table, dev);
+	if (!of_device_is_compatible(companion, match->compatible)) {
+		dev_err(dev, "Companion LVDS encoder is invalid\n");
+		ret = -ENXIO;
+		goto done;
+	}
+
+	lvds->companion = of_drm_find_bridge(companion);
+	if (!lvds->companion) {
+		ret = -EPROBE_DEFER;
+		goto done;
+	}
+
+	dev_dbg(dev, "Found companion encoder %pOF\n", companion);
+
+done:
+	of_node_put(companion);
+
+	return ret;
+}
+
 static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 {
 	struct device_node *local_output = NULL;
@@ -682,14 +751,26 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 
 	if (is_bridge) {
 		lvds->next_bridge = of_drm_find_bridge(remote);
-		if (!lvds->next_bridge)
+		if (!lvds->next_bridge) {
 			ret = -EPROBE_DEFER;
+			goto done;
+		}
+
+		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
+			lvds->dual_link = lvds->next_bridge->timings
+					? lvds->next_bridge->timings->dual_link
+					: false;
 	} else {
 		lvds->panel = of_drm_find_panel(remote);
-		if (IS_ERR(lvds->panel))
+		if (IS_ERR(lvds->panel)) {
 			ret = PTR_ERR(lvds->panel);
+			goto done;
+		}
 	}
 
+	if (lvds->dual_link)
+		ret = rcar_lvds_parse_dt_companion(lvds);
+
 done:
 	of_node_put(local_output);
 	of_node_put(remote_input);
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.h b/drivers/gpu/drm/rcar-du/rcar_lvds.h
index a709cae1bc32..222ec0e60785 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.h
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.h
@@ -15,6 +15,7 @@ struct drm_bridge;
 #if IS_ENABLED(CONFIG_DRM_RCAR_LVDS)
 int rcar_lvds_clk_enable(struct drm_bridge *bridge, unsigned long freq);
 void rcar_lvds_clk_disable(struct drm_bridge *bridge);
+bool rcar_lvds_dual_link(struct drm_bridge *bridge);
 #else
 static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge,
 				       unsigned long freq)
@@ -22,6 +23,10 @@ static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge,
 	return -ENOSYS;
 }
 static inline void rcar_lvds_clk_disable(struct drm_bridge *bridge) { }
+static inline bool rcar_lvds_dual_link(struct drm_bridge *bridge)
+{
+	return false;
+}
 #endif /* CONFIG_DRM_RCAR_LVDS */
 
 #endif /* __RCAR_LVDS_H__ */
-- 
Regards,

Laurent Pinchart

[PATCH v3 07/10] drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode

[Laurent Pinchart]

In dual-link LVDS mode, the LVDS1 encoder is used as a companion for
LVDS0, and both encoders transmit data from DU0. The LVDS1 output of DU1
can't be used in that case, don't create an encoder and connector for
it.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Changess since v2:

- Remove unneeded bridge NULL check
---
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 12 ++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c     |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
index 6c91753af7bc..0f00bdfe2366 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
@@ -16,6 +16,7 @@
 #include "rcar_du_drv.h"
 #include "rcar_du_encoder.h"
 #include "rcar_du_kms.h"
+#include "rcar_lvds.h"
 
 /* -----------------------------------------------------------------------------
  * Encoder
@@ -97,6 +98,17 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 		}
 	}
 
+	/*
+	 * On Gen3 skip the LVDS1 output if the LVDS1 encoder is used as a
+	 * companion for LVDS0 in dual-link mode.
+	 */
+	if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) {
+		if (rcar_lvds_dual_link(bridge)) {
+			ret = -ENOLINK;
+			goto done;
+		}
+	}
+
 	ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,
 			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret < 0)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index f8f7fff34dff..95c81e59e2f1 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -378,7 +378,7 @@ static int rcar_du_encoders_init_one(struct rcar_du_device *rcdu,
 	}
 
 	ret = rcar_du_encoder_init(rcdu, output, entity);
-	if (ret && ret != -EPROBE_DEFER)
+	if (ret && ret != -EPROBE_DEFER && ret != -ENOLINK)
 		dev_warn(rcdu->dev,
 			 "failed to initialize encoder %pOF on output %u (%d), skipping\n",
 			 entity, output, ret);
-- 
Regards,

Laurent Pinchart

[PATCH v3 08/10] arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1

[Laurent Pinchart]

Add the new renesas,companion property to the LVDS0 node to point to the
companion LVDS encoder LVDS1.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a77990.dtsi | 2 ++
 arch/arm64/boot/dts/renesas/r8a77995.dtsi | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
index 56cb566ffa09..7cf5963eb3ba 100644
--- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
@@ -1801,6 +1801,8 @@
 			resets = <&cpg 727>;
 			status = "disabled";
 
+			renesas,companion = <&lvds1>;
+
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
index 5bf3af246e14..94b5177eb152 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
@@ -1038,6 +1038,8 @@
 			resets = <&cpg 727>;
 			status = "disabled";
 
+			renesas,companion = <&lvds1>;
+
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
-- 
Regards,

Laurent Pinchart

[PATCH v3 09/10] [HACK] arm64: dts: renesas: draak: Enable LVDS dual-link operation

[Laurent Pinchart]

Enable and connect the second LVDS encoder to the second LVDS input of
the THC63LVD1024 for dual-link LVDS operation. This requires changing
the default settings of SW45 and SW47 to OFF and ON respectively.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../arm64/boot/dts/renesas/r8a77995-draak.dts | 24 +++++++++++++------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
index a7dc11e36fd9..0699f1c19b11 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
@@ -77,11 +77,18 @@
 
 			port@0 {
 				reg = <0>;
-				thc63lvd1024_in: endpoint {
+				thc63lvd1024_in0: endpoint {
 					remote-endpoint = <&lvds0_out>;
 				};
 			};
 
+			port@1 {
+				reg = <1>;
+				thc63lvd1024_in1: endpoint {
+					remote-endpoint = <&lvds1_out>;
+				};
+			};
+
 			port@2 {
 				reg = <2>;
 				thc63lvd1024_out: endpoint {
@@ -360,24 +367,27 @@
 	ports {
 		port@1 {
 			lvds0_out: endpoint {
-				remote-endpoint = <&thc63lvd1024_in>;
+				remote-endpoint = <&thc63lvd1024_in0>;
 			};
 		};
 	};
 };
 
 &lvds1 {
-	/*
-	 * Even though the LVDS1 output is not connected, the encoder must be
-	 * enabled to supply a pixel clock to the DU for the DPAD output when
-	 * LVDS0 is in use.
-	 */
 	status = "okay";
 
 	clocks = <&cpg CPG_MOD 727>,
 		 <&x12_clk>,
 		 <&extal_clk>;
 	clock-names = "fck", "dclkin.0", "extal";
+
+	ports {
+		port@1 {
+			lvds1_out: endpoint {
+				remote-endpoint = <&thc63lvd1024_in1>;
+			};
+		};
+	};
 };
 
 &ohci0 {
-- 
Regards,

Laurent Pinchart

[PATCH v3 10/10] [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation

[Laurent Pinchart]

Enable and connect the second LVDS encoder to the second LVDS input of
the THC63LVD1024 for dual-link LVDS operation. This requires changing
the default settings of SW45 and SW47 to OFF and ON respectively.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../arm64/boot/dts/renesas/r8a77990-ebisu.dts | 24 +++++++++++++------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
index c72772589953..988d82609f41 100644
--- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
@@ -93,11 +93,18 @@
 
 			port@0 {
 				reg = <0>;
-				thc63lvd1024_in: endpoint {
+				thc63lvd1024_in0: endpoint {
 					remote-endpoint = <&lvds0_out>;
 				};
 			};
 
+			port@1 {
+				reg = <1>;
+				thc63lvd1024_in1: endpoint {
+					remote-endpoint = <&lvds1_out>;
+				};
+			};
+
 			port@2 {
 				reg = <2>;
 				thc63lvd1024_out: endpoint {
@@ -482,24 +489,27 @@
 	ports {
 		port@1 {
 			lvds0_out: endpoint {
-				remote-endpoint = <&thc63lvd1024_in>;
+				remote-endpoint = <&thc63lvd1024_in0>;
 			};
 		};
 	};
 };
 
 &lvds1 {
-	/*
-	 * Even though the LVDS1 output is not connected, the encoder must be
-	 * enabled to supply a pixel clock to the DU for the DPAD output when
-	 * LVDS0 is in use.
-	 */
 	status = "okay";
 
 	clocks = <&cpg CPG_MOD 727>,
 		 <&x13_clk>,
 		 <&extal_clk>;
 	clock-names = "fck", "dclkin.0", "extal";
+
+	ports {
+		port@1 {
+			lvds1_out: endpoint {
+				remote-endpoint = <&thc63lvd1024_in1>;
+			};
+		};
+	};
 };
 
 &ohci0 {
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 04/10] dt-bindings: display: renesas: lvds: Add renesas, companion property

[Sam Ravnborg]

Hi Laurent.

Reading through this nice series.

On Tue, May 28, 2019 at 05:12:28PM +0300, Laurent Pinchart wrote:
> Add a new optional renesas,companion property to point to the companion
> LVDS encoder. This is used to support dual-link operation where the main
> LVDS encoder splits even-numbered and odd-numbered pixels between the
> two LVDS encoders.
> 
> The new property doesn't control the mode of operation, it only
> describes the relationship between the master and companion LVDS
> encoders.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Changes since v2:
> 
> - Clarify when the companion property is required or not allowed
> 
> Changes since v1:
> 
> - Fixed typo
> ---
>  .../devicetree/bindings/display/bridge/renesas,lvds.txt    | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> index 900a884ad9f5..2d24bd8cbec5 100644
> --- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> @@ -45,6 +45,13 @@ OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
>  
>  Each port shall have a single endpoint.
>  
> +Optional properties:
> +
> +- renesas,companion : phandle to the companion LVDS encoder. This property is
> +  mandatory for the first LVDS encoder on D3 and E3 SoCs, and shall point to
> +  the second encoder to be used as a companion in dual-link mode. It shall not
> +  be set for any other LVDS encoder.

If the D3 and E3 socs do not mandate the use of dual-link, then what to
do in the DT? Because according to the above this property must be
specified for D3 and E3 SOC's.

> +
>  
>  Example:

Always good with examples, maybe it comes later.

	Sam

Re: [PATCH v3 07/10] drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode

[Sam Ravnborg]

Hi Laurent.

On Tue, May 28, 2019 at 05:12:31PM +0300, Laurent Pinchart wrote:
> In dual-link LVDS mode, the LVDS1 encoder is used as a companion for
> LVDS0, and both encoders transmit data from DU0. The LVDS1 output of DU1
> can't be used in that case, don't create an encoder and connector for
> it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Changess since v2:
> 
> - Remove unneeded bridge NULL check
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 12 ++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c     |  2 +-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> index 6c91753af7bc..0f00bdfe2366 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -16,6 +16,7 @@
>  #include "rcar_du_drv.h"
>  #include "rcar_du_encoder.h"
>  #include "rcar_du_kms.h"
> +#include "rcar_lvds.h"
>  
>  /* -----------------------------------------------------------------------------
>   * Encoder
> @@ -97,6 +98,17 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  		}
>  	}
>  
> +	/*
> +	 * On Gen3 skip the LVDS1 output if the LVDS1 encoder is used as a
> +	 * companion for LVDS0 in dual-link mode.
> +	 */
> +	if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) {

Both subject and comment above says "On Gen3", but the code looks like
it implements "On Gen3 or newer" - due to use of ">=".
Looks wrong to me.

	Sam

Re: [PATCH v3 00/10] R-Car DU: LVDS dual-link mode support

[Sam Ravnborg]

Hi Laurent.

Nice series with small and well described patches.

> On Tue, May 28, 2019 at 05:12:24PM +0300, Laurent Pinchart wrote:
>> Hello everybody,
>> 
>> This patch series implements support for LVDS dual-link mode in the
>> R-Car DU and R-Car LVDS encoder drivers, and well as in the thc63lvd1024
>> LVDS decoder driver.

Patches looks good.
With my few comments addressed:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

(Do not feel too confident in all the stuff, r-b seems to give me too
much credit for spending less than half an hour reading the patches).

	Sam

Re: [PATCH v3 04/10] dt-bindings: display: renesas: lvds: Add renesas, companion property

[Laurent Pinchart]

Hi Sam,

On Tue, May 28, 2019 at 06:37:30PM +0200, Sam Ravnborg wrote:
> On Tue, May 28, 2019 at 05:12:28PM +0300, Laurent Pinchart wrote:
> > Add a new optional renesas,companion property to point to the companion
> > LVDS encoder. This is used to support dual-link operation where the main
> > LVDS encoder splits even-numbered and odd-numbered pixels between the
> > two LVDS encoders.
> > 
> > The new property doesn't control the mode of operation, it only
> > describes the relationship between the master and companion LVDS
> > encoders.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > Changes since v2:
> > 
> > - Clarify when the companion property is required or not allowed
> > 
> > Changes since v1:
> > 
> > - Fixed typo
> > ---
> >  .../devicetree/bindings/display/bridge/renesas,lvds.txt    | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> > index 900a884ad9f5..2d24bd8cbec5 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> > +++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> > @@ -45,6 +45,13 @@ OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> >  
> >  Each port shall have a single endpoint.
> >  
> > +Optional properties:
> > +
> > +- renesas,companion : phandle to the companion LVDS encoder. This property is
> > +  mandatory for the first LVDS encoder on D3 and E3 SoCs, and shall point to
> > +  the second encoder to be used as a companion in dual-link mode. It shall not
> > +  be set for any other LVDS encoder.
> 
> If the D3 and E3 socs do not mandate the use of dual-link, then what to
> do in the DT? Because according to the above this property must be
> specified for D3 and E3 SOC's.

This property doesn't enable dual-link mode, it only specifies the
companion LVDS encoder used for dual-link mode, when enabled (through
communication between the LVDS encoder and the LVDS receiver at
runtime).

Jacopo had a similar comment so I suppose this isn't clear. How would
you word it to make it clear ?

> > +
> >  
> >  Example:
> 
> Always good with examples, maybe it comes later.

Good point, I'll fix that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 07/10] drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode

[Laurent Pinchart]

Hi Sam,

On Tue, May 28, 2019 at 06:42:13PM +0200, Sam Ravnborg wrote:
> On Tue, May 28, 2019 at 05:12:31PM +0300, Laurent Pinchart wrote:
> > In dual-link LVDS mode, the LVDS1 encoder is used as a companion for
> > LVDS0, and both encoders transmit data from DU0. The LVDS1 output of DU1
> > can't be used in that case, don't create an encoder and connector for
> > it.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > Changess since v2:
> > 
> > - Remove unneeded bridge NULL check
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 12 ++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c     |  2 +-
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > index 6c91753af7bc..0f00bdfe2366 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > @@ -16,6 +16,7 @@
> >  #include "rcar_du_drv.h"
> >  #include "rcar_du_encoder.h"
> >  #include "rcar_du_kms.h"
> > +#include "rcar_lvds.h"
> >  
> >  /* -----------------------------------------------------------------------------
> >   * Encoder
> > @@ -97,6 +98,17 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * On Gen3 skip the LVDS1 output if the LVDS1 encoder is used as a
> > +	 * companion for LVDS0 in dual-link mode.
> > +	 */
> > +	if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) {
> 
> Both subject and comment above says "On Gen3", but the code looks like
> it implements "On Gen3 or newer" - due to use of ">=".
> Looks wrong to me.

Gen3 is the newest generation :-) We thus use >= through the DU and LVDS
drivers to prepare for support of Gen4, just in case.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 04/10] dt-bindings: display: renesas: lvds: Add renesas, companion property

[Sam Ravnborg]

Hi Laurent.

> > >  
> > > +Optional properties:
> > > +
> > > +- renesas,companion : phandle to the companion LVDS encoder. This property is
> > > +  mandatory for the first LVDS encoder on D3 and E3 SoCs, and shall point to
> > > +  the second encoder to be used as a companion in dual-link mode. It shall not
> > > +  be set for any other LVDS encoder.
> > 
> > If the D3 and E3 socs do not mandate the use of dual-link, then what to
> > do in the DT? Because according to the above this property must be
> > specified for D3 and E3 SOC's.
> 
> This property doesn't enable dual-link mode, it only specifies the
> companion LVDS encoder used for dual-link mode, when enabled (through
> communication between the LVDS encoder and the LVDS receiver at
> runtime).
> 
> Jacopo had a similar comment so I suppose this isn't clear. How would
> you word it to make it clear ?
Let me try:


- renesas,companion : phandle to the companion LVDS encoder. This property is
  mandatory for the first LVDS encoder on D3 and E3 SoCs when dual-link mode
  is supported.
  The property shall pont to the phandle of the second encoder to be used as a
  companion in dual-link mode. It shall not be set for any other LVDS encoder.

The main difference is "when dual-link mode is supported".
As per my understanding this property is only relevant when the actual
HW supports / uses dual-link mode.
So for a board that do not even wire up dual-link, then setting the
property would be confusing.

I hope this better describes my understanding.

	Sam

Re: [PATCH v3 07/10] drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode

[Sam Ravnborg]

Hi Laurent.

On Tue, May 28, 2019 at 07:50:52PM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> On Tue, May 28, 2019 at 06:42:13PM +0200, Sam Ravnborg wrote:
> > On Tue, May 28, 2019 at 05:12:31PM +0300, Laurent Pinchart wrote:
> > > In dual-link LVDS mode, the LVDS1 encoder is used as a companion for
> > > LVDS0, and both encoders transmit data from DU0. The LVDS1 output of DU1
> > > can't be used in that case, don't create an encoder and connector for
> > > it.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > > Changess since v2:
> > > 
> > > - Remove unneeded bridge NULL check
> > > ---
> > >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 12 ++++++++++++
> > >  drivers/gpu/drm/rcar-du/rcar_du_kms.c     |  2 +-
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > > index 6c91753af7bc..0f00bdfe2366 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > > @@ -16,6 +16,7 @@
> > >  #include "rcar_du_drv.h"
> > >  #include "rcar_du_encoder.h"
> > >  #include "rcar_du_kms.h"
> > > +#include "rcar_lvds.h"
> > >  
> > >  /* -----------------------------------------------------------------------------
> > >   * Encoder
> > > @@ -97,6 +98,17 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> > >  		}
> > >  	}
> > >  
> > > +	/*
> > > +	 * On Gen3 skip the LVDS1 output if the LVDS1 encoder is used as a
> > > +	 * companion for LVDS0 in dual-link mode.
> > > +	 */
> > > +	if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) {
> > 
> > Both subject and comment above says "On Gen3", but the code looks like
> > it implements "On Gen3 or newer" - due to use of ">=".
> > Looks wrong to me.
> 
> Gen3 is the newest generation :-) We thus use >= through the DU and LVDS
> drivers to prepare for support of Gen4, just in case.
OK, but I guess we agree that the comment needs a small update them.

Actually I implicitly reads that it is only from Gen3 onwards that the
LVDS1 encoder can be used as a companion.
My initial understanding reading the comment was that this implmented a
workaround for Gen3 - but it is a workarounf for missing features in
older than Gen3.
So, assuming this is correct, when trying to specify a companion on
older then Gen3 should result in some kind of error/warning?
(Maybe it does).

	Sam

Re: [PATCH v3 08/10] arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1

[Simon Horman]

On Tue, May 28, 2019 at 05:12:32PM +0300, Laurent Pinchart wrote:
> Add the new renesas,companion property to the LVDS0 node to point to the
> companion LVDS encoder LVDS1.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Hi Laurent,

is this patch ready to be merged?

Re: [PATCH v3 04/10] dt-bindings: display: renesas: lvds: Add renesas, companion property

[Laurent Pinchart]

Hi Sam,

On Tue, May 28, 2019 at 06:59:00PM +0200, Sam Ravnborg wrote:
> Hi Laurent.
> 
> > > >  
> > > > +Optional properties:
> > > > +
> > > > +- renesas,companion : phandle to the companion LVDS encoder. This property is
> > > > +  mandatory for the first LVDS encoder on D3 and E3 SoCs, and shall point to
> > > > +  the second encoder to be used as a companion in dual-link mode. It shall not
> > > > +  be set for any other LVDS encoder.
> > > 
> > > If the D3 and E3 socs do not mandate the use of dual-link, then what to
> > > do in the DT? Because according to the above this property must be
> > > specified for D3 and E3 SOC's.
> > 
> > This property doesn't enable dual-link mode, it only specifies the
> > companion LVDS encoder used for dual-link mode, when enabled (through
> > communication between the LVDS encoder and the LVDS receiver at
> > runtime).
> > 
> > Jacopo had a similar comment so I suppose this isn't clear. How would
> > you word it to make it clear ?
> 
> Let me try:
> 
> - renesas,companion : phandle to the companion LVDS encoder. This property is
>   mandatory for the first LVDS encoder on D3 and E3 SoCs when dual-link mode
>   is supported.
>   The property shall pont to the phandle of the second encoder to be used as a
>   companion in dual-link mode. It shall not be set for any other LVDS encoder.
> 
> The main difference is "when dual-link mode is supported".
> As per my understanding this property is only relevant when the actual
> HW supports / uses dual-link mode.
> So for a board that do not even wire up dual-link, then setting the
> property would be confusing.

That's not quite correct. The property shall be specified when the SoC
supports dual-link mode (which is the case for the D3 and E3 SoCs only),
regardless of whether the board is wired up in single-link or dual-link
mode. Selection of the mode is performed at runtime by looking at the
requirements of the LVDS sink, not based on the companion property in
DT. The renesas,companion property is thus SoC-specific, but not
board-specific.

> I hope this better describes my understanding.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 07/10] drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode

[Laurent Pinchart]

Hi Sam,

On Tue, May 28, 2019 at 07:02:42PM +0200, Sam Ravnborg wrote:
> On Tue, May 28, 2019 at 07:50:52PM +0300, Laurent Pinchart wrote:
> > On Tue, May 28, 2019 at 06:42:13PM +0200, Sam Ravnborg wrote:
> >> On Tue, May 28, 2019 at 05:12:31PM +0300, Laurent Pinchart wrote:
> >>> In dual-link LVDS mode, the LVDS1 encoder is used as a companion for
> >>> LVDS0, and both encoders transmit data from DU0. The LVDS1 output of DU1
> >>> can't be used in that case, don't create an encoder and connector for
> >>> it.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>> Changess since v2:
> >>> 
> >>> - Remove unneeded bridge NULL check
> >>> ---
> >>>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 12 ++++++++++++
> >>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c     |  2 +-
> >>>  2 files changed, 13 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> >>> index 6c91753af7bc..0f00bdfe2366 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> >>> @@ -16,6 +16,7 @@
> >>>  #include "rcar_du_drv.h"
> >>>  #include "rcar_du_encoder.h"
> >>>  #include "rcar_du_kms.h"
> >>> +#include "rcar_lvds.h"
> >>>  
> >>>  /* -----------------------------------------------------------------------------
> >>>   * Encoder
> >>> @@ -97,6 +98,17 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >>>  		}
> >>>  	}
> >>>  
> >>> +	/*
> >>> +	 * On Gen3 skip the LVDS1 output if the LVDS1 encoder is used as a
> >>> +	 * companion for LVDS0 in dual-link mode.
> >>> +	 */
> >>> +	if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) {
> >> 
> >> Both subject and comment above says "On Gen3", but the code looks like
> >> it implements "On Gen3 or newer" - due to use of ">=".
> >> Looks wrong to me.
> > 
> > Gen3 is the newest generation :-) We thus use >= through the DU and LVDS
> > drivers to prepare for support of Gen4, just in case.
>
> OK, but I guess we agree that the comment needs a small update them.
> 
> Actually I implicitly reads that it is only from Gen3 onwards that the
> LVDS1 encoder can be used as a companion.
> My initial understanding reading the comment was that this implmented a
> workaround for Gen3 - but it is a workarounf for missing features in
> older than Gen3.

I wouldn't say workaround, it just makes sure that we don't try to
support LVDS dual-mode on older SoCs as the feature was added in Gen3
hardware.

> So, assuming this is correct, when trying to specify a companion on
> older then Gen3 should result in some kind of error/warning?
> (Maybe it does).

The property is ignored in that case. I could add an error message, but
I'm not sure I should, as we don't usually check that DT nodes don't
contain any other property than the ones specified in the DT bindings
(an automatic DT runtime validator based on the YAML bindings could be
interesting ;-)).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 08/10] arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1

[Laurent Pinchart]

Hi Simon,

On Mon, Jun 03, 2019 at 01:40:45PM +0200, Simon Horman wrote:
> On Tue, May 28, 2019 at 05:12:32PM +0300, Laurent Pinchart wrote:
> > Add the new renesas,companion property to the LVDS0 node to point to the
> > companion LVDS encoder LVDS1.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> Hi Laurent,
> 
> is this patch ready to be merged?

I was hoping for an ack from the DT bindings maintainers for the
corresponding bindings changes, but I want to get this merged for the
next kernel release, so I may not get it. I'll ping you when I send the
pull request for the DT bindings and drivers changes, at that point this
patch should be picked up.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 08/10] arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1

[Simon Horman]

On Thu, Jun 06, 2019 at 10:59:57AM +0300, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Mon, Jun 03, 2019 at 01:40:45PM +0200, Simon Horman wrote:
> > On Tue, May 28, 2019 at 05:12:32PM +0300, Laurent Pinchart wrote:
> > > Add the new renesas,companion property to the LVDS0 node to point to the
> > > companion LVDS encoder LVDS1.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > 
> > Hi Laurent,
> > 
> > is this patch ready to be merged?
> 
> I was hoping for an ack from the DT bindings maintainers for the
> corresponding bindings changes, but I want to get this merged for the
> next kernel release, so I may not get it. I'll ping you when I send the
> pull request for the DT bindings and drivers changes, at that point this
> patch should be picked up.

Thanks Laurent, got it.

Re: [PATCH v3 04/10] dt-bindings: display: renesas: lvds: Add renesas, companion property

[Sam Ravnborg]

Hi Laurent.

> > 
> > The main difference is "when dual-link mode is supported".
> > As per my understanding this property is only relevant when the actual
> > HW supports / uses dual-link mode.
> > So for a board that do not even wire up dual-link, then setting the
> > property would be confusing.
> 
> That's not quite correct. The property shall be specified when the SoC
> supports dual-link mode (which is the case for the D3 and E3 SoCs only),
> regardless of whether the board is wired up in single-link or dual-link
> mode. Selection of the mode is performed at runtime by looking at the
> requirements of the LVDS sink, not based on the companion property in
> DT. The renesas,companion property is thus SoC-specific, but not
> board-specific.
Thanks for taking your time to clarify this.

	Sam

Re: [PATCH v3 07/10] drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode

[Sam Ravnborg]

Hi Laurent.

> > > Gen3 is the newest generation :-) We thus use >= through the DU and LVDS
> > > drivers to prepare for support of Gen4, just in case.
> >
> > OK, but I guess we agree that the comment needs a small update them.
> > 
> > Actually I implicitly reads that it is only from Gen3 onwards that the
> > LVDS1 encoder can be used as a companion.
> > My initial understanding reading the comment was that this implmented a
> > workaround for Gen3 - but it is a workarounf for missing features in
> > older than Gen3.
> 
> I wouldn't say workaround, it just makes sure that we don't try to
> support LVDS dual-mode on older SoCs as the feature was added in Gen3
> hardware.
> 
> > So, assuming this is correct, when trying to specify a companion on
> > older then Gen3 should result in some kind of error/warning?
> > (Maybe it does).
> 
> The property is ignored in that case. I could add an error message, but
> I'm not sure I should, as we don't usually check that DT nodes don't
> contain any other property than the ones specified in the DT bindings
> (an automatic DT runtime validator based on the YAML bindings could be
> interesting ;-)).
Again, thanks for taking your time.
This clarifies it nicely.

	Sam

Re: [PATCH v3 05/10] drm: rcar-du: lvds: Remove LVDS double-enable checks

[Kieran Bingham]

Hi Laurent,

On 28/05/2019 15:12, Laurent Pinchart wrote:
> The DRM core and DU driver guarantee that the LVDS bridge will not be
> double-enabled or double-disabled. Remove the corresponding unnecessary
> checks.

I'm glad to hear it - that's quite a few WARN_ON's removed which
hopefully is a good thing!

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 620b51aab291..a331f0c32187 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -63,7 +63,6 @@ struct rcar_lvds {
>  		struct clk *extal;		/* External clock */
>  		struct clk *dotclkin[2];	/* External DU clocks */
>  	} clocks;
> -	bool enabled;
>  
>  	struct drm_display_mode display_mode;
>  	enum rcar_lvds_mode mode;
> @@ -368,15 +367,12 @@ int rcar_lvds_clk_enable(struct drm_bridge *bridge, unsigned long freq)
>  
>  	dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq);
>  
> -	WARN_ON(lvds->enabled);
> -
>  	ret = clk_prepare_enable(lvds->clocks.mod);
>  	if (ret < 0)
>  		return ret;
>  
>  	__rcar_lvds_pll_setup_d3_e3(lvds, freq, true);
>  
> -	lvds->enabled = true;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(rcar_lvds_clk_enable);
> @@ -390,13 +386,9 @@ void rcar_lvds_clk_disable(struct drm_bridge *bridge)
>  
>  	dev_dbg(lvds->dev, "disabling LVDS PLL\n");
>  
> -	WARN_ON(!lvds->enabled);
> -
>  	rcar_lvds_write(lvds, LVDPLLCR, 0);
>  
>  	clk_disable_unprepare(lvds->clocks.mod);
> -
> -	lvds->enabled = false;
>  }
>  EXPORT_SYMBOL_GPL(rcar_lvds_clk_disable);
>  
> @@ -417,8 +409,6 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
>  	u32 lvdcr0;
>  	int ret;
>  
> -	WARN_ON(lvds->enabled);
> -
>  	ret = clk_prepare_enable(lvds->clocks.mod);
>  	if (ret < 0)
>  		return;
> @@ -507,16 +497,12 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
>  		drm_panel_prepare(lvds->panel);
>  		drm_panel_enable(lvds->panel);
>  	}
> -
> -	lvds->enabled = true;
>  }
>  
>  static void rcar_lvds_disable(struct drm_bridge *bridge)
>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>  
> -	WARN_ON(!lvds->enabled);
> -
>  	if (lvds->panel) {
>  		drm_panel_disable(lvds->panel);
>  		drm_panel_unprepare(lvds->panel);
> @@ -527,8 +513,6 @@ static void rcar_lvds_disable(struct drm_bridge *bridge)
>  	rcar_lvds_write(lvds, LVDPLLCR, 0);
>  
>  	clk_disable_unprepare(lvds->clocks.mod);
> -
> -	lvds->enabled = false;
>  }
>  
>  static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
> @@ -592,8 +576,6 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>  
> -	WARN_ON(lvds->enabled);
> -
>  	lvds->display_mode = *adjusted_mode;
>  
>  	rcar_lvds_get_lvds_mode(lvds);
> @@ -793,7 +775,6 @@ static int rcar_lvds_probe(struct platform_device *pdev)
>  
>  	lvds->dev = &pdev->dev;
>  	lvds->info = of_device_get_match_data(&pdev->dev);
> -	lvds->enabled = false;
>  
>  	ret = rcar_lvds_parse_dt(lvds);
>  	if (ret < 0)
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 02/10] dt-bindings: display: bridge: thc63lvd1024: Document dual-link operation

[Kieran Bingham]

Hi Laurent,

On 28/05/2019 15:12, Laurent Pinchart wrote:
> The THC63LVD1024 LVDS decoder can operate in two modes, single-link or
> dual-link. In dual-link mode both input ports are used to carry even-
> and odd-numbered pixels separately. Document this in the DT bindings,
> along with the related rules governing port and usage.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt          | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> index 37f0c04d5a28..d17d1e5820d7 100644
> --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -28,6 +28,12 @@ Optional video port nodes:
>  - port@1: Second LVDS input port
>  - port@3: Second digital CMOS/TTL parallel output
>  
> +The device can operate in single-link mode or dual-link mode. In single-link
> +mode, all pixels are received on port@0, and port@1 shall not contain any
> +endpoint. In dual-link mode, even-numbered pixels are received on port@0 and
> +odd-numbered pixels on port@1, and both port@0 and port@1 shall contain
> +endpoints.
> +

Your cover letter details 4 different modes of operation for this part.

Do you anticipate the other combinations {Single-in, dual-out; dual-in,
dual-out} being supported? Perhaps that would be defined by the relevant
endpoints being connected or not ?


You state that in dual-link mode, both port@0, and port@1 shall contain
endpoints, so that implies that you only expect to support dual-in with
the 'dual-link' property. If that is correct, should it be stated
explicitly?


Otherwise,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


>  Example:
>  --------
>  
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 00/10] R-Car DU: LVDS dual-link mode support

[Kieran Bingham]

Hi Laurent,

On 28/05/2019 15:12, Laurent Pinchart wrote:
> Hello everybody,
> 
> This patch series implements support for LVDS dual-link mode in the
> R-Car DU and R-Car LVDS encoder drivers, and well as in the thc63lvd1024
> LVDS decoder driver.
> 
> LVDS dual-link is a mode of operation where two individual LVDS links
> are operated together to carry even- and odd-numbered pixels separately.
> This doubles the possible bandwidth of the video transmission. Both the
> transmitter and the receiver need to support this mode of operation.
> 
> The R-Car D3 and E3 SoCs include two independent LVDS encoders that can
> be grouped together to operate in dual-link mode. When used separately,
> the LVDS encoders are connected to two different CRTCs and transmit
> independent video streams. When used in dual-link mode, the first LVDS
> encoder is connected to the first CRTC, and split even- and odd-numbered
> pixels. It transmits half of the pixels on its LVDS output, and sends
> the other half to the second LVDS encoder for transmittion over the
> second LVDS link. The second LVDS encoder thus operates under control of
> the first one, and isn't connected directly to a CRTC.
> 
> On the receiving side, the THC63LVD1024 LVDS-to-parallel bridge has two
> LVDS inputs and two parallel outputs. It can operate in four different
> modes:
> 
> - Single-in, single-out: The first LVDS input receives the video stream,
>   and the bridge outputs it on the first parallel output. The second
>   LVDS input and the second parallel output are not used.
> 
> - Single-in, dual-out: The first LVDS input receives the video stream,
>   and the bridge splits even- and odd-numbered pixels and outputs them
>   on the first and second parallel outputs. The second LVDS input is not
>   used.
> 
> - Dual-in, single-out: The two LVDS inputs are used in dual-link mode,
>   and the bridge combines the even- and odd-numbered pixels and outputs
>   them on the first parallel output. The second parallel output is not
>   used.
> 
> - Dual-in, dual-out: The two LVDS inputs are used in dual-link mode,
>   and the bridge outputs the even- and odd-numbered pixels on the first
>   parallel output.


Clarifying this, having checked with you, Dual-in, dual-out means 'even
pixels are received on the first input, and provided on the first
output, and odd pixels are received on the second input, and provided on
the second output'.


> The operating mode is selected by two input pins of the bridge, which
> are connected to DIP switches on the development boards I use. The mode
> is thus fixed from a Linux point of view.

Would there ever by a scenario where these could be connected to GPIO's
and changed dynamically? I guess that might not make much sense - as the
configuration is more use case dependant.


> Patch 01/10 adds a new dual_link boolen field to the drm_bridge_timings
> structure to let bridges report their LVDS mode of operation. Patch
> 02/10 clarifies the THC63LVD1024 DT bindings to document dual-link
> operation, and patch 03/10 implements dual-link support in the
> thc64lvd1024 bridge driver by setting the drm_bridge_timings dual_link
> field according to the mode selected through DT.
> 
> Patch 04/10 extends the R-Car LVDS DT bindings to specify the companion
> LVDS encoder for dual-link operation. Patches 05/10 then performs a
> small cleanup in the LVDS encoder driver. Patch 06/10 implements
> dual-link support in the LVDS encoder driver, which involves retrieving
> the operation mode from the LVDS receiver, locating the companion LVDS
> encoder, and configuring both encoders when dual-link operation is
> desired. The API towards the DU driver is also extended to report the
> mode of operation.
> 
> Patch 07/10 implements dual-link mode support in the DU driver. There is
> no specific configuration to be performed there, as dual-link is fully
> implemented in the LVDS encoder driver, but the DU driver has to skip
> creation of the DRM encoder and connector related to the second LVDS
> encoder when dual-link is used, as the second LVDS encoder operates as a
> slave of the first one, transparently from a CRTC (and thus userspace)
> perspective.
> 
> Patch 08/10 specifies the companion LVDS encoder in the D3 and E3 DT
> bindings. This by itself doesn't enable dual-link mode, the LVDS0
> encoder is still connected to the HDMI output through LVDS receiver, and
> the LVDS1 encoder is not used. Patches 09/10 and 10/10, not intended to
> be merged, enable dual-link operation for the D3 and E3 boards for
> testing and require flipping DIP switches on the boards.
> 
> The patches are based on top of my drm/du/next branch, and are available
> for convenience at
> 
>         git://linuxtv.org/pinchartl/media.git drm/du/lvds/dual-link
> 
> They have been tested successfully on the D3 Draak board. I expect them
> to work on E3 as well, but I don't have access to an Ebisu board to test
> this.
> 
> Laurent Pinchart (10):
>   drm: bridge: Add dual_link field to the drm_bridge_timings structure
>   dt-bindings: display: bridge: thc63lvd1024: Document dual-link
>     operation
>   drm: bridge: thc63: Report input bus mode through bridge timings
>   dt-bindings: display: renesas: lvds: Add renesas,companion property
>   drm: rcar-du: lvds: Remove LVDS double-enable checks
>   drm: rcar-du: lvds: Add support for dual-link mode
>   drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode
>   arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1
>   [HACK] arm64: dts: renesas: draak: Enable LVDS dual-link operation
>   [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation
> 
>  .../bindings/display/bridge/renesas,lvds.txt  |   7 +
>  .../display/bridge/thine,thc63lvd1024.txt     |   6 +
>  .../arm64/boot/dts/renesas/r8a77990-ebisu.dts |  24 +++-
>  arch/arm64/boot/dts/renesas/r8a77990.dtsi     |   2 +
>  .../arm64/boot/dts/renesas/r8a77995-draak.dts |  24 +++-
>  arch/arm64/boot/dts/renesas/r8a77995.dtsi     |   2 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c         |  54 ++++++--
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c     |  12 ++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c         |   2 +-
>  drivers/gpu/drm/rcar-du/rcar_lvds.c           | 126 +++++++++++++-----
>  drivers/gpu/drm/rcar-du/rcar_lvds.h           |   5 +
>  include/drm/drm_bridge.h                      |   8 ++
>  12 files changed, 214 insertions(+), 58 deletions(-)
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 00/10] R-Car DU: LVDS dual-link mode support

[Laurent Pinchart]

Hi Kieran,

On Fri, Jun 07, 2019 at 11:16:37PM +0100, Kieran Bingham wrote:
> On 28/05/2019 15:12, Laurent Pinchart wrote:
> > Hello everybody,
> > 
> > This patch series implements support for LVDS dual-link mode in the
> > R-Car DU and R-Car LVDS encoder drivers, and well as in the thc63lvd1024
> > LVDS decoder driver.
> > 
> > LVDS dual-link is a mode of operation where two individual LVDS links
> > are operated together to carry even- and odd-numbered pixels separately.
> > This doubles the possible bandwidth of the video transmission. Both the
> > transmitter and the receiver need to support this mode of operation.
> > 
> > The R-Car D3 and E3 SoCs include two independent LVDS encoders that can
> > be grouped together to operate in dual-link mode. When used separately,
> > the LVDS encoders are connected to two different CRTCs and transmit
> > independent video streams. When used in dual-link mode, the first LVDS
> > encoder is connected to the first CRTC, and split even- and odd-numbered
> > pixels. It transmits half of the pixels on its LVDS output, and sends
> > the other half to the second LVDS encoder for transmittion over the
> > second LVDS link. The second LVDS encoder thus operates under control of
> > the first one, and isn't connected directly to a CRTC.
> > 
> > On the receiving side, the THC63LVD1024 LVDS-to-parallel bridge has two
> > LVDS inputs and two parallel outputs. It can operate in four different
> > modes:
> > 
> > - Single-in, single-out: The first LVDS input receives the video stream,
> >   and the bridge outputs it on the first parallel output. The second
> >   LVDS input and the second parallel output are not used.
> > 
> > - Single-in, dual-out: The first LVDS input receives the video stream,
> >   and the bridge splits even- and odd-numbered pixels and outputs them
> >   on the first and second parallel outputs. The second LVDS input is not
> >   used.
> > 
> > - Dual-in, single-out: The two LVDS inputs are used in dual-link mode,
> >   and the bridge combines the even- and odd-numbered pixels and outputs
> >   them on the first parallel output. The second parallel output is not
> >   used.
> > 
> > - Dual-in, dual-out: The two LVDS inputs are used in dual-link mode,
> >   and the bridge outputs the even- and odd-numbered pixels on the first
> >   parallel output.
> 
> Clarifying this, having checked with you, Dual-in, dual-out means 'even
> pixels are received on the first input, and provided on the first
> output, and odd pixels are received on the second input, and provided on
> the second output'.

Yes, my bad, this is wrong in the cover letter.

> > The operating mode is selected by two input pins of the bridge, which
> > are connected to DIP switches on the development boards I use. The mode
> > is thus fixed from a Linux point of view.
> 
> Would there ever by a scenario where these could be connected to GPIO's
> and changed dynamically? I guess that might not make much sense - as the
> configuration is more use case dependant.

This can't be ruled out, but the use cases would indeed be limited. If
the need ever arises, we can just extend the DT bindings to specify
those GPIOs, and add a new in-kernel API at the drm_bridge level to
configure the mode dynamically.

> > Patch 01/10 adds a new dual_link boolen field to the drm_bridge_timings
> > structure to let bridges report their LVDS mode of operation. Patch
> > 02/10 clarifies the THC63LVD1024 DT bindings to document dual-link
> > operation, and patch 03/10 implements dual-link support in the
> > thc64lvd1024 bridge driver by setting the drm_bridge_timings dual_link
> > field according to the mode selected through DT.
> > 
> > Patch 04/10 extends the R-Car LVDS DT bindings to specify the companion
> > LVDS encoder for dual-link operation. Patches 05/10 then performs a
> > small cleanup in the LVDS encoder driver. Patch 06/10 implements
> > dual-link support in the LVDS encoder driver, which involves retrieving
> > the operation mode from the LVDS receiver, locating the companion LVDS
> > encoder, and configuring both encoders when dual-link operation is
> > desired. The API towards the DU driver is also extended to report the
> > mode of operation.
> > 
> > Patch 07/10 implements dual-link mode support in the DU driver. There is
> > no specific configuration to be performed there, as dual-link is fully
> > implemented in the LVDS encoder driver, but the DU driver has to skip
> > creation of the DRM encoder and connector related to the second LVDS
> > encoder when dual-link is used, as the second LVDS encoder operates as a
> > slave of the first one, transparently from a CRTC (and thus userspace)
> > perspective.
> > 
> > Patch 08/10 specifies the companion LVDS encoder in the D3 and E3 DT
> > bindings. This by itself doesn't enable dual-link mode, the LVDS0
> > encoder is still connected to the HDMI output through LVDS receiver, and
> > the LVDS1 encoder is not used. Patches 09/10 and 10/10, not intended to
> > be merged, enable dual-link operation for the D3 and E3 boards for
> > testing and require flipping DIP switches on the boards.
> > 
> > The patches are based on top of my drm/du/next branch, and are available
> > for convenience at
> > 
> >         git://linuxtv.org/pinchartl/media.git drm/du/lvds/dual-link
> > 
> > They have been tested successfully on the D3 Draak board. I expect them
> > to work on E3 as well, but I don't have access to an Ebisu board to test
> > this.
> > 
> > Laurent Pinchart (10):
> >   drm: bridge: Add dual_link field to the drm_bridge_timings structure
> >   dt-bindings: display: bridge: thc63lvd1024: Document dual-link
> >     operation
> >   drm: bridge: thc63: Report input bus mode through bridge timings
> >   dt-bindings: display: renesas: lvds: Add renesas,companion property
> >   drm: rcar-du: lvds: Remove LVDS double-enable checks
> >   drm: rcar-du: lvds: Add support for dual-link mode
> >   drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode
> >   arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1
> >   [HACK] arm64: dts: renesas: draak: Enable LVDS dual-link operation
> >   [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation
> > 
> >  .../bindings/display/bridge/renesas,lvds.txt  |   7 +
> >  .../display/bridge/thine,thc63lvd1024.txt     |   6 +
> >  .../arm64/boot/dts/renesas/r8a77990-ebisu.dts |  24 +++-
> >  arch/arm64/boot/dts/renesas/r8a77990.dtsi     |   2 +
> >  .../arm64/boot/dts/renesas/r8a77995-draak.dts |  24 +++-
> >  arch/arm64/boot/dts/renesas/r8a77995.dtsi     |   2 +
> >  drivers/gpu/drm/bridge/thc63lvd1024.c         |  54 ++++++--
> >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c     |  12 ++
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c         |   2 +-
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c           | 126 +++++++++++++-----
> >  drivers/gpu/drm/rcar-du/rcar_lvds.h           |   5 +
> >  include/drm/drm_bridge.h                      |   8 ++
> >  12 files changed, 214 insertions(+), 58 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/10] dt-bindings: display: bridge: thc63lvd1024: Document dual-link operation

[Laurent Pinchart]

Hi Kieran,

On Fri, Jun 07, 2019 at 11:15:06PM +0100, Kieran Bingham wrote:
> On 28/05/2019 15:12, Laurent Pinchart wrote:
> > The THC63LVD1024 LVDS decoder can operate in two modes, single-link or
> > dual-link. In dual-link mode both input ports are used to carry even-
> > and odd-numbered pixels separately. Document this in the DT bindings,
> > along with the related rules governing port and usage.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../bindings/display/bridge/thine,thc63lvd1024.txt          | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > index 37f0c04d5a28..d17d1e5820d7 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -28,6 +28,12 @@ Optional video port nodes:
> >  - port@1: Second LVDS input port
> >  - port@3: Second digital CMOS/TTL parallel output
> >  
> > +The device can operate in single-link mode or dual-link mode. In single-link
> > +mode, all pixels are received on port@0, and port@1 shall not contain any
> > +endpoint. In dual-link mode, even-numbered pixels are received on port@0 and
> > +odd-numbered pixels on port@1, and both port@0 and port@1 shall contain
> > +endpoints.
> > +
> 
> Your cover letter details 4 different modes of operation for this part.
> 
> Do you anticipate the other combinations {Single-in, dual-out; dual-in,
> dual-out} being supported? Perhaps that would be defined by the relevant
> endpoints being connected or not ?

I expect that someone might need those modes at some point, but I
haven't specified them on purpose, as I don't like writing DT bindings
that can't be tested. I however expoect that those additional modes can
be derived from the connected endpoints.

> You state that in dual-link mode, both port@0, and port@1 shall contain
> endpoints, so that implies that you only expect to support dual-in with
> the 'dual-link' property. If that is correct, should it be stated
> explicitly?

What do you mean by the 'dual-link' property ? The patch series defines
no such property.

> Otherwise,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> >  Example:
> >  --------
> >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 04/10] dt-bindings: display: renesas: lvds: Add renesas,companion property

[Kieran Bingham]

Hi Laurent,

On 28/05/2019 15:12, Laurent Pinchart wrote:
> Add a new optional renesas,companion property to point to the companion
> LVDS encoder. This is used to support dual-link operation where the main
> LVDS encoder splits even-numbered and odd-numbered pixels between the
> two LVDS encoders.
> 
> The new property doesn't control the mode of operation, it only
> describes the relationship between the master and companion LVDS
> encoders.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Changes since v2:
> 
> - Clarify when the companion property is required or not allowed
> 
> Changes since v1:
> 
> - Fixed typo
> ---
>  .../devicetree/bindings/display/bridge/renesas,lvds.txt    | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> index 900a884ad9f5..2d24bd8cbec5 100644
> --- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> @@ -45,6 +45,13 @@ OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
>  
>  Each port shall have a single endpoint.
>  
> +Optional properties:
> +> +- renesas,companion : phandle to the companion LVDS encoder. This
property is
> +  mandatory for the first LVDS encoder on D3 and E3 SoCs, and shall point to
> +  the second encoder to be used as a companion in dual-link mode. It shall not
> +  be set for any other LVDS encoder.
> +

I see Sam has already asked for an updated example, so with that:

I'm fine with the text above.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

>  
>  Example:
>  
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 02/10] dt-bindings: display: bridge: thc63lvd1024: Document dual-link operation

[Kieran Bingham]

Hi Laurent,

On 07/06/2019 23:30, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, Jun 07, 2019 at 11:15:06PM +0100, Kieran Bingham wrote:
>> On 28/05/2019 15:12, Laurent Pinchart wrote:
>>> The THC63LVD1024 LVDS decoder can operate in two modes, single-link or
>>> dual-link. In dual-link mode both input ports are used to carry even-
>>> and odd-numbered pixels separately. Document this in the DT bindings,
>>> along with the related rules governing port and usage.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  .../bindings/display/bridge/thine,thc63lvd1024.txt          | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> index 37f0c04d5a28..d17d1e5820d7 100644
>>> --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> @@ -28,6 +28,12 @@ Optional video port nodes:
>>>  - port@1: Second LVDS input port
>>>  - port@3: Second digital CMOS/TTL parallel output
>>>  
>>> +The device can operate in single-link mode or dual-link mode. In single-link
>>> +mode, all pixels are received on port@0, and port@1 shall not contain any
>>> +endpoint. In dual-link mode, even-numbered pixels are received on port@0 and
>>> +odd-numbered pixels on port@1, and both port@0 and port@1 shall contain
>>> +endpoints.
>>> +
>>
>> Your cover letter details 4 different modes of operation for this part.
>>
>> Do you anticipate the other combinations {Single-in, dual-out; dual-in,
>> dual-out} being supported? Perhaps that would be defined by the relevant
>> endpoints being connected or not ?
> 
> I expect that someone might need those modes at some point, but I
> haven't specified them on purpose, as I don't like writing DT bindings
> that can't be tested. I however expoect that those additional modes can
> be derived from the connected endpoints.
> 
>> You state that in dual-link mode, both port@0, and port@1 shall contain
>> endpoints, so that implies that you only expect to support dual-in with
>> the 'dual-link' property. If that is correct, should it be stated
>> explicitly?
> 
> What do you mean by the 'dual-link' property ? The patch series defines
> no such property.

Aha, my imagination is creating something from all the references to the
word 'dual-link' :-D

Ok - so it is just the existence of the endpoints which will
enable//configure the various modes of operation.

I guess that will become more clear when I get down to the driver patches :)



> 
>> Otherwise,
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>>>  Example:
>>>  --------
>>>  
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 03/10] drm: bridge: thc63: Report input bus mode through bridge timings

[Kieran Bingham]

Hi Laurent,

On 28/05/2019 15:12, Laurent Pinchart wrote:
> Set a drm_bridge_timings in the drm_bridge, and use it to report the
> input bus mode (single-link or dual-link). The other fields of the
> timings structure are kept to 0 as they do not apply to LVDS buses.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
> Changes since v1:
> 
> - Ignore disabled remote device
> ---
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 54 +++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> index b083a740565c..709dd28b43d6 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -31,6 +31,8 @@ struct thc63_dev {
>  
>  	struct drm_bridge bridge;
>  	struct drm_bridge *next;
> +
> +	struct drm_bridge_timings timings;

These are just input timings right?

>  };
>  
>  static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> @@ -48,15 +50,28 @@ static int thc63_attach(struct drm_bridge *bridge)
>  static enum drm_mode_status thc63_mode_valid(struct drm_bridge *bridge,
>  					const struct drm_display_mode *mode)
>  {
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	unsigned int min_freq;
> +	unsigned int max_freq;
> +
>  	/*
> -	 * The THC63LVD1024 clock frequency range is 8 to 135 MHz in single-in
> -	 * mode. Note that the limits are different in dual-in, single-out mode,
> -	 * and will need to be adjusted accordingly.
> +	 * The THC63LVD1024 pixel rate range is 8 to 135 MHz in all modes but
> +	 * dual-in, single-out where it is 40 to 150 MHz. As dual-in, dual-out

That comma is unfortunate, and makes me read the sentence as "in all
modes but dual-in, ... ... single out where it is 40 to 150 mhz (as if
we should single out the device for it's behaviour between 40 to 150mhz).

Perhaps we could enclose the mode in single quotes to denote that it is
a single description:

   ... in all modes but 'dual-in, single-out' where it ...

> +	 * isn't supported by the driver yet, simply derive the limits from the
> +	 * input mode.
>  	 */
> -	if (mode->clock < 8000)
> +	if (thc63->timings.dual_link) {
> +		min_freq = 40000;
> +		max_freq = 150000;
> +	} else {
> +		min_freq = 8000;
> +		max_freq = 135000;
> +	}
> +
> +	if (mode->clock < min_freq)
>  		return MODE_CLOCK_LOW;
>  
> -	if (mode->clock > 135000)
> +	if (mode->clock > max_freq)
>  		return MODE_CLOCK_HIGH;
>  
>  	return MODE_OK;
> @@ -101,19 +116,19 @@ static const struct drm_bridge_funcs thc63_bridge_func = {
>  
>  static int thc63_parse_dt(struct thc63_dev *thc63)
>  {
> -	struct device_node *thc63_out;
> +	struct device_node *endpoint;
>  	struct device_node *remote;
>  
> -	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> -						  THC63_RGB_OUT0, -1);
> -	if (!thc63_out) {
> +	endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> +						 THC63_RGB_OUT0, -1);
> +	if (!endpoint) {
>  		dev_err(thc63->dev, "Missing endpoint in port@%u\n",
>  			THC63_RGB_OUT0);
>  		return -ENODEV;
>  	}
>  
> -	remote = of_graph_get_remote_port_parent(thc63_out);
> -	of_node_put(thc63_out);
> +	remote = of_graph_get_remote_port_parent(endpoint);
> +	of_node_put(endpoint);
>  	if (!remote) {
>  		dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
>  			THC63_RGB_OUT0);
> @@ -132,6 +147,22 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
>  	if (!thc63->next)
>  		return -EPROBE_DEFER;
>  
> +	endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> +						 THC63_LVDS_IN1, -1);
> +	if (endpoint) {
> +		remote = of_graph_get_remote_port_parent(endpoint);
> +		of_node_put(endpoint);
> +
> +		if (remote) {
> +			if (of_device_is_available(remote))
> +				thc63->timings.dual_link = true;
> +			of_node_put(remote);
> +		}
> +	}
> +
> +	dev_dbg(thc63->dev, "operating in %s-link mode\n",
> +		thc63->timings.dual_link ? "dual" : "single");
> +
>  	return 0;
>  }
>  
> @@ -188,6 +219,7 @@ static int thc63_probe(struct platform_device *pdev)
>  	thc63->bridge.driver_private = thc63;
>  	thc63->bridge.of_node = pdev->dev.of_node;
>  	thc63->bridge.funcs = &thc63_bridge_func;
> +	thc63->bridge.timings = &thc63->timings;
>  
>  	drm_bridge_add(&thc63->bridge);
>  
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 06/10] drm: rcar-du: lvds: Add support for dual-link mode

[Kieran Bingham]

Hi Laurent,

On 28/05/2019 15:12, Laurent Pinchart wrote:
> In dual-link mode the LVDS0 encoder transmits even-numbered pixels, and
> sends odd-numbered pixels to the LVDS1 encoder for transmission on a
> separate link.
> 
> To implement support for this mode of operation, determine if the LVDS
> connection operates in dual-link mode by querying the next device in the
> pipeline, locate the companion encoder, and control it directly through
> its bridge operations.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Looks good to me.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> ---
> Changes since v2:
> 
> - Fail probe if the companion controller can't be found or is invalid
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 107 ++++++++++++++++++++++++----
>  drivers/gpu/drm/rcar-du/rcar_lvds.h |   5 ++
>  2 files changed, 99 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index a331f0c32187..d090191e858e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -66,6 +66,9 @@ struct rcar_lvds {
>  
>  	struct drm_display_mode display_mode;
>  	enum rcar_lvds_mode mode;
> +
> +	struct drm_bridge *companion;
> +	bool dual_link;
>  };
>  
>  #define bridge_to_rcar_lvds(bridge) \
> @@ -400,11 +403,6 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>  	const struct drm_display_mode *mode = &lvds->display_mode;
> -	/*
> -	 * FIXME: We should really retrieve the CRTC through the state, but how
> -	 * do we get a state pointer?
> -	 */
> -	struct drm_crtc *crtc = lvds->bridge.encoder->crtc;
>  	u32 lvdhcr;
>  	u32 lvdcr0;
>  	int ret;
> @@ -413,6 +411,10 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
>  	if (ret < 0)
>  		return;
>  
> +	/* Enable the companion LVDS encoder in dual-link mode. */
> +	if (lvds->dual_link && lvds->companion)
> +		lvds->companion->funcs->enable(lvds->companion);
> +
>  	/*
>  	 * Hardcode the channels and control signals routing for now.
>  	 *
> @@ -435,17 +437,33 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
>  	rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
>  
>  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> -		/* Disable dual-link mode. */
> -		rcar_lvds_write(lvds, LVDSTRIPE, 0);
> +		/*
> +		 * Configure vertical stripe based on the mode of operation of
> +		 * the connected device.
> +		 */
> +		rcar_lvds_write(lvds, LVDSTRIPE,
> +				lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
>  	}
>  
> -	/* PLL clock configuration. */
> -	lvds->info->pll_setup(lvds, mode->clock * 1000);
> +	/*
> +	 * PLL clock configuration on all instances but the companion in
> +	 * dual-link mode.
> +	 */
> +	if (!lvds->dual_link || lvds->companion)
> +		lvds->info->pll_setup(lvds, mode->clock * 1000);
>  
>  	/* Set the LVDS mode and select the input. */
>  	lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
> -	if (drm_crtc_index(crtc) == 2)
> -		lvdcr0 |= LVDCR0_DUSEL;
> +
> +	if (lvds->bridge.encoder) {
> +		/*
> +		 * FIXME: We should really retrieve the CRTC through the state,
> +		 * but how do we get a state pointer?
> +		 */
> +		if (drm_crtc_index(lvds->bridge.encoder->crtc) == 2)
> +			lvdcr0 |= LVDCR0_DUSEL;
> +	}
> +
>  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
>  
>  	/* Turn all the channels on. */
> @@ -512,6 +530,10 @@ static void rcar_lvds_disable(struct drm_bridge *bridge)
>  	rcar_lvds_write(lvds, LVDCR1, 0);
>  	rcar_lvds_write(lvds, LVDPLLCR, 0);
>  
> +	/* Disable the companion LVDS encoder in dual-link mode. */
> +	if (lvds->dual_link && lvds->companion)
> +		lvds->companion->funcs->disable(lvds->companion);
> +
>  	clk_disable_unprepare(lvds->clocks.mod);
>  }
>  
> @@ -628,10 +650,57 @@ static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
>  	.mode_set = rcar_lvds_mode_set,
>  };
>  
> +bool rcar_lvds_dual_link(struct drm_bridge *bridge)
> +{
> +	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> +
> +	return lvds->dual_link;
> +}
> +EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
> +
>  /* -----------------------------------------------------------------------------
>   * Probe & Remove
>   */
>  
> +static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> +{
> +	const struct of_device_id *match;
> +	struct device_node *companion;
> +	struct device *dev = lvds->dev;
> +	int ret = 0;
> +
> +	/* Locate the companion LVDS encoder for dual-link operation, if any. */
> +	companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
> +	if (!companion) {
> +		dev_err(dev, "Companion LVDS encoder not found\n");
> +		return -ENXIO;
> +	}
> +
> +	/*
> +	 * Sanity check: the companion encoder must have the same compatible
> +	 * string.
> +	 */
> +	match = of_match_device(dev->driver->of_match_table, dev);
> +	if (!of_device_is_compatible(companion, match->compatible)) {
> +		dev_err(dev, "Companion LVDS encoder is invalid\n");
> +		ret = -ENXIO;
> +		goto done;
> +	}
> +
> +	lvds->companion = of_drm_find_bridge(companion);
> +	if (!lvds->companion) {
> +		ret = -EPROBE_DEFER;
> +		goto done;
> +	}
> +
> +	dev_dbg(dev, "Found companion encoder %pOF\n", companion);
> +
> +done:
> +	of_node_put(companion);
> +
> +	return ret;
> +}
> +
>  static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  {
>  	struct device_node *local_output = NULL;
> @@ -682,14 +751,26 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  
>  	if (is_bridge) {
>  		lvds->next_bridge = of_drm_find_bridge(remote);
> -		if (!lvds->next_bridge)
> +		if (!lvds->next_bridge) {
>  			ret = -EPROBE_DEFER;
> +			goto done;
> +		}
> +
> +		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> +			lvds->dual_link = lvds->next_bridge->timings
> +					? lvds->next_bridge->timings->dual_link
> +					: false;
>  	} else {
>  		lvds->panel = of_drm_find_panel(remote);
> -		if (IS_ERR(lvds->panel))
> +		if (IS_ERR(lvds->panel)) {
>  			ret = PTR_ERR(lvds->panel);
> +			goto done;
> +		}
>  	}
>  
> +	if (lvds->dual_link)
> +		ret = rcar_lvds_parse_dt_companion(lvds);
> +
>  done:
>  	of_node_put(local_output);
>  	of_node_put(remote_input);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.h b/drivers/gpu/drm/rcar-du/rcar_lvds.h
> index a709cae1bc32..222ec0e60785 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.h
> @@ -15,6 +15,7 @@ struct drm_bridge;
>  #if IS_ENABLED(CONFIG_DRM_RCAR_LVDS)
>  int rcar_lvds_clk_enable(struct drm_bridge *bridge, unsigned long freq);
>  void rcar_lvds_clk_disable(struct drm_bridge *bridge);
> +bool rcar_lvds_dual_link(struct drm_bridge *bridge);
>  #else
>  static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge,
>  				       unsigned long freq)
> @@ -22,6 +23,10 @@ static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge,
>  	return -ENOSYS;
>  }
>  static inline void rcar_lvds_clk_disable(struct drm_bridge *bridge) { }
> +static inline bool rcar_lvds_dual_link(struct drm_bridge *bridge)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_DRM_RCAR_LVDS */
>  
>  #endif /* __RCAR_LVDS_H__ */
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 08/10] arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1

[Kieran Bingham]

Hi Laurent,

On 28/05/2019 15:12, Laurent Pinchart wrote:
> Add the new renesas,companion property to the LVDS0 node to point to the
> companion LVDS encoder LVDS1.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  arch/arm64/boot/dts/renesas/r8a77990.dtsi | 2 ++
>  arch/arm64/boot/dts/renesas/r8a77995.dtsi | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> index 56cb566ffa09..7cf5963eb3ba 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> @@ -1801,6 +1801,8 @@
>  			resets = <&cpg 727>;
>  			status = "disabled";
>  
> +			renesas,companion = <&lvds1>;
> +
>  			ports {
>  				#address-cells = <1>;
>  				#size-cells = <0>;
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> index 5bf3af246e14..94b5177eb152 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> @@ -1038,6 +1038,8 @@
>  			resets = <&cpg 727>;
>  			status = "disabled";
>  
> +			renesas,companion = <&lvds1>;
> +
>  			ports {
>  				#address-cells = <1>;
>  				#size-cells = <0>;
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 07/10] drm: rcar-du: Skip LVDS1 output on Gen3 when using dual-link LVDS mode

[Kieran Bingham]

Hi Laurent,

On 28/05/2019 15:12, Laurent Pinchart wrote:
> In dual-link LVDS mode, the LVDS1 encoder is used as a companion for
> LVDS0, and both encoders transmit data from DU0. The LVDS1 output of DU1
> can't be used in that case, don't create an encoder and connector for
> it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

And finally, Last one...

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
> Changess since v2:
> 
> - Remove unneeded bridge NULL check
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 12 ++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c     |  2 +-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> index 6c91753af7bc..0f00bdfe2366 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -16,6 +16,7 @@
>  #include "rcar_du_drv.h"
>  #include "rcar_du_encoder.h"
>  #include "rcar_du_kms.h"
> +#include "rcar_lvds.h"
>  
>  /* -----------------------------------------------------------------------------
>   * Encoder
> @@ -97,6 +98,17 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  		}
>  	}
>  
> +	/*
> +	 * On Gen3 skip the LVDS1 output if the LVDS1 encoder is used as a
> +	 * companion for LVDS0 in dual-link mode.
> +	 */
> +	if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) {
> +		if (rcar_lvds_dual_link(bridge)) {
> +			ret = -ENOLINK;
> +			goto done;
> +		}
> +	}
> +
>  	ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,
>  			       DRM_MODE_ENCODER_NONE, NULL);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index f8f7fff34dff..95c81e59e2f1 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -378,7 +378,7 @@ static int rcar_du_encoders_init_one(struct rcar_du_device *rcdu,
>  	}
>  
>  	ret = rcar_du_encoder_init(rcdu, output, entity);
> -	if (ret && ret != -EPROBE_DEFER)
> +	if (ret && ret != -EPROBE_DEFER && ret != -ENOLINK)
>  		dev_warn(rcdu->dev,
>  			 "failed to initialize encoder %pOF on output %u (%d), skipping\n",
>  			 entity, output, ret);
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 08/10] arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1

[Laurent Pinchart]

Hi Simon,

On Thu, Jun 06, 2019 at 10:51:09AM +0200, Simon Horman wrote:
> On Thu, Jun 06, 2019 at 10:59:57AM +0300, Laurent Pinchart wrote:
> > On Mon, Jun 03, 2019 at 01:40:45PM +0200, Simon Horman wrote:
> >> On Tue, May 28, 2019 at 05:12:32PM +0300, Laurent Pinchart wrote:
> >>> Add the new renesas,companion property to the LVDS0 node to point to the
> >>> companion LVDS encoder LVDS1.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> 
> >> Hi Laurent,
> >> 
> >> is this patch ready to be merged?
> > 
> > I was hoping for an ack from the DT bindings maintainers for the
> > corresponding bindings changes, but I want to get this merged for the
> > next kernel release, so I may not get it. I'll ping you when I send the
> > pull request for the DT bindings and drivers changes, at that point this
> > patch should be picked up.
> 
> Thanks Laurent, got it.

The DT bindings and driver changes have been merged, so please go ahead
and pick this patch for v5.3.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 08/10] arm64: dts: renesas: r8a7799[05]: Point LVDS0 to its companion LVDS1

[Simon Horman]

On Wed, Jun 12, 2019 at 01:21:24PM +0300, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Thu, Jun 06, 2019 at 10:51:09AM +0200, Simon Horman wrote:
> > On Thu, Jun 06, 2019 at 10:59:57AM +0300, Laurent Pinchart wrote:
> > > On Mon, Jun 03, 2019 at 01:40:45PM +0200, Simon Horman wrote:
> > >> On Tue, May 28, 2019 at 05:12:32PM +0300, Laurent Pinchart wrote:
> > >>> Add the new renesas,companion property to the LVDS0 node to point to the
> > >>> companion LVDS encoder LVDS1.
> > >>> 
> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >>> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >> 
> > >> Hi Laurent,
> > >> 
> > >> is this patch ready to be merged?
> > > 
> > > I was hoping for an ack from the DT bindings maintainers for the
> > > corresponding bindings changes, but I want to get this merged for the
> > > next kernel release, so I may not get it. I'll ping you when I send the
> > > pull request for the DT bindings and drivers changes, at that point this
> > > patch should be picked up.
> > 
> > Thanks Laurent, got it.
> 
> The DT bindings and driver changes have been merged, so please go ahead
> and pick this patch for v5.3.

Thanks Laurent,

done.

RE: [PATCH v3 10/10] [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation

[Fabrizio Castro]

Hello Jacopo,

> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 28 May 2019 15:13
> Subject: [PATCH v3 10/10] [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation
> 
> Enable and connect the second LVDS encoder to the second LVDS input of
> the THC63LVD1024 for dual-link LVDS operation. This requires changing
> the default settings of SW45 and SW47 to OFF and ON respectively.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

How did you test this patch on Ebisu (kernel branch, configuration, switches,etc.)?

I tested both linux-next and drm/du/lvds/dual-link and they are not working for me.
The base configuration I am using is coming from arch/arm64/configs/defconfig from each respective branch, on top of that I am enabling the remaining bits and pieces.
I have tried the suggested configuration of the switches for this patch, basically nothing is happening. I have also tried reverting the changes made by this patch (on both branches), and of course I have reverted the selection for the switches as well, and even single-link doesn't work for me. Single-link support from the BSP version of the kernel (4.14.75-ltsi) works for me, that confirms the configuration of the switches I am using when testing single-link should be okay.

If, in the single-link use case from drm/du/lvds/dual-link, I connect lvds1 to the vga-encoder in the DT (like for the BSP DT, but I can see from the schematics that ADV7123 is actually connected to DU, like the configuration in the DT upstream), then HDMI works as expected (most of the time). 

I wonder if for some reason we may end up using the wrong lvds encoder at times, or no encoder at all?

Have you seen this problem? Am I missing something obvious here?

Thanks,
Fab

> ---
>  .../arm64/boot/dts/renesas/r8a77990-ebisu.dts | 24 +++++++++++++------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> index c72772589953..988d82609f41 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> @@ -93,11 +93,18 @@
> 
>  			port@0 {
>  				reg = <0>;
> -				thc63lvd1024_in: endpoint {
> +				thc63lvd1024_in0: endpoint {
>  					remote-endpoint = <&lvds0_out>;
>  				};
>  			};
> 
> +			port@1 {
> +				reg = <1>;
> +				thc63lvd1024_in1: endpoint {
> +					remote-endpoint = <&lvds1_out>;
> +				};
> +			};
> +
>  			port@2 {
>  				reg = <2>;
>  				thc63lvd1024_out: endpoint {
> @@ -482,24 +489,27 @@
>  	ports {
>  		port@1 {
>  			lvds0_out: endpoint {
> -				remote-endpoint = <&thc63lvd1024_in>;
> +				remote-endpoint = <&thc63lvd1024_in0>;
>  			};
>  		};
>  	};
>  };
> 
>  &lvds1 {
> -	/*
> -	 * Even though the LVDS1 output is not connected, the encoder must be
> -	 * enabled to supply a pixel clock to the DU for the DPAD output when
> -	 * LVDS0 is in use.
> -	 */
>  	status = "okay";
> 
>  	clocks = <&cpg CPG_MOD 727>,
>  		 <&x13_clk>,
>  		 <&extal_clk>;
>  	clock-names = "fck", "dclkin.0", "extal";
> +
> +	ports {
> +		port@1 {
> +			lvds1_out: endpoint {
> +				remote-endpoint = <&thc63lvd1024_in1>;
> +			};
> +		};
> +	};
>  };
> 
>  &ohci0 {
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v3 10/10] [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation

[Jacopo Mondi]

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

HI Fabrizio,

On Mon, Jul 22, 2019 at 11:27:26AM +0000, Fabrizio Castro wrote:
> Hello Jacopo,
>
> > From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> > Sent: 28 May 2019 15:13
> > Subject: [PATCH v3 10/10] [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation
> >
> > Enable and connect the second LVDS encoder to the second LVDS input of
> > the THC63LVD1024 for dual-link LVDS operation. This requires changing
> > the default settings of SW45 and SW47 to OFF and ON respectively.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> How did you test this patch on Ebisu (kernel branch, configuration, switches,etc.)?
>

I tested the branch provided by Laurent and mentioned in this cover
letter.

The branch is now rebased on v5.2-rc1 as you can see, I cannot tell at
the time which one was the release tag on which it was based on, but I
suspect 5.0 or either v4.20. Maye Laurent remembers it.

> I tested both linux-next and drm/du/lvds/dual-link and they are not working for me.
> The base configuration I am using is coming from arch/arm64/configs/defconfig from each respective branch, on top of that I am enabling the remaining bits and pieces.
> I have tried the suggested configuration of the switches for this patch, basically nothing is happening. I have also tried reverting the changes made by this patch (on both branches), and of course I have reverted the selection for the switches as well, and even single-link doesn't work for me. Single-link support from the BSP version of the kernel (4.14.75-ltsi) works for me, that confirms the configuration of the switches I am using when testing single-link should be okay.
>
> If, in the single-link use case from drm/du/lvds/dual-link, I connect lvds1 to the vga-encoder in the DT (like for the BSP DT, but I can see from the schematics that ADV7123 is actually connected to DU, like the configuration in the DT upstream), then HDMI works as expected (most of the time).
>
> I wonder if for some reason we may end up using the wrong lvds encoder at times, or no encoder at all?
>
> Have you seen this problem? Am I missing something obvious here?

The branch I tested at the time worked out the box, but now, I see
several different problems, and this morning I ran severl tests. Here
it is a summary:

Laurent's drm/du/lvds/dual-link and drm/du/next are v5.2-rc1 based.
I see an error which makes Ebisu fail to boot on those branches and
plain v5.2 (both -rc1 and final v5.2)

The issue is related to bd9571 which seems to be an MFD gpio expander:

[    2.748694] bd9571mwv 8-0030: Device: BD9571MWV rev. 4
[   28.754865] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
[   28.761094] rcu:     1-...0: (2 ticks this GP) idle=352/1/0x4000000000000000 softirq=45/46 fqs=3250

sometimes it even fail when detecting the chip:
        i2c-sh_mobile e60b0000.i2c: Transfer request timed out                                                                                                       │
        bd9571mwv 8-0030: Failed to read vendor code register (ret=-110)

I managed to boot the board once with no error, so I wonder if it is
not my board which has issues?

Anyway, this seems unrelated to the lvds dual link mode, but prevents
me from testing anything on v5.2. I wonder if you ever seen anything similar...

So I went and tested v5.1. Plain v5.1 with SW45 and SW47 in their
'default' position gives me a working output with single mode.

I then cherry-picked the patches from Laurent's drm/du/lvds/dual-link
and applied on top of v5.1 and it seems like DU does not get probed
there.

To be sure I was testing the same patches I tested at the time I gave
my tag to this series I manually applied the patches from this series
(not the one in Laurent's tree, but the ones here sent) on top of v5.1
and I got the same result, DU was not probed correctly.

I had a look at diff in the encoder registration process between 5.0 and
5.2 and I don't see much differences, so I suspect it might be some
config option I have missed as well?

Could you be a bit more precise on which tools are you using for
testing and which failures you see? Are those the same I see here?

I'm testing using kms-tests or kmsxx alternatively, and made sure the
DU driver was loaded (or not) with modeprint from drm's modetest
suite.  Basically, everything is mentioned here:
uttps://elinux.org/R-Car/Devices#DU

I could start reporting the v5.2 failure on Ebisu if you confirm me
you see the same on your side, then trying to get to the bottom of the
DU/lvds issue.

Thanks
   j

>
> Thanks,
> Fab
>
> > ---
> >  .../arm64/boot/dts/renesas/r8a77990-ebisu.dts | 24 +++++++++++++------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> > index c72772589953..988d82609f41 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> > +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> > @@ -93,11 +93,18 @@
> >
> >  			port@0 {
> >  				reg = <0>;
> > -				thc63lvd1024_in: endpoint {
> > +				thc63lvd1024_in0: endpoint {
> >  					remote-endpoint = <&lvds0_out>;
> >  				};
> >  			};
> >
> > +			port@1 {
> > +				reg = <1>;
> > +				thc63lvd1024_in1: endpoint {
> > +					remote-endpoint = <&lvds1_out>;
> > +				};
> > +			};
> > +
> >  			port@2 {
> >  				reg = <2>;
> >  				thc63lvd1024_out: endpoint {
> > @@ -482,24 +489,27 @@
> >  	ports {
> >  		port@1 {
> >  			lvds0_out: endpoint {
> > -				remote-endpoint = <&thc63lvd1024_in>;
> > +				remote-endpoint = <&thc63lvd1024_in0>;
> >  			};
> >  		};
> >  	};
> >  };
> >
> >  &lvds1 {
> > -	/*
> > -	 * Even though the LVDS1 output is not connected, the encoder must be
> > -	 * enabled to supply a pixel clock to the DU for the DPAD output when
> > -	 * LVDS0 is in use.
> > -	 */
> >  	status = "okay";
> >
> >  	clocks = <&cpg CPG_MOD 727>,
> >  		 <&x13_clk>,
> >  		 <&extal_clk>;
> >  	clock-names = "fck", "dclkin.0", "extal";
> > +
> > +	ports {
> > +		port@1 {
> > +			lvds1_out: endpoint {
> > +				remote-endpoint = <&thc63lvd1024_in1>;
> > +			};
> > +		};
> > +	};
> >  };
> >
> >  &ohci0 {
> > --
> > Regards,
> >
> > Laurent Pinchart
>

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

RE: [PATCH v3 10/10] [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation

[Fabrizio Castro]

Hello Jacopo,

Thank you for getting back to me!

> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Jacopo Mondi
> Sent: 23 July 2019 11:30
> Subject: Re: [PATCH v3 10/10] [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation
> 
> HI Fabrizio,
> 
> On Mon, Jul 22, 2019 at 11:27:26AM +0000, Fabrizio Castro wrote:
> > Hello Jacopo,
> >
> > > From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> > > Sent: 28 May 2019 15:13
> > > Subject: [PATCH v3 10/10] [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation
> > >
> > > Enable and connect the second LVDS encoder to the second LVDS input of
> > > the THC63LVD1024 for dual-link LVDS operation. This requires changing
> > > the default settings of SW45 and SW47 to OFF and ON respectively.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > How did you test this patch on Ebisu (kernel branch, configuration, switches,etc.)?
> >
> 
> I tested the branch provided by Laurent and mentioned in this cover
> letter.
> 
> The branch is now rebased on v5.2-rc1 as you can see, I cannot tell at
> the time which one was the release tag on which it was based on, but I
> suspect 5.0 or either v4.20. Maye Laurent remembers it.
> 
> > I tested both linux-next and drm/du/lvds/dual-link and they are not working for me.
> > The base configuration I am using is coming from arch/arm64/configs/defconfig from each respective branch, on top of that I am
> enabling the remaining bits and pieces.
> > I have tried the suggested configuration of the switches for this patch, basically nothing is happening. I have also tried reverting the
> changes made by this patch (on both branches), and of course I have reverted the selection for the switches as well, and even single-
> link doesn't work for me. Single-link support from the BSP version of the kernel (4.14.75-ltsi) works for me, that confirms the
> configuration of the switches I am using when testing single-link should be okay.
> >
> > If, in the single-link use case from drm/du/lvds/dual-link, I connect lvds1 to the vga-encoder in the DT (like for the BSP DT, but I can
> see from the schematics that ADV7123 is actually connected to DU, like the configuration in the DT upstream), then HDMI works as
> expected (most of the time).
> >
> > I wonder if for some reason we may end up using the wrong lvds encoder at times, or no encoder at all?
> >
> > Have you seen this problem? Am I missing something obvious here?
> 
> The branch I tested at the time worked out the box, but now, I see
> several different problems, and this morning I ran severl tests. Here
> it is a summary:
> 
> Laurent's drm/du/lvds/dual-link and drm/du/next are v5.2-rc1 based.
> I see an error which makes Ebisu fail to boot on those branches and
> plain v5.2 (both -rc1 and final v5.2)
> 
> The issue is related to bd9571 which seems to be an MFD gpio expander:
> 
> [    2.748694] bd9571mwv 8-0030: Device: BD9571MWV rev. 4
> [   28.754865] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
> [   28.761094] rcu:     1-...0: (2 ticks this GP) idle=352/1/0x4000000000000000 softirq=45/46 fqs=3250
> 
> sometimes it even fail when detecting the chip:
>         i2c-sh_mobile e60b0000.i2c: Transfer request timed out                                                                                                       │
>         bd9571mwv 8-0030: Failed to read vendor code register (ret=-110)
> 
> I managed to boot the board once with no error, so I wonder if it is
> not my board which has issues?
> 
> Anyway, this seems unrelated to the lvds dual link mode, but prevents
> me from testing anything on v5.2. I wonder if you ever seen anything similar...

I have just retested plain v5.2 on the Ebisu board (I am using the 4D, which
version of Ebisu are you using?) and I don't seem to have that specific issue.
Also, this time around the single-link configuration is working out of the box,
which is interesting.

> 
> So I went and tested v5.1. Plain v5.1 with SW45 and SW47 in their
> 'default' position gives me a working output with single mode.
> 
> I then cherry-picked the patches from Laurent's drm/du/lvds/dual-link
> and applied on top of v5.1 and it seems like DU does not get probed
> there.
> 
> To be sure I was testing the same patches I tested at the time I gave
> my tag to this series I manually applied the patches from this series
> (not the one in Laurent's tree, but the ones here sent) on top of v5.1
> and I got the same result, DU was not probed correctly.
> 
> I had a look at diff in the encoder registration process between 5.0 and
> 5.2 and I don't see much differences, so I suspect it might be some
> config option I have missed as well?
> 
> Could you be a bit more precise on which tools are you using for
> testing and which failures you see? Are those the same I see here?

Even in my case DU doesn't get probed correctly either, rcar_du_probe
gets called into a couple of times, and both times it returns -EPROBE_DEFER.

I am using kms-tests at the moment.

> 
> I'm testing using kms-tests or kmsxx alternatively, and made sure the
> DU driver was loaded (or not) with modeprint from drm's modetest
> suite.  Basically, everything is mentioned here:
> uttps://elinux.org/R-Car/Devices#DU
> 
> I could start reporting the v5.2 failure on Ebisu if you confirm me
> you see the same on your side, then trying to get to the bottom of the
> DU/lvds issue.

I am looking into dual-link support for RZ/G2E, and that's why I am interested
in seeing dual-link working on Ebisu. I am going to have a deeper look into
what's happening on Ebisu now that I know that you are having similar troubles
(besides bd9571mwv), but I may have more questions soon for you if you don't
mind?

Thanks,
Fab

> 
> Thanks
>    j
> 
> >
> > Thanks,
> > Fab
> >
> > > ---
> > >  .../arm64/boot/dts/renesas/r8a77990-ebisu.dts | 24 +++++++++++++------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> > > index c72772589953..988d82609f41 100644
> > > --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> > > +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> > > @@ -93,11 +93,18 @@
> > >
> > >  			port@0 {
> > >  				reg = <0>;
> > > -				thc63lvd1024_in: endpoint {
> > > +				thc63lvd1024_in0: endpoint {
> > >  					remote-endpoint = <&lvds0_out>;
> > >  				};
> > >  			};
> > >
> > > +			port@1 {
> > > +				reg = <1>;
> > > +				thc63lvd1024_in1: endpoint {
> > > +					remote-endpoint = <&lvds1_out>;
> > > +				};
> > > +			};
> > > +
> > >  			port@2 {
> > >  				reg = <2>;
> > >  				thc63lvd1024_out: endpoint {
> > > @@ -482,24 +489,27 @@
> > >  	ports {
> > >  		port@1 {
> > >  			lvds0_out: endpoint {
> > > -				remote-endpoint = <&thc63lvd1024_in>;
> > > +				remote-endpoint = <&thc63lvd1024_in0>;
> > >  			};
> > >  		};
> > >  	};
> > >  };
> > >
> > >  &lvds1 {
> > > -	/*
> > > -	 * Even though the LVDS1 output is not connected, the encoder must be
> > > -	 * enabled to supply a pixel clock to the DU for the DPAD output when
> > > -	 * LVDS0 is in use.
> > > -	 */
> > >  	status = "okay";
> > >
> > >  	clocks = <&cpg CPG_MOD 727>,
> > >  		 <&x13_clk>,
> > >  		 <&extal_clk>;
> > >  	clock-names = "fck", "dclkin.0", "extal";
> > > +
> > > +	ports {
> > > +		port@1 {
> > > +			lvds1_out: endpoint {
> > > +				remote-endpoint = <&thc63lvd1024_in1>;
> > > +			};
> > > +		};
> > > +	};
> > >  };
> > >
> > >  &ohci0 {
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> >

Re: [PATCH v3 01/10] drm: bridge: Add dual_link field to the drm_bridge_timings structure

[Neil Armstrong]

On 28/05/2019 16:12, Laurent Pinchart wrote:
> Extend the drm_bridge_timings structure with a new dual_link field to
> indicate that the bridge's input bus carries data on two separate
> physical links. The first use case is LVDS dual-link mode where even-
> and odd-numbered pixels are transferred on separate LVDS links.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/drm/drm_bridge.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index d4428913a4e1..aea1fcfd92a7 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -265,6 +265,14 @@ struct drm_bridge_timings {
>  	 * input signal after the clock edge.
>  	 */
>  	u32 hold_time_ps;
> +	/**
> +	 * @dual_link:
> +	 *
> +	 * True if the bus operates in dual-link mode. The exact meaning is
> +	 * dependent on the bus type. For LVDS buses, this indicates that even-
> +	 * and odd-numbered pixels are received on separate links.
> +	 */
> +	bool dual_link;
>  };
>  
>  /**
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH v3 03/10] drm: bridge: thc63: Report input bus mode through bridge timings

[Neil Armstrong]

On 28/05/2019 16:12, Laurent Pinchart wrote:
> Set a drm_bridge_timings in the drm_bridge, and use it to report the
> input bus mode (single-link or dual-link). The other fields of the
> timings structure are kept to 0 as they do not apply to LVDS buses.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Changes since v1:
> 
> - Ignore disabled remote device
> ---
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 54 +++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> index b083a740565c..709dd28b43d6 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -31,6 +31,8 @@ struct thc63_dev {
>  
>  	struct drm_bridge bridge;
>  	struct drm_bridge *next;
> +
> +	struct drm_bridge_timings timings;
>  };
>  
>  static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> @@ -48,15 +50,28 @@ static int thc63_attach(struct drm_bridge *bridge)
>  static enum drm_mode_status thc63_mode_valid(struct drm_bridge *bridge,
>  					const struct drm_display_mode *mode)
>  {
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	unsigned int min_freq;
> +	unsigned int max_freq;
> +
>  	/*
> -	 * The THC63LVD1024 clock frequency range is 8 to 135 MHz in single-in
> -	 * mode. Note that the limits are different in dual-in, single-out mode,
> -	 * and will need to be adjusted accordingly.
> +	 * The THC63LVD1024 pixel rate range is 8 to 135 MHz in all modes but
> +	 * dual-in, single-out where it is 40 to 150 MHz. As dual-in, dual-out
> +	 * isn't supported by the driver yet, simply derive the limits from the
> +	 * input mode.
>  	 */
> -	if (mode->clock < 8000)
> +	if (thc63->timings.dual_link) {
> +		min_freq = 40000;
> +		max_freq = 150000;
> +	} else {
> +		min_freq = 8000;
> +		max_freq = 135000;
> +	}
> +
> +	if (mode->clock < min_freq)
>  		return MODE_CLOCK_LOW;
>  
> -	if (mode->clock > 135000)
> +	if (mode->clock > max_freq)
>  		return MODE_CLOCK_HIGH;
>  
>  	return MODE_OK;
> @@ -101,19 +116,19 @@ static const struct drm_bridge_funcs thc63_bridge_func = {
>  
>  static int thc63_parse_dt(struct thc63_dev *thc63)
>  {
> -	struct device_node *thc63_out;
> +	struct device_node *endpoint;
>  	struct device_node *remote;
>  
> -	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> -						  THC63_RGB_OUT0, -1);
> -	if (!thc63_out) {
> +	endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> +						 THC63_RGB_OUT0, -1);
> +	if (!endpoint) {
>  		dev_err(thc63->dev, "Missing endpoint in port@%u\n",
>  			THC63_RGB_OUT0);
>  		return -ENODEV;
>  	}
>  
> -	remote = of_graph_get_remote_port_parent(thc63_out);
> -	of_node_put(thc63_out);
> +	remote = of_graph_get_remote_port_parent(endpoint);
> +	of_node_put(endpoint);
>  	if (!remote) {
>  		dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
>  			THC63_RGB_OUT0);
> @@ -132,6 +147,22 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
>  	if (!thc63->next)
>  		return -EPROBE_DEFER;
>  
> +	endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> +						 THC63_LVDS_IN1, -1);
> +	if (endpoint) {
> +		remote = of_graph_get_remote_port_parent(endpoint);
> +		of_node_put(endpoint);
> +
> +		if (remote) {
> +			if (of_device_is_available(remote))
> +				thc63->timings.dual_link = true;
> +			of_node_put(remote);
> +		}
> +	}
> +
> +	dev_dbg(thc63->dev, "operating in %s-link mode\n",
> +		thc63->timings.dual_link ? "dual" : "single");
> +
>  	return 0;
>  }
>  
> @@ -188,6 +219,7 @@ static int thc63_probe(struct platform_device *pdev)
>  	thc63->bridge.driver_private = thc63;
>  	thc63->bridge.of_node = pdev->dev.of_node;
>  	thc63->bridge.funcs = &thc63_bridge_func;
> +	thc63->bridge.timings = &thc63->timings;
>  
>  	drm_bridge_add(&thc63->bridge);
>  
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>