[PATCH 00/22] R-Car Gen3 HDMI output support

[PATCH 00/22] R-Car Gen3 HDMI output support

[Laurent Pinchart]

Hello,

This patch series implements support for the HDMI output on Renesas R-Car Gen3
SoCs, and more specifically on the R-Car H3.

R-Car Gen3 SoCs include one or multiple Synopsys DWC HDMI TX controllers. The
series thus starts with 13 cleanup or feature patches for the dw-hdmi driver.
Patches 01/22 to 05/22 are small miscellaneous cleanups. Patch 06/22 fixes a
crash when the HPD interrupt is generated before the bridge gets attached to a
DRM device.

Patch 07/22 then starts refactoring the API towards platform glue by moving
common I/O and clock resource allocation to common code. Patches 08/22 and
09/22 continue by moving connector creation to the bridge attach operation to
comply with the DRM bridge API. Patch 10/22 implements a new API for platform
glue to register the bridge with the DRM core, for platform that doesn't use
the component framework. Patches 11/22 to 13/22 finally abstract PHY setup to
allow platforms to implement configuration for custom PHYs.

The next three patches add glue code for the DWC HDMI on Renesas R-Car Gen3
platforms. Patch 14/22 rework the DWC HDMI DT bindings to make them more
modular, and patch 15/22 makes use of them to document the R-Car Gen DWC HDMI
TX DT bindings. Patch 16/22 then implements the platform glue code.

The next three patches implement support for the R8A7795 HDMI outputs in the
R-Car DU driver. Patch 17/22 fixes a bug that makes the driver defer probe
forever if an encoder is disabled. Patch 18/22 implement DPLL support to
generate more precise clocks required by the HDMI controller, and patch 19/22
then enables the two HDMI output on the R-Car H3 SoC.

The last three patches enable the HDMI outputs on the H3 Salvator-X board by
adding the HDMI encoders to the R-Car H3 DT (20/22), adding HDMI connectors
and enabling the encoders (21/22) and adding the external dot clocks (22/22).

The code has been tested on a Renesas Salvator-X H3 board, and modifications
to the dw-hdmi Freescale and Rockchip platform glues have been compile-tested.

More improvements to the dw-hdmi driver would be beneficial. The PHY
modularization is based on the limited understanding of the HDMI PHYs used on
the three supported platforms and will possibly be enhanced later when adding
support for more platforms. The DT bindings would also benefit from a
standardized definition of the ports, and the reg-io-width property could
possibly be deprecated in favour of encoding the information in platform glue
code. The patch series is however big enough as is to be submitted for review
and merge before the dw-hdmi driver achieves perfection.

Kieran Bingham (4):
  drm: bridge: dw-hdmi: Remove unused function parameter
  drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter
  drm: bridge: dw-hdmi: Abstract the platform PHY configuration
  drm: bridge: dw-hdmi: Replace device type with platform quirks

Koji Matsuoka (4):
  drm: rcar-du: Add Gen3 HDMI encoder support
  drm: rcar-du: Add DPLL support
  drm: rcar-du: Add HDMI outputs to R8A7795 device description
  arm64: dts: r8a7795: salvator-x: Enable HDMI outputs

Laurent Pinchart (13):
  drm: bridge: dw-hdmi: Merge __hdmi_phy_i2c_write and
    hdmi_phy_i2c_write
  drm: bridge: dw-hdmi: Remove unneeded arguments to bind/unbind
    functions
  drm: bridge: dw-hdmi: Embed drm_bridge in struct dw_hdmi
  drm: bridge: dw-hdmi: Remove encoder field from struct dw_hdmi
  drm: bridge: dw-hdmi: Don't forward HPD events to DRM core before
    attach
  drm: bridge: dw-hdmi: Move IRQ and IO resource allocation to common
    code
  drm: bridge: dw-hdmi: Reorder functions to prepare for next commit
  drm: bridge: dw-hdmi: Create connector in the bridge attach operation
  drm: bridge: dw-hdmi: Implement DRM bridge registration
  dt-bindings: display: dw-hdmi: Clean up DT bindings documentation
  dt-bindings: display: renesas: Add R-Car Gen3 HDMI TX DT bindings
  drm: rcar-du: Skip disabled outputs
  arm64: dts: r8a7795: salvator-x: Add DU1 and DU2 external dot clocks

Ulrich Hecht (1):
  arm64: dts: r8a7795: Add HDMI encoder support

 .../devicetree/bindings/display/bridge/dw_hdmi.txt |  85 ++---
 .../bindings/display/bridge/renesas,dw-hdmi.txt    |  75 +++++
 .../devicetree/bindings/display/imx/hdmi.txt       |  51 +--
 .../bindings/display/rockchip/dw_hdmi-rockchip.txt |  43 ++-
 MAINTAINERS                                        |   1 +
 arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts |  73 +++++
 arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  50 +++
 drivers/gpu/drm/bridge/dw-hdmi.c                   | 364 ++++++++++++---------
 drivers/gpu/drm/bridge/dw-hdmi.h                   |   4 +-
 drivers/gpu/drm/imx/dw_hdmi-imx.c                  |  19 +-
 drivers/gpu/drm/rcar-du/Kconfig                    |   8 +
 drivers/gpu/drm/rcar-du/Makefile                   |   1 +
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  81 ++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h             |   4 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c              |  13 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   1 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c              |   7 +
 drivers/gpu/drm/rcar-du/rcar_du_regs.h             |  23 ++
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c             | 105 ++++++
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        |  17 +-
 include/drm/bridge/dw_hdmi.h                       |  35 +-
 21 files changed, 765 insertions(+), 295 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c

-- 
Regards,

Laurent Pinchart

[PATCH 00/22] R-Car Gen3 HDMI output support

[Laurent Pinchart]

Hello,

This patch series implements support for the HDMI output on Renesas R-Car Gen3
SoCs, and more specifically on the R-Car H3.

R-Car Gen3 SoCs include one or multiple Synopsys DWC HDMI TX controllers. The
series thus starts with 13 cleanup or feature patches for the dw-hdmi driver.
Patches 01/22 to 05/22 are small miscellaneous cleanups. Patch 06/22 fixes a
crash when the HPD interrupt is generated before the bridge gets attached to a
DRM device.

Patch 07/22 then starts refactoring the API towards platform glue by moving
common I/O and clock resource allocation to common code. Patches 08/22 and
09/22 continue by moving connector creation to the bridge attach operation to
comply with the DRM bridge API. Patch 10/22 implements a new API for platform
glue to register the bridge with the DRM core, for platform that doesn't use
the component framework. Patches 11/22 to 13/22 finally abstract PHY setup to
allow platforms to implement configuration for custom PHYs.

The next three patches add glue code for the DWC HDMI on Renesas R-Car Gen3
platforms. Patch 14/22 rework the DWC HDMI DT bindings to make them more
modular, and patch 15/22 makes use of them to document the R-Car Gen DWC HDMI
TX DT bindings. Patch 16/22 then implements the platform glue code.

The next three patches implement support for the R8A7795 HDMI outputs in the
R-Car DU driver. Patch 17/22 fixes a bug that makes the driver defer probe
forever if an encoder is disabled. Patch 18/22 implement DPLL support to
generate more precise clocks required by the HDMI controller, and patch 19/22
then enables the two HDMI output on the R-Car H3 SoC.

The last three patches enable the HDMI outputs on the H3 Salvator-X board by
adding the HDMI encoders to the R-Car H3 DT (20/22), adding HDMI connectors
and enabling the encoders (21/22) and adding the external dot clocks (22/22).

The code has been tested on a Renesas Salvator-X H3 board, and modifications
to the dw-hdmi Freescale and Rockchip platform glues have been compile-tested.

More improvements to the dw-hdmi driver would be beneficial. The PHY
modularization is based on the limited understanding of the HDMI PHYs used on
the three supported platforms and will possibly be enhanced later when adding
support for more platforms. The DT bindings would also benefit from a
standardized definition of the ports, and the reg-io-width property could
possibly be deprecated in favour of encoding the information in platform glue
code. The patch series is however big enough as is to be submitted for review
and merge before the dw-hdmi driver achieves perfection.

Kieran Bingham (4):
  drm: bridge: dw-hdmi: Remove unused function parameter
  drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter
  drm: bridge: dw-hdmi: Abstract the platform PHY configuration
  drm: bridge: dw-hdmi: Replace device type with platform quirks

Koji Matsuoka (4):
  drm: rcar-du: Add Gen3 HDMI encoder support
  drm: rcar-du: Add DPLL support
  drm: rcar-du: Add HDMI outputs to R8A7795 device description
  arm64: dts: r8a7795: salvator-x: Enable HDMI outputs

Laurent Pinchart (13):
  drm: bridge: dw-hdmi: Merge __hdmi_phy_i2c_write and
    hdmi_phy_i2c_write
  drm: bridge: dw-hdmi: Remove unneeded arguments to bind/unbind
    functions
  drm: bridge: dw-hdmi: Embed drm_bridge in struct dw_hdmi
  drm: bridge: dw-hdmi: Remove encoder field from struct dw_hdmi
  drm: bridge: dw-hdmi: Don't forward HPD events to DRM core before
    attach
  drm: bridge: dw-hdmi: Move IRQ and IO resource allocation to common
    code
  drm: bridge: dw-hdmi: Reorder functions to prepare for next commit
  drm: bridge: dw-hdmi: Create connector in the bridge attach operation
  drm: bridge: dw-hdmi: Implement DRM bridge registration
  dt-bindings: display: dw-hdmi: Clean up DT bindings documentation
  dt-bindings: display: renesas: Add R-Car Gen3 HDMI TX DT bindings
  drm: rcar-du: Skip disabled outputs
  arm64: dts: r8a7795: salvator-x: Add DU1 and DU2 external dot clocks

Ulrich Hecht (1):
  arm64: dts: r8a7795: Add HDMI encoder support

 .../devicetree/bindings/display/bridge/dw_hdmi.txt |  85 ++---
 .../bindings/display/bridge/renesas,dw-hdmi.txt    |  75 +++++
 .../devicetree/bindings/display/imx/hdmi.txt       |  51 +--
 .../bindings/display/rockchip/dw_hdmi-rockchip.txt |  43 ++-
 MAINTAINERS                                        |   1 +
 arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts |  73 +++++
 arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  50 +++
 drivers/gpu/drm/bridge/dw-hdmi.c                   | 364 ++++++++++++---------
 drivers/gpu/drm/bridge/dw-hdmi.h                   |   4 +-
 drivers/gpu/drm/imx/dw_hdmi-imx.c                  |  19 +-
 drivers/gpu/drm/rcar-du/Kconfig                    |   8 +
 drivers/gpu/drm/rcar-du/Makefile                   |   1 +
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  81 ++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h             |   4 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c              |  13 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   1 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c              |   7 +
 drivers/gpu/drm/rcar-du/rcar_du_regs.h             |  23 ++
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c             | 105 ++++++
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        |  17 +-
 include/drm/bridge/dw_hdmi.h                       |  35 +-
 21 files changed, 765 insertions(+), 295 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/22] drm: bridge: dw-hdmi: Merge __hdmi_phy_i2c_write and hdmi_phy_i2c_write

[Laurent Pinchart]

The latter is just an int wrapper around the former void function that
unconditionally returns 0. As the return value is never checked, merge
the two functions into one.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index f5009ae39b89..a6685502571e 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -868,7 +868,7 @@ static bool hdmi_phy_wait_i2c_done(struct dw_hdmi *hdmi, int msec)
 	return true;
 }
 
-static void __hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
+static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
 				 unsigned char addr)
 {
 	hdmi_writeb(hdmi, 0xFF, HDMI_IH_I2CMPHY_STAT0);
@@ -882,13 +882,6 @@ static void __hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
 	hdmi_phy_wait_i2c_done(hdmi, 1000);
 }
 
-static int hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
-			      unsigned char addr)
-{
-	__hdmi_phy_i2c_write(hdmi, data, addr);
-	return 0;
-}
-
 static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable)
 {
 	hdmi_mask_writeb(hdmi, !enable, HDMI_PHY_CONF0,
-- 
Regards,

Laurent Pinchart

[PATCH 02/22] drm: bridge: dw-hdmi: Remove unneeded arguments to bind/unbind functions

[Laurent Pinchart]

The master argument isn't used. The data argument, a void pointer, is
used by the bind function only where it's cast to a drm_device pointer,
which can easily be obtained from the encoder argument instead. Remove
them.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c            | 8 +++-----
 drivers/gpu/drm/imx/dw_hdmi-imx.c           | 4 ++--
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 4 ++--
 include/drm/bridge/dw_hdmi.h                | 5 ++---
 4 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index a6685502571e..f86894622070 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1854,12 +1854,10 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
 	return 0;
 }
 
-int dw_hdmi_bind(struct device *dev, struct device *master,
-		 void *data, struct drm_encoder *encoder,
+int dw_hdmi_bind(struct device *dev, struct drm_encoder *encoder,
 		 struct resource *iores, int irq,
 		 const struct dw_hdmi_plat_data *plat_data)
 {
-	struct drm_device *drm = data;
 	struct device_node *np = dev->of_node;
 	struct platform_device_info pdevinfo;
 	struct device_node *ddc_node;
@@ -1992,7 +1990,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 	if (ret)
 		goto err_iahb;
 
-	ret = dw_hdmi_register(drm, hdmi);
+	ret = dw_hdmi_register(encoder->dev, hdmi);
 	if (ret)
 		goto err_iahb;
 
@@ -2059,7 +2057,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_bind);
 
-void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
+void dw_hdmi_unbind(struct device *dev)
 {
 	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
 
diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index 359cd2765552..f79665801543 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -249,7 +249,7 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
+	ret = dw_hdmi_bind(dev, encoder, iores, irq, plat_data);
 
 	/*
 	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
@@ -264,7 +264,7 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 static void dw_hdmi_imx_unbind(struct device *dev, struct device *master,
 			       void *data)
 {
-	return dw_hdmi_unbind(dev, master, data);
+	return dw_hdmi_unbind(dev);
 }
 
 static const struct component_ops dw_hdmi_imx_ops = {
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 0665fb915579..e8fb5c56b224 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -301,7 +301,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
+	ret = dw_hdmi_bind(dev, encoder, iores, irq, plat_data);
 
 	/*
 	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
@@ -316,7 +316,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
 				    void *data)
 {
-	return dw_hdmi_unbind(dev, master, data);
+	return dw_hdmi_unbind(dev);
 }
 
 static const struct component_ops dw_hdmi_rockchip_ops = {
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index bae79f3c4d28..11edda631a9d 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -56,9 +56,8 @@ struct dw_hdmi_plat_data {
 					   struct drm_display_mode *mode);
 };
 
-void dw_hdmi_unbind(struct device *dev, struct device *master, void *data);
-int dw_hdmi_bind(struct device *dev, struct device *master,
-		 void *data, struct drm_encoder *encoder,
+void dw_hdmi_unbind(struct device *dev);
+int dw_hdmi_bind(struct device *dev, struct drm_encoder *encoder,
 		 struct resource *iores, int irq,
 		 const struct dw_hdmi_plat_data *plat_data);
 
-- 
Regards,

Laurent Pinchart

[PATCH 03/22] drm: bridge: dw-hdmi: Remove unused function parameter

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The 'prep' parameter passed to hdmi_phy_configure() is useless. It is
hardcoded as 0, and if set, simply prevents the configure function from
executing.

Remove it.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index f86894622070..5f8044a7d602 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -931,7 +931,7 @@ static void dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)
 			 HDMI_PHY_CONF0_SELDIPIF_MASK);
 }
 
-static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep,
+static int hdmi_phy_configure(struct dw_hdmi *hdmi,
 			      unsigned char res, int cscon)
 {
 	unsigned res_idx;
@@ -941,9 +941,6 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep,
 	const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
 	const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
 
-	if (prep)
-		return -EINVAL;
-
 	switch (res) {
 	case 0:	/* color resolution 0 is 8 bit colour depth */
 	case 8:
@@ -1072,7 +1069,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
 		dw_hdmi_phy_enable_powerdown(hdmi, true);
 
 		/* Enable CSC */
-		ret = hdmi_phy_configure(hdmi, 0, 8, cscon);
+		ret = hdmi_phy_configure(hdmi, 8, cscon);
 		if (ret)
 			return ret;
 	}
-- 
Regards,

Laurent Pinchart

[PATCH 03/22] drm: bridge: dw-hdmi: Remove unused function parameter

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The 'prep' parameter passed to hdmi_phy_configure() is useless. It is
hardcoded as 0, and if set, simply prevents the configure function from
executing.

Remove it.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index f86894622070..5f8044a7d602 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -931,7 +931,7 @@ static void dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)
 			 HDMI_PHY_CONF0_SELDIPIF_MASK);
 }
 
-static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep,
+static int hdmi_phy_configure(struct dw_hdmi *hdmi,
 			      unsigned char res, int cscon)
 {
 	unsigned res_idx;
@@ -941,9 +941,6 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep,
 	const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
 	const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
 
-	if (prep)
-		return -EINVAL;
-
 	switch (res) {
 	case 0:	/* color resolution 0 is 8 bit colour depth */
 	case 8:
@@ -1072,7 +1069,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
 		dw_hdmi_phy_enable_powerdown(hdmi, true);
 
 		/* Enable CSC */
-		ret = hdmi_phy_configure(hdmi, 0, 8, cscon);
+		ret = hdmi_phy_configure(hdmi, 8, cscon);
 		if (ret)
 			return ret;
 	}
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/22] drm: bridge: dw-hdmi: Embed drm_bridge in struct dw_hdmi

[Laurent Pinchart]

The drm_bridge instance is always needed, there's no point in allocating
it separately.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 5f8044a7d602..2c85b6c07a80 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -116,7 +116,7 @@ struct dw_hdmi_i2c {
 struct dw_hdmi {
 	struct drm_connector connector;
 	struct drm_encoder *encoder;
-	struct drm_bridge *bridge;
+	struct drm_bridge bridge;
 
 	struct platform_device *audio;
 	enum dw_hdmi_devtype dev_type;
@@ -1806,7 +1806,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
 		dev_dbg(hdmi->dev, "EVENT=%s\n",
 			phy_int_pol & HDMI_PHY_HPD ? "plugin" : "plugout");
-		drm_helper_hpd_irq_event(hdmi->bridge->dev);
+		drm_helper_hpd_irq_event(hdmi->bridge.dev);
 	}
 
 	hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);
@@ -1819,16 +1819,9 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
 {
 	struct drm_encoder *encoder = hdmi->encoder;
-	struct drm_bridge *bridge;
+	struct drm_bridge *bridge = &hdmi->bridge;
 	int ret;
 
-	bridge = devm_kzalloc(drm->dev, sizeof(*bridge), GFP_KERNEL);
-	if (!bridge) {
-		DRM_ERROR("Failed to allocate drm bridge\n");
-		return -ENOMEM;
-	}
-
-	hdmi->bridge = bridge;
 	bridge->driver_private = hdmi;
 	bridge->funcs = &dw_hdmi_bridge_funcs;
 	ret = drm_bridge_attach(encoder, bridge, NULL);
-- 
Regards,

Laurent Pinchart

[PATCH 05/22] drm: bridge: dw-hdmi: Remove encoder field from struct dw_hdmi

[Laurent Pinchart]

The field isn't needed, remove it.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 2c85b6c07a80..ef10bb866b2f 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -115,7 +115,6 @@ struct dw_hdmi_i2c {
 
 struct dw_hdmi {
 	struct drm_connector connector;
-	struct drm_encoder *encoder;
 	struct drm_bridge bridge;
 
 	struct platform_device *audio;
@@ -1816,9 +1815,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
+static int dw_hdmi_register(struct drm_encoder *encoder, struct dw_hdmi *hdmi)
 {
-	struct drm_encoder *encoder = hdmi->encoder;
 	struct drm_bridge *bridge = &hdmi->bridge;
 	int ret;
 
@@ -1835,7 +1833,7 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
 	drm_connector_helper_add(&hdmi->connector,
 				 &dw_hdmi_connector_helper_funcs);
 
-	drm_connector_init(drm, &hdmi->connector,
+	drm_connector_init(encoder->dev, &hdmi->connector,
 			   &dw_hdmi_connector_funcs,
 			   DRM_MODE_CONNECTOR_HDMIA);
 
@@ -1867,7 +1865,6 @@ int dw_hdmi_bind(struct device *dev, struct drm_encoder *encoder,
 	hdmi->dev = dev;
 	hdmi->dev_type = plat_data->dev_type;
 	hdmi->sample_rate = 48000;
-	hdmi->encoder = encoder;
 	hdmi->disabled = true;
 	hdmi->rxsense = true;
 	hdmi->phy_mask = (u8)~(HDMI_PHY_HPD | HDMI_PHY_RX_SENSE);
@@ -1980,7 +1977,7 @@ int dw_hdmi_bind(struct device *dev, struct drm_encoder *encoder,
 	if (ret)
 		goto err_iahb;
 
-	ret = dw_hdmi_register(encoder->dev, hdmi);
+	ret = dw_hdmi_register(encoder, hdmi);
 	if (ret)
 		goto err_iahb;
 
-- 
Regards,

Laurent Pinchart

[PATCH 06/22] drm: bridge: dw-hdmi: Don't forward HPD events to DRM core before attach

[Laurent Pinchart]

Hotplug events should only be forwarded to the DRM core by the interrupt
handler when the bridge has been attached, otherwise the DRM device
pointer will be NULL, resulting in a crash.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index ef10bb866b2f..488dc1a9204f 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1805,7 +1805,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
 		dev_dbg(hdmi->dev, "EVENT=%s\n",
 			phy_int_pol & HDMI_PHY_HPD ? "plugin" : "plugout");
-		drm_helper_hpd_irq_event(hdmi->bridge.dev);
+		if (hdmi->bridge.dev)
+			drm_helper_hpd_irq_event(hdmi->bridge.dev);
 	}
 
 	hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);
-- 
Regards,

Laurent Pinchart

[PATCH 07/22] drm: bridge: dw-hdmi: Move IRQ and IO resource allocation to common code

[Laurent Pinchart]

There's no need to duplicate identical code in multiple drivers (two at
the moment, one more to come soon). Move it to the dw-hdmi core where it
can be shared. If resource allocation ever becomes device-specific later
we'll always have the option of splitting it out again.

While it at pass the platform device to the bind function to avoid
having to cast struct device to struct platform_device.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c            | 13 ++++++++++---
 drivers/gpu/drm/imx/dw_hdmi-imx.c           | 12 +-----------
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 12 +-----------
 include/drm/bridge/dw_hdmi.h                |  3 +--
 4 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 488dc1a9204f..563cbec47da6 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1843,14 +1843,16 @@ static int dw_hdmi_register(struct drm_encoder *encoder, struct dw_hdmi *hdmi)
 	return 0;
 }
 
-int dw_hdmi_bind(struct device *dev, struct drm_encoder *encoder,
-		 struct resource *iores, int irq,
+int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 		 const struct dw_hdmi_plat_data *plat_data)
 {
+	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct platform_device_info pdevinfo;
 	struct device_node *ddc_node;
 	struct dw_hdmi *hdmi;
+	struct resource *iores;
+	int irq;
 	int ret;
 	u32 val = 1;
 	u8 config0;
@@ -1903,6 +1905,7 @@ int dw_hdmi_bind(struct device *dev, struct drm_encoder *encoder,
 		dev_dbg(hdmi->dev, "no ddc property found\n");
 	}
 
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	hdmi->regs = devm_ioremap_resource(dev, iores);
 	if (IS_ERR(hdmi->regs)) {
 		ret = PTR_ERR(hdmi->regs);
@@ -1945,6 +1948,10 @@ int dw_hdmi_bind(struct device *dev, struct drm_encoder *encoder,
 
 	initialize_hdmi_ih_mutes(hdmi);
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		goto err_iahb;
+
 	ret = devm_request_threaded_irq(dev, irq, dw_hdmi_hardirq,
 					dw_hdmi_irq, IRQF_SHARED,
 					dev_name(dev), hdmi);
@@ -2025,7 +2032,7 @@ int dw_hdmi_bind(struct device *dev, struct drm_encoder *encoder,
 	if (hdmi->i2c)
 		dw_hdmi_i2c_init(hdmi);
 
-	dev_set_drvdata(dev, hdmi);
+	platform_set_drvdata(pdev, hdmi);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index f79665801543..f645275e6e63 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -207,8 +207,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	struct drm_device *drm = data;
 	struct drm_encoder *encoder;
 	struct imx_hdmi *hdmi;
-	struct resource *iores;
-	int irq;
 	int ret;
 
 	if (!pdev->dev.of_node)
@@ -223,14 +221,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	hdmi->dev = &pdev->dev;
 	encoder = &hdmi->encoder;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
-	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!iores)
-		return -ENXIO;
-
 	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
 	/*
 	 * If we failed to find the CRTC(s) which this encoder is
@@ -249,7 +239,7 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(dev, encoder, iores, irq, plat_data);
+	ret = dw_hdmi_bind(pdev, encoder, plat_data);
 
 	/*
 	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index e8fb5c56b224..a6d4a0236e8f 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -257,8 +257,6 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	struct drm_device *drm = data;
 	struct drm_encoder *encoder;
 	struct rockchip_hdmi *hdmi;
-	struct resource *iores;
-	int irq;
 	int ret;
 
 	if (!pdev->dev.of_node)
@@ -273,14 +271,6 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->dev = &pdev->dev;
 	encoder = &hdmi->encoder;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
-	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!iores)
-		return -ENXIO;
-
 	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
 	/*
 	 * If we failed to find the CRTC(s) which this encoder is
@@ -301,7 +291,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(dev, encoder, iores, irq, plat_data);
+	ret = dw_hdmi_bind(pdev, encoder, plat_data);
 
 	/*
 	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 11edda631a9d..94ff6edd070b 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -57,8 +57,7 @@ struct dw_hdmi_plat_data {
 };
 
 void dw_hdmi_unbind(struct device *dev);
-int dw_hdmi_bind(struct device *dev, struct drm_encoder *encoder,
-		 struct resource *iores, int irq,
+int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 		 const struct dw_hdmi_plat_data *plat_data);
 
 void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
-- 
Regards,

Laurent Pinchart

[PATCH 08/22] drm: bridge: dw-hdmi: Reorder functions to prepare for next commit

[Laurent Pinchart]

The next commit will reference structures and functions in a way that
currently requires forward declarations. Reorder the functions to avoid
that. No functional change to the code is performed.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 72 ++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 563cbec47da6..92ce9e532603 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1575,42 +1575,6 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi *hdmi)
 		hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
 }
 
-static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
-				    struct drm_display_mode *orig_mode,
-				    struct drm_display_mode *mode)
-{
-	struct dw_hdmi *hdmi = bridge->driver_private;
-
-	mutex_lock(&hdmi->mutex);
-
-	/* Store the display mode for plugin/DKMS poweron events */
-	memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode));
-
-	mutex_unlock(&hdmi->mutex);
-}
-
-static void dw_hdmi_bridge_disable(struct drm_bridge *bridge)
-{
-	struct dw_hdmi *hdmi = bridge->driver_private;
-
-	mutex_lock(&hdmi->mutex);
-	hdmi->disabled = true;
-	dw_hdmi_update_power(hdmi);
-	dw_hdmi_update_phy_mask(hdmi);
-	mutex_unlock(&hdmi->mutex);
-}
-
-static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
-{
-	struct dw_hdmi *hdmi = bridge->driver_private;
-
-	mutex_lock(&hdmi->mutex);
-	hdmi->disabled = false;
-	dw_hdmi_update_power(hdmi);
-	dw_hdmi_update_phy_mask(hdmi);
-	mutex_unlock(&hdmi->mutex);
-}
-
 static enum drm_connector_status
 dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
@@ -1703,6 +1667,42 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs =
 	.best_encoder = drm_atomic_helper_best_encoder,
 };
 
+static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *orig_mode,
+				    struct drm_display_mode *mode)
+{
+	struct dw_hdmi *hdmi = bridge->driver_private;
+
+	mutex_lock(&hdmi->mutex);
+
+	/* Store the display mode for plugin/DKMS poweron events */
+	memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode));
+
+	mutex_unlock(&hdmi->mutex);
+}
+
+static void dw_hdmi_bridge_disable(struct drm_bridge *bridge)
+{
+	struct dw_hdmi *hdmi = bridge->driver_private;
+
+	mutex_lock(&hdmi->mutex);
+	hdmi->disabled = true;
+	dw_hdmi_update_power(hdmi);
+	dw_hdmi_update_phy_mask(hdmi);
+	mutex_unlock(&hdmi->mutex);
+}
+
+static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
+{
+	struct dw_hdmi *hdmi = bridge->driver_private;
+
+	mutex_lock(&hdmi->mutex);
+	hdmi->disabled = false;
+	dw_hdmi_update_power(hdmi);
+	dw_hdmi_update_phy_mask(hdmi);
+	mutex_unlock(&hdmi->mutex);
+}
+
 static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
 	.enable = dw_hdmi_bridge_enable,
 	.disable = dw_hdmi_bridge_disable,
-- 
Regards,

Laurent Pinchart

[PATCH 09/22] drm: bridge: dw-hdmi: Create connector in the bridge attach operation

[Laurent Pinchart]

The DRM device is not guaranteed by the bridge API to be available
before the attach callback. The driver performs properly at the moment
as it doesn't use the drm_bridge_add() registration method. As this will
be changed later, move connector creation to attach time to ensure
compatibility with the API.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 92ce9e532603..88cd40adb977 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1667,6 +1667,25 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs =
 	.best_encoder = drm_atomic_helper_best_encoder,
 };
 
+static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
+{
+	struct dw_hdmi *hdmi = bridge->driver_private;
+	struct drm_encoder *encoder = bridge->encoder;
+	struct drm_connector *connector = &hdmi->connector;
+
+	connector->interlace_allowed = 1;
+	connector->polled = DRM_CONNECTOR_POLL_HPD;
+
+	drm_connector_helper_add(connector, &dw_hdmi_connector_helper_funcs);
+
+	drm_connector_init(bridge->dev, connector, &dw_hdmi_connector_funcs,
+			   DRM_MODE_CONNECTOR_HDMIA);
+
+	drm_mode_connector_attach_encoder(connector, encoder);
+
+	return 0;
+}
+
 static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
 				    struct drm_display_mode *orig_mode,
 				    struct drm_display_mode *mode)
@@ -1704,6 +1723,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
 }
 
 static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
+	.attach = dw_hdmi_bridge_attach,
 	.enable = dw_hdmi_bridge_enable,
 	.disable = dw_hdmi_bridge_disable,
 	.mode_set = dw_hdmi_bridge_mode_set,
@@ -1829,17 +1849,6 @@ static int dw_hdmi_register(struct drm_encoder *encoder, struct dw_hdmi *hdmi)
 		return -EINVAL;
 	}
 
-	hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
-
-	drm_connector_helper_add(&hdmi->connector,
-				 &dw_hdmi_connector_helper_funcs);
-
-	drm_connector_init(encoder->dev, &hdmi->connector,
-			   &dw_hdmi_connector_funcs,
-			   DRM_MODE_CONNECTOR_HDMIA);
-
-	drm_mode_connector_attach_encoder(&hdmi->connector, encoder);
-
 	return 0;
 }
 
@@ -1862,8 +1871,6 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 	if (!hdmi)
 		return -ENOMEM;
 
-	hdmi->connector.interlace_allowed = 1;
-
 	hdmi->plat_data = plat_data;
 	hdmi->dev = dev;
 	hdmi->dev_type = plat_data->dev_type;
-- 
Regards,

Laurent Pinchart

[PATCH 10/22] drm: bridge: dw-hdmi: Implement DRM bridge registration

[Laurent Pinchart]

As an option for drivers not based on the component framework, register
the bridge with the DRM core with the DRM bridge API. Existing drivers
based on dw_hdmi_bind() and dw_hdmi_unbind() are not affected as those
functions are preserved with their current behaviour.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 112 ++++++++++++++++++++++++++++-----------
 include/drm/bridge/dw_hdmi.h     |   3 ++
 2 files changed, 83 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 88cd40adb977..107fea49c4c6 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1836,24 +1836,9 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int dw_hdmi_register(struct drm_encoder *encoder, struct dw_hdmi *hdmi)
-{
-	struct drm_bridge *bridge = &hdmi->bridge;
-	int ret;
-
-	bridge->driver_private = hdmi;
-	bridge->funcs = &dw_hdmi_bridge_funcs;
-	ret = drm_bridge_attach(encoder, bridge, NULL);
-	if (ret) {
-		DRM_ERROR("Failed to initialize bridge with drm\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
-		 const struct dw_hdmi_plat_data *plat_data)
+static struct dw_hdmi *
+__dw_hdmi_probe(struct platform_device *pdev,
+		const struct dw_hdmi_plat_data *plat_data)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
@@ -1869,7 +1854,7 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 
 	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
 	if (!hdmi)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	hdmi->plat_data = plat_data;
 	hdmi->dev = dev;
@@ -1896,7 +1881,7 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 		break;
 	default:
 		dev_err(dev, "reg-io-width must be 1 or 4\n");
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
@@ -1905,7 +1890,7 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 		of_node_put(ddc_node);
 		if (!hdmi->ddc) {
 			dev_dbg(hdmi->dev, "failed to read ddc node\n");
-			return -EPROBE_DEFER;
+			return ERR_PTR(-EPROBE_DEFER);
 		}
 
 	} else {
@@ -1956,8 +1941,10 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 	initialize_hdmi_ih_mutes(hdmi);
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
+	if (irq < 0) {
+		ret = irq;
 		goto err_iahb;
+	}
 
 	ret = devm_request_threaded_irq(dev, irq, dw_hdmi_hardirq,
 					dw_hdmi_irq, IRQF_SHARED,
@@ -1988,11 +1975,11 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
 		    HDMI_IH_PHY_STAT0);
 
-	ret = dw_hdmi_fb_registered(hdmi);
-	if (ret)
-		goto err_iahb;
+	hdmi->bridge.driver_private = hdmi;
+	hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
+	hdmi->bridge.of_node = pdev->dev.of_node;
 
-	ret = dw_hdmi_register(encoder, hdmi);
+	ret = dw_hdmi_fb_registered(hdmi);
 	if (ret)
 		goto err_iahb;
 
@@ -2041,7 +2028,7 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 
 	platform_set_drvdata(pdev, hdmi);
 
-	return 0;
+	return hdmi;
 
 err_iahb:
 	if (hdmi->i2c) {
@@ -2055,14 +2042,11 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 err_res:
 	i2c_put_adapter(hdmi->ddc);
 
-	return ret;
+	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(dw_hdmi_bind);
 
-void dw_hdmi_unbind(struct device *dev)
+static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
 {
-	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
-
 	if (hdmi->audio && !IS_ERR(hdmi->audio))
 		platform_device_unregister(hdmi->audio);
 
@@ -2077,6 +2061,70 @@ void dw_hdmi_unbind(struct device *dev)
 	else
 		i2c_put_adapter(hdmi->ddc);
 }
+
+/* -----------------------------------------------------------------------------
+ * Probe/remove API, used from platforms based on the DRM bridge API.
+ */
+int dw_hdmi_probe(struct platform_device *pdev,
+		  const struct dw_hdmi_plat_data *plat_data)
+{
+	struct dw_hdmi *hdmi;
+	int ret;
+
+	hdmi = __dw_hdmi_probe(pdev, plat_data);
+	if (IS_ERR(hdmi))
+		return PTR_ERR(hdmi);
+
+	ret = drm_bridge_add(&hdmi->bridge);
+	if (ret < 0) {
+		__dw_hdmi_remove(hdmi);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_probe);
+
+void dw_hdmi_remove(struct platform_device *pdev)
+{
+	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&hdmi->bridge);
+
+	__dw_hdmi_remove(hdmi);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_remove);
+
+/* -----------------------------------------------------------------------------
+ * Bind/unbind API, used from platforms based on the component framework.
+ */
+int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
+		 const struct dw_hdmi_plat_data *plat_data)
+{
+	struct dw_hdmi *hdmi;
+	int ret;
+
+	hdmi = __dw_hdmi_probe(pdev, plat_data);
+	if (IS_ERR(hdmi))
+		return PTR_ERR(hdmi);
+
+	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL);
+	if (ret) {
+		dw_hdmi_remove(pdev);
+		DRM_ERROR("Failed to initialize bridge with drm\n");
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_bind);
+
+void dw_hdmi_unbind(struct device *dev)
+{
+	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
+
+	__dw_hdmi_remove(hdmi);
+}
 EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
 
 MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 94ff6edd070b..3bb22a849830 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -56,6 +56,9 @@ struct dw_hdmi_plat_data {
 					   struct drm_display_mode *mode);
 };
 
+int dw_hdmi_probe(struct platform_device *pdev,
+		  const struct dw_hdmi_plat_data *plat_data);
+void dw_hdmi_remove(struct platform_device *pdev);
 void dw_hdmi_unbind(struct device *dev);
 int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 		 const struct dw_hdmi_plat_data *plat_data);
-- 
Regards,

Laurent Pinchart

[PATCH 11/22] drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The current code hard codes the call of hdmi_phy_configure() to be 8bpp
and provides extraneous error checking to verify that this hardcoded
value is correct.

Simplify the passing of the data by setting the parameter to be of the
enum type it represents rather than converting and then verifying the
value. This will allow the compiler to check the value is acceptable
based on the type, and remove the dead code that we currently have.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 20 ++------------------
 include/drm/bridge/dw_hdmi.h     |  2 +-
 2 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 107fea49c4c6..074ceb1e4897 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -931,30 +931,14 @@ static void dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)
 }
 
 static int hdmi_phy_configure(struct dw_hdmi *hdmi,
-			      unsigned char res, int cscon)
+			      enum dw_hdmi_resolution res_idx, int cscon)
 {
-	unsigned res_idx;
 	u8 val, msec;
 	const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
 	const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
 	const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
 	const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
 
-	switch (res) {
-	case 0:	/* color resolution 0 is 8 bit colour depth */
-	case 8:
-		res_idx = DW_HDMI_RES_8;
-		break;
-	case 10:
-		res_idx = DW_HDMI_RES_10;
-		break;
-	case 12:
-		res_idx = DW_HDMI_RES_12;
-		break;
-	default:
-		return -EINVAL;
-	}
-
 	/* PLL/MPLL Cfg - always match on final entry */
 	for (; mpll_config->mpixelclock != ~0UL; mpll_config++)
 		if (hdmi->hdmi_data.video_mode.mpixelclock <=
@@ -1068,7 +1052,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
 		dw_hdmi_phy_enable_powerdown(hdmi, true);
 
 		/* Enable CSC */
-		ret = hdmi_phy_configure(hdmi, 8, cscon);
+		ret = hdmi_phy_configure(hdmi, DW_HDMI_RES_8, cscon);
 		if (ret)
 			return ret;
 	}
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 3bb22a849830..e551b457c100 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -14,7 +14,7 @@
 
 struct dw_hdmi;
 
-enum {
+enum dw_hdmi_resolution {
 	DW_HDMI_RES_8,
 	DW_HDMI_RES_10,
 	DW_HDMI_RES_12,
-- 
Regards,

Laurent Pinchart

[PATCH 12/22] drm: bridge: dw-hdmi: Abstract the platform PHY configuration

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Platforms implement the dw-hdmi with a separate PHY entity. It is
configured over it's own I2C bus. To allow for different PHY's to be
configured from the dw-hdmi driver, abstract the actual programming of
the PHY to its own functions, as configured by the platform.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c            | 79 ++++++++++++++++++-----------
 drivers/gpu/drm/imx/dw_hdmi-imx.c           |  2 +
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  1 +
 include/drm/bridge/dw_hdmi.h                | 12 +++++
 4 files changed, 63 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 074ceb1e4897..06a44a2cdf3b 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -867,8 +867,8 @@ static bool hdmi_phy_wait_i2c_done(struct dw_hdmi *hdmi, int msec)
 	return true;
 }
 
-static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
-				 unsigned char addr)
+void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
+			   unsigned char addr)
 {
 	hdmi_writeb(hdmi, 0xFF, HDMI_IH_I2CMPHY_STAT0);
 	hdmi_writeb(hdmi, addr, HDMI_PHY_I2CM_ADDRESS_ADDR);
@@ -880,6 +880,7 @@ static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
 		    HDMI_PHY_I2CM_OPERATION_ADDR);
 	hdmi_phy_wait_i2c_done(hdmi, 1000);
 }
+EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_write);
 
 static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable)
 {
@@ -930,38 +931,61 @@ static void dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)
 			 HDMI_PHY_CONF0_SELDIPIF_MASK);
 }
 
-static int hdmi_phy_configure(struct dw_hdmi *hdmi,
-			      enum dw_hdmi_resolution res_idx, int cscon)
+int dw_hdmi_phy_configure_synopsys(struct dw_hdmi *hdmi,
+				   const struct dw_hdmi_plat_data *pdata,
+				   unsigned long mpixelclock,
+				   enum dw_hdmi_resolution resolution)
+
 {
-	u8 val, msec;
-	const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
 	const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
 	const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
 	const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
 
 	/* PLL/MPLL Cfg - always match on final entry */
 	for (; mpll_config->mpixelclock != ~0UL; mpll_config++)
-		if (hdmi->hdmi_data.video_mode.mpixelclock <=
-		    mpll_config->mpixelclock)
+		if (mpixelclock <= mpll_config->mpixelclock)
 			break;
 
 	for (; curr_ctrl->mpixelclock != ~0UL; curr_ctrl++)
-		if (hdmi->hdmi_data.video_mode.mpixelclock <=
-		    curr_ctrl->mpixelclock)
+		if (mpixelclock <= curr_ctrl->mpixelclock)
 			break;
 
 	for (; phy_config->mpixelclock != ~0UL; phy_config++)
-		if (hdmi->hdmi_data.video_mode.mpixelclock <=
-		    phy_config->mpixelclock)
+		if (mpixelclock <= phy_config->mpixelclock)
 			break;
 
 	if (mpll_config->mpixelclock == ~0UL ||
 	    curr_ctrl->mpixelclock == ~0UL ||
-	    phy_config->mpixelclock == ~0UL) {
-		dev_err(hdmi->dev, "Pixel clock %d - unsupported by HDMI\n",
-			hdmi->hdmi_data.video_mode.mpixelclock);
+	    phy_config->mpixelclock == ~0UL)
 		return -EINVAL;
-	}
+
+	dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[resolution].cpce, 0x06);
+	dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[resolution].gmp, 0x15);
+
+	/* CURRCTRL */
+	dw_hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[resolution], 0x10);
+
+	dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x13); /* PLLPHBYCTRL */
+	dw_hdmi_phy_i2c_write(hdmi, 0x0006, 0x17);
+
+	dw_hdmi_phy_i2c_write(hdmi, phy_config->term, 0x19); /* TXTERM */
+	dw_hdmi_phy_i2c_write(hdmi, phy_config->sym_ctr, 0x09); /* CKSYMTXCTRL */
+	dw_hdmi_phy_i2c_write(hdmi, phy_config->vlev_ctr, 0x0E); /* VLEVCTRL */
+
+	/* REMOVE CLK TERM */
+	dw_hdmi_phy_i2c_write(hdmi, 0x8000, 0x05); /* CKCALCTRL */
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_phy_configure_synopsys);
+
+static int hdmi_phy_configure(struct dw_hdmi *hdmi,
+			      enum dw_hdmi_resolution resolution, int cscon)
+{
+	const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
+	unsigned long mpixelclock = hdmi->hdmi_data.video_mode.mpixelclock;
+	u8 val, msec;
+	int ret;
 
 	/* Enable csc path */
 	if (cscon)
@@ -988,21 +1012,14 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
 		    HDMI_PHY_I2CM_SLAVE_ADDR);
 	hdmi_phy_test_clear(hdmi, 0);
 
-	hdmi_phy_i2c_write(hdmi, mpll_config->res[res_idx].cpce, 0x06);
-	hdmi_phy_i2c_write(hdmi, mpll_config->res[res_idx].gmp, 0x15);
-
-	/* CURRCTRL */
-	hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[res_idx], 0x10);
-
-	hdmi_phy_i2c_write(hdmi, 0x0000, 0x13);  /* PLLPHBYCTRL */
-	hdmi_phy_i2c_write(hdmi, 0x0006, 0x17);
-
-	hdmi_phy_i2c_write(hdmi, phy_config->term, 0x19);  /* TXTERM */
-	hdmi_phy_i2c_write(hdmi, phy_config->sym_ctr, 0x09); /* CKSYMTXCTRL */
-	hdmi_phy_i2c_write(hdmi, phy_config->vlev_ctr, 0x0E); /* VLEVCTRL */
-
-	/* REMOVE CLK TERM */
-	hdmi_phy_i2c_write(hdmi, 0x8000, 0x05);  /* CKCALCTRL */
+	/* Write to the PHY as configured by the platform */
+	ret = pdata->configure_phy(hdmi, pdata, mpixelclock, resolution);
+	if (ret) {
+		dev_err(hdmi->dev,
+			"PHY configuration failed (clock %lu resolution %u)\n",
+			mpixelclock, resolution);
+		return ret;
+	}
 
 	dw_hdmi_phy_enable_powerdown(hdmi, false);
 
diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index f645275e6e63..f1cb25c429cf 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -177,6 +177,7 @@ static struct dw_hdmi_plat_data imx6q_hdmi_drv_data = {
 	.phy_config = imx_phy_config,
 	.dev_type   = IMX6Q_HDMI,
 	.mode_valid = imx6q_hdmi_mode_valid,
+	.configure_phy = dw_hdmi_phy_configure_synopsys,
 };
 
 static struct dw_hdmi_plat_data imx6dl_hdmi_drv_data = {
@@ -185,6 +186,7 @@ static struct dw_hdmi_plat_data imx6dl_hdmi_drv_data = {
 	.phy_config = imx_phy_config,
 	.dev_type = IMX6DL_HDMI,
 	.mode_valid = imx6dl_hdmi_mode_valid,
+	.configure_phy = dw_hdmi_phy_configure_synopsys,
 };
 
 static const struct of_device_id dw_hdmi_imx_dt_ids[] = {
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index a6d4a0236e8f..55b1f2f27d6e 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -237,6 +237,7 @@ static const struct dw_hdmi_plat_data rockchip_hdmi_drv_data = {
 	.mpll_cfg   = rockchip_mpll_cfg,
 	.cur_ctr    = rockchip_cur_ctr,
 	.phy_config = rockchip_phy_config,
+	.configure_phy = dw_hdmi_phy_configure_synopsys,
 	.dev_type   = RK3288_HDMI,
 };
 
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index e551b457c100..fa7655836c81 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -52,6 +52,10 @@ struct dw_hdmi_plat_data {
 	const struct dw_hdmi_mpll_config *mpll_cfg;
 	const struct dw_hdmi_curr_ctrl *cur_ctr;
 	const struct dw_hdmi_phy_config *phy_config;
+	int (*configure_phy)(struct dw_hdmi *hdmi,
+			     const struct dw_hdmi_plat_data *pdata,
+			     unsigned long mpixelclock,
+			     enum dw_hdmi_resolution resolution);
 	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
 					   struct drm_display_mode *mode);
 };
@@ -67,4 +71,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
 void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
 void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
 
+/* PHY configuration */
+void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
+			   unsigned char addr);
+int dw_hdmi_phy_configure_synopsys(struct dw_hdmi *hdmi,
+				   const struct dw_hdmi_plat_data *pdata,
+				   unsigned long mpixelclock,
+				   enum dw_hdmi_resolution resolution);
+
 #endif /* __IMX_HDMI_H__ */
-- 
Regards,

Laurent Pinchart

[PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The dw-hdmi driver declares a dev_type to distinguish platform specific
changes. Replace this with a quirk field, so that the platform can
specify the required quirks for the driver, rather than the driver
becoming conditional on multiple platforms.

As part of this, we rename the dw-hdmi 'spare' which is defined as the
SVSRET bit in later documentation.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
 drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
 drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
 include/drm/bridge/dw_hdmi.h                | 12 +++++-------
 5 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 06a44a2cdf3b..26607185722f 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -118,7 +118,6 @@ struct dw_hdmi {
 	struct drm_bridge bridge;
 
 	struct platform_device *audio;
-	enum dw_hdmi_devtype dev_type;
 	struct device *dev;
 	struct clk *isfr_clk;
 	struct clk *iahb_clk;
@@ -896,11 +895,11 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi *hdmi, u8 enable)
 			 HDMI_PHY_CONF0_ENTMDS_MASK);
 }
 
-static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
+static void dw_hdmi_phy_enable_svsret(struct dw_hdmi *hdmi, u8 enable)
 {
 	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
-			 HDMI_PHY_CONF0_SPARECTRL_OFFSET,
-			 HDMI_PHY_CONF0_SPARECTRL_MASK);
+			 HDMI_PHY_CONF0_SVSRET_OFFSET,
+			 HDMI_PHY_CONF0_SVSRET_MASK);
 }
 
 static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
@@ -1031,8 +1030,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
 	dw_hdmi_phy_gen2_txpwron(hdmi, 1);
 	dw_hdmi_phy_gen2_pddq(hdmi, 0);
 
-	if (hdmi->dev_type == RK3288_HDMI)
-		dw_hdmi_phy_enable_spare(hdmi, 1);
+	if (pdata->quirks & DW_HDMI_QUIRK_PHY_SVSRET)
+		dw_hdmi_phy_enable_svsret(hdmi, 1);
 
 	/*Wait for PHY PLL lock */
 	msec = 5;
@@ -1348,7 +1347,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
 	hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
 
 	val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
-	if (hdmi->dev_type == IMX6DL_HDMI) {
+	if (hdmi->plat_data->quirks & DW_HDMI_QUIRK_FC_INVIDCONF) {
 		hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
 		return;
 	}
@@ -1859,7 +1858,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
 
 	hdmi->plat_data = plat_data;
 	hdmi->dev = dev;
-	hdmi->dev_type = plat_data->dev_type;
 	hdmi->sample_rate = 48000;
 	hdmi->disabled = true;
 	hdmi->rxsense = true;
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
index 55135bbd0c16..08235aef2fa3 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/dw-hdmi.h
@@ -847,8 +847,8 @@ enum {
 	HDMI_PHY_CONF0_PDZ_OFFSET = 7,
 	HDMI_PHY_CONF0_ENTMDS_MASK = 0x40,
 	HDMI_PHY_CONF0_ENTMDS_OFFSET = 6,
-	HDMI_PHY_CONF0_SPARECTRL_MASK = 0x20,
-	HDMI_PHY_CONF0_SPARECTRL_OFFSET = 5,
+	HDMI_PHY_CONF0_SVSRET_MASK = 0x20,
+	HDMI_PHY_CONF0_SVSRET_OFFSET = 5,
 	HDMI_PHY_CONF0_GEN2_PDDQ_MASK = 0x10,
 	HDMI_PHY_CONF0_GEN2_PDDQ_OFFSET = 4,
 	HDMI_PHY_CONF0_GEN2_TXPWRON_MASK = 0x8,
diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index f1cb25c429cf..404def112f5d 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -175,7 +175,6 @@ static struct dw_hdmi_plat_data imx6q_hdmi_drv_data = {
 	.mpll_cfg   = imx_mpll_cfg,
 	.cur_ctr    = imx_cur_ctr,
 	.phy_config = imx_phy_config,
-	.dev_type   = IMX6Q_HDMI,
 	.mode_valid = imx6q_hdmi_mode_valid,
 	.configure_phy = dw_hdmi_phy_configure_synopsys,
 };
@@ -184,7 +183,7 @@ static struct dw_hdmi_plat_data imx6dl_hdmi_drv_data = {
 	.mpll_cfg = imx_mpll_cfg,
 	.cur_ctr  = imx_cur_ctr,
 	.phy_config = imx_phy_config,
-	.dev_type = IMX6DL_HDMI,
+	.quirks   = DW_HDMI_QUIRK_FC_INVIDCONF,
 	.mode_valid = imx6dl_hdmi_mode_valid,
 	.configure_phy = dw_hdmi_phy_configure_synopsys,
 };
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 55b1f2f27d6e..4f6ff30fa091 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -238,7 +238,7 @@ static const struct dw_hdmi_plat_data rockchip_hdmi_drv_data = {
 	.cur_ctr    = rockchip_cur_ctr,
 	.phy_config = rockchip_phy_config,
 	.configure_phy = dw_hdmi_phy_configure_synopsys,
-	.dev_type   = RK3288_HDMI,
+	.quirks     = DW_HDMI_QUIRK_PHY_SVSRET,
 };
 
 static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index fa7655836c81..4b12b8fe765e 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -21,12 +21,6 @@ enum dw_hdmi_resolution {
 	DW_HDMI_RES_MAX,
 };
 
-enum dw_hdmi_devtype {
-	IMX6Q_HDMI,
-	IMX6DL_HDMI,
-	RK3288_HDMI,
-};
-
 struct dw_hdmi_mpll_config {
 	unsigned long mpixelclock;
 	struct {
@@ -47,11 +41,15 @@ struct dw_hdmi_phy_config {
 	u16 vlev_ctr;   /* voltage level control */
 };
 
+/* Define DW HDMI platform-specific quirks */
+#define DW_HDMI_QUIRK_FC_INVIDCONF		BIT(0)
+#define DW_HDMI_QUIRK_PHY_SVSRET		BIT(1)
+
 struct dw_hdmi_plat_data {
-	enum dw_hdmi_devtype dev_type;
 	const struct dw_hdmi_mpll_config *mpll_cfg;
 	const struct dw_hdmi_curr_ctrl *cur_ctr;
 	const struct dw_hdmi_phy_config *phy_config;
+	unsigned int quirks;
 	int (*configure_phy)(struct dw_hdmi *hdmi,
 			     const struct dw_hdmi_plat_data *pdata,
 			     unsigned long mpixelclock,
-- 
Regards,

Laurent Pinchart

[PATCH 14/22] dt-bindings: display: dw-hdmi: Clean up DT bindings documentation

[Laurent Pinchart]

Make it clear that the core bridge/dw_hdmi.txt document isn't a device
tree binding by itself but is meant to be referenced by platform device
tree bindings, and update the Rockchip and Freescale DWC HDMI TX
bindings to reference it.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../devicetree/bindings/display/bridge/dw_hdmi.txt | 85 +++++++++-------------
 .../devicetree/bindings/display/imx/hdmi.txt       | 51 +++++++------
 .../bindings/display/rockchip/dw_hdmi-rockchip.txt | 43 +++++++----
 3 files changed, 91 insertions(+), 88 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt b/Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt
index 5e9a84d6e5f1..33bf981fbe33 100644
--- a/Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt
@@ -1,52 +1,33 @@
-DesignWare HDMI bridge bindings
-
-Required properties:
-- compatible: platform specific such as:
-   * "snps,dw-hdmi-tx"
-   * "fsl,imx6q-hdmi"
-   * "fsl,imx6dl-hdmi"
-   * "rockchip,rk3288-dw-hdmi"
-- reg: Physical base address and length of the controller's registers.
-- interrupts: The HDMI interrupt number
-- clocks, clock-names : must have the phandles to the HDMI iahb and isfr clocks,
-  as described in Documentation/devicetree/bindings/clock/clock-bindings.txt,
-  the clocks are soc specific, the clock-names should be "iahb", "isfr"
--port@[X]: SoC specific port nodes with endpoint definitions as defined
-   in Documentation/devicetree/bindings/media/video-interfaces.txt,
-   please refer to the SoC specific binding document:
-    * Documentation/devicetree/bindings/display/imx/hdmi.txt
-    * Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
-
-Optional properties
-- reg-io-width: the width of the reg:1,4, default set to 1 if not present
-- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing,
-  if the property is omitted, a functionally reduced I2C bus
-  controller on DW HDMI is probed
-- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
-
-Example:
-	hdmi: hdmi@0120000 {
-		compatible = "fsl,imx6q-hdmi";
-		reg = <0x00120000 0x9000>;
-		interrupts = <0 115 0x04>;
-		gpr = <&gpr>;
-		clocks = <&clks 123>, <&clks 124>;
-		clock-names = "iahb", "isfr";
-		ddc-i2c-bus = <&i2c2>;
-
-		port@0 {
-			reg = <0>;
-
-			hdmi_mux_0: endpoint {
-				remote-endpoint = <&ipu1_di0_hdmi>;
-			};
-		};
-
-		port@1 {
-			reg = <1>;
-
-			hdmi_mux_1: endpoint {
-				remote-endpoint = <&ipu1_di1_hdmi>;
-			};
-		};
-	};
+Synopsys DesignWare HDMI TX Encoder
+===================================
+
+This document defines device tree properties for the Synopsys DesignWare HDMI
+TX Encoder (DWC HDMI TX). It doesn't constitue a device tree binding
+specification by itself but is meant to be referenced by platform-specific
+device tree bindings.
+
+When referenced from platform device tree bindings the properties defined in
+this document are defined as follows. The platform device tree bindings are
+responsible for defining whether each property is required or optional.
+
+- reg: Memory mapped base address and length of the DWC HDMI TX registers.
+
+- reg-io-width: Width of the registers specified by the reg property. The
+  value is expressed in bytes and must be equal to 1 or 4 if specified. The
+  register width defaults to 1 if the property is not present.
+
+- interrupts: Reference to the DWC HDMI TX interrupt.
+
+- clocks: References to all the clocks specified in the clock-names property
+  as specified in Documentation/devicetree/bindings/clock/clock-bindings.txt.
+
+- clock-names: The DWC HDMI TX uses the following clocks.
+
+  - "iahb" is the bus clock for either AHB and APB (mandatory).
+  - "isfr" is the internal register configuration clock (mandatory).
+  - "cec" is the HDMI CEC controller main clock (optional).
+
+- ports: The connectivity of the DWC HDMI TX with the rest of the system is
+  expressed in using ports as specified in the device graph bindings defined
+  in Documentation/devicetree/bindings/graph.txt. The numbering of the ports
+  is platform-specific.
diff --git a/Documentation/devicetree/bindings/display/imx/hdmi.txt b/Documentation/devicetree/bindings/display/imx/hdmi.txt
index 1b756cf9afb0..66a8f86e5d12 100644
--- a/Documentation/devicetree/bindings/display/imx/hdmi.txt
+++ b/Documentation/devicetree/bindings/display/imx/hdmi.txt
@@ -1,29 +1,36 @@
-Device-Tree bindings for HDMI Transmitter
+Freescale i.MX6 DWC HDMI TX Encoder
+===================================
 
-HDMI Transmitter
-================
+The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
+with a companion PHY IP.
+
+These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
+Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
+following device-specific properties.
 
-The HDMI Transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
-with accompanying PHY IP.
 
 Required properties:
- - #address-cells : should be <1>
- - #size-cells : should be <0>
- - compatible : should be "fsl,imx6q-hdmi" or "fsl,imx6dl-hdmi".
- - gpr : should be <&gpr>.
-   The phandle points to the iomuxc-gpr region containing the HDMI
-   multiplexer control register.
- - clocks, clock-names : phandles to the HDMI iahb and isrf clocks, as described
-   in Documentation/devicetree/bindings/clock/clock-bindings.txt and
-   Documentation/devicetree/bindings/clock/imx6q-clock.txt.
- - port@[0-4]: Up to four port nodes with endpoint definitions as defined in
-   Documentation/devicetree/bindings/media/video-interfaces.txt,
-   corresponding to the four inputs to the HDMI multiplexer.
-
-Optional properties:
- - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
-
-example:
+
+- compatible : Shall be one of "fsl,imx6q-hdmi" or "fsl,imx6dl-hdmi".
+- reg: See dw_hdmi.txt.
+- interrupts: HDMI interrupt number
+- clocks: See dw_hdmi.txt.
+- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
+- ports: See dw_hdmi.txt. The DWC HDMI shall have between one and four ports,
+  numbered 0 to 3, corresponding to the four inputs of the HDMI multiplexer.
+  Each port shall have a single endpoint.
+- gpr : Shall contain a phandle to the iomuxc-gpr region containing the HDMI
+  multiplexer control register.
+
+Optional properties
+
+- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
+  or the functionally-reduced I2C master contained in the DWC HDMI. When
+  connected to a system I2C master this property contains a phandle to that
+  I2C master controller.
+
+
+Example:
 
 	gpr: iomuxc-gpr@020e0000 {
 		/* ... */
diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
index 668091f27674..046076c6b277 100644
--- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
@@ -1,24 +1,39 @@
-Rockchip specific extensions to the Synopsys Designware HDMI
-================================
+Rockchip DWC HDMI TX Encoder
+============================
+
+The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
+with a companion PHY IP.
+
+These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
+Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
+following device-specific properties.
+
 
 Required properties:
-- compatible: "rockchip,rk3288-dw-hdmi";
-- reg: Physical base address and length of the controller's registers.
-- clocks: phandle to hdmi iahb and isfr clocks.
-- clock-names: should be "iahb" "isfr"
-- rockchip,grf: this soc should set GRF regs to mux vopl/vopb.
+
+- compatible: Shall contain "rockchip,rk3288-dw-hdmi".
+- reg: See dw_hdmi.txt.
+- reg-io-width: See dw_hdmi.txt. Shall be 4.
 - interrupts: HDMI interrupt number
-- ports: contain a port node with endpoint definitions as defined in
-  Documentation/devicetree/bindings/media/video-interfaces.txt. For
-  vopb,set the reg = <0> and set the reg = <1> for vopl.
-- reg-io-width: the width of the reg:1,4, the value should be 4 on
-  rk3288 platform
+- clocks: See dw_hdmi.txt.
+- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
+- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
+  corresponding to the video input of the controller. The port shall have two
+  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
+- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
 
 Optional properties
-- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
-- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
+
+- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
+  or the functionally-reduced I2C master contained in the DWC HDMI. When
+  connected to a system I2C master this property contains a phandle to that
+  I2C master controller.
+- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
+- clock-names: May contain "cec" as defined in dw_hdmi.txt.
+
 
 Example:
+
 hdmi: hdmi@ff980000 {
 	compatible = "rockchip,rk3288-dw-hdmi";
 	reg = <0xff980000 0x20000>;
-- 
Regards,

Laurent Pinchart

[PATCH 15/22] dt-bindings: display: renesas: Add R-Car Gen3 HDMI TX DT bindings

[Laurent Pinchart]

The Renesas R-Car Gen3 SoCs use a Synopsys DWC HDMI TX encoder IP. Add
corresponding device tree bindings based on the DWC HDMI TX bindings
model.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 .../bindings/display/bridge/renesas,dw-hdmi.txt    | 75 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt b/Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
new file mode 100644
index 000000000000..f6b3f36d422b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
@@ -0,0 +1,75 @@
+Renesas Gen3 DWC HDMI TX Encoder
+================================
+
+The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
+with a companion PHY IP.
+
+These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
+Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
+following device-specific properties.
+
+
+Required properties:
+
+- compatible : Shall contain one or more of
+  - "renesas,r8a7795-hdmi" for R8A7795 (R-Car H3) compatible HDMI TX
+  - "renesas,rcar-gen3-hdmi" for the generic R-Car Gen3 compatible HDMI TX
+
+    When compatible with generic versions, nodes must list the SoC-specific
+    version corresponding to the platform first, followed by the
+    family-specific version.
+
+- reg: See dw_hdmi.txt.
+- interrupts: HDMI interrupt number
+- clocks: See dw_hdmi.txt.
+- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
+- ports: See dw_hdmi.txt. The DWC HDMI shall have one port numbered 0
+  corresponding to the video input of the controller and one port numbered 1
+  corresponding to its HDMI output. Each port shall have a single endpoint.
+
+Optional properties:
+
+- power-domains: Shall reference the power domain that contains the DWC HDMI,
+  if any.
+
+
+Example:
+
+	hdmi0: hdmi0@fead0000 {
+		compatible = "renesas,r8a7795-dw-hdmi";
+		reg = <0 0xfead0000 0 0x10000>;
+		interrupts = <0 389 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cpg CPG_CORE R8A7795_CLK_S0D4>, <&cpg CPG_MOD 729>;
+		clock-names = "iahb", "isfr";
+		power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+		status = "disabled";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port@0 {
+				reg = <0>;
+				dw_hdmi0_in: endpoint {
+					remote-endpoint = <&du_out_hdmi0>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				rcar_dw_hdmi0_out: endpoint {
+					remote-endpoint = <&hdmi0_con>;
+				};
+			};
+		};
+	};
+
+	hdmi0-out {
+		compatible = "hdmi-connector";
+		label = "HDMI0 OUT";
+		type = "a";
+
+		port {
+			hdmi0_con: endpoint {
+				remote-endpoint = <&rcar_dw_hdmi0_out>;
+			};
+		};
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 30c873192458..a513fda157d0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4231,6 +4231,7 @@ S:	Supported
 F:	drivers/gpu/drm/rcar-du/
 F:	drivers/gpu/drm/shmobile/
 F:	include/linux/platform_data/shmob_drm.h
+F:	Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
 F:	Documentation/devicetree/bindings/display/renesas,du.txt
 
 DRM DRIVER FOR QXL VIRTUAL GPU
-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 15/22] dt-bindings: display: renesas: Add R-Car Gen3 HDMI TX DT bindings

[Laurent Pinchart]

The Renesas R-Car Gen3 SoCs use a Synopsys DWC HDMI TX encoder IP. Add
corresponding device tree bindings based on the DWC HDMI TX bindings
model.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../bindings/display/bridge/renesas,dw-hdmi.txt    | 75 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt b/Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
new file mode 100644
index 000000000000..f6b3f36d422b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
@@ -0,0 +1,75 @@
+Renesas Gen3 DWC HDMI TX Encoder
+================================
+
+The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
+with a companion PHY IP.
+
+These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
+Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
+following device-specific properties.
+
+
+Required properties:
+
+- compatible : Shall contain one or more of
+  - "renesas,r8a7795-hdmi" for R8A7795 (R-Car H3) compatible HDMI TX
+  - "renesas,rcar-gen3-hdmi" for the generic R-Car Gen3 compatible HDMI TX
+
+    When compatible with generic versions, nodes must list the SoC-specific
+    version corresponding to the platform first, followed by the
+    family-specific version.
+
+- reg: See dw_hdmi.txt.
+- interrupts: HDMI interrupt number
+- clocks: See dw_hdmi.txt.
+- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
+- ports: See dw_hdmi.txt. The DWC HDMI shall have one port numbered 0
+  corresponding to the video input of the controller and one port numbered 1
+  corresponding to its HDMI output. Each port shall have a single endpoint.
+
+Optional properties:
+
+- power-domains: Shall reference the power domain that contains the DWC HDMI,
+  if any.
+
+
+Example:
+
+	hdmi0: hdmi0@fead0000 {
+		compatible = "renesas,r8a7795-dw-hdmi";
+		reg = <0 0xfead0000 0 0x10000>;
+		interrupts = <0 389 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cpg CPG_CORE R8A7795_CLK_S0D4>, <&cpg CPG_MOD 729>;
+		clock-names = "iahb", "isfr";
+		power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+		status = "disabled";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port@0 {
+				reg = <0>;
+				dw_hdmi0_in: endpoint {
+					remote-endpoint = <&du_out_hdmi0>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				rcar_dw_hdmi0_out: endpoint {
+					remote-endpoint = <&hdmi0_con>;
+				};
+			};
+		};
+	};
+
+	hdmi0-out {
+		compatible = "hdmi-connector";
+		label = "HDMI0 OUT";
+		type = "a";
+
+		port {
+			hdmi0_con: endpoint {
+				remote-endpoint = <&rcar_dw_hdmi0_out>;
+			};
+		};
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 30c873192458..a513fda157d0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4231,6 +4231,7 @@ S:	Supported
 F:	drivers/gpu/drm/rcar-du/
 F:	drivers/gpu/drm/shmobile/
 F:	include/linux/platform_data/shmob_drm.h
+F:	Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
 F:	Documentation/devicetree/bindings/display/renesas,du.txt
 
 DRM DRIVER FOR QXL VIRTUAL GPU
-- 
Regards,

Laurent Pinchart

[PATCH 16/22] drm: rcar-du: Add Gen3 HDMI encoder support

[Laurent Pinchart]

From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

The R-Car Gen3 SoCs include on-chip DesignWare HDMI encoders. Support
them with a platform driver to provide platform glue data to the dw-hdmi
driver.

The driver is a complete rewrite of code coming from the Renesas BSP,
save for the values in the PHY parameters table.

Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/Kconfig        |   8 +++
 drivers/gpu/drm/rcar-du/Makefile       |   1 +
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 105 +++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 06121eeba9e5..a356d2f2f6d6 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -11,6 +11,14 @@ config DRM_RCAR_DU
 	  Choose this option if you have an R-Car chipset.
 	  If M is selected the module will be called rcar-du-drm.
 
+config DRM_RCAR_DW_HDMI
+	bool "R-Car DU Gen3 HDMI Encoder Support"
+	depends on DRM_RCAR_DU
+	depends on OF
+	select DRM_DW_HDMI
+	help
+	  Enable support for R-Car Gen3 internal HDMI encoder.
+
 config DRM_RCAR_LVDS
 	bool "R-Car DU LVDS Encoder Support"
 	depends on DRM_RCAR_DU
diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
index a492e6858691..2131e722de3b 100644
--- a/drivers/gpu/drm/rcar-du/Makefile
+++ b/drivers/gpu/drm/rcar-du/Makefile
@@ -11,3 +11,4 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)	+= rcar_du_lvdsenc.o
 rcar-du-drm-$(CONFIG_DRM_RCAR_VSP)	+= rcar_du_vsp.o
 
 obj-$(CONFIG_DRM_RCAR_DU)		+= rcar-du-drm.o
+obj-$(CONFIG_DRM_RCAR_DW_HDMI)		+= rcar_dw_hdmi.o
diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
new file mode 100644
index 000000000000..1921d7aa6aed
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
@@ -0,0 +1,105 @@
+/*
+ * R-Car Gen3 HDMI PHY
+ *
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ *
+ * Contact: Laurent Pinchart (laurent.pinchart@ideasonboard.com)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <drm/bridge/dw_hdmi.h>
+
+#define RCAR_HDMI_PHY_OPMODE_PLLCFG	0x06	/* Mode of operation and PLL dividers */
+#define RCAR_HDMI_PHY_PLLCURRGMPCTRL	0x10	/* PLL current and Gmp (conductance) */
+#define RCAR_HDMI_PHY_PLLDIVCTRL	0x11	/* PLL dividers */
+
+struct rcar_hdmi_phy_params {
+	unsigned long mpixelclock;
+	u16 opmode_div;	/* Mode of operation and PLL dividers */
+	u16 curr_gmp;	/* PLL current and Gmp (conductance) */
+	u16 div;	/* PLL dividers */
+};
+
+static const struct rcar_hdmi_phy_params rcar_hdmi_phy_params[] = {
+	{ 35500000,  0x0003, 0x0344, 0x0328 },
+	{ 44900000,  0x0003, 0x0285, 0x0128 },
+	{ 71000000,  0x0002, 0x1184, 0x0314 },
+	{ 90000000,  0x0002, 0x1144, 0x0114 },
+	{ 140250000, 0x0001, 0x20c4, 0x030a },
+	{ 182750000, 0x0001, 0x2084, 0x010a },
+	{ 281250000, 0x0000, 0x0084, 0x0305 },
+	{ 297000000, 0x0000, 0x0084, 0x0105 },
+	{ ~0UL,      0x0000, 0x0000, 0x0000 },
+};
+
+static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
+				   const struct dw_hdmi_plat_data *pdata,
+				   unsigned long mpixelclock,
+				   enum dw_hdmi_resolution resolution)
+{
+	const struct rcar_hdmi_phy_params *params = rcar_hdmi_phy_params;
+
+	if (resolution != DW_HDMI_RES_8)
+		return -EINVAL;
+
+	for (; params && params->mpixelclock != ~0UL; ++params) {
+		if (mpixelclock <= params->mpixelclock)
+			break;
+	}
+
+	if (params->mpixelclock == ~0UL)
+		return -EINVAL;
+
+	dw_hdmi_phy_i2c_write(hdmi, params->opmode_div,
+			      RCAR_HDMI_PHY_OPMODE_PLLCFG);
+	dw_hdmi_phy_i2c_write(hdmi, params->curr_gmp,
+			      RCAR_HDMI_PHY_PLLCURRGMPCTRL);
+	dw_hdmi_phy_i2c_write(hdmi, params->div, RCAR_HDMI_PHY_PLLDIVCTRL);
+
+	return 0;
+}
+
+static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
+	.configure_phy	= rcar_hdmi_phy_configure,
+	.quirks		= DW_HDMI_QUIRK_PHY_SVSRET,
+};
+
+static int rcar_dw_hdmi_probe(struct platform_device *pdev)
+{
+	return dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data);
+}
+
+static int rcar_dw_hdmi_remove(struct platform_device *pdev)
+{
+	dw_hdmi_remove(pdev);
+
+	return 0;
+}
+
+static const struct of_device_id rcar_dw_hdmi_of_table[] = {
+	{ .compatible = "renesas,rcar-gen3-hdmi" },
+	{ /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rcar_dw_hdmi_of_table);
+
+static struct platform_driver rcar_dw_hdmi_platform_driver = {
+	.probe		= rcar_dw_hdmi_probe,
+	.remove		= rcar_dw_hdmi_remove,
+	.driver		= {
+		.name	= "rcar-dw-hdmi",
+		.of_match_table = rcar_dw_hdmi_of_table,
+	},
+};
+
+module_platform_driver(rcar_dw_hdmi_platform_driver);
+
+MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
+MODULE_DESCRIPTION("Renesas R-Car Gen3 HDMI Encoder Driver");
+MODULE_LICENSE("GPL");
-- 
Regards,

Laurent Pinchart

[PATCH 17/22] drm: rcar-du: Skip disabled outputs

[Laurent Pinchart]

When a DT node connected to a DU output is disabled no bridge will ever
be instantiated for it. Skip the output in that case.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index aa84689043ae..7a6bd8bdba05 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -384,6 +384,13 @@ static int rcar_du_encoders_init_one(struct rcar_du_device *rcdu,
 		return -ENODEV;
 	}
 
+	if (!of_device_is_available(entity)) {
+		dev_dbg(rcdu->dev,
+			"connected entity %s is disabled, skipping\n",
+			entity->full_name);
+		return -ENODEV;
+	}
+
 	entity_ep_node = of_parse_phandle(ep->local_node, "remote-endpoint", 0);
 
 	for_each_endpoint_of_node(entity, ep_node) {
-- 
Regards,

Laurent Pinchart

[PATCH 18/22] drm: rcar-du: Add DPLL support

[Laurent Pinchart]

From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

The implementation hardcodes a workaround for the H3 ES1.x SoC
regardless of the SoC revision, as the workaround can be safely applied
on all devices in the Gen3 family without any side effect.

Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 81 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  1 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
 drivers/gpu/drm/rcar-du/rcar_du_regs.h | 23 ++++++++++
 4 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index a2ec6d8796a0..cec20abd61aa 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -106,9 +106,62 @@ static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
  * Hardware Setup
  */
 
+struct dpll_info {
+	unsigned int output;
+	unsigned int fdpll;
+	unsigned int n;
+	unsigned int m;
+};
+
+static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
+				 struct dpll_info *dpll,
+				 unsigned long input,
+				 unsigned long target)
+{
+	unsigned long best_diff = (unsigned long)-1;
+	unsigned long diff;
+	unsigned int fdpll;
+	unsigned int m;
+	unsigned int n;
+
+	for (n = 39; n < 120; n++) {
+		for (m = 0; m < 4; m++) {
+			for (fdpll = 1; fdpll < 32; fdpll++) {
+				unsigned long output;
+
+				/* 1/2 (FRQSEL=1) for duty rate 50% */
+				output = input * (n + 1) / (m + 1)
+				       / (fdpll + 1) / 2;
+
+				if (output >= 400000000)
+					continue;
+
+				diff = abs((long)output - (long)target);
+				if (best_diff > diff) {
+					best_diff = diff;
+					dpll->n = n;
+					dpll->m = m;
+					dpll->fdpll = fdpll;
+					dpll->output = output;
+				}
+
+				if (diff == 0)
+					goto done;
+			}
+		}
+	}
+
+done:
+	dev_dbg(rcrtc->group->dev->dev,
+		"output:%u, fdpll:%u, n:%u, m:%u, diff:%lu\n",
+		 dpll->output, dpll->fdpll, dpll->n, dpll->m,
+		 best_diff);
+}
+
 static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 {
 	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
+	struct rcar_du_device *rcdu = rcrtc->group->dev;
 	unsigned long mode_clock = mode->clock * 1000;
 	unsigned long clk;
 	u32 value;
@@ -124,12 +177,18 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	escr = div | ESCR_DCLKSEL_CLKS;
 
 	if (rcrtc->extclock) {
+		struct dpll_info dpll = { 0 };
 		unsigned long extclk;
 		unsigned long extrate;
 		unsigned long rate;
 		u32 extdiv;
 
 		extclk = clk_get_rate(rcrtc->extclock);
+		if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
+			rcar_du_dpll_divider(rcrtc, &dpll, extclk, mode_clock);
+			extclk = dpll.output;
+		}
+
 		extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
 		extdiv = clamp(extdiv, 1U, 64U) - 1;
 
@@ -140,7 +199,27 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 		    abs((long)rate - (long)mode_clock)) {
 			dev_dbg(rcrtc->group->dev->dev,
 				"crtc%u: using external clock\n", rcrtc->index);
-			escr = extdiv | ESCR_DCLKSEL_DCLKIN;
+
+			if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
+				u32 dpllcr = DPLLCR_CODE | DPLLCR_CLKE
+					   | DPLLCR_FDPLL(dpll.fdpll)
+					   | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
+					   | DPLLCR_STBY;
+
+				if (rcrtc->index == 1)
+					dpllcr |= DPLLCR_PLCS1
+					       |  DPLLCR_INCS_DOTCLKIN1;
+				else
+					dpllcr |= DPLLCR_PLCS0
+					       |  DPLLCR_INCS_DOTCLKIN0;
+
+				rcar_du_group_write(rcrtc->group, DPLLCR,
+						    dpllcr);
+
+				escr = ESCR_DCLKSEL_DCLKIN | 1;
+			} else {
+				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
+			}
 		}
 	}
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 4da5edacab19..74352140f55f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -163,6 +163,7 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
 		},
 	},
 	.num_lvds = 1,
+	.dpll_ch =  BIT(1) | BIT(2),
 };
 
 static const struct rcar_du_device_info rcar_du_r8a7796_info = {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index 9393b26efe89..c4b6b7238601 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -65,6 +65,7 @@ struct rcar_du_device_info {
 	unsigned int num_crtcs;
 	struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX];
 	unsigned int num_lvds;
+	unsigned int dpll_ch;
 };
 
 #define RCAR_DU_MAX_CRTCS		4
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index fedb0161e234..d5bae99d3cfe 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -277,6 +277,29 @@
 #define DEFR10_TSEL_H3_TCON1	(0 << 1) /* DEFR102 register only (DU2/DU3) */
 #define DEFR10_DEFE10		(1 << 0)
 
+#define DPLLCR			0x20044
+#define DPLLCR_CODE		(0x95 << 24)
+#define DPLLCR_PLCS1		(1 << 23)
+/*
+ * PLCS0 is bit 21, but H3 ES1.x requires bit 20 to be set as well. As bit 20
+ * isn't implemented by other SoC in the Gen3 family it can safely be set
+ * unconditionally.
+ */
+#define DPLLCR_PLCS0		(3 << 20)
+#define DPLLCR_CLKE		(1 << 18)
+#define DPLLCR_FDPLL(n)		((n) << 12)
+#define DPLLCR_N(n)		((n) << 5)
+#define DPLLCR_M(n)		((n) << 3)
+#define DPLLCR_STBY		(1 << 2)
+#define DPLLCR_INCS_DOTCLKIN0	(0 << 0)
+#define DPLLCR_INCS_DOTCLKIN1	(1 << 1)
+
+#define DPLLC2R			0x20048
+#define DPLLC2R_CODE		(0x95 << 24)
+#define DPLLC2R_SELC		(1 << 12)
+#define DPLLC2R_M(n)		((n) << 8)
+#define DPLLC2R_FDPLL(n)	((n) << 0)
+
 /* -----------------------------------------------------------------------------
  * Display Timing Generation Registers
  */
-- 
Regards,

Laurent Pinchart

[PATCH 19/22] drm: rcar-du: Add HDMI outputs to R8A7795 device description

[Laurent Pinchart]

From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

Update the device description with the two available HDMI outputs.

Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 12 ++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 6f08b7e7db06..459e5390d6e0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -1,7 +1,7 @@
 /*
  * rcar_du_crtc.h  --  R-Car Display Unit CRTCs
  *
- * Copyright (C) 2013-2014 Renesas Electronics Corporation
+ * Copyright (C) 2013-2015 Renesas Electronics Corporation
  *
  * Contact: Laurent Pinchart (laurent.pinchart@ideasonboard.com)
  *
@@ -61,6 +61,8 @@ enum rcar_du_output {
 	RCAR_DU_OUTPUT_DPAD1,
 	RCAR_DU_OUTPUT_LVDS0,
 	RCAR_DU_OUTPUT_LVDS1,
+	RCAR_DU_OUTPUT_HDMI0,
+	RCAR_DU_OUTPUT_HDMI1,
 	RCAR_DU_OUTPUT_TCON,
 	RCAR_DU_OUTPUT_MAX,
 };
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 74352140f55f..3107719c4653 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -150,13 +150,21 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
 		  | RCAR_DU_FEATURE_VSP1_SOURCE,
 	.num_crtcs = 4,
 	.routes = {
-		/* R8A7795 has one RGB output, one LVDS output and two
-		 * (currently unsupported) HDMI outputs.
+		/* R8A7795 has one RGB output, two HDMI outputs and one
+		 * LVDS output.
 		 */
 		[RCAR_DU_OUTPUT_DPAD0] = {
 			.possible_crtcs = BIT(3),
 			.port = 0,
 		},
+		[RCAR_DU_OUTPUT_HDMI0] = {
+			.possible_crtcs = BIT(1),
+			.port = 1,
+		},
+		[RCAR_DU_OUTPUT_HDMI1] = {
+			.possible_crtcs = BIT(2),
+			.port = 2,
+		},
 		[RCAR_DU_OUTPUT_LVDS0] = {
 			.possible_crtcs = BIT(0),
 			.port = 3,
-- 
Regards,

Laurent Pinchart

[PATCH 20/22] arm64: dts: r8a7795: Add HDMI encoder support

[Laurent Pinchart]

From: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

Add DT nodes for the two HDMI encoders in disabled state.

Based on work by Koji Matsuoka.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 50 ++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 9b717dee1f7b..eceb62ee88df 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1590,11 +1590,13 @@
 				port@1 {
 					reg = <1>;
 					du_out_hdmi0: endpoint {
+						remote-endpoint = <&dw_hdmi0_in>;
 					};
 				};
 				port@2 {
 					reg = <2>;
 					du_out_hdmi1: endpoint {
+						remote-endpoint = <&dw_hdmi1_in>;
 					};
 				};
 				port@3 {
@@ -1604,5 +1606,53 @@
 				};
 			};
 		};
+
+		hdmi0: hdmi0@fead0000 {
+			compatible = "renesas,r8a7795-hdmi", "renesas,rcar-gen3-hdmi";
+			reg = <0 0xfead0000 0 0x10000>;
+			interrupts = <0 389 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_CORE R8A7795_CLK_S0D4>, <&cpg CPG_MOD 729>;
+			clock-names = "iahb", "isfr";
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				port@0 {
+					reg = <0>;
+					dw_hdmi0_in: endpoint {
+						remote-endpoint = <&du_out_hdmi0>;
+					};
+				};
+				port@1 {
+					reg = <1>;
+				};
+			};
+		};
+
+		hdmi1: hdmi1@feae0000 {
+			compatible = "renesas,r8a7795-hdmi", "renesas,rcar-gen3-hdmi";
+			reg = <0 0xfeae0000 0 0x10000>;
+			interrupts = <0 436 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_CORE R8A7795_CLK_S0D4>, <&cpg CPG_MOD 728>;
+			clock-names = "iahb", "isfr";
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				port@0 {
+					reg = <0>;
+					dw_hdmi1_in: endpoint {
+						remote-endpoint = <&du_out_hdmi1>;
+					};
+				};
+				port@1 {
+					reg = <1>;
+				};
+			};
+		};
 	};
 };
-- 
Regards,

Laurent Pinchart

[PATCH 21/22] arm64: dts: r8a7795: salvator-x: Enable HDMI outputs

[Laurent Pinchart]

From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

Instantiate the HDMI connectors and enable the encoders.

Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
index 73a22b93f7b1..08554b4d7e86 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
@@ -147,6 +147,30 @@
 		};
 	};
 
+	hdmi0-out {
+		compatible = "hdmi-connector";
+		label = "HDMI0 OUT";
+		type = "a";
+
+		port {
+			hdmi0_con: endpoint {
+				remote-endpoint = <&rcar_dw_hdmi0_out>;
+			};
+		};
+	};
+
+	hdmi1-out {
+		compatible = "hdmi-connector";
+		label = "HDMI1 OUT";
+		type = "a";
+
+		port {
+			hdmi1_con: endpoint {
+				remote-endpoint = <&rcar_dw_hdmi1_out>;
+			};
+		};
+	};
+
 	vga-encoder {
 		compatible = "adi,adv7123";
 
@@ -190,6 +214,32 @@
 	};
 };
 
+&hdmi0 {
+	status = "okay";
+
+	ports {
+		port@1 {
+			reg = <1>;
+			rcar_dw_hdmi0_out: endpoint {
+				remote-endpoint = <&hdmi0_con>;
+			};
+		};
+	};
+};
+
+&hdmi1 {
+	status = "okay";
+
+	ports {
+		port@1 {
+			reg = <1>;
+			rcar_dw_hdmi1_out: endpoint {
+				remote-endpoint = <&hdmi1_con>;
+			};
+		};
+	};
+};
+
 &du {
 	pinctrl-0 = <&du_pins>;
 	pinctrl-names = "default";
-- 
Regards,

Laurent Pinchart

[PATCH 22/22] arm64: dts: r8a7795: salvator-x: Add DU1 and DU2 external dot clocks

[Laurent Pinchart]

The DU1 and DU2 external dot clocks are fixed frequency clock generators
running at 33MHz.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
index 08554b4d7e86..b6dc6f5188d6 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
@@ -147,6 +147,19 @@
 		};
 	};
 
+	/* External DU dot clocks */
+	x21_clk: x21-clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <33000000>;
+	};
+
+	x22_clk: x22-clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <33000000>;
+	};
+
 	hdmi0-out {
 		compatible = "hdmi-connector";
 		label = "HDMI0 OUT";
@@ -245,6 +258,16 @@
 	pinctrl-names = "default";
 	status = "okay";
 
+	clocks = <&cpg CPG_MOD 724>,
+		 <&cpg CPG_MOD 723>,
+		 <&cpg CPG_MOD 722>,
+		 <&cpg CPG_MOD 721>,
+		 <&cpg CPG_MOD 727>,
+		 <&x21_clk>,
+		 <&x22_clk>;
+	clock-names = "du.0", "du.1", "du.2", "du.3", "lvds.0",
+		      "dclkin.1", "dclkin.2";
+
 	ports {
 		port@0 {
 			endpoint {
-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/22] R-Car Gen3 HDMI output support

[Jose Abreu]

Hi Laurent,


On 01-12-2016 23:43, Laurent Pinchart wrote:
> Hello,
>
> This patch series implements support for the HDMI output on Renesas R-Car Gen3
> SoCs, and more specifically on the R-Car H3.
>
> R-Car Gen3 SoCs include one or multiple Synopsys DWC HDMI TX controllers. The
> series thus starts with 13 cleanup or feature patches for the dw-hdmi driver.
> Patches 01/22 to 05/22 are small miscellaneous cleanups. Patch 06/22 fixes a
> crash when the HPD interrupt is generated before the bridge gets attached to a
> DRM device.
>
> Patch 07/22 then starts refactoring the API towards platform glue by moving
> common I/O and clock resource allocation to common code. Patches 08/22 and
> 09/22 continue by moving connector creation to the bridge attach operation to
> comply with the DRM bridge API. Patch 10/22 implements a new API for platform
> glue to register the bridge with the DRM core, for platform that doesn't use
> the component framework. Patches 11/22 to 13/22 finally abstract PHY setup to
> allow platforms to implement configuration for custom PHYs.
>
> The next three patches add glue code for the DWC HDMI on Renesas R-Car Gen3
> platforms. Patch 14/22 rework the DWC HDMI DT bindings to make them more
> modular, and patch 15/22 makes use of them to document the R-Car Gen DWC HDMI
> TX DT bindings. Patch 16/22 then implements the platform glue code.
>
> The next three patches implement support for the R8A7795 HDMI outputs in the
> R-Car DU driver. Patch 17/22 fixes a bug that makes the driver defer probe
> forever if an encoder is disabled. Patch 18/22 implement DPLL support to
> generate more precise clocks required by the HDMI controller, and patch 19/22
> then enables the two HDMI output on the R-Car H3 SoC.
>
> The last three patches enable the HDMI outputs on the H3 Salvator-X board by
> adding the HDMI encoders to the R-Car H3 DT (20/22), adding HDMI connectors
> and enabling the encoders (21/22) and adding the external dot clocks (22/22).
>
> The code has been tested on a Renesas Salvator-X H3 board, and modifications
> to the dw-hdmi Freescale and Rockchip platform glues have been compile-tested.
>
> More improvements to the dw-hdmi driver would be beneficial. The PHY
> modularization is based on the limited understanding of the HDMI PHYs used on
> the three supported platforms and will possibly be enhanced later when adding
> support for more platforms. The DT bindings would also benefit from a
> standardized definition of the ports, and the reg-io-width property could
> possibly be deprecated in favour of encoding the information in platform glue
> code. The patch series is however big enough as is to be submitted for review
> and merge before the dw-hdmi driver achieves perfection.
>
> Kieran Bingham (4):
>   drm: bridge: dw-hdmi: Remove unused function parameter
>   drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter
>   drm: bridge: dw-hdmi: Abstract the platform PHY configuration
>   drm: bridge: dw-hdmi: Replace device type with platform quirks
>
> Koji Matsuoka (4):
>   drm: rcar-du: Add Gen3 HDMI encoder support
>   drm: rcar-du: Add DPLL support
>   drm: rcar-du: Add HDMI outputs to R8A7795 device description
>   arm64: dts: r8a7795: salvator-x: Enable HDMI outputs
>
> Laurent Pinchart (13):
>   drm: bridge: dw-hdmi: Merge __hdmi_phy_i2c_write and
>     hdmi_phy_i2c_write
>   drm: bridge: dw-hdmi: Remove unneeded arguments to bind/unbind
>     functions
>   drm: bridge: dw-hdmi: Embed drm_bridge in struct dw_hdmi
>   drm: bridge: dw-hdmi: Remove encoder field from struct dw_hdmi
>   drm: bridge: dw-hdmi: Don't forward HPD events to DRM core before
>     attach
>   drm: bridge: dw-hdmi: Move IRQ and IO resource allocation to common
>     code
>   drm: bridge: dw-hdmi: Reorder functions to prepare for next commit
>   drm: bridge: dw-hdmi: Create connector in the bridge attach operation
>   drm: bridge: dw-hdmi: Implement DRM bridge registration
>   dt-bindings: display: dw-hdmi: Clean up DT bindings documentation
>   dt-bindings: display: renesas: Add R-Car Gen3 HDMI TX DT bindings
>   drm: rcar-du: Skip disabled outputs
>   arm64: dts: r8a7795: salvator-x: Add DU1 and DU2 external dot clocks
>
> Ulrich Hecht (1):
>   arm64: dts: r8a7795: Add HDMI encoder support
>
>  .../devicetree/bindings/display/bridge/dw_hdmi.txt |  85 ++---
>  .../bindings/display/bridge/renesas,dw-hdmi.txt    |  75 +++++
>  .../devicetree/bindings/display/imx/hdmi.txt       |  51 +--
>  .../bindings/display/rockchip/dw_hdmi-rockchip.txt |  43 ++-
>  MAINTAINERS                                        |   1 +
>  arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts |  73 +++++
>  arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  50 +++
>  drivers/gpu/drm/bridge/dw-hdmi.c                   | 364 ++++++++++++---------
>  drivers/gpu/drm/bridge/dw-hdmi.h                   |   4 +-
>  drivers/gpu/drm/imx/dw_hdmi-imx.c                  |  19 +-
>  drivers/gpu/drm/rcar-du/Kconfig                    |   8 +
>  drivers/gpu/drm/rcar-du/Makefile                   |   1 +
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  81 ++++-
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h             |   4 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c              |  13 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   1 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c              |   7 +
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h             |  23 ++
>  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c             | 105 ++++++
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        |  17 +-
>  include/drm/bridge/dw_hdmi.h                       |  35 +-
>  21 files changed, 765 insertions(+), 295 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
>

Patches 01 to 13 are: Reviewed-by: Jose Abreu <joabreu@synopsys.com>

I have one minor comment for patch 12.

Best regards,
Jose Miguel Abreu

Re: [PATCH 00/22] R-Car Gen3 HDMI output support

[Jose Abreu]

Hi Laurent,


On 01-12-2016 23:43, Laurent Pinchart wrote:
> Hello,
>
> This patch series implements support for the HDMI output on Renesas R-Car Gen3
> SoCs, and more specifically on the R-Car H3.
>
> R-Car Gen3 SoCs include one or multiple Synopsys DWC HDMI TX controllers. The
> series thus starts with 13 cleanup or feature patches for the dw-hdmi driver.
> Patches 01/22 to 05/22 are small miscellaneous cleanups. Patch 06/22 fixes a
> crash when the HPD interrupt is generated before the bridge gets attached to a
> DRM device.
>
> Patch 07/22 then starts refactoring the API towards platform glue by moving
> common I/O and clock resource allocation to common code. Patches 08/22 and
> 09/22 continue by moving connector creation to the bridge attach operation to
> comply with the DRM bridge API. Patch 10/22 implements a new API for platform
> glue to register the bridge with the DRM core, for platform that doesn't use
> the component framework. Patches 11/22 to 13/22 finally abstract PHY setup to
> allow platforms to implement configuration for custom PHYs.
>
> The next three patches add glue code for the DWC HDMI on Renesas R-Car Gen3
> platforms. Patch 14/22 rework the DWC HDMI DT bindings to make them more
> modular, and patch 15/22 makes use of them to document the R-Car Gen DWC HDMI
> TX DT bindings. Patch 16/22 then implements the platform glue code.
>
> The next three patches implement support for the R8A7795 HDMI outputs in the
> R-Car DU driver. Patch 17/22 fixes a bug that makes the driver defer probe
> forever if an encoder is disabled. Patch 18/22 implement DPLL support to
> generate more precise clocks required by the HDMI controller, and patch 19/22
> then enables the two HDMI output on the R-Car H3 SoC.
>
> The last three patches enable the HDMI outputs on the H3 Salvator-X board by
> adding the HDMI encoders to the R-Car H3 DT (20/22), adding HDMI connectors
> and enabling the encoders (21/22) and adding the external dot clocks (22/22).
>
> The code has been tested on a Renesas Salvator-X H3 board, and modifications
> to the dw-hdmi Freescale and Rockchip platform glues have been compile-tested.
>
> More improvements to the dw-hdmi driver would be beneficial. The PHY
> modularization is based on the limited understanding of the HDMI PHYs used on
> the three supported platforms and will possibly be enhanced later when adding
> support for more platforms. The DT bindings would also benefit from a
> standardized definition of the ports, and the reg-io-width property could
> possibly be deprecated in favour of encoding the information in platform glue
> code. The patch series is however big enough as is to be submitted for review
> and merge before the dw-hdmi driver achieves perfection.
>
> Kieran Bingham (4):
>   drm: bridge: dw-hdmi: Remove unused function parameter
>   drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter
>   drm: bridge: dw-hdmi: Abstract the platform PHY configuration
>   drm: bridge: dw-hdmi: Replace device type with platform quirks
>
> Koji Matsuoka (4):
>   drm: rcar-du: Add Gen3 HDMI encoder support
>   drm: rcar-du: Add DPLL support
>   drm: rcar-du: Add HDMI outputs to R8A7795 device description
>   arm64: dts: r8a7795: salvator-x: Enable HDMI outputs
>
> Laurent Pinchart (13):
>   drm: bridge: dw-hdmi: Merge __hdmi_phy_i2c_write and
>     hdmi_phy_i2c_write
>   drm: bridge: dw-hdmi: Remove unneeded arguments to bind/unbind
>     functions
>   drm: bridge: dw-hdmi: Embed drm_bridge in struct dw_hdmi
>   drm: bridge: dw-hdmi: Remove encoder field from struct dw_hdmi
>   drm: bridge: dw-hdmi: Don't forward HPD events to DRM core before
>     attach
>   drm: bridge: dw-hdmi: Move IRQ and IO resource allocation to common
>     code
>   drm: bridge: dw-hdmi: Reorder functions to prepare for next commit
>   drm: bridge: dw-hdmi: Create connector in the bridge attach operation
>   drm: bridge: dw-hdmi: Implement DRM bridge registration
>   dt-bindings: display: dw-hdmi: Clean up DT bindings documentation
>   dt-bindings: display: renesas: Add R-Car Gen3 HDMI TX DT bindings
>   drm: rcar-du: Skip disabled outputs
>   arm64: dts: r8a7795: salvator-x: Add DU1 and DU2 external dot clocks
>
> Ulrich Hecht (1):
>   arm64: dts: r8a7795: Add HDMI encoder support
>
>  .../devicetree/bindings/display/bridge/dw_hdmi.txt |  85 ++---
>  .../bindings/display/bridge/renesas,dw-hdmi.txt    |  75 +++++
>  .../devicetree/bindings/display/imx/hdmi.txt       |  51 +--
>  .../bindings/display/rockchip/dw_hdmi-rockchip.txt |  43 ++-
>  MAINTAINERS                                        |   1 +
>  arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts |  73 +++++
>  arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  50 +++
>  drivers/gpu/drm/bridge/dw-hdmi.c                   | 364 ++++++++++++---------
>  drivers/gpu/drm/bridge/dw-hdmi.h                   |   4 +-
>  drivers/gpu/drm/imx/dw_hdmi-imx.c                  |  19 +-
>  drivers/gpu/drm/rcar-du/Kconfig                    |   8 +
>  drivers/gpu/drm/rcar-du/Makefile                   |   1 +
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  81 ++++-
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h             |   4 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c              |  13 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   1 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c              |   7 +
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h             |  23 ++
>  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c             | 105 ++++++
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        |  17 +-
>  include/drm/bridge/dw_hdmi.h                       |  35 +-
>  21 files changed, 765 insertions(+), 295 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
>

Patches 01 to 13 are: Reviewed-by: Jose Abreu <joabreu@synopsys.com>

I have one minor comment for patch 12.

Best regards,
Jose Miguel Abreu

Re: [PATCH 12/22] drm: bridge: dw-hdmi: Abstract the platform PHY configuration

[Jose Abreu]

Hi Laurent,


On 01-12-2016 23:43, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> Platforms implement the dw-hdmi with a separate PHY entity. It is
> configured over it's own I2C bus. To allow for different PHY's to be
> configured from the dw-hdmi driver, abstract the actual programming of
> the PHY to its own functions, as configured by the platform.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c            | 79 ++++++++++++++++++-----------
>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  2 +
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  1 +
>  include/drm/bridge/dw_hdmi.h                | 12 +++++
>  4 files changed, 63 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 074ceb1e4897..06a44a2cdf3b 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -867,8 +867,8 @@ static bool hdmi_phy_wait_i2c_done(struct dw_hdmi *hdmi, int msec)
>  	return true;
>  }
>  
> -static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> -				 unsigned char addr)
> +void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> +			   unsigned char addr)
>  {
>  	hdmi_writeb(hdmi, 0xFF, HDMI_IH_I2CMPHY_STAT0);
>  	hdmi_writeb(hdmi, addr, HDMI_PHY_I2CM_ADDRESS_ADDR);
> @@ -880,6 +880,7 @@ static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
>  		    HDMI_PHY_I2CM_OPERATION_ADDR);
>  	hdmi_phy_wait_i2c_done(hdmi, 1000);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_write);
>  
>  static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable)
>  {
> @@ -930,38 +931,61 @@ static void dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)
>  			 HDMI_PHY_CONF0_SELDIPIF_MASK);
>  }
>  
> -static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> -			      enum dw_hdmi_resolution res_idx, int cscon)
> +int dw_hdmi_phy_configure_synopsys(struct dw_hdmi *hdmi,
> +				   const struct dw_hdmi_plat_data *pdata,
> +				   unsigned long mpixelclock,
> +				   enum dw_hdmi_resolution resolution)
> +
>  {
> -	u8 val, msec;
> -	const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
>  	const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
>  	const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
>  	const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
>  
>  	/* PLL/MPLL Cfg - always match on final entry */
>  	for (; mpll_config->mpixelclock != ~0UL; mpll_config++)
> -		if (hdmi->hdmi_data.video_mode.mpixelclock <=
> -		    mpll_config->mpixelclock)
> +		if (mpixelclock <= mpll_config->mpixelclock)
>  			break;
>  
>  	for (; curr_ctrl->mpixelclock != ~0UL; curr_ctrl++)
> -		if (hdmi->hdmi_data.video_mode.mpixelclock <=
> -		    curr_ctrl->mpixelclock)
> +		if (mpixelclock <= curr_ctrl->mpixelclock)
>  			break;
>  
>  	for (; phy_config->mpixelclock != ~0UL; phy_config++)
> -		if (hdmi->hdmi_data.video_mode.mpixelclock <=
> -		    phy_config->mpixelclock)
> +		if (mpixelclock <= phy_config->mpixelclock)
>  			break;
>  
>  	if (mpll_config->mpixelclock == ~0UL ||
>  	    curr_ctrl->mpixelclock == ~0UL ||
> -	    phy_config->mpixelclock == ~0UL) {
> -		dev_err(hdmi->dev, "Pixel clock %d - unsupported by HDMI\n",
> -			hdmi->hdmi_data.video_mode.mpixelclock);
> +	    phy_config->mpixelclock == ~0UL)
>  		return -EINVAL;
> -	}
> +
> +	dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[resolution].cpce, 0x06);
> +	dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[resolution].gmp, 0x15);
> +
> +	/* CURRCTRL */
> +	dw_hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[resolution], 0x10);
> +
> +	dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x13); /* PLLPHBYCTRL */
> +	dw_hdmi_phy_i2c_write(hdmi, 0x0006, 0x17);
> +
> +	dw_hdmi_phy_i2c_write(hdmi, phy_config->term, 0x19); /* TXTERM */
> +	dw_hdmi_phy_i2c_write(hdmi, phy_config->sym_ctr, 0x09); /* CKSYMTXCTRL */
> +	dw_hdmi_phy_i2c_write(hdmi, phy_config->vlev_ctr, 0x0E); /* VLEVCTRL */
> +
> +	/* REMOVE CLK TERM */
> +	dw_hdmi_phy_i2c_write(hdmi, 0x8000, 0x05); /* CKCALCTRL */
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_configure_synopsys);
> +
> +static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> +			      enum dw_hdmi_resolution resolution, int cscon)
> +{
> +	const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
> +	unsigned long mpixelclock = hdmi->hdmi_data.video_mode.mpixelclock;
> +	u8 val, msec;
> +	int ret;
>  
>  	/* Enable csc path */
>  	if (cscon)
> @@ -988,21 +1012,14 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
>  		    HDMI_PHY_I2CM_SLAVE_ADDR);
>  	hdmi_phy_test_clear(hdmi, 0);
>  
> -	hdmi_phy_i2c_write(hdmi, mpll_config->res[res_idx].cpce, 0x06);
> -	hdmi_phy_i2c_write(hdmi, mpll_config->res[res_idx].gmp, 0x15);
> -
> -	/* CURRCTRL */
> -	hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[res_idx], 0x10);
> -
> -	hdmi_phy_i2c_write(hdmi, 0x0000, 0x13);  /* PLLPHBYCTRL */
> -	hdmi_phy_i2c_write(hdmi, 0x0006, 0x17);
> -
> -	hdmi_phy_i2c_write(hdmi, phy_config->term, 0x19);  /* TXTERM */
> -	hdmi_phy_i2c_write(hdmi, phy_config->sym_ctr, 0x09); /* CKSYMTXCTRL */
> -	hdmi_phy_i2c_write(hdmi, phy_config->vlev_ctr, 0x0E); /* VLEVCTRL */
> -
> -	/* REMOVE CLK TERM */
> -	hdmi_phy_i2c_write(hdmi, 0x8000, 0x05);  /* CKCALCTRL */
> +	/* Write to the PHY as configured by the platform */
> +	ret = pdata->configure_phy(hdmi, pdata, mpixelclock, resolution);
> +	if (ret) {
> +		dev_err(hdmi->dev,
> +			"PHY configuration failed (clock %lu resolution %u)\n",
> +			mpixelclock, resolution);
> +		return ret;
> +	}
>  
>  	dw_hdmi_phy_enable_powerdown(hdmi, false);
>  
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index f645275e6e63..f1cb25c429cf 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -177,6 +177,7 @@ static struct dw_hdmi_plat_data imx6q_hdmi_drv_data = {
>  	.phy_config = imx_phy_config,
>  	.dev_type   = IMX6Q_HDMI,
>  	.mode_valid = imx6q_hdmi_mode_valid,
> +	.configure_phy = dw_hdmi_phy_configure_synopsys,
>  };
>  
>  static struct dw_hdmi_plat_data imx6dl_hdmi_drv_data = {
> @@ -185,6 +186,7 @@ static struct dw_hdmi_plat_data imx6dl_hdmi_drv_data = {
>  	.phy_config = imx_phy_config,
>  	.dev_type = IMX6DL_HDMI,
>  	.mode_valid = imx6dl_hdmi_mode_valid,
> +	.configure_phy = dw_hdmi_phy_configure_synopsys,
>  };
>  
>  static const struct of_device_id dw_hdmi_imx_dt_ids[] = {
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index a6d4a0236e8f..55b1f2f27d6e 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -237,6 +237,7 @@ static const struct dw_hdmi_plat_data rockchip_hdmi_drv_data = {
>  	.mpll_cfg   = rockchip_mpll_cfg,
>  	.cur_ctr    = rockchip_cur_ctr,
>  	.phy_config = rockchip_phy_config,
> +	.configure_phy = dw_hdmi_phy_configure_synopsys,
>  	.dev_type   = RK3288_HDMI,
>  };
>  
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index e551b457c100..fa7655836c81 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -52,6 +52,10 @@ struct dw_hdmi_plat_data {
>  	const struct dw_hdmi_mpll_config *mpll_cfg;
>  	const struct dw_hdmi_curr_ctrl *cur_ctr;
>  	const struct dw_hdmi_phy_config *phy_config;
> +	int (*configure_phy)(struct dw_hdmi *hdmi,
> +			     const struct dw_hdmi_plat_data *pdata,
> +			     unsigned long mpixelclock,
> +			     enum dw_hdmi_resolution resolution);

I think the name of this callback here is a little bit misleading
because you are only configuring the phy pll. Maybe
.configure_phy_pll() would be more suitable.

>  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>  					   struct drm_display_mode *mode);
>  };
> @@ -67,4 +71,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
>  void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
>  void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
>  
> +/* PHY configuration */
> +void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> +			   unsigned char addr);
> +int dw_hdmi_phy_configure_synopsys(struct dw_hdmi *hdmi,
> +				   const struct dw_hdmi_plat_data *pdata,
> +				   unsigned long mpixelclock,
> +				   enum dw_hdmi_resolution resolution);
> +
>  #endif /* __IMX_HDMI_H__ */

Best regards,
Jose Miguel Abreu

Re: [PATCH 12/22] drm: bridge: dw-hdmi: Abstract the platform PHY configuration

[Jose Abreu]

Hi Laurent,


On 01-12-2016 23:43, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> Platforms implement the dw-hdmi with a separate PHY entity. It is
> configured over it's own I2C bus. To allow for different PHY's to be
> configured from the dw-hdmi driver, abstract the actual programming of
> the PHY to its own functions, as configured by the platform.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c            | 79 ++++++++++++++++++-----------
>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  2 +
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  1 +
>  include/drm/bridge/dw_hdmi.h                | 12 +++++
>  4 files changed, 63 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 074ceb1e4897..06a44a2cdf3b 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -867,8 +867,8 @@ static bool hdmi_phy_wait_i2c_done(struct dw_hdmi *hdmi, int msec)
>  	return true;
>  }
>  
> -static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> -				 unsigned char addr)
> +void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> +			   unsigned char addr)
>  {
>  	hdmi_writeb(hdmi, 0xFF, HDMI_IH_I2CMPHY_STAT0);
>  	hdmi_writeb(hdmi, addr, HDMI_PHY_I2CM_ADDRESS_ADDR);
> @@ -880,6 +880,7 @@ static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
>  		    HDMI_PHY_I2CM_OPERATION_ADDR);
>  	hdmi_phy_wait_i2c_done(hdmi, 1000);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_write);
>  
>  static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable)
>  {
> @@ -930,38 +931,61 @@ static void dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)
>  			 HDMI_PHY_CONF0_SELDIPIF_MASK);
>  }
>  
> -static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> -			      enum dw_hdmi_resolution res_idx, int cscon)
> +int dw_hdmi_phy_configure_synopsys(struct dw_hdmi *hdmi,
> +				   const struct dw_hdmi_plat_data *pdata,
> +				   unsigned long mpixelclock,
> +				   enum dw_hdmi_resolution resolution)
> +
>  {
> -	u8 val, msec;
> -	const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
>  	const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
>  	const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
>  	const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
>  
>  	/* PLL/MPLL Cfg - always match on final entry */
>  	for (; mpll_config->mpixelclock != ~0UL; mpll_config++)
> -		if (hdmi->hdmi_data.video_mode.mpixelclock <=
> -		    mpll_config->mpixelclock)
> +		if (mpixelclock <= mpll_config->mpixelclock)
>  			break;
>  
>  	for (; curr_ctrl->mpixelclock != ~0UL; curr_ctrl++)
> -		if (hdmi->hdmi_data.video_mode.mpixelclock <=
> -		    curr_ctrl->mpixelclock)
> +		if (mpixelclock <= curr_ctrl->mpixelclock)
>  			break;
>  
>  	for (; phy_config->mpixelclock != ~0UL; phy_config++)
> -		if (hdmi->hdmi_data.video_mode.mpixelclock <=
> -		    phy_config->mpixelclock)
> +		if (mpixelclock <= phy_config->mpixelclock)
>  			break;
>  
>  	if (mpll_config->mpixelclock == ~0UL ||
>  	    curr_ctrl->mpixelclock == ~0UL ||
> -	    phy_config->mpixelclock == ~0UL) {
> -		dev_err(hdmi->dev, "Pixel clock %d - unsupported by HDMI\n",
> -			hdmi->hdmi_data.video_mode.mpixelclock);
> +	    phy_config->mpixelclock == ~0UL)
>  		return -EINVAL;
> -	}
> +
> +	dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[resolution].cpce, 0x06);
> +	dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[resolution].gmp, 0x15);
> +
> +	/* CURRCTRL */
> +	dw_hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[resolution], 0x10);
> +
> +	dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x13); /* PLLPHBYCTRL */
> +	dw_hdmi_phy_i2c_write(hdmi, 0x0006, 0x17);
> +
> +	dw_hdmi_phy_i2c_write(hdmi, phy_config->term, 0x19); /* TXTERM */
> +	dw_hdmi_phy_i2c_write(hdmi, phy_config->sym_ctr, 0x09); /* CKSYMTXCTRL */
> +	dw_hdmi_phy_i2c_write(hdmi, phy_config->vlev_ctr, 0x0E); /* VLEVCTRL */
> +
> +	/* REMOVE CLK TERM */
> +	dw_hdmi_phy_i2c_write(hdmi, 0x8000, 0x05); /* CKCALCTRL */
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_configure_synopsys);
> +
> +static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> +			      enum dw_hdmi_resolution resolution, int cscon)
> +{
> +	const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
> +	unsigned long mpixelclock = hdmi->hdmi_data.video_mode.mpixelclock;
> +	u8 val, msec;
> +	int ret;
>  
>  	/* Enable csc path */
>  	if (cscon)
> @@ -988,21 +1012,14 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
>  		    HDMI_PHY_I2CM_SLAVE_ADDR);
>  	hdmi_phy_test_clear(hdmi, 0);
>  
> -	hdmi_phy_i2c_write(hdmi, mpll_config->res[res_idx].cpce, 0x06);
> -	hdmi_phy_i2c_write(hdmi, mpll_config->res[res_idx].gmp, 0x15);
> -
> -	/* CURRCTRL */
> -	hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[res_idx], 0x10);
> -
> -	hdmi_phy_i2c_write(hdmi, 0x0000, 0x13);  /* PLLPHBYCTRL */
> -	hdmi_phy_i2c_write(hdmi, 0x0006, 0x17);
> -
> -	hdmi_phy_i2c_write(hdmi, phy_config->term, 0x19);  /* TXTERM */
> -	hdmi_phy_i2c_write(hdmi, phy_config->sym_ctr, 0x09); /* CKSYMTXCTRL */
> -	hdmi_phy_i2c_write(hdmi, phy_config->vlev_ctr, 0x0E); /* VLEVCTRL */
> -
> -	/* REMOVE CLK TERM */
> -	hdmi_phy_i2c_write(hdmi, 0x8000, 0x05);  /* CKCALCTRL */
> +	/* Write to the PHY as configured by the platform */
> +	ret = pdata->configure_phy(hdmi, pdata, mpixelclock, resolution);
> +	if (ret) {
> +		dev_err(hdmi->dev,
> +			"PHY configuration failed (clock %lu resolution %u)\n",
> +			mpixelclock, resolution);
> +		return ret;
> +	}
>  
>  	dw_hdmi_phy_enable_powerdown(hdmi, false);
>  
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index f645275e6e63..f1cb25c429cf 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -177,6 +177,7 @@ static struct dw_hdmi_plat_data imx6q_hdmi_drv_data = {
>  	.phy_config = imx_phy_config,
>  	.dev_type   = IMX6Q_HDMI,
>  	.mode_valid = imx6q_hdmi_mode_valid,
> +	.configure_phy = dw_hdmi_phy_configure_synopsys,
>  };
>  
>  static struct dw_hdmi_plat_data imx6dl_hdmi_drv_data = {
> @@ -185,6 +186,7 @@ static struct dw_hdmi_plat_data imx6dl_hdmi_drv_data = {
>  	.phy_config = imx_phy_config,
>  	.dev_type = IMX6DL_HDMI,
>  	.mode_valid = imx6dl_hdmi_mode_valid,
> +	.configure_phy = dw_hdmi_phy_configure_synopsys,
>  };
>  
>  static const struct of_device_id dw_hdmi_imx_dt_ids[] = {
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index a6d4a0236e8f..55b1f2f27d6e 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -237,6 +237,7 @@ static const struct dw_hdmi_plat_data rockchip_hdmi_drv_data = {
>  	.mpll_cfg   = rockchip_mpll_cfg,
>  	.cur_ctr    = rockchip_cur_ctr,
>  	.phy_config = rockchip_phy_config,
> +	.configure_phy = dw_hdmi_phy_configure_synopsys,
>  	.dev_type   = RK3288_HDMI,
>  };
>  
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index e551b457c100..fa7655836c81 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -52,6 +52,10 @@ struct dw_hdmi_plat_data {
>  	const struct dw_hdmi_mpll_config *mpll_cfg;
>  	const struct dw_hdmi_curr_ctrl *cur_ctr;
>  	const struct dw_hdmi_phy_config *phy_config;
> +	int (*configure_phy)(struct dw_hdmi *hdmi,
> +			     const struct dw_hdmi_plat_data *pdata,
> +			     unsigned long mpixelclock,
> +			     enum dw_hdmi_resolution resolution);

I think the name of this callback here is a little bit misleading
because you are only configuring the phy pll. Maybe
.configure_phy_pll() would be more suitable.

>  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>  					   struct drm_display_mode *mode);
>  };
> @@ -67,4 +71,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
>  void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
>  void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
>  
> +/* PHY configuration */
> +void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> +			   unsigned char addr);
> +int dw_hdmi_phy_configure_synopsys(struct dw_hdmi *hdmi,
> +				   const struct dw_hdmi_plat_data *pdata,
> +				   unsigned long mpixelclock,
> +				   enum dw_hdmi_resolution resolution);
> +
>  #endif /* __IMX_HDMI_H__ */

Best regards,
Jose Miguel Abreu

Re: [PATCH 12/22] drm: bridge: dw-hdmi: Abstract the platform PHY configuration

[Laurent Pinchart]

Hi Jose,

On Friday 02 Dec 2016 11:15:19 Jose Abreu wrote:
> On 01-12-2016 23:43, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > Platforms implement the dw-hdmi with a separate PHY entity. It is
> > configured over it's own I2C bus. To allow for different PHY's to be
> > configured from the dw-hdmi driver, abstract the actual programming of
> > the PHY to its own functions, as configured by the platform.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> > drivers/gpu/drm/bridge/dw-hdmi.c            | 79 +++++++++++++++---------
> > drivers/gpu/drm/imx/dw_hdmi-imx.c           |  2 +
> > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  1 +
> > include/drm/bridge/dw_hdmi.h                | 12 +++++
> > 4 files changed, 63 insertions(+), 31 deletions(-)

[snip]

> > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> > index e551b457c100..fa7655836c81 100644
> > --- a/include/drm/bridge/dw_hdmi.h
> > +++ b/include/drm/bridge/dw_hdmi.h
> > @@ -52,6 +52,10 @@ struct dw_hdmi_plat_data {
> >  	const struct dw_hdmi_mpll_config *mpll_cfg;
> >  	const struct dw_hdmi_curr_ctrl *cur_ctr;
> >  	const struct dw_hdmi_phy_config *phy_config;
> > +	int (*configure_phy)(struct dw_hdmi *hdmi,
> > +			     const struct dw_hdmi_plat_data *pdata,
> > +			     unsigned long mpixelclock,
> > +			     enum dw_hdmi_resolution resolution);
> 
> I think the name of this callback here is a little bit misleading
> because you are only configuring the phy pll. Maybe
> .configure_phy_pll() would be more suitable.

Isn't there more than the PLL ? For instance register TXTERM configures the 
transmission line termination.

> >  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
> >  	
> >  					   struct drm_display_mode *mode);
> >  
> >  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 12/22] drm: bridge: dw-hdmi: Abstract the platform PHY configuration

[Laurent Pinchart]

Hi Jose,

On Friday 02 Dec 2016 11:15:19 Jose Abreu wrote:
> On 01-12-2016 23:43, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > Platforms implement the dw-hdmi with a separate PHY entity. It is
> > configured over it's own I2C bus. To allow for different PHY's to be
> > configured from the dw-hdmi driver, abstract the actual programming of
> > the PHY to its own functions, as configured by the platform.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> > drivers/gpu/drm/bridge/dw-hdmi.c            | 79 +++++++++++++++---------
> > drivers/gpu/drm/imx/dw_hdmi-imx.c           |  2 +
> > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  1 +
> > include/drm/bridge/dw_hdmi.h                | 12 +++++
> > 4 files changed, 63 insertions(+), 31 deletions(-)

[snip]

> > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> > index e551b457c100..fa7655836c81 100644
> > --- a/include/drm/bridge/dw_hdmi.h
> > +++ b/include/drm/bridge/dw_hdmi.h
> > @@ -52,6 +52,10 @@ struct dw_hdmi_plat_data {
> >  	const struct dw_hdmi_mpll_config *mpll_cfg;
> >  	const struct dw_hdmi_curr_ctrl *cur_ctr;
> >  	const struct dw_hdmi_phy_config *phy_config;
> > +	int (*configure_phy)(struct dw_hdmi *hdmi,
> > +			     const struct dw_hdmi_plat_data *pdata,
> > +			     unsigned long mpixelclock,
> > +			     enum dw_hdmi_resolution resolution);
> 
> I think the name of this callback here is a little bit misleading
> because you are only configuring the phy pll. Maybe
> .configure_phy_pll() would be more suitable.

Isn't there more than the PLL ? For instance register TXTERM configures the 
transmission line termination.

> >  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
> >  	
> >  					   struct drm_display_mode *mode);
> >  
> >  };

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/22] drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter

[Russell King - ARM Linux]

On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The current code hard codes the call of hdmi_phy_configure() to be 8bpp
> and provides extraneous error checking to verify that this hardcoded
> value is correct.
> 
> Simplify the passing of the data by setting the parameter to be of the
> enum type it represents rather than converting and then verifying the
> value. This will allow the compiler to check the value is acceptable
> based on the type, and remove the dead code that we currently have.

I think you're expecting too much of the compiler there.  There's no
requirement for the compiler to check that an enum type is passed one
of it's defined values.

Try building this and see if it even produces a warning:

enum foo {
	FOO_1,
	FOO_2,
};

int func(enum foo foo)
{
	return foo;
}

int test_1(void)
{
	return func(FOO_1);
}

int test_2(void)
{
	return func(5);
}

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Russell King - ARM Linux]

On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The dw-hdmi driver declares a dev_type to distinguish platform specific
> changes. Replace this with a quirk field, so that the platform can
> specify the required quirks for the driver, rather than the driver
> becoming conditional on multiple platforms.
> 
> As part of this, we rename the dw-hdmi 'spare' which is defined as the
> SVSRET bit in later documentation.

I'd really prefer that we did not go down the broken route of adding
a set of "quirk" flags - look at what a mess SDHCI has become through
allowing that kind of practice.

I'd much rather we find a saner structure to this - and we know that
the hardware has ID registers in it which can be used (so far) to
identify the buggy hardware.


> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
>  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
>  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
>  5 files changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 06a44a2cdf3b..26607185722f 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -118,7 +118,6 @@ struct dw_hdmi {
>  	struct drm_bridge bridge;
>  
>  	struct platform_device *audio;
> -	enum dw_hdmi_devtype dev_type;
>  	struct device *dev;
>  	struct clk *isfr_clk;
>  	struct clk *iahb_clk;
> @@ -896,11 +895,11 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi *hdmi, u8 enable)
>  			 HDMI_PHY_CONF0_ENTMDS_MASK);
>  }
>  
> -static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
> +static void dw_hdmi_phy_enable_svsret(struct dw_hdmi *hdmi, u8 enable)
>  {
>  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
> -			 HDMI_PHY_CONF0_SPARECTRL_OFFSET,
> -			 HDMI_PHY_CONF0_SPARECTRL_MASK);
> +			 HDMI_PHY_CONF0_SVSRET_OFFSET,
> +			 HDMI_PHY_CONF0_SVSRET_MASK);
>  }
>  
>  static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
> @@ -1031,8 +1030,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
>  	dw_hdmi_phy_gen2_txpwron(hdmi, 1);
>  	dw_hdmi_phy_gen2_pddq(hdmi, 0);
>  
> -	if (hdmi->dev_type == RK3288_HDMI)
> -		dw_hdmi_phy_enable_spare(hdmi, 1);
> +	if (pdata->quirks & DW_HDMI_QUIRK_PHY_SVSRET)
> +		dw_hdmi_phy_enable_svsret(hdmi, 1);

If we get a proper split between the encoder and the PHY, this should be
dealt with at the PHY side of the driver.

>  
>  	/*Wait for PHY PLL lock */
>  	msec = 5;
> @@ -1348,7 +1347,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
>  	hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
>  
>  	val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
> -	if (hdmi->dev_type == IMX6DL_HDMI) {
> +	if (hdmi->plat_data->quirks & DW_HDMI_QUIRK_FC_INVIDCONF) {
>  		hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
>  		return;
>  	}

This is a bug found on iMX6DL versions of the IP - I don't have a 6DL
board powered up at the moment to grab its revision information, but
it would be nice to make that conditional on the revision.  From what
I gather, it's a workaround issued by Synopsis rather than specific
to the (ex)FSL implementation.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hi Russell,

On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > The dw-hdmi driver declares a dev_type to distinguish platform specific
> > changes. Replace this with a quirk field, so that the platform can
> > specify the required quirks for the driver, rather than the driver
> > becoming conditional on multiple platforms.
> > 
> > As part of this, we rename the dw-hdmi 'spare' which is defined as the
> > SVSRET bit in later documentation.
> 
> I'd really prefer that we did not go down the broken route of adding
> a set of "quirk" flags - look at what a mess SDHCI has become through
> allowing that kind of practice.
> 
> I'd much rather we find a saner structure to this - and we know that
> the hardware has ID registers in it which can be used (so far) to
> identify the buggy hardware.

I'd much prefer something that would allow runtime identification of the 
device and the corresponding actions to be taken. However, the amount of 
documentation we have on the DWC HDMI TX IP core (and the associated PHY) is 
pretty limited, given that Synopsys doesn't make the documentation available 
publicly. Changes made to the IP core by integrators could complicate this 
further. I'm trying to gather as much information as possible to make clean 
the code up, for instance by trying to identify the PHYs used on the various 
platforms we support. Progress is slow on that front, there isn't enough 
leaked information available online :-) I haven't given up though, but I'll 
need more time.

I don't like quirks much either. They are however already used today, even if 
we trigger them through dev_type instead of quirk flags. This patch came from 
a previous version found in a BSP that simply sprinkled several if (hdmi-
>dev_type == RCAR_HDMI) through the code. For instance,

-	if (hdmi->dev_type == RK3288_HDMI)
+	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
		dw_hdmi_phy_enable_spare(hdmi, 1);

which I think is worse than flags as it would quickly degenerate to spaghetti 
code.

For this specific case, we've managed to identify that on Renesas platforms 
the bit set by this function is called SVSRET. Its usage isn't clear yet, but 
I suspect it to control one of the PHY input control signals, like the other 
bits in the same register. I'm trying to get more information to clean the 
implementation further, hopefully with a way to determine whether the signal 
is used based on PHY identification.

This is all work in progress, and if anyone has access to any documentation 
and can provide additional information I'll be grateful.

> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
> >  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
> >  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
> >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
> >  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
> >  5 files changed, 15 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/dw-hdmi.c index 06a44a2cdf3b..26607185722f
> > 100644
> > --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> > @@ -118,7 +118,6 @@ struct dw_hdmi {
> >  	struct drm_bridge bridge;
> >  	struct platform_device *audio;
> > 
> > -	enum dw_hdmi_devtype dev_type;
> >  	struct device *dev;
> >  	struct clk *isfr_clk;
> >  	struct clk *iahb_clk;
> > @@ -896,11 +895,11 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
> > *hdmi, u8 enable)
> >  			 HDMI_PHY_CONF0_ENTMDS_MASK);
> >  }
> > 
> > -static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
> > +static void dw_hdmi_phy_enable_svsret(struct dw_hdmi *hdmi, u8 enable)
> >  {
> >  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
> > -			 HDMI_PHY_CONF0_SPARECTRL_OFFSET,
> > -			 HDMI_PHY_CONF0_SPARECTRL_MASK);
> > +			 HDMI_PHY_CONF0_SVSRET_OFFSET,
> > +			 HDMI_PHY_CONF0_SVSRET_MASK);
> >  }
> >  
> >  static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
> > @@ -1031,8 +1030,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> >  	dw_hdmi_phy_gen2_txpwron(hdmi, 1);
> >  	dw_hdmi_phy_gen2_pddq(hdmi, 0);
> > 
> > -	if (hdmi->dev_type == RK3288_HDMI)
> > -		dw_hdmi_phy_enable_spare(hdmi, 1);
> > +	if (pdata->quirks & DW_HDMI_QUIRK_PHY_SVSRET)
> > +		dw_hdmi_phy_enable_svsret(hdmi, 1);
> 
> If we get a proper split between the encoder and the PHY, this should be
> dealt with at the PHY side of the driver.

That's possible, and some other PHY-related setup code might be better placed 
in the PHY side as well. My problem, as explained above, is that I don't have 
enough information yet to design a perfect API. I think this patch is a good 
step forward, but it should not be the last one. I wish I could spend more 
time writing clean code instead of trying to reverse engineer the behaviour of 
the PHY :-)

> >  	/*Wait for PHY PLL lock */
> >  	msec = 5;
> > 
> > @@ -1348,7 +1347,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi
> > *hdmi)
> >  	hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
> >  	
> >  	val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
> > -	if (hdmi->dev_type == IMX6DL_HDMI) {
> > +	if (hdmi->plat_data->quirks & DW_HDMI_QUIRK_FC_INVIDCONF) {
> >  		hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
> >  		return;
> >  	}
> 
> This is a bug found on iMX6DL versions of the IP - I don't have a 6DL
> board powered up at the moment to grab its revision information, but
> it would be nice to make that conditional on the revision.  From what
> I gather, it's a workaround issued by Synopsis rather than specific
> to the (ex)FSL implementation.

DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.

Do you know why this function needs to write to the HDMI_FC_INVIDCONF register 
four times in the normal case, and once only for IMX6DL ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hi Russell,

On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > The dw-hdmi driver declares a dev_type to distinguish platform specific
> > changes. Replace this with a quirk field, so that the platform can
> > specify the required quirks for the driver, rather than the driver
> > becoming conditional on multiple platforms.
> > 
> > As part of this, we rename the dw-hdmi 'spare' which is defined as the
> > SVSRET bit in later documentation.
> 
> I'd really prefer that we did not go down the broken route of adding
> a set of "quirk" flags - look at what a mess SDHCI has become through
> allowing that kind of practice.
> 
> I'd much rather we find a saner structure to this - and we know that
> the hardware has ID registers in it which can be used (so far) to
> identify the buggy hardware.

I'd much prefer something that would allow runtime identification of the 
device and the corresponding actions to be taken. However, the amount of 
documentation we have on the DWC HDMI TX IP core (and the associated PHY) is 
pretty limited, given that Synopsys doesn't make the documentation available 
publicly. Changes made to the IP core by integrators could complicate this 
further. I'm trying to gather as much information as possible to make clean 
the code up, for instance by trying to identify the PHYs used on the various 
platforms we support. Progress is slow on that front, there isn't enough 
leaked information available online :-) I haven't given up though, but I'll 
need more time.

I don't like quirks much either. They are however already used today, even if 
we trigger them through dev_type instead of quirk flags. This patch came from 
a previous version found in a BSP that simply sprinkled several if (hdmi-
>dev_type == RCAR_HDMI) through the code. For instance,

-	if (hdmi->dev_type == RK3288_HDMI)
+	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
		dw_hdmi_phy_enable_spare(hdmi, 1);

which I think is worse than flags as it would quickly degenerate to spaghetti 
code.

For this specific case, we've managed to identify that on Renesas platforms 
the bit set by this function is called SVSRET. Its usage isn't clear yet, but 
I suspect it to control one of the PHY input control signals, like the other 
bits in the same register. I'm trying to get more information to clean the 
implementation further, hopefully with a way to determine whether the signal 
is used based on PHY identification.

This is all work in progress, and if anyone has access to any documentation 
and can provide additional information I'll be grateful.

> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
> >  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
> >  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
> >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
> >  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
> >  5 files changed, 15 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/dw-hdmi.c index 06a44a2cdf3b..26607185722f
> > 100644
> > --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> > @@ -118,7 +118,6 @@ struct dw_hdmi {
> >  	struct drm_bridge bridge;
> >  	struct platform_device *audio;
> > 
> > -	enum dw_hdmi_devtype dev_type;
> >  	struct device *dev;
> >  	struct clk *isfr_clk;
> >  	struct clk *iahb_clk;
> > @@ -896,11 +895,11 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
> > *hdmi, u8 enable)
> >  			 HDMI_PHY_CONF0_ENTMDS_MASK);
> >  }
> > 
> > -static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
> > +static void dw_hdmi_phy_enable_svsret(struct dw_hdmi *hdmi, u8 enable)
> >  {
> >  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
> > -			 HDMI_PHY_CONF0_SPARECTRL_OFFSET,
> > -			 HDMI_PHY_CONF0_SPARECTRL_MASK);
> > +			 HDMI_PHY_CONF0_SVSRET_OFFSET,
> > +			 HDMI_PHY_CONF0_SVSRET_MASK);
> >  }
> >  
> >  static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
> > @@ -1031,8 +1030,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> >  	dw_hdmi_phy_gen2_txpwron(hdmi, 1);
> >  	dw_hdmi_phy_gen2_pddq(hdmi, 0);
> > 
> > -	if (hdmi->dev_type == RK3288_HDMI)
> > -		dw_hdmi_phy_enable_spare(hdmi, 1);
> > +	if (pdata->quirks & DW_HDMI_QUIRK_PHY_SVSRET)
> > +		dw_hdmi_phy_enable_svsret(hdmi, 1);
> 
> If we get a proper split between the encoder and the PHY, this should be
> dealt with at the PHY side of the driver.

That's possible, and some other PHY-related setup code might be better placed 
in the PHY side as well. My problem, as explained above, is that I don't have 
enough information yet to design a perfect API. I think this patch is a good 
step forward, but it should not be the last one. I wish I could spend more 
time writing clean code instead of trying to reverse engineer the behaviour of 
the PHY :-)

> >  	/*Wait for PHY PLL lock */
> >  	msec = 5;
> > 
> > @@ -1348,7 +1347,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi
> > *hdmi)
> >  	hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
> >  	
> >  	val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
> > -	if (hdmi->dev_type == IMX6DL_HDMI) {
> > +	if (hdmi->plat_data->quirks & DW_HDMI_QUIRK_FC_INVIDCONF) {
> >  		hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
> >  		return;
> >  	}
> 
> This is a bug found on iMX6DL versions of the IP - I don't have a 6DL
> board powered up at the moment to grab its revision information, but
> it would be nice to make that conditional on the revision.  From what
> I gather, it's a workaround issued by Synopsis rather than specific
> to the (ex)FSL implementation.

DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.

Do you know why this function needs to write to the HDMI_FC_INVIDCONF register 
four times in the normal case, and once only for IMX6DL ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/22] drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter

[Laurent Pinchart]

Hi Russell,

On Friday 02 Dec 2016 14:18:08 Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > The current code hard codes the call of hdmi_phy_configure() to be 8bpp
> > and provides extraneous error checking to verify that this hardcoded
> > value is correct.
> > 
> > Simplify the passing of the data by setting the parameter to be of the
> > enum type it represents rather than converting and then verifying the
> > value. This will allow the compiler to check the value is acceptable
> > based on the type, and remove the dead code that we currently have.
> 
> I think you're expecting too much of the compiler there.  There's no
> requirement for the compiler to check that an enum type is passed one
> of it's defined values.

You're right.

Given that the current driver hardcodes the resolution value to 8bpp, how 
about just dropping the argument ? We can always add it back later if/when 
needed.

> Try building this and see if it even produces a warning:
> 
> enum foo {
> 	FOO_1,
> 	FOO_2,
> };
> 
> int func(enum foo foo)
> {
> 	return foo;
> }
> 
> int test_1(void)
> {
> 	return func(FOO_1);
> }
> 
> int test_2(void)
> {
> 	return func(5);
> }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 11/22] drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter

[Laurent Pinchart]

Hi Russell,

On Friday 02 Dec 2016 14:18:08 Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > The current code hard codes the call of hdmi_phy_configure() to be 8bpp
> > and provides extraneous error checking to verify that this hardcoded
> > value is correct.
> > 
> > Simplify the passing of the data by setting the parameter to be of the
> > enum type it represents rather than converting and then verifying the
> > value. This will allow the compiler to check the value is acceptable
> > based on the type, and remove the dead code that we currently have.
> 
> I think you're expecting too much of the compiler there.  There's no
> requirement for the compiler to check that an enum type is passed one
> of it's defined values.

You're right.

Given that the current driver hardcodes the resolution value to 8bpp, how 
about just dropping the argument ? We can always add it back later if/when 
needed.

> Try building this and see if it even produces a warning:
> 
> enum foo {
> 	FOO_1,
> 	FOO_2,
> };
> 
> int func(enum foo foo)
> {
> 	return foo;
> }
> 
> int test_1(void)
> {
> 	return func(FOO_1);
> }
> 
> int test_2(void)
> {
> 	return func(5);
> }

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Russell King - ARM Linux]

On Fri, Dec 02, 2016 at 05:43:43PM +0200, Laurent Pinchart wrote:
> DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.
> 
> Do you know why this function needs to write to the HDMI_FC_INVIDCONF
> register  four times in the normal case, and once only for IMX6DL ?

(I don't have much time at present, I'm buried in ARM64 crud trying to
get a platform to boot, and working out how to debug an ARM64 platform
that even earlycon doesn't work on... no printascii() types of easy
debug facilities on ARM64 make this job several orders of magnitude
harder than it needs to be...)

It prevents a magenta line down the left hand side of the screen, which
is caused when the frame composer in the HDMI Tx gets confused -
according to the errata, it does a load of maths when you write to the
frame composer registers, and sometimes it doesn't update properly.

So, the four writes (and the number is critical) is there to persuade
the IP to do the maths with the right values so the internal timings
are correct.

The rather confusing thing is - it's actually IMX6Q which has the more
severe errata, not IMX6DL - the workaround of four writes is applied
in the 6Q case.

It's covered by ERR004308 in the IMX6Q Errata document (search for
IMX6DQCE).  It isn't mentioned in the IMX6DL documentation, but it
seems that similar workaround of one write is necessary there.

Some of this was determined by experimentation in conjunction with the
errata documentation - I remember it took a while to get it right so
that we didn't ever see the magenta line.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 11/22] drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter

[Russell King - ARM Linux]

On Fri, Dec 02, 2016 at 05:51:18PM +0200, Laurent Pinchart wrote:
> Hi Russell,
> 
> On Friday 02 Dec 2016 14:18:08 Russell King - ARM Linux wrote:
> > On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
> > > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > 
> > > The current code hard codes the call of hdmi_phy_configure() to be 8bpp
> > > and provides extraneous error checking to verify that this hardcoded
> > > value is correct.
> > > 
> > > Simplify the passing of the data by setting the parameter to be of the
> > > enum type it represents rather than converting and then verifying the
> > > value. This will allow the compiler to check the value is acceptable
> > > based on the type, and remove the dead code that we currently have.
> > 
> > I think you're expecting too much of the compiler there.  There's no
> > requirement for the compiler to check that an enum type is passed one
> > of it's defined values.
> 
> You're right.
> 
> Given that the current driver hardcodes the resolution value to 8bpp, how 
> about just dropping the argument ? We can always add it back later if/when 
> needed.

Definitely - there's no point having features in the driver which no one
uses.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hi Russell,

On Friday 02 Dec 2016 16:08:17 Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 05:43:43PM +0200, Laurent Pinchart wrote:
> > DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.
> > 
> > Do you know why this function needs to write to the HDMI_FC_INVIDCONF
> > register  four times in the normal case, and once only for IMX6DL ?
> 
> (I don't have much time at present, I'm buried in ARM64 crud trying to
> get a platform to boot, and working out how to debug an ARM64 platform
> that even earlycon doesn't work on... no printascii() types of easy
> debug facilities on ARM64 make this job several orders of magnitude
> harder than it needs to be...)

Thanks all the more for taking time to reply.

> It prevents a magenta line down the left hand side of the screen, which
> is caused when the frame composer in the HDMI Tx gets confused -
> according to the errata, it does a load of maths when you write to the
> frame composer registers, and sometimes it doesn't update properly.
> 
> So, the four writes (and the number is critical) is there to persuade
> the IP to do the maths with the right values so the internal timings
> are correct.
> 
> The rather confusing thing is - it's actually IMX6Q which has the more
> severe errata, not IMX6DL - the workaround of four writes is applied
> in the 6Q case.
> 
> It's covered by ERR004308 in the IMX6Q Errata document (search for
> IMX6DQCE).  It isn't mentioned in the IMX6DL documentation, but it
> seems that similar workaround of one write is necessary there.
> 
> Some of this was determined by experimentation in conjunction with the
> errata documentation - I remember it took a while to get it right so
> that we didn't ever see the magenta line.

That's interesting. I'll test the different options on my Renesas platform (no 
write, one write, four writes) and see what is needed. Could anyone perform 
the same tests on a Rockchip RK3288 system ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hi Russell,

On Friday 02 Dec 2016 16:08:17 Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 05:43:43PM +0200, Laurent Pinchart wrote:
> > DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.
> > 
> > Do you know why this function needs to write to the HDMI_FC_INVIDCONF
> > register  four times in the normal case, and once only for IMX6DL ?
> 
> (I don't have much time at present, I'm buried in ARM64 crud trying to
> get a platform to boot, and working out how to debug an ARM64 platform
> that even earlycon doesn't work on... no printascii() types of easy
> debug facilities on ARM64 make this job several orders of magnitude
> harder than it needs to be...)

Thanks all the more for taking time to reply.

> It prevents a magenta line down the left hand side of the screen, which
> is caused when the frame composer in the HDMI Tx gets confused -
> according to the errata, it does a load of maths when you write to the
> frame composer registers, and sometimes it doesn't update properly.
> 
> So, the four writes (and the number is critical) is there to persuade
> the IP to do the maths with the right values so the internal timings
> are correct.
> 
> The rather confusing thing is - it's actually IMX6Q which has the more
> severe errata, not IMX6DL - the workaround of four writes is applied
> in the 6Q case.
> 
> It's covered by ERR004308 in the IMX6Q Errata document (search for
> IMX6DQCE).  It isn't mentioned in the IMX6DL documentation, but it
> seems that similar workaround of one write is necessary there.
> 
> Some of this was determined by experimentation in conjunction with the
> errata documentation - I remember it took a while to get it right so
> that we didn't ever see the magenta line.

That's interesting. I'll test the different options on my Renesas platform (no 
write, one write, four writes) and see what is needed. Could anyone perform 
the same tests on a Rockchip RK3288 system ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/22] drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter

[Kieran Bingham]

On 02/12/16 14:18, Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> The current code hard codes the call of hdmi_phy_configure() to be 8bpp
>> and provides extraneous error checking to verify that this hardcoded
>> value is correct.
>>
>> Simplify the passing of the data by setting the parameter to be of the
>> enum type it represents rather than converting and then verifying the
>> value. This will allow the compiler to check the value is acceptable
>> based on the type, and remove the dead code that we currently have.
> 
> I think you're expecting too much of the compiler there.  There's no
> requirement for the compiler to check that an enum type is passed one
> of it's defined values.
> 
> Try building this and see if it even produces a warning:
> 
> enum foo {
> 	FOO_1,
> 	FOO_2,
> };
> 
> int func(enum foo foo)
> {
> 	return foo;
> }
> 
> int test_1(void)
> {
> 	return func(FOO_1);
> }
> 
> int test_2(void)
> {
> 	return func(5);
> }
> 

Ahh, yes - Sorry - I appear to have got confused between the effects of
gcc/g++.

I knew I had it in my head that the compiler can do enum-type checking
... but it was from when I was working on  C++ projects.

gcc /tmp/test.c -o /tmp/test
	<no stderr output>

g++     /tmp/test.cpp   -o /tmp/test
/tmp/test.cpp: In function �int test_2()�:
/tmp/test.cpp:23:16: error: invalid conversion from �int� to �foo�
[-fpermissive]
   return func(5);

C++ will provide type checking on enums, but of course not C.

Sorry for the confusion, and it looks like Laurent is handling this
already, Thanks


Regards
--
Kieran Bingham

Re: [PATCH 11/22] drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter

[Kieran Bingham]

On 02/12/16 14:18, Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> The current code hard codes the call of hdmi_phy_configure() to be 8bpp
>> and provides extraneous error checking to verify that this hardcoded
>> value is correct.
>>
>> Simplify the passing of the data by setting the parameter to be of the
>> enum type it represents rather than converting and then verifying the
>> value. This will allow the compiler to check the value is acceptable
>> based on the type, and remove the dead code that we currently have.
> 
> I think you're expecting too much of the compiler there.  There's no
> requirement for the compiler to check that an enum type is passed one
> of it's defined values.
> 
> Try building this and see if it even produces a warning:
> 
> enum foo {
> 	FOO_1,
> 	FOO_2,
> };
> 
> int func(enum foo foo)
> {
> 	return foo;
> }
> 
> int test_1(void)
> {
> 	return func(FOO_1);
> }
> 
> int test_2(void)
> {
> 	return func(5);
> }
> 

Ahh, yes - Sorry - I appear to have got confused between the effects of
gcc/g++.

I knew I had it in my head that the compiler can do enum-type checking
... but it was from when I was working on  C++ projects.

gcc /tmp/test.c -o /tmp/test
	<no stderr output>

g++     /tmp/test.cpp   -o /tmp/test
/tmp/test.cpp: In function ‘int test_2()’:
/tmp/test.cpp:23:16: error: invalid conversion from ‘int’ to ‘foo’
[-fpermissive]
   return func(5);

C++ will provide type checking on enums, but of course not C.

Sorry for the confusion, and it looks like Laurent is handling this
already, Thanks


Regards
--
Kieran Bingham

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Jose Abreu]

Hi Laurent,


On 02-12-2016 15:43, Laurent Pinchart wrote:
> Hi Russell,
>
> On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
>> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
>>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>
>>> The dw-hdmi driver declares a dev_type to distinguish platform specific
>>> changes. Replace this with a quirk field, so that the platform can
>>> specify the required quirks for the driver, rather than the driver
>>> becoming conditional on multiple platforms.
>>>
>>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
>>> SVSRET bit in later documentation.
>> I'd really prefer that we did not go down the broken route of adding
>> a set of "quirk" flags - look at what a mess SDHCI has become through
>> allowing that kind of practice.
>>
>> I'd much rather we find a saner structure to this - and we know that
>> the hardware has ID registers in it which can be used (so far) to
>> identify the buggy hardware.
> I'd much prefer something that would allow runtime identification of the 
> device and the corresponding actions to be taken. However, the amount of 
> documentation we have on the DWC HDMI TX IP core (and the associated PHY) is 
> pretty limited, given that Synopsys doesn't make the documentation available 
> publicly. Changes made to the IP core by integrators could complicate this 
> further. I'm trying to gather as much information as possible to make clean 
> the code up, for instance by trying to identify the PHYs used on the various 
> platforms we support. Progress is slow on that front, there isn't enough 
> leaked information available online :-) I haven't given up though, but I'll 
> need more time.
>
> I don't like quirks much either. They are however already used today, even if 
> we trigger them through dev_type instead of quirk flags. This patch came from 
> a previous version found in a BSP that simply sprinkled several if (hdmi-
>> dev_type == RCAR_HDMI) through the code. For instance,
> -	if (hdmi->dev_type == RK3288_HDMI)
> +	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
> 		dw_hdmi_phy_enable_spare(hdmi, 1);
>
> which I think is worse than flags as it would quickly degenerate to spaghetti 
> code.
>
> For this specific case, we've managed to identify that on Renesas platforms 
> the bit set by this function is called SVSRET. Its usage isn't clear yet, but 
> I suspect it to control one of the PHY input control signals, like the other 
> bits in the same register. I'm trying to get more information to clean the 
> implementation further, hopefully with a way to determine whether the signal 
> is used based on PHY identification.

SVSRET is a low power mode consumption and is a PHY input signal
as you suggested. Most of the configurable input signals of the
PHY are available by the controller regbank. I don't think it is
possible to detect this at runtime, I think you have at least to
hardcode which version of the PHY you are using.

I would suggest that maybe all the PHY logic should be extracted
and then use callbacks to glue controller and phy. Then,
depending on the PHY you could use empty stubs if, for example, a
given PHY did not support SVSRET. Still, I don't know if this is
the best option. What I do know is that there are a large number
of PHY's with different flavors that can use the same controller.
The controller has different versions also, and each version can
have quirks but I think it would be easier to manage this driver
if we had a clear distinction between PHY and controller.


>
> This is all work in progress, and if anyone has access to any documentation 
> and can provide additional information I'll be grateful.
>
>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>
>>>  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
>>>  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
>>>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
>>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
>>>  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
>>>  5 files changed, 15 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
>>> b/drivers/gpu/drm/bridge/dw-hdmi.c index 06a44a2cdf3b..26607185722f
>>> 100644
>>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>>> @@ -118,7 +118,6 @@ struct dw_hdmi {
>>>  	struct drm_bridge bridge;
>>>  	struct platform_device *audio;
>>>
>>> -	enum dw_hdmi_devtype dev_type;
>>>  	struct device *dev;
>>>  	struct clk *isfr_clk;
>>>  	struct clk *iahb_clk;
>>> @@ -896,11 +895,11 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
>>> *hdmi, u8 enable)
>>>  			 HDMI_PHY_CONF0_ENTMDS_MASK);
>>>  }
>>>
>>> -static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
>>> +static void dw_hdmi_phy_enable_svsret(struct dw_hdmi *hdmi, u8 enable)
>>>  {
>>>  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
>>> -			 HDMI_PHY_CONF0_SPARECTRL_OFFSET,
>>> -			 HDMI_PHY_CONF0_SPARECTRL_MASK);
>>> +			 HDMI_PHY_CONF0_SVSRET_OFFSET,
>>> +			 HDMI_PHY_CONF0_SVSRET_MASK);
>>>  }
>>>  
>>>  static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
>>> @@ -1031,8 +1030,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
>>>  	dw_hdmi_phy_gen2_txpwron(hdmi, 1);
>>>  	dw_hdmi_phy_gen2_pddq(hdmi, 0);
>>>
>>> -	if (hdmi->dev_type == RK3288_HDMI)
>>> -		dw_hdmi_phy_enable_spare(hdmi, 1);
>>> +	if (pdata->quirks & DW_HDMI_QUIRK_PHY_SVSRET)
>>> +		dw_hdmi_phy_enable_svsret(hdmi, 1);
>> If we get a proper split between the encoder and the PHY, this should be
>> dealt with at the PHY side of the driver.
> That's possible, and some other PHY-related setup code might be better placed 
> in the PHY side as well. My problem, as explained above, is that I don't have 
> enough information yet to design a perfect API. I think this patch is a good 
> step forward, but it should not be the last one. I wish I could spend more 
> time writing clean code instead of trying to reverse engineer the behaviour of 
> the PHY :-)
>
>>>  	/*Wait for PHY PLL lock */
>>>  	msec = 5;
>>>
>>> @@ -1348,7 +1347,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi
>>> *hdmi)
>>>  	hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
>>>  	
>>>  	val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
>>> -	if (hdmi->dev_type == IMX6DL_HDMI) {
>>> +	if (hdmi->plat_data->quirks & DW_HDMI_QUIRK_FC_INVIDCONF) {
>>>  		hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
>>>  		return;
>>>  	}
>> This is a bug found on iMX6DL versions of the IP - I don't have a 6DL
>> board powered up at the moment to grab its revision information, but
>> it would be nice to make that conditional on the revision.  From what
>> I gather, it's a workaround issued by Synopsis rather than specific
>> to the (ex)FSL implementation.
> DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.
>
> Do you know why this function needs to write to the HDMI_FC_INVIDCONF register 
> four times in the normal case, and once only for IMX6DL ?
>

Best regards,
Jose Miguel Abreu

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Jose Abreu]

Hi Laurent,


On 02-12-2016 15:43, Laurent Pinchart wrote:
> Hi Russell,
>
> On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
>> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
>>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>
>>> The dw-hdmi driver declares a dev_type to distinguish platform specific
>>> changes. Replace this with a quirk field, so that the platform can
>>> specify the required quirks for the driver, rather than the driver
>>> becoming conditional on multiple platforms.
>>>
>>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
>>> SVSRET bit in later documentation.
>> I'd really prefer that we did not go down the broken route of adding
>> a set of "quirk" flags - look at what a mess SDHCI has become through
>> allowing that kind of practice.
>>
>> I'd much rather we find a saner structure to this - and we know that
>> the hardware has ID registers in it which can be used (so far) to
>> identify the buggy hardware.
> I'd much prefer something that would allow runtime identification of the 
> device and the corresponding actions to be taken. However, the amount of 
> documentation we have on the DWC HDMI TX IP core (and the associated PHY) is 
> pretty limited, given that Synopsys doesn't make the documentation available 
> publicly. Changes made to the IP core by integrators could complicate this 
> further. I'm trying to gather as much information as possible to make clean 
> the code up, for instance by trying to identify the PHYs used on the various 
> platforms we support. Progress is slow on that front, there isn't enough 
> leaked information available online :-) I haven't given up though, but I'll 
> need more time.
>
> I don't like quirks much either. They are however already used today, even if 
> we trigger them through dev_type instead of quirk flags. This patch came from 
> a previous version found in a BSP that simply sprinkled several if (hdmi-
>> dev_type == RCAR_HDMI) through the code. For instance,
> -	if (hdmi->dev_type == RK3288_HDMI)
> +	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
> 		dw_hdmi_phy_enable_spare(hdmi, 1);
>
> which I think is worse than flags as it would quickly degenerate to spaghetti 
> code.
>
> For this specific case, we've managed to identify that on Renesas platforms 
> the bit set by this function is called SVSRET. Its usage isn't clear yet, but 
> I suspect it to control one of the PHY input control signals, like the other 
> bits in the same register. I'm trying to get more information to clean the 
> implementation further, hopefully with a way to determine whether the signal 
> is used based on PHY identification.

SVSRET is a low power mode consumption and is a PHY input signal
as you suggested. Most of the configurable input signals of the
PHY are available by the controller regbank. I don't think it is
possible to detect this at runtime, I think you have at least to
hardcode which version of the PHY you are using.

I would suggest that maybe all the PHY logic should be extracted
and then use callbacks to glue controller and phy. Then,
depending on the PHY you could use empty stubs if, for example, a
given PHY did not support SVSRET. Still, I don't know if this is
the best option. What I do know is that there are a large number
of PHY's with different flavors that can use the same controller.
The controller has different versions also, and each version can
have quirks but I think it would be easier to manage this driver
if we had a clear distinction between PHY and controller.


>
> This is all work in progress, and if anyone has access to any documentation 
> and can provide additional information I'll be grateful.
>
>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>
>>>  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
>>>  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
>>>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
>>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
>>>  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
>>>  5 files changed, 15 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
>>> b/drivers/gpu/drm/bridge/dw-hdmi.c index 06a44a2cdf3b..26607185722f
>>> 100644
>>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>>> @@ -118,7 +118,6 @@ struct dw_hdmi {
>>>  	struct drm_bridge bridge;
>>>  	struct platform_device *audio;
>>>
>>> -	enum dw_hdmi_devtype dev_type;
>>>  	struct device *dev;
>>>  	struct clk *isfr_clk;
>>>  	struct clk *iahb_clk;
>>> @@ -896,11 +895,11 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
>>> *hdmi, u8 enable)
>>>  			 HDMI_PHY_CONF0_ENTMDS_MASK);
>>>  }
>>>
>>> -static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
>>> +static void dw_hdmi_phy_enable_svsret(struct dw_hdmi *hdmi, u8 enable)
>>>  {
>>>  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
>>> -			 HDMI_PHY_CONF0_SPARECTRL_OFFSET,
>>> -			 HDMI_PHY_CONF0_SPARECTRL_MASK);
>>> +			 HDMI_PHY_CONF0_SVSRET_OFFSET,
>>> +			 HDMI_PHY_CONF0_SVSRET_MASK);
>>>  }
>>>  
>>>  static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
>>> @@ -1031,8 +1030,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
>>>  	dw_hdmi_phy_gen2_txpwron(hdmi, 1);
>>>  	dw_hdmi_phy_gen2_pddq(hdmi, 0);
>>>
>>> -	if (hdmi->dev_type == RK3288_HDMI)
>>> -		dw_hdmi_phy_enable_spare(hdmi, 1);
>>> +	if (pdata->quirks & DW_HDMI_QUIRK_PHY_SVSRET)
>>> +		dw_hdmi_phy_enable_svsret(hdmi, 1);
>> If we get a proper split between the encoder and the PHY, this should be
>> dealt with at the PHY side of the driver.
> That's possible, and some other PHY-related setup code might be better placed 
> in the PHY side as well. My problem, as explained above, is that I don't have 
> enough information yet to design a perfect API. I think this patch is a good 
> step forward, but it should not be the last one. I wish I could spend more 
> time writing clean code instead of trying to reverse engineer the behaviour of 
> the PHY :-)
>
>>>  	/*Wait for PHY PLL lock */
>>>  	msec = 5;
>>>
>>> @@ -1348,7 +1347,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi
>>> *hdmi)
>>>  	hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
>>>  	
>>>  	val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
>>> -	if (hdmi->dev_type == IMX6DL_HDMI) {
>>> +	if (hdmi->plat_data->quirks & DW_HDMI_QUIRK_FC_INVIDCONF) {
>>>  		hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
>>>  		return;
>>>  	}
>> This is a bug found on iMX6DL versions of the IP - I don't have a 6DL
>> board powered up at the moment to grab its revision information, but
>> it would be nice to make that conditional on the revision.  From what
>> I gather, it's a workaround issued by Synopsis rather than specific
>> to the (ex)FSL implementation.
> DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.
>
> Do you know why this function needs to write to the HDMI_FC_INVIDCONF register 
> four times in the normal case, and once only for IMX6DL ?
>

Best regards,
Jose Miguel Abreu

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hi Jose,

On Monday 05 Dec 2016 10:50:19 Jose Abreu wrote:
> On 02-12-2016 15:43, Laurent Pinchart wrote:
> > On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
> >> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
> >>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>> 
> >>> The dw-hdmi driver declares a dev_type to distinguish platform specific
> >>> changes. Replace this with a quirk field, so that the platform can
> >>> specify the required quirks for the driver, rather than the driver
> >>> becoming conditional on multiple platforms.
> >>> 
> >>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
> >>> SVSRET bit in later documentation.
> >> 
> >> I'd really prefer that we did not go down the broken route of adding
> >> a set of "quirk" flags - look at what a mess SDHCI has become through
> >> allowing that kind of practice.
> >> 
> >> I'd much rather we find a saner structure to this - and we know that
> >> the hardware has ID registers in it which can be used (so far) to
> >> identify the buggy hardware.
> > 
> > I'd much prefer something that would allow runtime identification of the
> > device and the corresponding actions to be taken. However, the amount of
> > documentation we have on the DWC HDMI TX IP core (and the associated PHY)
> > is pretty limited, given that Synopsys doesn't make the documentation
> > available publicly. Changes made to the IP core by integrators could
> > complicate this further. I'm trying to gather as much information as
> > possible to make clean the code up, for instance by trying to identify
> > the PHYs used on the various platforms we support. Progress is slow on
> > that front, there isn't enough leaked information available online :-) I
> > haven't given up though, but I'll need more time.
> > 
> > I don't like quirks much either. They are however already used today, even
> > if we trigger them through dev_type instead of quirk flags. This patch
> > came from a previous version found in a BSP that simply sprinkled several
> > if (hdmi-> dev_type == RCAR_HDMI) through the code. For instance,
> > 
> > -	if (hdmi->dev_type == RK3288_HDMI)
> > +	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
> > 		dw_hdmi_phy_enable_spare(hdmi, 1);
> > 
> > which I think is worse than flags as it would quickly degenerate to
> > spaghetti code.
> > 
> > For this specific case, we've managed to identify that on Renesas
> > platforms the bit set by this function is called SVSRET. Its usage isn't
> > clear yet, but I suspect it to control one of the PHY input control
> > signals, like the other bits in the same register. I'm trying to get more
> > information to clean the implementation further, hopefully with a way to
> > determine whether the signal is used based on PHY identification.
> 
> SVSRET is a low power mode consumption and is a PHY input signal
> as you suggested.

Thank you for the confirmation. Would you happen to know what SVSRET stands 
for ?

> Most of the configurable input signals of the PHY are available by the
> controller regbank. I don't think it is possible to detect this at runtime,
> I think you have at least to hardcode which version of the PHY you are
> using.
>
> I would suggest that maybe all the PHY logic should be extracted and then
> use callbacks to glue controller and phy. Then, depending on the PHY you
> could use empty stubs if, for example, a given PHY did not support SVSRET.
> Still, I don't know if this is the best option. What I do know is that there
> are a large number of PHY's with different flavors that can use the same
> controller. The controller has different versions also, and each version can
> have quirks but I think it would be easier to manage this driver if we had a
> clear distinction between PHY and controller.

Agreed, I'd like to go in that direction. What makes it quite difficult is the 
lack of documentation about the PHYs :-) I've found six different PHY types 
that can be identified by the CONFIG2_ID register:

Bits    | Field         	| Description
------------------------------------------------------------------------------
7-0     | phytype       	| PHY interface
        |               	| 0x00: Legacy PHY (HDMI TX PHY)
        |               	| 0xb2: MHL PHY + HEAC PHY
        |               	| 0xc2: MHL PHY
        |               	| 0xe2: HDMI 3D TX PHY + HEAC PHY
        |               	| 0xf2: HDMI 3D TX PHY
        |               	| 0xf3: HDMI2 TX PHY

I'm sure there's more than that. In particular I wonder how external vendor 
PHYs are identified.

I'm also wondering whether there's a need to keep support for the legacy PHY 
signals (ENTMDS and PDZ in the PHY_CONF0 register). As far as I understand 
they're not used by the Gen2 PHYs (including the external vendor PHYs), but I 
can't confirm that without more documentation (although I could test that on 
the platforms I have access to).

> > This is all work in progress, and if anyone has access to any
> > documentation and can provide additional information I'll be grateful.
> > 
> >>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
> >>>  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
> >>>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
> >>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
> >>>  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
> >>>  5 files changed, 15 insertions(+), 20 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hi Jose,

On Monday 05 Dec 2016 10:50:19 Jose Abreu wrote:
> On 02-12-2016 15:43, Laurent Pinchart wrote:
> > On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
> >> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
> >>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>> 
> >>> The dw-hdmi driver declares a dev_type to distinguish platform specific
> >>> changes. Replace this with a quirk field, so that the platform can
> >>> specify the required quirks for the driver, rather than the driver
> >>> becoming conditional on multiple platforms.
> >>> 
> >>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
> >>> SVSRET bit in later documentation.
> >> 
> >> I'd really prefer that we did not go down the broken route of adding
> >> a set of "quirk" flags - look at what a mess SDHCI has become through
> >> allowing that kind of practice.
> >> 
> >> I'd much rather we find a saner structure to this - and we know that
> >> the hardware has ID registers in it which can be used (so far) to
> >> identify the buggy hardware.
> > 
> > I'd much prefer something that would allow runtime identification of the
> > device and the corresponding actions to be taken. However, the amount of
> > documentation we have on the DWC HDMI TX IP core (and the associated PHY)
> > is pretty limited, given that Synopsys doesn't make the documentation
> > available publicly. Changes made to the IP core by integrators could
> > complicate this further. I'm trying to gather as much information as
> > possible to make clean the code up, for instance by trying to identify
> > the PHYs used on the various platforms we support. Progress is slow on
> > that front, there isn't enough leaked information available online :-) I
> > haven't given up though, but I'll need more time.
> > 
> > I don't like quirks much either. They are however already used today, even
> > if we trigger them through dev_type instead of quirk flags. This patch
> > came from a previous version found in a BSP that simply sprinkled several
> > if (hdmi-> dev_type == RCAR_HDMI) through the code. For instance,
> > 
> > -	if (hdmi->dev_type == RK3288_HDMI)
> > +	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
> > 		dw_hdmi_phy_enable_spare(hdmi, 1);
> > 
> > which I think is worse than flags as it would quickly degenerate to
> > spaghetti code.
> > 
> > For this specific case, we've managed to identify that on Renesas
> > platforms the bit set by this function is called SVSRET. Its usage isn't
> > clear yet, but I suspect it to control one of the PHY input control
> > signals, like the other bits in the same register. I'm trying to get more
> > information to clean the implementation further, hopefully with a way to
> > determine whether the signal is used based on PHY identification.
> 
> SVSRET is a low power mode consumption and is a PHY input signal
> as you suggested.

Thank you for the confirmation. Would you happen to know what SVSRET stands 
for ?

> Most of the configurable input signals of the PHY are available by the
> controller regbank. I don't think it is possible to detect this at runtime,
> I think you have at least to hardcode which version of the PHY you are
> using.
>
> I would suggest that maybe all the PHY logic should be extracted and then
> use callbacks to glue controller and phy. Then, depending on the PHY you
> could use empty stubs if, for example, a given PHY did not support SVSRET.
> Still, I don't know if this is the best option. What I do know is that there
> are a large number of PHY's with different flavors that can use the same
> controller. The controller has different versions also, and each version can
> have quirks but I think it would be easier to manage this driver if we had a
> clear distinction between PHY and controller.

Agreed, I'd like to go in that direction. What makes it quite difficult is the 
lack of documentation about the PHYs :-) I've found six different PHY types 
that can be identified by the CONFIG2_ID register:

Bits    | Field         	| Description
------------------------------------------------------------------------------
7-0     | phytype       	| PHY interface
        |               	| 0x00: Legacy PHY (HDMI TX PHY)
        |               	| 0xb2: MHL PHY + HEAC PHY
        |               	| 0xc2: MHL PHY
        |               	| 0xe2: HDMI 3D TX PHY + HEAC PHY
        |               	| 0xf2: HDMI 3D TX PHY
        |               	| 0xf3: HDMI2 TX PHY

I'm sure there's more than that. In particular I wonder how external vendor 
PHYs are identified.

I'm also wondering whether there's a need to keep support for the legacy PHY 
signals (ENTMDS and PDZ in the PHY_CONF0 register). As far as I understand 
they're not used by the Gen2 PHYs (including the external vendor PHYs), but I 
can't confirm that without more documentation (although I could test that on 
the platforms I have access to).

> > This is all work in progress, and if anyone has access to any
> > documentation and can provide additional information I'll be grateful.
> > 
> >>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
> >>>  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
> >>>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
> >>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
> >>>  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
> >>>  5 files changed, 15 insertions(+), 20 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hello,

On Friday 02 Dec 2016 18:45:46 Laurent Pinchart wrote:
> On Friday 02 Dec 2016 16:08:17 Russell King - ARM Linux wrote:
> > On Fri, Dec 02, 2016 at 05:43:43PM +0200, Laurent Pinchart wrote:
> >> DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.
> >> 
> >> Do you know why this function needs to write to the HDMI_FC_INVIDCONF
> >> register  four times in the normal case, and once only for IMX6DL ?
> > 
> > (I don't have much time at present, I'm buried in ARM64 crud trying to
> > get a platform to boot, and working out how to debug an ARM64 platform
> > that even earlycon doesn't work on... no printascii() types of easy
> > debug facilities on ARM64 make this job several orders of magnitude
> > harder than it needs to be...)
> 
> Thanks all the more for taking time to reply.
> 
> > It prevents a magenta line down the left hand side of the screen, which
> > is caused when the frame composer in the HDMI Tx gets confused -
> > according to the errata, it does a load of maths when you write to the
> > frame composer registers, and sometimes it doesn't update properly.
> > 
> > So, the four writes (and the number is critical) is there to persuade
> > the IP to do the maths with the right values so the internal timings
> > are correct.
> > 
> > The rather confusing thing is - it's actually IMX6Q which has the more
> > severe errata, not IMX6DL - the workaround of four writes is applied
> > in the 6Q case.
> > 
> > It's covered by ERR004308 in the IMX6Q Errata document (search for
> > IMX6DQCE).  It isn't mentioned in the IMX6DL documentation, but it
> > seems that similar workaround of one write is necessary there.
> > 
> > Some of this was determined by experimentation in conjunction with the
> > errata documentation - I remember it took a while to get it right so
> > that we didn't ever see the magenta line.
> 
> That's interesting. I'll test the different options on my Renesas platform
> (no write, one write, four writes) and see what is needed. Could anyone
> perform the same tests on a Rockchip RK3288 system ?

Daniel Stone has been nice enough to test HDMI output without the errata 
workaround on RK3288 and hasn't noticed any issue. I've performed the same 
test on R-Car H3 and haven't noticed any issue either.

The i.MX6DL and i.MX6Q use a DWC HDMI TX version 1.31a and 1.30a respectively, 
while RK3288 and R-Car H3 uses v2.00a and v2.01a. We could enable the 
workaround based on the HDMI TX version.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hello,

On Friday 02 Dec 2016 18:45:46 Laurent Pinchart wrote:
> On Friday 02 Dec 2016 16:08:17 Russell King - ARM Linux wrote:
> > On Fri, Dec 02, 2016 at 05:43:43PM +0200, Laurent Pinchart wrote:
> >> DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.
> >> 
> >> Do you know why this function needs to write to the HDMI_FC_INVIDCONF
> >> register  four times in the normal case, and once only for IMX6DL ?
> > 
> > (I don't have much time at present, I'm buried in ARM64 crud trying to
> > get a platform to boot, and working out how to debug an ARM64 platform
> > that even earlycon doesn't work on... no printascii() types of easy
> > debug facilities on ARM64 make this job several orders of magnitude
> > harder than it needs to be...)
> 
> Thanks all the more for taking time to reply.
> 
> > It prevents a magenta line down the left hand side of the screen, which
> > is caused when the frame composer in the HDMI Tx gets confused -
> > according to the errata, it does a load of maths when you write to the
> > frame composer registers, and sometimes it doesn't update properly.
> > 
> > So, the four writes (and the number is critical) is there to persuade
> > the IP to do the maths with the right values so the internal timings
> > are correct.
> > 
> > The rather confusing thing is - it's actually IMX6Q which has the more
> > severe errata, not IMX6DL - the workaround of four writes is applied
> > in the 6Q case.
> > 
> > It's covered by ERR004308 in the IMX6Q Errata document (search for
> > IMX6DQCE).  It isn't mentioned in the IMX6DL documentation, but it
> > seems that similar workaround of one write is necessary there.
> > 
> > Some of this was determined by experimentation in conjunction with the
> > errata documentation - I remember it took a while to get it right so
> > that we didn't ever see the magenta line.
> 
> That's interesting. I'll test the different options on my Renesas platform
> (no write, one write, four writes) and see what is needed. Could anyone
> perform the same tests on a Rockchip RK3288 system ?

Daniel Stone has been nice enough to test HDMI output without the errata 
workaround on RK3288 and hasn't noticed any issue. I've performed the same 
test on R-Car H3 and haven't noticed any issue either.

The i.MX6DL and i.MX6Q use a DWC HDMI TX version 1.31a and 1.30a respectively, 
while RK3288 and R-Car H3 uses v2.00a and v2.01a. We could enable the 
workaround based on the HDMI TX version.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Jose Abreu]

Hi Laurent,


On 05-12-2016 11:32, Laurent Pinchart wrote:
> Hi Jose,
>
> On Monday 05 Dec 2016 10:50:19 Jose Abreu wrote:
>> On 02-12-2016 15:43, Laurent Pinchart wrote:
>>> On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
>>>> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
>>>>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>>>
>>>>> The dw-hdmi driver declares a dev_type to distinguish platform specific
>>>>> changes. Replace this with a quirk field, so that the platform can
>>>>> specify the required quirks for the driver, rather than the driver
>>>>> becoming conditional on multiple platforms.
>>>>>
>>>>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
>>>>> SVSRET bit in later documentation.
>>>> I'd really prefer that we did not go down the broken route of adding
>>>> a set of "quirk" flags - look at what a mess SDHCI has become through
>>>> allowing that kind of practice.
>>>>
>>>> I'd much rather we find a saner structure to this - and we know that
>>>> the hardware has ID registers in it which can be used (so far) to
>>>> identify the buggy hardware.
>>> I'd much prefer something that would allow runtime identification of the
>>> device and the corresponding actions to be taken. However, the amount of
>>> documentation we have on the DWC HDMI TX IP core (and the associated PHY)
>>> is pretty limited, given that Synopsys doesn't make the documentation
>>> available publicly. Changes made to the IP core by integrators could
>>> complicate this further. I'm trying to gather as much information as
>>> possible to make clean the code up, for instance by trying to identify
>>> the PHYs used on the various platforms we support. Progress is slow on
>>> that front, there isn't enough leaked information available online :-) I
>>> haven't given up though, but I'll need more time.
>>>
>>> I don't like quirks much either. They are however already used today, even
>>> if we trigger them through dev_type instead of quirk flags. This patch
>>> came from a previous version found in a BSP that simply sprinkled several
>>> if (hdmi-> dev_type == RCAR_HDMI) through the code. For instance,
>>>
>>> -	if (hdmi->dev_type == RK3288_HDMI)
>>> +	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
>>> 		dw_hdmi_phy_enable_spare(hdmi, 1);
>>>
>>> which I think is worse than flags as it would quickly degenerate to
>>> spaghetti code.
>>>
>>> For this specific case, we've managed to identify that on Renesas
>>> platforms the bit set by this function is called SVSRET. Its usage isn't
>>> clear yet, but I suspect it to control one of the PHY input control
>>> signals, like the other bits in the same register. I'm trying to get more
>>> information to clean the implementation further, hopefully with a way to
>>> determine whether the signal is used based on PHY identification.
>> SVSRET is a low power mode consumption and is a PHY input signal
>> as you suggested.
> Thank you for the confirmation. Would you happen to know what SVSRET stands 
> for ?

Have no info about that. Sorry.

>
>> Most of the configurable input signals of the PHY are available by the
>> controller regbank. I don't think it is possible to detect this at runtime,
>> I think you have at least to hardcode which version of the PHY you are
>> using.
>>
>> I would suggest that maybe all the PHY logic should be extracted and then
>> use callbacks to glue controller and phy. Then, depending on the PHY you
>> could use empty stubs if, for example, a given PHY did not support SVSRET.
>> Still, I don't know if this is the best option. What I do know is that there
>> are a large number of PHY's with different flavors that can use the same
>> controller. The controller has different versions also, and each version can
>> have quirks but I think it would be easier to manage this driver if we had a
>> clear distinction between PHY and controller.
> Agreed, I'd like to go in that direction. What makes it quite difficult is the 
> lack of documentation about the PHYs :-) I've found six different PHY types 
> that can be identified by the CONFIG2_ID register:
>
> Bits    | Field         	| Description
> ------------------------------------------------------------------------------
> 7-0     | phytype       	| PHY interface
>         |               	| 0x00: Legacy PHY (HDMI TX PHY)
>         |               	| 0xb2: MHL PHY + HEAC PHY
>         |               	| 0xc2: MHL PHY
>         |               	| 0xe2: HDMI 3D TX PHY + HEAC PHY
>         |               	| 0xf2: HDMI 3D TX PHY
>         |               	| 0xf3: HDMI2 TX PHY
>
> I'm sure there's more than that. In particular I wonder how external vendor 
> PHYs are identified.

0xFE.

>
> I'm also wondering whether there's a need to keep support for the legacy PHY 
> signals (ENTMDS and PDZ in the PHY_CONF0 register). As far as I understand 
> they're not used by the Gen2 PHYs (including the external vendor PHYs), but I 
> can't confirm that without more documentation (although I could test that on 
> the platforms I have access to).

You are correct. Not available on Gen2 and on external phys.

>>> This is all work in progress, and if anyone has access to any
>>> documentation and can provide additional information I'll be grateful.
>>>
>>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>>> Signed-off-by: Laurent Pinchart
>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>> ---
>>>>>
>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
>>>>>  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
>>>>>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
>>>>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
>>>>>  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
>>>>>  5 files changed, 15 insertions(+), 20 deletions(-)

Best regards,
Jose Miguel Abreu

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Jose Abreu]

Hi Laurent,


On 05-12-2016 11:32, Laurent Pinchart wrote:
> Hi Jose,
>
> On Monday 05 Dec 2016 10:50:19 Jose Abreu wrote:
>> On 02-12-2016 15:43, Laurent Pinchart wrote:
>>> On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
>>>> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
>>>>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>>>
>>>>> The dw-hdmi driver declares a dev_type to distinguish platform specific
>>>>> changes. Replace this with a quirk field, so that the platform can
>>>>> specify the required quirks for the driver, rather than the driver
>>>>> becoming conditional on multiple platforms.
>>>>>
>>>>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
>>>>> SVSRET bit in later documentation.
>>>> I'd really prefer that we did not go down the broken route of adding
>>>> a set of "quirk" flags - look at what a mess SDHCI has become through
>>>> allowing that kind of practice.
>>>>
>>>> I'd much rather we find a saner structure to this - and we know that
>>>> the hardware has ID registers in it which can be used (so far) to
>>>> identify the buggy hardware.
>>> I'd much prefer something that would allow runtime identification of the
>>> device and the corresponding actions to be taken. However, the amount of
>>> documentation we have on the DWC HDMI TX IP core (and the associated PHY)
>>> is pretty limited, given that Synopsys doesn't make the documentation
>>> available publicly. Changes made to the IP core by integrators could
>>> complicate this further. I'm trying to gather as much information as
>>> possible to make clean the code up, for instance by trying to identify
>>> the PHYs used on the various platforms we support. Progress is slow on
>>> that front, there isn't enough leaked information available online :-) I
>>> haven't given up though, but I'll need more time.
>>>
>>> I don't like quirks much either. They are however already used today, even
>>> if we trigger them through dev_type instead of quirk flags. This patch
>>> came from a previous version found in a BSP that simply sprinkled several
>>> if (hdmi-> dev_type == RCAR_HDMI) through the code. For instance,
>>>
>>> -	if (hdmi->dev_type == RK3288_HDMI)
>>> +	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
>>> 		dw_hdmi_phy_enable_spare(hdmi, 1);
>>>
>>> which I think is worse than flags as it would quickly degenerate to
>>> spaghetti code.
>>>
>>> For this specific case, we've managed to identify that on Renesas
>>> platforms the bit set by this function is called SVSRET. Its usage isn't
>>> clear yet, but I suspect it to control one of the PHY input control
>>> signals, like the other bits in the same register. I'm trying to get more
>>> information to clean the implementation further, hopefully with a way to
>>> determine whether the signal is used based on PHY identification.
>> SVSRET is a low power mode consumption and is a PHY input signal
>> as you suggested.
> Thank you for the confirmation. Would you happen to know what SVSRET stands 
> for ?

Have no info about that. Sorry.

>
>> Most of the configurable input signals of the PHY are available by the
>> controller regbank. I don't think it is possible to detect this at runtime,
>> I think you have at least to hardcode which version of the PHY you are
>> using.
>>
>> I would suggest that maybe all the PHY logic should be extracted and then
>> use callbacks to glue controller and phy. Then, depending on the PHY you
>> could use empty stubs if, for example, a given PHY did not support SVSRET.
>> Still, I don't know if this is the best option. What I do know is that there
>> are a large number of PHY's with different flavors that can use the same
>> controller. The controller has different versions also, and each version can
>> have quirks but I think it would be easier to manage this driver if we had a
>> clear distinction between PHY and controller.
> Agreed, I'd like to go in that direction. What makes it quite difficult is the 
> lack of documentation about the PHYs :-) I've found six different PHY types 
> that can be identified by the CONFIG2_ID register:
>
> Bits    | Field         	| Description
> ------------------------------------------------------------------------------
> 7-0     | phytype       	| PHY interface
>         |               	| 0x00: Legacy PHY (HDMI TX PHY)
>         |               	| 0xb2: MHL PHY + HEAC PHY
>         |               	| 0xc2: MHL PHY
>         |               	| 0xe2: HDMI 3D TX PHY + HEAC PHY
>         |               	| 0xf2: HDMI 3D TX PHY
>         |               	| 0xf3: HDMI2 TX PHY
>
> I'm sure there's more than that. In particular I wonder how external vendor 
> PHYs are identified.

0xFE.

>
> I'm also wondering whether there's a need to keep support for the legacy PHY 
> signals (ENTMDS and PDZ in the PHY_CONF0 register). As far as I understand 
> they're not used by the Gen2 PHYs (including the external vendor PHYs), but I 
> can't confirm that without more documentation (although I could test that on 
> the platforms I have access to).

You are correct. Not available on Gen2 and on external phys.

>>> This is all work in progress, and if anyone has access to any
>>> documentation and can provide additional information I'll be grateful.
>>>
>>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>>> Signed-off-by: Laurent Pinchart
>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>> ---
>>>>>
>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
>>>>>  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
>>>>>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
>>>>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
>>>>>  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
>>>>>  5 files changed, 15 insertions(+), 20 deletions(-)

Best regards,
Jose Miguel Abreu

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hi Jose,

On Monday 05 Dec 2016 12:31:30 Jose Abreu wrote:
> On 05-12-2016 11:32, Laurent Pinchart wrote:
> > On Monday 05 Dec 2016 10:50:19 Jose Abreu wrote:
> >> On 02-12-2016 15:43, Laurent Pinchart wrote:
> >>> On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
> >>>> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
> >>>>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>>> 
> >>>>> The dw-hdmi driver declares a dev_type to distinguish platform
> >>>>> specific changes. Replace this with a quirk field, so that the
> >>>>> platform can specify the required quirks for the driver, rather than
> >>>>> the driver becoming conditional on multiple platforms.
> >>>>> 
> >>>>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
> >>>>> SVSRET bit in later documentation.
> >>>> 
> >>>> I'd really prefer that we did not go down the broken route of adding
> >>>> a set of "quirk" flags - look at what a mess SDHCI has become through
> >>>> allowing that kind of practice.
> >>>> 
> >>>> I'd much rather we find a saner structure to this - and we know that
> >>>> the hardware has ID registers in it which can be used (so far) to
> >>>> identify the buggy hardware.
> >>> 
> >>> I'd much prefer something that would allow runtime identification of the
> >>> device and the corresponding actions to be taken. However, the amount of
> >>> documentation we have on the DWC HDMI TX IP core (and the associated
> >>> PHY) is pretty limited, given that Synopsys doesn't make the
> >>> documentation available publicly. Changes made to the IP core by
> >>> integrators could complicate this further. I'm trying to gather as much
> >>> information as possible to make clean the code up, for instance by
> >>> trying to identify the PHYs used on the various platforms we support.
> >>> Progress is slow on that front, there isn't enough leaked information
> >>> available online :-) I haven't given up though, but I'll need more time.
> >>> 
> >>> I don't like quirks much either. They are however already used today,
> >>> even if we trigger them through dev_type instead of quirk flags. This
> >>> patch came from a previous version found in a BSP that simply sprinkled
> >>> several if (hdmi-> dev_type == RCAR_HDMI) through the code. For
> >>> instance,
> >>> 
> >>> -	if (hdmi->dev_type == RK3288_HDMI)
> >>> +	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
> >>> 		dw_hdmi_phy_enable_spare(hdmi, 1);
> >>> 
> >>> which I think is worse than flags as it would quickly degenerate to
> >>> spaghetti code.
> >>> 
> >>> For this specific case, we've managed to identify that on Renesas
> >>> platforms the bit set by this function is called SVSRET. Its usage isn't
> >>> clear yet, but I suspect it to control one of the PHY input control
> >>> signals, like the other bits in the same register. I'm trying to get
> >>> more information to clean the implementation further, hopefully with a
> >>> way to determine whether the signal is used based on PHY identification.
> >> 
> >> SVSRET is a low power mode consumption and is a PHY input signal
> >> as you suggested.
> > 
> > Thank you for the confirmation. Would you happen to know what SVSRET
> > stands for ?
> 
> Have no info about that. Sorry.
> 
> >> Most of the configurable input signals of the PHY are available by the
> >> controller regbank. I don't think it is possible to detect this at
> >> runtime, I think you have at least to hardcode which version of the PHY
> >> you are using.
> >> 
> >> I would suggest that maybe all the PHY logic should be extracted and then
> >> use callbacks to glue controller and phy. Then, depending on the PHY you
> >> could use empty stubs if, for example, a given PHY did not support
> >> SVSRET. Still, I don't know if this is the best option. What I do know is
> >> that there are a large number of PHY's with different flavors that can
> >> use the same controller. The controller has different versions also, and
> >> each version can have quirks but I think it would be easier to manage
> >> this driver if we had a clear distinction between PHY and controller.
> > 
> > Agreed, I'd like to go in that direction. What makes it quite difficult is
> > the lack of documentation about the PHYs :-) I've found six different PHY
> > types that can be identified by the CONFIG2_ID register:
> > 
> > Bits    | Field         	| Description
> > --------------------------------------------------------------------------
> > 7-0     | phytype       	| PHY interface
> >         |               	| 0x00: Legacy PHY (HDMI TX PHY)
> >         |               	| 0xb2: MHL PHY + HEAC PHY
> >         |               	| 0xc2: MHL PHY
> >         |               	| 0xe2: HDMI 3D TX PHY + HEAC PHY
> >         |               	| 0xf2: HDMI 3D TX PHY
> >         |               	| 0xf3: HDMI2 TX PHY
> > 
> > I'm sure there's more than that. In particular I wonder how external
> > vendor PHYs are identified.
> 
> 0xFE.

Thank you. That's the value reported by Allwinner platforms, which expose 
their PHY control registers through APB instead of the internal I2C bus. It 
all starts making sense :-)

> > I'm also wondering whether there's a need to keep support for the legacy
> > PHY signals (ENTMDS and PDZ in the PHY_CONF0 register). As far as I
> > understand they're not used by the Gen2 PHYs (including the external
> > vendor PHYs), but I can't confirm that without more documentation
> > (although I could test that on the platforms I have access to).
> 
> You are correct. Not available on Gen2 and on external phys.

Thank you.

> >>> This is all work in progress, and if anyone has access to any
> >>> documentation and can provide additional information I'll be grateful.
> >>> 
> >>>>> Signed-off-by: Kieran Bingham
> >>>>> <kieran.bingham+renesas@ideasonboard.com>
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
> >>>>>  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
> >>>>>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
> >>>>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
> >>>>>  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
> >>>>>  5 files changed, 15 insertions(+), 20 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Laurent Pinchart]

Hi Jose,

On Monday 05 Dec 2016 12:31:30 Jose Abreu wrote:
> On 05-12-2016 11:32, Laurent Pinchart wrote:
> > On Monday 05 Dec 2016 10:50:19 Jose Abreu wrote:
> >> On 02-12-2016 15:43, Laurent Pinchart wrote:
> >>> On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
> >>>> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
> >>>>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>>> 
> >>>>> The dw-hdmi driver declares a dev_type to distinguish platform
> >>>>> specific changes. Replace this with a quirk field, so that the
> >>>>> platform can specify the required quirks for the driver, rather than
> >>>>> the driver becoming conditional on multiple platforms.
> >>>>> 
> >>>>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
> >>>>> SVSRET bit in later documentation.
> >>>> 
> >>>> I'd really prefer that we did not go down the broken route of adding
> >>>> a set of "quirk" flags - look at what a mess SDHCI has become through
> >>>> allowing that kind of practice.
> >>>> 
> >>>> I'd much rather we find a saner structure to this - and we know that
> >>>> the hardware has ID registers in it which can be used (so far) to
> >>>> identify the buggy hardware.
> >>> 
> >>> I'd much prefer something that would allow runtime identification of the
> >>> device and the corresponding actions to be taken. However, the amount of
> >>> documentation we have on the DWC HDMI TX IP core (and the associated
> >>> PHY) is pretty limited, given that Synopsys doesn't make the
> >>> documentation available publicly. Changes made to the IP core by
> >>> integrators could complicate this further. I'm trying to gather as much
> >>> information as possible to make clean the code up, for instance by
> >>> trying to identify the PHYs used on the various platforms we support.
> >>> Progress is slow on that front, there isn't enough leaked information
> >>> available online :-) I haven't given up though, but I'll need more time.
> >>> 
> >>> I don't like quirks much either. They are however already used today,
> >>> even if we trigger them through dev_type instead of quirk flags. This
> >>> patch came from a previous version found in a BSP that simply sprinkled
> >>> several if (hdmi-> dev_type == RCAR_HDMI) through the code. For
> >>> instance,
> >>> 
> >>> -	if (hdmi->dev_type == RK3288_HDMI)
> >>> +	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
> >>> 		dw_hdmi_phy_enable_spare(hdmi, 1);
> >>> 
> >>> which I think is worse than flags as it would quickly degenerate to
> >>> spaghetti code.
> >>> 
> >>> For this specific case, we've managed to identify that on Renesas
> >>> platforms the bit set by this function is called SVSRET. Its usage isn't
> >>> clear yet, but I suspect it to control one of the PHY input control
> >>> signals, like the other bits in the same register. I'm trying to get
> >>> more information to clean the implementation further, hopefully with a
> >>> way to determine whether the signal is used based on PHY identification.
> >> 
> >> SVSRET is a low power mode consumption and is a PHY input signal
> >> as you suggested.
> > 
> > Thank you for the confirmation. Would you happen to know what SVSRET
> > stands for ?
> 
> Have no info about that. Sorry.
> 
> >> Most of the configurable input signals of the PHY are available by the
> >> controller regbank. I don't think it is possible to detect this at
> >> runtime, I think you have at least to hardcode which version of the PHY
> >> you are using.
> >> 
> >> I would suggest that maybe all the PHY logic should be extracted and then
> >> use callbacks to glue controller and phy. Then, depending on the PHY you
> >> could use empty stubs if, for example, a given PHY did not support
> >> SVSRET. Still, I don't know if this is the best option. What I do know is
> >> that there are a large number of PHY's with different flavors that can
> >> use the same controller. The controller has different versions also, and
> >> each version can have quirks but I think it would be easier to manage
> >> this driver if we had a clear distinction between PHY and controller.
> > 
> > Agreed, I'd like to go in that direction. What makes it quite difficult is
> > the lack of documentation about the PHYs :-) I've found six different PHY
> > types that can be identified by the CONFIG2_ID register:
> > 
> > Bits    | Field         	| Description
> > --------------------------------------------------------------------------
> > 7-0     | phytype       	| PHY interface
> >         |               	| 0x00: Legacy PHY (HDMI TX PHY)
> >         |               	| 0xb2: MHL PHY + HEAC PHY
> >         |               	| 0xc2: MHL PHY
> >         |               	| 0xe2: HDMI 3D TX PHY + HEAC PHY
> >         |               	| 0xf2: HDMI 3D TX PHY
> >         |               	| 0xf3: HDMI2 TX PHY
> > 
> > I'm sure there's more than that. In particular I wonder how external
> > vendor PHYs are identified.
> 
> 0xFE.

Thank you. That's the value reported by Allwinner platforms, which expose 
their PHY control registers through APB instead of the internal I2C bus. It 
all starts making sense :-)

> > I'm also wondering whether there's a need to keep support for the legacy
> > PHY signals (ENTMDS and PDZ in the PHY_CONF0 register). As far as I
> > understand they're not used by the Gen2 PHYs (including the external
> > vendor PHYs), but I can't confirm that without more documentation
> > (although I could test that on the platforms I have access to).
> 
> You are correct. Not available on Gen2 and on external phys.

Thank you.

> >>> This is all work in progress, and if anyone has access to any
> >>> documentation and can provide additional information I'll be grateful.
> >>> 
> >>>>> Signed-off-by: Kieran Bingham
> >>>>> <kieran.bingham+renesas@ideasonboard.com>
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
> >>>>>  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
> >>>>>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
> >>>>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
> >>>>>  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
> >>>>>  5 files changed, 15 insertions(+), 20 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/22] dt-bindings: display: dw-hdmi: Clean up DT bindings documentation

[Rob Herring]

On Fri, Dec 02, 2016 at 01:43:29AM +0200, Laurent Pinchart wrote:
> Make it clear that the core bridge/dw_hdmi.txt document isn't a device
> tree binding by itself but is meant to be referenced by platform device
> tree bindings, and update the Rockchip and Freescale DWC HDMI TX
> bindings to reference it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../devicetree/bindings/display/bridge/dw_hdmi.txt | 85 +++++++++-------------
>  .../devicetree/bindings/display/imx/hdmi.txt       | 51 +++++++------
>  .../bindings/display/rockchip/dw_hdmi-rockchip.txt | 43 +++++++----
>  3 files changed, 91 insertions(+), 88 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

Really, the ddc-i2c-bus prop needs to move to a connector node, but 
that's a separate problem. We might also want to drop the 'ddc-' part 
and have a generic way to refer to an i2c bus. There's a couple of other 
cases.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/22] dt-bindings: display: dw-hdmi: Clean up DT bindings documentation

[Rob Herring]

On Fri, Dec 02, 2016 at 01:43:29AM +0200, Laurent Pinchart wrote:
> Make it clear that the core bridge/dw_hdmi.txt document isn't a device
> tree binding by itself but is meant to be referenced by platform device
> tree bindings, and update the Rockchip and Freescale DWC HDMI TX
> bindings to reference it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../devicetree/bindings/display/bridge/dw_hdmi.txt | 85 +++++++++-------------
>  .../devicetree/bindings/display/imx/hdmi.txt       | 51 +++++++------
>  .../bindings/display/rockchip/dw_hdmi-rockchip.txt | 43 +++++++----
>  3 files changed, 91 insertions(+), 88 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

Really, the ddc-i2c-bus prop needs to move to a connector node, but 
that's a separate problem. We might also want to drop the 'ddc-' part 
and have a generic way to refer to an i2c bus. There's a couple of other 
cases.

Rob

Re: [PATCH 15/22] dt-bindings: display: renesas: Add R-Car Gen3 HDMI TX DT bindings

[Rob Herring]

On Fri, Dec 02, 2016 at 01:43:30AM +0200, Laurent Pinchart wrote:
> The Renesas R-Car Gen3 SoCs use a Synopsys DWC HDMI TX encoder IP. Add
> corresponding device tree bindings based on the DWC HDMI TX bindings
> model.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../bindings/display/bridge/renesas,dw-hdmi.txt    | 75 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt

Acked-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 15/22] dt-bindings: display: renesas: Add R-Car Gen3 HDMI TX DT bindings

[Rob Herring]

On Fri, Dec 02, 2016 at 01:43:30AM +0200, Laurent Pinchart wrote:
> The Renesas R-Car Gen3 SoCs use a Synopsys DWC HDMI TX encoder IP. Add
> corresponding device tree bindings based on the DWC HDMI TX bindings
> model.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../bindings/display/bridge/renesas,dw-hdmi.txt    | 75 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 14/22] dt-bindings: display: dw-hdmi: Clean up DT bindings documentation

[Laurent Pinchart]

Hi Rob,

On Tuesday 06 Dec 2016 15:15:50 Rob Herring wrote:
> On Fri, Dec 02, 2016 at 01:43:29AM +0200, Laurent Pinchart wrote:
> > Make it clear that the core bridge/dw_hdmi.txt document isn't a device
> > tree binding by itself but is meant to be referenced by platform device
> > tree bindings, and update the Rockchip and Freescale DWC HDMI TX
> > bindings to reference it.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../devicetree/bindings/display/bridge/dw_hdmi.txt | 85 +++++++----------
> >  .../devicetree/bindings/display/imx/hdmi.txt       | 51 +++++++------
> >  .../bindings/display/rockchip/dw_hdmi-rockchip.txt | 43 +++++++----
> >  3 files changed, 91 insertions(+), 88 deletions(-)
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
> Really, the ddc-i2c-bus prop needs to move to a connector node, but
> that's a separate problem.

I fully agree. One step at a time though :-)

> We might also want to drop the 'ddc-' part and have a generic way to refer
> to an i2c bus. There's a couple of other cases.

Right, but a DT node might need to refer to multiple I2C buses (probably not 
for connectors though). The ddc- prefix serves a similar purpose than the 
power supply or GPIO name in the *-supply or *-gpios properties.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/22] dt-bindings: display: dw-hdmi: Clean up DT bindings documentation

[Laurent Pinchart]

Hi Rob,

On Tuesday 06 Dec 2016 15:15:50 Rob Herring wrote:
> On Fri, Dec 02, 2016 at 01:43:29AM +0200, Laurent Pinchart wrote:
> > Make it clear that the core bridge/dw_hdmi.txt document isn't a device
> > tree binding by itself but is meant to be referenced by platform device
> > tree bindings, and update the Rockchip and Freescale DWC HDMI TX
> > bindings to reference it.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../devicetree/bindings/display/bridge/dw_hdmi.txt | 85 +++++++----------
> >  .../devicetree/bindings/display/imx/hdmi.txt       | 51 +++++++------
> >  .../bindings/display/rockchip/dw_hdmi-rockchip.txt | 43 +++++++----
> >  3 files changed, 91 insertions(+), 88 deletions(-)
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
> Really, the ddc-i2c-bus prop needs to move to a connector node, but
> that's a separate problem.

I fully agree. One step at a time though :-)

> We might also want to drop the 'ddc-' part and have a generic way to refer
> to an i2c bus. There's a couple of other cases.

Right, but a DT node might need to refer to multiple I2C buses (probably not 
for connectors though). The ddc- prefix serves a similar purpose than the 
power supply or GPIO name in the *-supply or *-gpios properties.

-- 
Regards,

Laurent Pinchart