[PATCH v4 00/10] MIPS: JZ4780 and CI20 HDMI

[PATCH v4 00/10] MIPS: JZ4780 and CI20 HDMI

[H. Nikolaus Schaller]

- fix setting output_port = 1 (issue found by paul@crapouillou.net)
- ci20.dts: convert to use hdmi-connector (by hns@goldelico.com)
- add a hdmi-regulator to control +5V power (by hns@goldelico.com)
- added a fix to dw-hdmi to call drm_kms_helper_hotplug_event on plugin event detection (by hns@goldelico.com)
- always allocate extended descriptor but initialize only for jz4780 (by hns@goldelico.com)
- updated to work on top of "[PATCH v3 0/6] drm/ingenic: Various improvements v3" (by paul@crapouillou.net)
- rebased to v5.13-rc3

PATCH V3 2021-08-08 07:10:50:
This series adds HDMI support for JZ4780 and CI20 board (and fixes one IPU related issue in registration error path)
- [patch 1/8] switched from mode_fixup to atomic_check (suggested by robert.foss@linaro.org)
  - the call to the dw-hdmi specialization is still called mode_fixup
- [patch 3/8] diverse fixes for ingenic-drm-drv (suggested by paul@crapouillou.net)
  - factor out some non-HDMI features of the jz4780 into a separate patch
  - multiple fixes around max height
  - do not change regmap config but a copy on stack
  - define some constants
  - factor out fixing of drm_init error path for IPU into separate patch
  - use FIELD_PREP()
- [patch 8/8] conversion to component framework dropped (suggested by Laurent.pinchart@ideasonboard.com and paul@crapouillou.net)

PATCH V2 2021-08-05 16:08:05:
This series adds HDMI support for JZ4780 and CI20 board

V2:
- code and commit messages revisited for checkpatch warnings
- rebased on v5.14-rc4
- include (failed, hence RFC 8/8) attempt to convert to component framework
  (was suggested by Paul Cercueil <paul@crapouillou.net> a while ago)


H. Nikolaus Schaller (2):
  drm/bridge: synopsis: Fix to properly handle HPD
  MIPS: defconfig: CI20: configure for DRM_DW_HDMI_JZ4780

Paul Boddie (7):
  drm/ingenic: Fix drm_init error path if IPU was registered
  drm/ingenic: Add support for JZ4780 and HDMI output
  drm/bridge: synopsis: Add mode_fixup and bridge timings support
  drm/ingenic: Add dw-hdmi driver for jz4780
  MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD
    controllers
  MIPS: DTS: CI20: Add DT nodes for HDMI setup
  drm/ingenic: add some jz4780 specific features

Sam Ravnborg (1):
  dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

 .../bindings/display/ingenic-jz4780-hdmi.yaml |  85 +++++++++++
 arch/mips/boot/dts/ingenic/ci20.dts           |  67 +++++++++
 arch/mips/boot/dts/ingenic/jz4780.dtsi        |  45 ++++++
 arch/mips/configs/ci20_defconfig              |   6 +
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     |   9 ++
 drivers/gpu/drm/ingenic/Kconfig               |   9 ++
 drivers/gpu/drm/ingenic/Makefile              |   1 +
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c     | 135 ++++++++++++++++-
 drivers/gpu/drm/ingenic/ingenic-drm.h         |  42 ++++++
 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c     | 142 ++++++++++++++++++
 include/drm/bridge/dw_hdmi.h                  |   5 +
 11 files changed, 540 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
 create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c

-- 
2.31.1

[PATCH v4 01/10] drm/ingenic: Fix drm_init error path if IPU was registered

[H. Nikolaus Schaller]

From: Paul Boddie <paul@boddie.org.uk>

If ingenic drm driver can not be registered, the IPU driver won't be
deregistered.

Code structure is chosen in preparation to add hdmi unregistration
in error case following the same pattern by a later patch.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 35b61657d9f6..f73522bdacaa 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -1498,7 +1498,16 @@ static int ingenic_drm_init(void)
 			return err;
 	}
 
-	return platform_driver_register(&ingenic_drm_driver);
+	err = platform_driver_register(&ingenic_drm_driver);
+	if (err)
+		goto err_ipu_unreg;
+
+	return 0;
+
+err_ipu_unreg:
+	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
+		platform_driver_unregister(ingenic_ipu_driver_ptr);
+	return err;
 }
 module_init(ingenic_drm_init);
 
-- 
2.31.1

[PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

From: Paul Boddie <paul@boddie.org.uk>

Add support for the LCD controller present on JZ4780 SoCs.
This SoC uses 8-byte descriptors which extend the current
4-byte descriptors used for other Ingenic SoCs.

Tested on MIPS Creator CI20 board.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +++++++++++++++++++++--
 drivers/gpu/drm/ingenic/ingenic-drm.h     | 42 +++++++++++
 2 files changed, 122 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index f73522bdacaa..e2df4b085905 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -6,6 +6,7 @@
 
 #include "ingenic-drm.h"
 
+#include <linux/bitfield.h>
 #include <linux/component.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
@@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc {
 	u32 addr;
 	u32 id;
 	u32 cmd;
+	/* extended hw descriptor for jz4780 */
+	u32 offsize;
+	u32 pagewidth;
+	u32 cpos;
+	u32 dessize;
 } __aligned(16);
 
 struct ingenic_dma_hwdescs {
@@ -60,9 +66,11 @@ struct jz_soc_info {
 	bool needs_dev_clk;
 	bool has_osd;
 	bool map_noncoherent;
+	bool use_extended_hwdesc;
 	unsigned int max_width, max_height;
 	const u32 *formats_f0, *formats_f1;
 	unsigned int num_formats_f0, num_formats_f1;
+	unsigned int max_reg;
 };
 
 struct ingenic_drm_private_state {
@@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
 	}
 }
 
-static const struct regmap_config ingenic_drm_regmap_config = {
+static struct regmap_config ingenic_drm_regmap_config = {
 	.reg_bits = 32,
 	.val_bits = 32,
 	.reg_stride = 4,
 
-	.max_register = JZ_REG_LCD_SIZE1,
 	.writeable_reg = ingenic_drm_writeable_reg,
 };
 
@@ -663,6 +670,37 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
 		hwdesc->next = dma_hwdesc_addr(priv, next_id);
 
+		if (priv->soc_info->use_extended_hwdesc) {
+			hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE;
+
+			/* Extended 8-byte descriptor */
+			hwdesc->cpos = 0;
+			hwdesc->offsize = 0;
+			hwdesc->pagewidth = 0;
+
+			switch (newstate->fb->format->format) {
+			case DRM_FORMAT_XRGB1555:
+				hwdesc->cpos |= JZ_LCD_CPOS_RGB555;
+				fallthrough;
+			case DRM_FORMAT_RGB565:
+				hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16;
+				break;
+			case DRM_FORMAT_XRGB8888:
+				hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
+				break;
+			}
+			hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
+					    (JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 <<
+					     JZ_LCD_CPOS_COEFFICIENT_OFFSET);
+
+			hwdesc->dessize =
+				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
+				FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK <<
+					   JZ_LCD_DESSIZE_HEIGHT_OFFSET, height - 1) |
+				FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK <<
+					   JZ_LCD_DESSIZE_WIDTH_OFFSET, width - 1);
+		}
+
 		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
 			fourcc = newstate->fb->format->format;
 
@@ -694,6 +732,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
 	}
 
+	/* set use of the 8-word descriptor and OSD foreground usage. */
+	if (priv->soc_info->use_extended_hwdesc)
+		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
+
 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
 		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
@@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	struct drm_encoder *encoder;
 	struct ingenic_drm_bridge *ib;
 	struct drm_device *drm;
+	struct regmap_config regmap_config;
 	void __iomem *base;
 	long parent_rate;
 	unsigned int i, clone_mask = 0;
@@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 		return PTR_ERR(base);
 	}
 
+	regmap_config = ingenic_drm_regmap_config;
+	regmap_config.max_register = soc_info->max_reg;
 	priv->map = devm_regmap_init_mmio(dev, base,
-					  &ingenic_drm_regmap_config);
+					  &regmap_config);
 	if (IS_ERR(priv->map)) {
 		dev_err(dev, "Failed to create regmap\n");
 		return PTR_ERR(priv->map);
@@ -1274,7 +1319,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 	/* Enable OSD if available */
 	if (soc_info->has_osd)
-		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
+		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
 
 	mutex_init(&priv->clk_mutex);
 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
@@ -1444,6 +1489,7 @@ static const struct jz_soc_info jz4740_soc_info = {
 	.formats_f1 = jz4740_formats,
 	.num_formats_f1 = ARRAY_SIZE(jz4740_formats),
 	/* JZ4740 has only one plane */
+	.max_reg = JZ_REG_LCD_SIZE1,
 };
 
 static const struct jz_soc_info jz4725b_soc_info = {
@@ -1456,6 +1502,7 @@ static const struct jz_soc_info jz4725b_soc_info = {
 	.num_formats_f1 = ARRAY_SIZE(jz4725b_formats_f1),
 	.formats_f0 = jz4725b_formats_f0,
 	.num_formats_f0 = ARRAY_SIZE(jz4725b_formats_f0),
+	.max_reg = JZ_REG_LCD_SIZE1,
 };
 
 static const struct jz_soc_info jz4770_soc_info = {
@@ -1468,12 +1515,28 @@ static const struct jz_soc_info jz4770_soc_info = {
 	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
 	.formats_f0 = jz4770_formats_f0,
 	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
+	.max_reg = JZ_REG_LCD_SIZE1,
+};
+
+static const struct jz_soc_info jz4780_soc_info = {
+	.needs_dev_clk = true,
+	.has_osd = true,
+	.use_extended_hwdesc = true,
+	.max_width = 4096,
+	.max_height = 2048,
+	/* REVISIT: do we support formats different from jz4770? */
+	.formats_f1 = jz4770_formats_f1,
+	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
+	.formats_f0 = jz4770_formats_f0,
+	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
+	.max_reg = JZ_REG_LCD_PCFG,
 };
 
 static const struct of_device_id ingenic_drm_of_match[] = {
 	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
 	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info },
 	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
+	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, ingenic_drm_of_match);
@@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
 {
 	int err;
 
+	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
+		err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
+		if (err)
+			return err;
+	}
+
 	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) {
 		err = platform_driver_register(ingenic_ipu_driver_ptr);
 		if (err)
-			return err;
+			goto err_hdmi_unreg;
 	}
 
 	err = platform_driver_register(&ingenic_drm_driver);
@@ -1507,6 +1576,10 @@ static int ingenic_drm_init(void)
 err_ipu_unreg:
 	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
 		platform_driver_unregister(ingenic_ipu_driver_ptr);
+err_hdmi_unreg:
+	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))
+		platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
+
 	return err;
 }
 module_init(ingenic_drm_init);
@@ -1515,6 +1588,8 @@ static void ingenic_drm_exit(void)
 {
 	platform_driver_unregister(&ingenic_drm_driver);
 
+	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))
+		platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
 	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
 		platform_driver_unregister(ingenic_ipu_driver_ptr);
 }
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h
index 22654ac1dde1..13dbc5d25c3b 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm.h
+++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
@@ -44,8 +44,11 @@
 #define JZ_REG_LCD_XYP1				0x124
 #define JZ_REG_LCD_SIZE0			0x128
 #define JZ_REG_LCD_SIZE1			0x12c
+#define JZ_REG_LCD_PCFG				0x2c0
 
 #define JZ_LCD_CFG_SLCD				BIT(31)
+#define JZ_LCD_CFG_DESCRIPTOR_8			BIT(28)
+#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN	BIT(25)
 #define JZ_LCD_CFG_PS_DISABLE			BIT(23)
 #define JZ_LCD_CFG_CLS_DISABLE			BIT(22)
 #define JZ_LCD_CFG_SPL_DISABLE			BIT(21)
@@ -63,6 +66,7 @@
 #define JZ_LCD_CFG_DE_ACTIVE_LOW		BIT(9)
 #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW		BIT(8)
 #define JZ_LCD_CFG_18_BIT			BIT(7)
+#define JZ_LCD_CFG_24_BIT			BIT(6)
 #define JZ_LCD_CFG_PDW				(BIT(5) | BIT(4))
 
 #define JZ_LCD_CFG_MODE_GENERIC_16BIT		0
@@ -132,6 +136,7 @@
 #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
 #define JZ_LCD_CMD_EOF_IRQ			BIT(30)
 #define JZ_LCD_CMD_ENABLE_PAL			BIT(28)
+#define JZ_LCD_CMD_FRM_ENABLE			BIT(26)
 
 #define JZ_LCD_SYNC_MASK			0x3ff
 
@@ -153,6 +158,7 @@
 #define JZ_LCD_RGBC_EVEN_BGR			(0x5 << 0)
 
 #define JZ_LCD_OSDC_OSDEN			BIT(0)
+#define JZ_LCD_OSDC_ALPHAEN			BIT(2)
 #define JZ_LCD_OSDC_F0EN			BIT(3)
 #define JZ_LCD_OSDC_F1EN			BIT(4)
 
@@ -176,6 +182,41 @@
 #define JZ_LCD_SIZE01_WIDTH_LSB			0
 #define JZ_LCD_SIZE01_HEIGHT_LSB		16
 
+#define JZ_LCD_DESSIZE_ALPHA_OFFSET		24
+#define JZ_LCD_DESSIZE_HEIGHT_OFFSET		12
+#define JZ_LCD_DESSIZE_WIDTH_OFFSET		0
+#define JZ_LCD_DESSIZE_HEIGHT_MASK		0xfff
+#define JZ_LCD_DESSIZE_WIDTH_MASK		0xfff
+
+/* TODO: 4,5 and 7 match the above BPP */
+#define JZ_LCD_CPOS_BPP_15_16			(4 << 27)
+#define JZ_LCD_CPOS_BPP_18_24			(5 << 27)
+#define JZ_LCD_CPOS_BPP_30			(7 << 27)
+#define JZ_LCD_CPOS_RGB555			BIT(30)
+#define JZ_LCD_CPOS_PREMULTIPLY_LCD		BIT(26)
+#define JZ_LCD_CPOS_COEFFICIENT_OFFSET		24
+#define JZ_LCD_CPOS_COEFFICIENT_0		0
+#define JZ_LCD_CPOS_COEFFICIENT_1		1
+#define JZ_LCD_CPOS_COEFFICIENT_ALPHA1		2
+#define JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1	3
+
+#define JZ_LCD_RGBC_RGB_PADDING			BIT(15)
+#define JZ_LCD_RGBC_RGB_PADDING_FIRST		BIT(14)
+#define JZ_LCD_RGBC_422				BIT(8)
+#define JZ_LCD_RGBC_RGB_FORMAT_ENABLE		BIT(7)
+
+#define JZ_LCD_PCFG_PRI_MODE			BIT(31)
+#define JZ_LCD_PCFG_HP_BST_4			(0 << 28)
+#define JZ_LCD_PCFG_HP_BST_8			(1 << 28)
+#define JZ_LCD_PCFG_HP_BST_16			(2 << 28)
+#define JZ_LCD_PCFG_HP_BST_32			(3 << 28)
+#define JZ_LCD_PCFG_HP_BST_64			(4 << 28)
+#define JZ_LCD_PCFG_HP_BST_16_CONT		(5 << 28)
+#define JZ_LCD_PCFG_HP_BST_DISABLE		(7 << 28)
+#define JZ_LCD_PCFG_THRESHOLD2_OFFSET		18
+#define JZ_LCD_PCFG_THRESHOLD1_OFFSET		9
+#define JZ_LCD_PCFG_THRESHOLD0_OFFSET		0
+
 struct device;
 struct drm_plane;
 struct drm_plane_state;
@@ -187,5 +228,6 @@ void ingenic_drm_plane_disable(struct device *dev, struct drm_plane *plane);
 bool ingenic_drm_map_noncoherent(const struct device *dev);
 
 extern struct platform_driver *ingenic_ipu_driver_ptr;
+extern struct platform_driver *ingenic_dw_hdmi_driver_ptr;
 
 #endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */
-- 
2.31.1

Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

Hi Nikolaus / Paul,

Le lun., sept. 27 2021 at 18:44:20 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> From: Paul Boddie <paul@boddie.org.uk>
> 
> Add support for the LCD controller present on JZ4780 SoCs.
> This SoC uses 8-byte descriptors which extend the current
> 4-byte descriptors used for other Ingenic SoCs.
> 
> Tested on MIPS Creator CI20 board.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 
> +++++++++++++++++++++--
>  drivers/gpu/drm/ingenic/ingenic-drm.h     | 42 +++++++++++
>  2 files changed, 122 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index f73522bdacaa..e2df4b085905 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -6,6 +6,7 @@
> 
>  #include "ingenic-drm.h"
> 
> +#include <linux/bitfield.h>
>  #include <linux/component.h>
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
> @@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc {
>  	u32 addr;
>  	u32 id;
>  	u32 cmd;
> +	/* extended hw descriptor for jz4780 */
> +	u32 offsize;
> +	u32 pagewidth;
> +	u32 cpos;
> +	u32 dessize;
>  } __aligned(16);
> 
>  struct ingenic_dma_hwdescs {
> @@ -60,9 +66,11 @@ struct jz_soc_info {
>  	bool needs_dev_clk;
>  	bool has_osd;
>  	bool map_noncoherent;
> +	bool use_extended_hwdesc;
>  	unsigned int max_width, max_height;
>  	const u32 *formats_f0, *formats_f1;
>  	unsigned int num_formats_f0, num_formats_f1;
> +	unsigned int max_reg;
>  };
> 
>  struct ingenic_drm_private_state {
> @@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct 
> device *dev, unsigned int reg)
>  	}
>  }
> 
> -static const struct regmap_config ingenic_drm_regmap_config = {
> +static struct regmap_config ingenic_drm_regmap_config = {
>  	.reg_bits = 32,
>  	.val_bits = 32,
>  	.reg_stride = 4,
> 
> -	.max_register = JZ_REG_LCD_SIZE1,
>  	.writeable_reg = ingenic_drm_writeable_reg,
>  };
> 
> @@ -663,6 +670,37 @@ static void 
> ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>  		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
>  		hwdesc->next = dma_hwdesc_addr(priv, next_id);
> 
> +		if (priv->soc_info->use_extended_hwdesc) {
> +			hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE;
> +
> +			/* Extended 8-byte descriptor */
> +			hwdesc->cpos = 0;
> +			hwdesc->offsize = 0;
> +			hwdesc->pagewidth = 0;
> +
> +			switch (newstate->fb->format->format) {
> +			case DRM_FORMAT_XRGB1555:
> +				hwdesc->cpos |= JZ_LCD_CPOS_RGB555;
> +				fallthrough;
> +			case DRM_FORMAT_RGB565:
> +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16;
> +				break;
> +			case DRM_FORMAT_XRGB8888:
> +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
> +				break;
> +			}
> +			hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
> +					    (JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 <<
> +					     JZ_LCD_CPOS_COEFFICIENT_OFFSET);
> +
> +			hwdesc->dessize =
> +				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
> +				FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK <<
> +					   JZ_LCD_DESSIZE_HEIGHT_OFFSET, height - 1) |
> +				FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK <<
> +					   JZ_LCD_DESSIZE_WIDTH_OFFSET, width - 1);
> +		}
> +
>  		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>  			fourcc = newstate->fb->format->format;
> 
> @@ -694,6 +732,10 @@ static void 
> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>  		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>  	}
> 
> +	/* set use of the 8-word descriptor and OSD foreground usage. */
> +	if (priv->soc_info->use_extended_hwdesc)
> +		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
> +
>  	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>  		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
>  	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> @@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, 
> bool has_components)
>  	struct drm_encoder *encoder;
>  	struct ingenic_drm_bridge *ib;
>  	struct drm_device *drm;
> +	struct regmap_config regmap_config;
>  	void __iomem *base;
>  	long parent_rate;
>  	unsigned int i, clone_mask = 0;
> @@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct device 
> *dev, bool has_components)
>  		return PTR_ERR(base);
>  	}
> 
> +	regmap_config = ingenic_drm_regmap_config;
> +	regmap_config.max_register = soc_info->max_reg;
>  	priv->map = devm_regmap_init_mmio(dev, base,
> -					  &ingenic_drm_regmap_config);
> +					  &regmap_config);

Could you split the code that makes .max_reg configurable per-SoC into 
its own patch?

>  	if (IS_ERR(priv->map)) {
>  		dev_err(dev, "Failed to create regmap\n");
>  		return PTR_ERR(priv->map);
> @@ -1274,7 +1319,7 @@ static int ingenic_drm_bind(struct device *dev, 
> bool has_components)
> 
>  	/* Enable OSD if available */
>  	if (soc_info->has_osd)
> -		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);

Why?

> 
>  	mutex_init(&priv->clk_mutex);
>  	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
> @@ -1444,6 +1489,7 @@ static const struct jz_soc_info jz4740_soc_info 
> = {
>  	.formats_f1 = jz4740_formats,
>  	.num_formats_f1 = ARRAY_SIZE(jz4740_formats),
>  	/* JZ4740 has only one plane */
> +	.max_reg = JZ_REG_LCD_SIZE1,
>  };
> 
>  static const struct jz_soc_info jz4725b_soc_info = {
> @@ -1456,6 +1502,7 @@ static const struct jz_soc_info 
> jz4725b_soc_info = {
>  	.num_formats_f1 = ARRAY_SIZE(jz4725b_formats_f1),
>  	.formats_f0 = jz4725b_formats_f0,
>  	.num_formats_f0 = ARRAY_SIZE(jz4725b_formats_f0),
> +	.max_reg = JZ_REG_LCD_SIZE1,
>  };
> 
>  static const struct jz_soc_info jz4770_soc_info = {
> @@ -1468,12 +1515,28 @@ static const struct jz_soc_info 
> jz4770_soc_info = {
>  	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
>  	.formats_f0 = jz4770_formats_f0,
>  	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
> +	.max_reg = JZ_REG_LCD_SIZE1,
> +};
> +
> +static const struct jz_soc_info jz4780_soc_info = {
> +	.needs_dev_clk = true,
> +	.has_osd = true,
> +	.use_extended_hwdesc = true,
> +	.max_width = 4096,
> +	.max_height = 2048,
> +	/* REVISIT: do we support formats different from jz4770? */
> +	.formats_f1 = jz4770_formats_f1,
> +	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
> +	.formats_f0 = jz4770_formats_f0,
> +	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
> +	.max_reg = JZ_REG_LCD_PCFG,
>  };
> 
>  static const struct of_device_id ingenic_drm_of_match[] = {
>  	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
>  	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info },
>  	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
> +	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, ingenic_drm_of_match);
> @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
>  {
>  	int err;
> 
> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
> +		err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
> +		if (err)
> +			return err;
> +	}

I don't see why you need to register the ingenic-dw-hdmi driver here. 
Just register it in the ingenic-dw-hdmi driver.

Cheers,
-Paul

> +
>  	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) {
>  		err = platform_driver_register(ingenic_ipu_driver_ptr);
>  		if (err)
> -			return err;
> +			goto err_hdmi_unreg;
>  	}
> 
>  	err = platform_driver_register(&ingenic_drm_driver);
> @@ -1507,6 +1576,10 @@ static int ingenic_drm_init(void)
>  err_ipu_unreg:
>  	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
>  		platform_driver_unregister(ingenic_ipu_driver_ptr);
> +err_hdmi_unreg:
> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))
> +		platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
> +
>  	return err;
>  }
>  module_init(ingenic_drm_init);
> @@ -1515,6 +1588,8 @@ static void ingenic_drm_exit(void)
>  {
>  	platform_driver_unregister(&ingenic_drm_driver);
> 
> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))
> +		platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
>  	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
>  		platform_driver_unregister(ingenic_ipu_driver_ptr);
>  }
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h 
> b/drivers/gpu/drm/ingenic/ingenic-drm.h
> index 22654ac1dde1..13dbc5d25c3b 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm.h
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
> @@ -44,8 +44,11 @@
>  #define JZ_REG_LCD_XYP1				0x124
>  #define JZ_REG_LCD_SIZE0			0x128
>  #define JZ_REG_LCD_SIZE1			0x12c
> +#define JZ_REG_LCD_PCFG				0x2c0
> 
>  #define JZ_LCD_CFG_SLCD				BIT(31)
> +#define JZ_LCD_CFG_DESCRIPTOR_8			BIT(28)
> +#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN	BIT(25)
>  #define JZ_LCD_CFG_PS_DISABLE			BIT(23)
>  #define JZ_LCD_CFG_CLS_DISABLE			BIT(22)
>  #define JZ_LCD_CFG_SPL_DISABLE			BIT(21)
> @@ -63,6 +66,7 @@
>  #define JZ_LCD_CFG_DE_ACTIVE_LOW		BIT(9)
>  #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW		BIT(8)
>  #define JZ_LCD_CFG_18_BIT			BIT(7)
> +#define JZ_LCD_CFG_24_BIT			BIT(6)
>  #define JZ_LCD_CFG_PDW				(BIT(5) | BIT(4))
> 
>  #define JZ_LCD_CFG_MODE_GENERIC_16BIT		0
> @@ -132,6 +136,7 @@
>  #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
>  #define JZ_LCD_CMD_EOF_IRQ			BIT(30)
>  #define JZ_LCD_CMD_ENABLE_PAL			BIT(28)
> +#define JZ_LCD_CMD_FRM_ENABLE			BIT(26)
> 
>  #define JZ_LCD_SYNC_MASK			0x3ff
> 
> @@ -153,6 +158,7 @@
>  #define JZ_LCD_RGBC_EVEN_BGR			(0x5 << 0)
> 
>  #define JZ_LCD_OSDC_OSDEN			BIT(0)
> +#define JZ_LCD_OSDC_ALPHAEN			BIT(2)
>  #define JZ_LCD_OSDC_F0EN			BIT(3)
>  #define JZ_LCD_OSDC_F1EN			BIT(4)
> 
> @@ -176,6 +182,41 @@
>  #define JZ_LCD_SIZE01_WIDTH_LSB			0
>  #define JZ_LCD_SIZE01_HEIGHT_LSB		16
> 
> +#define JZ_LCD_DESSIZE_ALPHA_OFFSET		24
> +#define JZ_LCD_DESSIZE_HEIGHT_OFFSET		12
> +#define JZ_LCD_DESSIZE_WIDTH_OFFSET		0
> +#define JZ_LCD_DESSIZE_HEIGHT_MASK		0xfff
> +#define JZ_LCD_DESSIZE_WIDTH_MASK		0xfff
> +
> +/* TODO: 4,5 and 7 match the above BPP */
> +#define JZ_LCD_CPOS_BPP_15_16			(4 << 27)
> +#define JZ_LCD_CPOS_BPP_18_24			(5 << 27)
> +#define JZ_LCD_CPOS_BPP_30			(7 << 27)
> +#define JZ_LCD_CPOS_RGB555			BIT(30)
> +#define JZ_LCD_CPOS_PREMULTIPLY_LCD		BIT(26)
> +#define JZ_LCD_CPOS_COEFFICIENT_OFFSET		24
> +#define JZ_LCD_CPOS_COEFFICIENT_0		0
> +#define JZ_LCD_CPOS_COEFFICIENT_1		1
> +#define JZ_LCD_CPOS_COEFFICIENT_ALPHA1		2
> +#define JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1	3
> +
> +#define JZ_LCD_RGBC_RGB_PADDING			BIT(15)
> +#define JZ_LCD_RGBC_RGB_PADDING_FIRST		BIT(14)
> +#define JZ_LCD_RGBC_422				BIT(8)
> +#define JZ_LCD_RGBC_RGB_FORMAT_ENABLE		BIT(7)
> +
> +#define JZ_LCD_PCFG_PRI_MODE			BIT(31)
> +#define JZ_LCD_PCFG_HP_BST_4			(0 << 28)
> +#define JZ_LCD_PCFG_HP_BST_8			(1 << 28)
> +#define JZ_LCD_PCFG_HP_BST_16			(2 << 28)
> +#define JZ_LCD_PCFG_HP_BST_32			(3 << 28)
> +#define JZ_LCD_PCFG_HP_BST_64			(4 << 28)
> +#define JZ_LCD_PCFG_HP_BST_16_CONT		(5 << 28)
> +#define JZ_LCD_PCFG_HP_BST_DISABLE		(7 << 28)
> +#define JZ_LCD_PCFG_THRESHOLD2_OFFSET		18
> +#define JZ_LCD_PCFG_THRESHOLD1_OFFSET		9
> +#define JZ_LCD_PCFG_THRESHOLD0_OFFSET		0
> +
>  struct device;
>  struct drm_plane;
>  struct drm_plane_state;
> @@ -187,5 +228,6 @@ void ingenic_drm_plane_disable(struct device 
> *dev, struct drm_plane *plane);
>  bool ingenic_drm_map_noncoherent(const struct device *dev);
> 
>  extern struct platform_driver *ingenic_ipu_driver_ptr;
> +extern struct platform_driver *ingenic_dw_hdmi_driver_ptr;
> 
>  #endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */
> --
> 2.31.1
> 

Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi,

> Am 28.09.2021 um 11:35 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus / Paul,
> 
> Le lun., sept. 27 2021 at 18:44:20 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> From: Paul Boddie <paul@boddie.org.uk>
>> Add support for the LCD controller present on JZ4780 SoCs.
>> This SoC uses 8-byte descriptors which extend the current
>> 4-byte descriptors used for other Ingenic SoCs.
>> Tested on MIPS Creator CI20 board.
>> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +++++++++++++++++++++--
>> drivers/gpu/drm/ingenic/ingenic-drm.h     | 42 +++++++++++
>> 2 files changed, 122 insertions(+), 5 deletions(-)
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> index f73522bdacaa..e2df4b085905 100644
>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> @@ -6,6 +6,7 @@
>> #include "ingenic-drm.h"
>> +#include <linux/bitfield.h>
>> #include <linux/component.h>
>> #include <linux/clk.h>
>> #include <linux/dma-mapping.h>
>> @@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc {
>> 	u32 addr;
>> 	u32 id;
>> 	u32 cmd;
>> +	/* extended hw descriptor for jz4780 */
>> +	u32 offsize;
>> +	u32 pagewidth;
>> +	u32 cpos;
>> +	u32 dessize;
>> } __aligned(16);
>> struct ingenic_dma_hwdescs {
>> @@ -60,9 +66,11 @@ struct jz_soc_info {
>> 	bool needs_dev_clk;
>> 	bool has_osd;
>> 	bool map_noncoherent;
>> +	bool use_extended_hwdesc;
>> 	unsigned int max_width, max_height;
>> 	const u32 *formats_f0, *formats_f1;
>> 	unsigned int num_formats_f0, num_formats_f1;
>> +	unsigned int max_reg;
>> };
>> struct ingenic_drm_private_state {
>> @@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
>> 	}
>> }
>> -static const struct regmap_config ingenic_drm_regmap_config = {
>> +static struct regmap_config ingenic_drm_regmap_config = {
>> 	.reg_bits = 32,
>> 	.val_bits = 32,
>> 	.reg_stride = 4,
>> -	.max_register = JZ_REG_LCD_SIZE1,
>> 	.writeable_reg = ingenic_drm_writeable_reg,
>> };
>> @@ -663,6 +670,37 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>> 		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
>> 		hwdesc->next = dma_hwdesc_addr(priv, next_id);
>> +		if (priv->soc_info->use_extended_hwdesc) {
>> +			hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE;
>> +
>> +			/* Extended 8-byte descriptor */
>> +			hwdesc->cpos = 0;
>> +			hwdesc->offsize = 0;
>> +			hwdesc->pagewidth = 0;
>> +
>> +			switch (newstate->fb->format->format) {
>> +			case DRM_FORMAT_XRGB1555:
>> +				hwdesc->cpos |= JZ_LCD_CPOS_RGB555;
>> +				fallthrough;
>> +			case DRM_FORMAT_RGB565:
>> +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16;
>> +				break;
>> +			case DRM_FORMAT_XRGB8888:
>> +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
>> +				break;
>> +			}
>> +			hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
>> +					    (JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 <<
>> +					     JZ_LCD_CPOS_COEFFICIENT_OFFSET);
>> +
>> +			hwdesc->dessize =
>> +				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
>> +				FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK <<
>> +					   JZ_LCD_DESSIZE_HEIGHT_OFFSET, height - 1) |
>> +				FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK <<
>> +					   JZ_LCD_DESSIZE_WIDTH_OFFSET, width - 1);
>> +		}
>> +
>> 		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> 			fourcc = newstate->fb->format->format;
>> @@ -694,6 +732,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>> 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>> 	}
>> +	/* set use of the 8-word descriptor and OSD foreground usage. */
>> +	if (priv->soc_info->use_extended_hwdesc)
>> +		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
>> +
>> 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>> 		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
>> 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>> @@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	struct drm_encoder *encoder;
>> 	struct ingenic_drm_bridge *ib;
>> 	struct drm_device *drm;
>> +	struct regmap_config regmap_config;
>> 	void __iomem *base;
>> 	long parent_rate;
>> 	unsigned int i, clone_mask = 0;
>> @@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 		return PTR_ERR(base);
>> 	}
>> +	regmap_config = ingenic_drm_regmap_config;
>> +	regmap_config.max_register = soc_info->max_reg;
>> 	priv->map = devm_regmap_init_mmio(dev, base,
>> -					  &ingenic_drm_regmap_config);
>> +					  &regmap_config);
> 
> Could you split the code that makes .max_reg configurable per-SoC into its own patch?

Yes.

> 
>> 	if (IS_ERR(priv->map)) {
>> 		dev_err(dev, "Failed to create regmap\n");
>> 		return PTR_ERR(priv->map);
>> @@ -1274,7 +1319,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	/* Enable OSD if available */
>> 	if (soc_info->has_osd)
>> -		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
> 
> Why?

If I remember we should not assume that all others bits in JZ_REG_LCD_OSDC
can be safely overwritten by 0, although their reset state is 0 as well.

These are several alpha-blending bits and interrupt masks in the same register.
Apparently only in jz4780.

> 
>> 	mutex_init(&priv->clk_mutex);
>> 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
>> @@ -1444,6 +1489,7 @@ static const struct jz_soc_info jz4740_soc_info = {
>> 	.formats_f1 = jz4740_formats,
>> 	.num_formats_f1 = ARRAY_SIZE(jz4740_formats),
>> 	/* JZ4740 has only one plane */
>> +	.max_reg = JZ_REG_LCD_SIZE1,
>> };
>> static const struct jz_soc_info jz4725b_soc_info = {
>> @@ -1456,6 +1502,7 @@ static const struct jz_soc_info jz4725b_soc_info = {
>> 	.num_formats_f1 = ARRAY_SIZE(jz4725b_formats_f1),
>> 	.formats_f0 = jz4725b_formats_f0,
>> 	.num_formats_f0 = ARRAY_SIZE(jz4725b_formats_f0),
>> +	.max_reg = JZ_REG_LCD_SIZE1,
>> };
>> static const struct jz_soc_info jz4770_soc_info = {
>> @@ -1468,12 +1515,28 @@ static const struct jz_soc_info jz4770_soc_info = {
>> 	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
>> 	.formats_f0 = jz4770_formats_f0,
>> 	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
>> +	.max_reg = JZ_REG_LCD_SIZE1,
>> +};
>> +
>> +static const struct jz_soc_info jz4780_soc_info = {
>> +	.needs_dev_clk = true,
>> +	.has_osd = true,
>> +	.use_extended_hwdesc = true,
>> +	.max_width = 4096,
>> +	.max_height = 2048,
>> +	/* REVISIT: do we support formats different from jz4770? */
>> +	.formats_f1 = jz4770_formats_f1,
>> +	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
>> +	.formats_f0 = jz4770_formats_f0,
>> +	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
>> +	.max_reg = JZ_REG_LCD_PCFG,
>> };
>> static const struct of_device_id ingenic_drm_of_match[] = {
>> 	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
>> 	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info },
>> 	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
>> +	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
>> 	{ /* sentinel */ },
>> };
>> MODULE_DEVICE_TABLE(of, ingenic_drm_of_match);
>> @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
>> {
>> 	int err;
>> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
>> +		err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
>> +		if (err)
>> +			return err;
>> +	}
> 
> I don't see why you need to register the ingenic-dw-hdmi driver here. Just register it in the ingenic-dw-hdmi driver.

Ok, I never though about this (as the code was not from me). We apparently just followed the IPU code pattern (learning by example).

It indeed looks not necessary and would also avoid the ingenic_dw_hdmi_driver_ptr dependency.

But: what is ingenic_ipu_driver_ptr then good for?

If we can get rid of this as well, we can drop patch 1/10 ("drm/ingenic: Fix drm_init error path if IPU was registered") completely.

> 
> Cheers,
> -Paul
> 

BR,
Nikolaus

Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,

> Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
>>> @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
>>> {
>>> 	int err;
>>> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
>>> +		err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
>>> +		if (err)
>>> +			return err;
>>> +	}
>> 
>> I don't see why you need to register the ingenic-dw-hdmi driver here. Just register it in the ingenic-dw-hdmi driver.
> 
> Ok, I never though about this (as the code was not from me). We apparently just followed the IPU code pattern (learning by example).
> 
> It indeed looks not necessary and would also avoid the ingenic_dw_hdmi_driver_ptr dependency.
> 
> But: what is ingenic_ipu_driver_ptr then good for?
> 
> If we can get rid of this as well, we can drop patch 1/10 ("drm/ingenic: Fix drm_init error path if IPU was registered") completely.

A quick test shows that it *is* required. At least if I configure everything as modules.
But like you I can't explain why.

Well, just a very rough idea (may be wrong): the bridge chain is not like an i2c bus and
clients are not automatically loaded/probed if linked in the device tree. Therefore the
consumer (ingenic_drm_drv) must register the "clients" like IPU and HDMI.

BR,
Nikolaus

Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,


> Am 28.09.2021 um 14:06 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Paul,
> 
>> Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>> 
>>>> @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
>>>> {
>>>> 	int err;
>>>> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
>>>> +		err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
>>>> +		if (err)
>>>> +			return err;
>>>> +	}
>>> 
>>> I don't see why you need to register the ingenic-dw-hdmi driver here. Just register it in the ingenic-dw-hdmi driver.
>> 
>> Ok, I never though about this (as the code was not from me). We apparently just followed the IPU code pattern (learning by example).
>> 
>> It indeed looks not necessary and would also avoid the ingenic_dw_hdmi_driver_ptr dependency.
>> 
>> But: what is ingenic_ipu_driver_ptr then good for?
>> 
>> If we can get rid of this as well, we can drop patch 1/10 ("drm/ingenic: Fix drm_init error path if IPU was registered") completely.
> 
> A quick test shows that it *is* required. At least if I configure everything as modules.
> But like you I can't explain why.
> 
> Well, just a very rough idea (may be wrong): the bridge chain is not like an i2c bus and
> clients are not automatically loaded/probed if linked in the device tree. Therefore the
> consumer (ingenic_drm_drv) must register the "clients" like IPU and HDMI.

Any suggestion how to proceed here for v5?

BR,
Nikolaus

Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

Hi,

Le mar., sept. 28 2021 at 14:06:03 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller 
>> <hns@goldelico.com>:
>> 
>>>>  @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
>>>>  {
>>>>  	int err;
>>>>  +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
>>>>  +		err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
>>>>  +		if (err)
>>>>  +			return err;
>>>>  +	}
>>> 
>>>  I don't see why you need to register the ingenic-dw-hdmi driver 
>>> here. Just register it in the ingenic-dw-hdmi driver.
>> 
>>  Ok, I never though about this (as the code was not from me). We 
>> apparently just followed the IPU code pattern (learning by example).
>> 
>>  It indeed looks not necessary and would also avoid the 
>> ingenic_dw_hdmi_driver_ptr dependency.
>> 
>>  But: what is ingenic_ipu_driver_ptr then good for?
>> 

It's done this way because ingenic-drm-drv.c and ingenic-ipu.c are both 
compiled within the same module ingenic-drm.

I'm not sure this is still required, maybe ingenic-ipu.c can be its own 
module now.

>> 
>>  If we can get rid of this as well, we can drop patch 1/10 
>> ("drm/ingenic: Fix drm_init error path if IPU was registered") 
>> completely.
> 
> A quick test shows that it *is* required. At least if I configure 
> everything as modules.
> But like you I can't explain why.

Well, a quick test here shows that it is not required, at least when 
configuring with everything built-in.

-Paul

> Well, just a very rough idea (may be wrong): the bridge chain is not 
> like an i2c bus and
> clients are not automatically loaded/probed if linked in the device 
> tree. Therefore the
> consumer (ingenic_drm_drv) must register the "clients" like IPU and 
> HDMI.
> 
> BR,
> Nikolaus
> 

Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,

> Am 29.09.2021 um 16:30 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi,
> 
> Le mar., sept. 28 2021 at 14:06:03 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>>> Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>>>> @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
>>>>> {
>>>>> 	int err;
>>>>> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
>>>>> +		err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
>>>>> +		if (err)
>>>>> +			return err;
>>>>> +	}
>>>> I don't see why you need to register the ingenic-dw-hdmi driver here. Just register it in the ingenic-dw-hdmi driver.
>>> Ok, I never though about this (as the code was not from me). We apparently just followed the IPU code pattern (learning by example).
>>> It indeed looks not necessary and would also avoid the ingenic_dw_hdmi_driver_ptr dependency.
>>> But: what is ingenic_ipu_driver_ptr then good for?
> 
> It's done this way because ingenic-drm-drv.c and ingenic-ipu.c are both compiled within the same module ingenic-drm.

Ah, I see. Hadn't checked this.

> I'm not sure this is still required, maybe ingenic-ipu.c can be its own module now.

What I have seen is that it has its own compatible record. So there could be load-on-demand by DTS.

> 
>>> If we can get rid of this as well, we can drop patch 1/10 ("drm/ingenic: Fix drm_init error path if IPU was registered") completely.
>> A quick test shows that it *is* required. At least if I configure everything as modules.
>> But like you I can't explain why.
> 
> Well, a quick test here shows that it is not required, at least when configuring with everything built-in.

IMHO the hdmi driver (module) should be loaded on demand. Not everyone wants to have it.

Well, that is the problem that needs to be solved...

BR and thanks,
Nikolaus

[PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[H. Nikolaus Schaller]

From: Sam Ravnborg <sam@ravnborg.org>

Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
Based on .txt binding from Zubair Lutfullah Kakakhel

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
---
 .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
new file mode 100644
index 000000000000..5e60cdac4f63
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bindings for Ingenic JZ4780 HDMI Transmitter
+
+maintainers:
+  - H. Nikolaus Schaller <hns@goldelico.com>
+
+description: |
+  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
+  TX controller IP with accompanying PHY IP.
+
+allOf:
+  - $ref: panel/panel-common.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: ingenic,jz4780-dw-hdmi
+
+  reg:
+    maxItems: 1
+    description: the address & size of the LCD controller registers
+
+  reg-io-width:
+    const: 4
+
+  interrupts:
+    maxItems: 1
+    description: Specifies the interrupt provided by parent
+
+  clocks:
+    maxItems: 2
+    description: Clock specifiers for isrf and iahb clocks
+
+  clock-names:
+    items:
+      - const: isfr
+      - const: iahb
+
+  hdmi-regulator: true
+    description: Optional regulator to provide +5V at the connector
+  ddc-i2c-bus: true
+    description: An I2C interface if the internal DDC I2C driver is not to be used
+  ports: true
+
+required:
+    - compatible
+    - clocks
+    - clock-names
+    - ports
+    - reg-io-width
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/jz4780-cgu.h>
+
+    hdmi: hdmi@10180000 {
+        compatible = "ingenic,jz4780-dw-hdmi";
+        reg = <0x10180000 0x8000>;
+        reg-io-width = <4>;
+        ddc-i2c-bus = <&i2c4>;
+        interrupt-parent = <&intc>;
+        interrupts = <3>;
+        clocks = <&cgu JZ4780_CLK_HDMI>, <&cgu JZ4780_CLK_AHB0>;
+        clock-names = "isfr", "iahb";
+
+        ports {
+            hdmi_in: port {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                hdmi_in_lcd: endpoint@0 {
+                    reg = <0>;
+                    remote-endpoint = <&jz4780_out_hdmi>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.31.1

[PATCH v4 03/10] dt-bindings: display: Add ingenic, jz4780-dw-hdmi DT Schema

[H. Nikolaus Schaller]

From: Sam Ravnborg <sam@ravnborg.org>

Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
Based on .txt binding from Zubair Lutfullah Kakakhel

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
---
 .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
new file mode 100644
index 000000000000..5e60cdac4f63
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bindings for Ingenic JZ4780 HDMI Transmitter
+
+maintainers:
+  - H. Nikolaus Schaller <hns@goldelico.com>
+
+description: |
+  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
+  TX controller IP with accompanying PHY IP.
+
+allOf:
+  - $ref: panel/panel-common.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: ingenic,jz4780-dw-hdmi
+
+  reg:
+    maxItems: 1
+    description: the address & size of the LCD controller registers
+
+  reg-io-width:
+    const: 4
+
+  interrupts:
+    maxItems: 1
+    description: Specifies the interrupt provided by parent
+
+  clocks:
+    maxItems: 2
+    description: Clock specifiers for isrf and iahb clocks
+
+  clock-names:
+    items:
+      - const: isfr
+      - const: iahb
+
+  hdmi-regulator: true
+    description: Optional regulator to provide +5V at the connector
+  ddc-i2c-bus: true
+    description: An I2C interface if the internal DDC I2C driver is not to be used
+  ports: true
+
+required:
+    - compatible
+    - clocks
+    - clock-names
+    - ports
+    - reg-io-width
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/jz4780-cgu.h>
+
+    hdmi: hdmi@10180000 {
+        compatible = "ingenic,jz4780-dw-hdmi";
+        reg = <0x10180000 0x8000>;
+        reg-io-width = <4>;
+        ddc-i2c-bus = <&i2c4>;
+        interrupt-parent = <&intc>;
+        interrupts = <3>;
+        clocks = <&cgu JZ4780_CLK_HDMI>, <&cgu JZ4780_CLK_AHB0>;
+        clock-names = "isfr", "iahb";
+
+        ports {
+            hdmi_in: port {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                hdmi_in_lcd: endpoint@0 {
+                    reg = <0>;
+                    remote-endpoint = <&jz4780_out_hdmi>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.31.1

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[maxime]

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

Hi,

On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote:
> From: Sam Ravnborg <sam@ravnborg.org>
> 
> Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
> Based on .txt binding from Zubair Lutfullah Kakakhel
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
> new file mode 100644
> index 000000000000..5e60cdac4f63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bindings for Ingenic JZ4780 HDMI Transmitter
> +
> +maintainers:
> +  - H. Nikolaus Schaller <hns@goldelico.com>
> +
> +description: |
> +  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
> +  TX controller IP with accompanying PHY IP.
> +
> +allOf:
> +  - $ref: panel/panel-common.yaml#

Is it a panel though?

> +properties:
> +  compatible:
> +    items:
> +      - const: ingenic,jz4780-dw-hdmi

This can just be a const, there's no need for the items

> +
> +  reg:
> +    maxItems: 1
> +    description: the address & size of the LCD controller registers

There's no need for that description, it's obvious enough

> +  reg-io-width:
> +    const: 4

If it's fixed, why do you need it in the first place?

> +  interrupts:
> +    maxItems: 1
> +    description: Specifies the interrupt provided by parent

There's no need for that description, it's obvious enough

> +  clocks:
> +    maxItems: 2
> +    description: Clock specifiers for isrf and iahb clocks

This can be defined as

clocks:
  items:
    - description: isrf
    - description: iahb

A better description about what these clocks are would be nice as well

> +  clock-names:
> +    items:
> +      - const: isfr

Is it isfr or isrf?

> +      - const: iahb
> +
> +  hdmi-regulator: true
> +    description: Optional regulator to provide +5V at the connector

regulators need to be suffixed by -supply

You also can just provide the description, you don't need the true there

> +  ddc-i2c-bus: true

ditto

> +    description: An I2C interface if the internal DDC I2C driver is not to be used
> +  ports: true

If there's a single port, you don't need ports

You should also include /schemas/graph.yaml#/$defs/port-base

Maxime


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

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[maxime]

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

Hi,

On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote:
> From: Sam Ravnborg <sam@ravnborg.org>
> 
> Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
> Based on .txt binding from Zubair Lutfullah Kakakhel
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
> new file mode 100644
> index 000000000000..5e60cdac4f63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bindings for Ingenic JZ4780 HDMI Transmitter
> +
> +maintainers:
> +  - H. Nikolaus Schaller <hns@goldelico.com>
> +
> +description: |
> +  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
> +  TX controller IP with accompanying PHY IP.
> +
> +allOf:
> +  - $ref: panel/panel-common.yaml#

Is it a panel though?

> +properties:
> +  compatible:
> +    items:
> +      - const: ingenic,jz4780-dw-hdmi

This can just be a const, there's no need for the items

> +
> +  reg:
> +    maxItems: 1
> +    description: the address & size of the LCD controller registers

There's no need for that description, it's obvious enough

> +  reg-io-width:
> +    const: 4

If it's fixed, why do you need it in the first place?

> +  interrupts:
> +    maxItems: 1
> +    description: Specifies the interrupt provided by parent

There's no need for that description, it's obvious enough

> +  clocks:
> +    maxItems: 2
> +    description: Clock specifiers for isrf and iahb clocks

This can be defined as

clocks:
  items:
    - description: isrf
    - description: iahb

A better description about what these clocks are would be nice as well

> +  clock-names:
> +    items:
> +      - const: isfr

Is it isfr or isrf?

> +      - const: iahb
> +
> +  hdmi-regulator: true
> +    description: Optional regulator to provide +5V at the connector

regulators need to be suffixed by -supply

You also can just provide the description, you don't need the true there

> +  ddc-i2c-bus: true

ditto

> +    description: An I2C interface if the internal DDC I2C driver is not to be used
> +  ports: true

If there's a single port, you don't need ports

You should also include /schemas/graph.yaml#/$defs/port-base

Maxime


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

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[H. Nikolaus Schaller]

Hi,

> Am 27.09.2021 um 19:07 schrieb maxime@cerno.tech:
> 
> Hi,
> 
> On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote:
>> From: Sam Ravnborg <sam@ravnborg.org>
>> 
>> Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
>> Based on .txt binding from Zubair Lutfullah Kakakhel
>> 
>> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> ---
>> .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
>> 1 file changed, 85 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
>> new file mode 100644
>> index 000000000000..5e60cdac4f63
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
>> @@ -0,0 +1,85 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Bindings for Ingenic JZ4780 HDMI Transmitter
>> +
>> +maintainers:
>> +  - H. Nikolaus Schaller <hns@goldelico.com>
>> +
>> +description: |
>> +  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
>> +  TX controller IP with accompanying PHY IP.
>> +
>> +allOf:
>> +  - $ref: panel/panel-common.yaml#
> 
> Is it a panel though?

Good question. 

Appears to have to be changed to

  - $ref: bridge/synopsys,dw-hdmi.yaml#

> 
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: ingenic,jz4780-dw-hdmi
> 
> This can just be a const, there's no need for the items

Maybe starting with an enum is better if more compatible strings are to be added.

> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: the address & size of the LCD controller registers
> 
> There's no need for that description, it's obvious enough

Indeed.

> 
>> +  reg-io-width:
>> +    const: 4
> 
> If it's fixed, why do you need it in the first place?

There is a fixed default of 1 if not specified.

> 
>> +  interrupts:
>> +    maxItems: 1
>> +    description: Specifies the interrupt provided by parent
> 
> There's no need for that description, it's obvious enough

Indeed.

> 
>> +  clocks:
>> +    maxItems: 2
>> +    description: Clock specifiers for isrf and iahb clocks
> 
> This can be defined as
> 
> clocks:
>  items:
>    - description: isrf
>    - description: iahb
> 
> A better description about what these clocks are would be nice as well

Generally I see that this all is nowadays not independent of

Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml

where there is already a description.

On the other hand every SoC specialization runs its own copy. e.g.

Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml
Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam

> 
>> +  clock-names:
>> +    items:
>> +      - const: isfr
> 
> Is it isfr or isrf?

isfr. Seems to be a typo in the description. See bridge/synopsys,dw-hdmi.yaml#

One question to the yaml specialists:

since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we have to repeat?
Or can we reduce to just the changes?

[I am still not familiar enough with the yaml stuff to understand if it has sort
of inheritance like device tree include files, so that you just have to change
relevant properties]

> 
>> +      - const: iahb

would it make sense to add additionalItems: false here?

In the jz4780 case there are just two clocks while other specializations
use more and synopsys,dw-hdmi.yaml# defines additionalItems: true.

>> +
>> +  hdmi-regulator: true
>> +    description: Optional regulator to provide +5V at the connector
> 
> regulators need to be suffixed by -supply

My omission...

And, it should be "hdmi-5v-supply" to match driver and device tree.

> 
> You also can just provide the description, you don't need the true there
> 
>> +  ddc-i2c-bus: true
> 
> ditto

Ok

> 
>> +    description: An I2C interface if the internal DDC I2C driver is not to be used
>> +  ports: true
> 
> If there's a single port, you don't need ports

There can be two ports - one for input from LCDC and one
for output (HDMI connector). But explicitly defining an output
port is optional to some extent (depending on driver structure).

> 
> You should also include /schemas/graph.yaml#/$defs/port-base

Ok.

BR and thanks,
Nikolaus

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic, jz4780-dw-hdmi DT Schema

[H. Nikolaus Schaller]

Hi,

> Am 27.09.2021 um 19:07 schrieb maxime@cerno.tech:
> 
> Hi,
> 
> On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote:
>> From: Sam Ravnborg <sam@ravnborg.org>
>> 
>> Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
>> Based on .txt binding from Zubair Lutfullah Kakakhel
>> 
>> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> ---
>> .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
>> 1 file changed, 85 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
>> new file mode 100644
>> index 000000000000..5e60cdac4f63
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
>> @@ -0,0 +1,85 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Bindings for Ingenic JZ4780 HDMI Transmitter
>> +
>> +maintainers:
>> +  - H. Nikolaus Schaller <hns@goldelico.com>
>> +
>> +description: |
>> +  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
>> +  TX controller IP with accompanying PHY IP.
>> +
>> +allOf:
>> +  - $ref: panel/panel-common.yaml#
> 
> Is it a panel though?

Good question. 

Appears to have to be changed to

  - $ref: bridge/synopsys,dw-hdmi.yaml#

> 
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: ingenic,jz4780-dw-hdmi
> 
> This can just be a const, there's no need for the items

Maybe starting with an enum is better if more compatible strings are to be added.

> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: the address & size of the LCD controller registers
> 
> There's no need for that description, it's obvious enough

Indeed.

> 
>> +  reg-io-width:
>> +    const: 4
> 
> If it's fixed, why do you need it in the first place?

There is a fixed default of 1 if not specified.

> 
>> +  interrupts:
>> +    maxItems: 1
>> +    description: Specifies the interrupt provided by parent
> 
> There's no need for that description, it's obvious enough

Indeed.

> 
>> +  clocks:
>> +    maxItems: 2
>> +    description: Clock specifiers for isrf and iahb clocks
> 
> This can be defined as
> 
> clocks:
>  items:
>    - description: isrf
>    - description: iahb
> 
> A better description about what these clocks are would be nice as well

Generally I see that this all is nowadays not independent of

Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml

where there is already a description.

On the other hand every SoC specialization runs its own copy. e.g.

Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml
Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam

> 
>> +  clock-names:
>> +    items:
>> +      - const: isfr
> 
> Is it isfr or isrf?

isfr. Seems to be a typo in the description. See bridge/synopsys,dw-hdmi.yaml#

One question to the yaml specialists:

since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we have to repeat?
Or can we reduce to just the changes?

[I am still not familiar enough with the yaml stuff to understand if it has sort
of inheritance like device tree include files, so that you just have to change
relevant properties]

> 
>> +      - const: iahb

would it make sense to add additionalItems: false here?

In the jz4780 case there are just two clocks while other specializations
use more and synopsys,dw-hdmi.yaml# defines additionalItems: true.

>> +
>> +  hdmi-regulator: true
>> +    description: Optional regulator to provide +5V at the connector
> 
> regulators need to be suffixed by -supply

My omission...

And, it should be "hdmi-5v-supply" to match driver and device tree.

> 
> You also can just provide the description, you don't need the true there
> 
>> +  ddc-i2c-bus: true
> 
> ditto

Ok

> 
>> +    description: An I2C interface if the internal DDC I2C driver is not to be used
>> +  ports: true
> 
> If there's a single port, you don't need ports

There can be two ports - one for input from LCDC and one
for output (HDMI connector). But explicitly defining an output
port is optional to some extent (depending on driver structure).

> 
> You should also include /schemas/graph.yaml#/$defs/port-base

Ok.

BR and thanks,
Nikolaus

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[Maxime Ripard]

On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote:
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - const: ingenic,jz4780-dw-hdmi
> > 
> > This can just be a const, there's no need for the items
> 
> Maybe starting with an enum is better if more compatible strings are to be added.

it's still fairly easy to change if needed, there's no need to confuse
anyone.

> > 
> >> +  reg-io-width:
> >> +    const: 4
> > 
> > If it's fixed, why do you need it in the first place?
> 
> There is a fixed default of 1 if not specified.

My point was more about why do you need to have that property at all?
Can't you just drop it and assume that the register width is 32 bits if
it's all you will ever run on?

> >> +  clocks:
> >> +    maxItems: 2
> >> +    description: Clock specifiers for isrf and iahb clocks
> > 
> > This can be defined as
> > 
> > clocks:
> >  items:
> >    - description: isrf
> >    - description: iahb
> > 
> > A better description about what these clocks are would be nice as well
> 
> Generally I see that this all is nowadays not independent of
> 
> Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml
> 
> where there is already a description.

Ok, good then

> On the other hand every SoC specialization runs its own copy. e.g.
> 
> Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml
> Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam
> 
> > 
> >> +  clock-names:
> >> +    items:
> >> +      - const: isfr
> > 
> > Is it isfr or isrf?
> 
> isfr. Seems to be a typo in the description. See
> bridge/synopsys,dw-hdmi.yaml#
> 
> One question to the yaml specialists:
> 
> since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we
> have to repeat? Or can we reduce to just the changes?

If you add the ref you mentionned above, you don't have to repeat
yourself indeed. You can just put clock-names: true

> [I am still not familiar enough with the yaml stuff to understand if
> it has sort of inheritance like device tree include files, so that you
> just have to change relevant properties]

Kind of, but not entirely. schemas are all applied separately, unlike DT
includes that will just expand to one big DT. In practice, it means that
your device must validate against all the schemas, not just the sum of
them.

For example, if you have a generic schema that has:

properties:
  compatible:
    const: vendor,my-generic-compatible


and your schema that extends the generic binding, with a ref to the
generic one that has:

properties:
  compatible:
    items:
      - const: other-vendor,my-device-compatible
      - const: vendor,my-generic-compatible


It will still fail since the generic schema expects only a single
compatible, whereas your device would have two.

> > 
> >> +      - const: iahb
> 
> would it make sense to add additionalItems: false here?
> 
> In the jz4780 case there are just two clocks while other specializations
> use more and synopsys,dw-hdmi.yaml# defines additionalItems: true.

If you want to refine the generic one, and it's all the clocks you ever
expect then there's no need for additionalItems

> > 
> >> +    description: An I2C interface if the internal DDC I2C driver is not to be used
> >> +  ports: true
> > 
> > If there's a single port, you don't need ports
> 
> There can be two ports - one for input from LCDC and one
> for output (HDMI connector). But explicitly defining an output
> port is optional to some extent (depending on driver structure).

This needs to be defined then (and port@0 made mandatory)

Maxime

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[Maxime Ripard]

On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote:
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - const: ingenic,jz4780-dw-hdmi
> > 
> > This can just be a const, there's no need for the items
> 
> Maybe starting with an enum is better if more compatible strings are to be added.

it's still fairly easy to change if needed, there's no need to confuse
anyone.

> > 
> >> +  reg-io-width:
> >> +    const: 4
> > 
> > If it's fixed, why do you need it in the first place?
> 
> There is a fixed default of 1 if not specified.

My point was more about why do you need to have that property at all?
Can't you just drop it and assume that the register width is 32 bits if
it's all you will ever run on?

> >> +  clocks:
> >> +    maxItems: 2
> >> +    description: Clock specifiers for isrf and iahb clocks
> > 
> > This can be defined as
> > 
> > clocks:
> >  items:
> >    - description: isrf
> >    - description: iahb
> > 
> > A better description about what these clocks are would be nice as well
> 
> Generally I see that this all is nowadays not independent of
> 
> Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml
> 
> where there is already a description.

Ok, good then

> On the other hand every SoC specialization runs its own copy. e.g.
> 
> Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml
> Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam
> 
> > 
> >> +  clock-names:
> >> +    items:
> >> +      - const: isfr
> > 
> > Is it isfr or isrf?
> 
> isfr. Seems to be a typo in the description. See
> bridge/synopsys,dw-hdmi.yaml#
> 
> One question to the yaml specialists:
> 
> since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we
> have to repeat? Or can we reduce to just the changes?

If you add the ref you mentionned above, you don't have to repeat
yourself indeed. You can just put clock-names: true

> [I am still not familiar enough with the yaml stuff to understand if
> it has sort of inheritance like device tree include files, so that you
> just have to change relevant properties]

Kind of, but not entirely. schemas are all applied separately, unlike DT
includes that will just expand to one big DT. In practice, it means that
your device must validate against all the schemas, not just the sum of
them.

For example, if you have a generic schema that has:

properties:
  compatible:
    const: vendor,my-generic-compatible


and your schema that extends the generic binding, with a ref to the
generic one that has:

properties:
  compatible:
    items:
      - const: other-vendor,my-device-compatible
      - const: vendor,my-generic-compatible


It will still fail since the generic schema expects only a single
compatible, whereas your device would have two.

> > 
> >> +      - const: iahb
> 
> would it make sense to add additionalItems: false here?
> 
> In the jz4780 case there are just two clocks while other specializations
> use more and synopsys,dw-hdmi.yaml# defines additionalItems: true.

If you want to refine the generic one, and it's all the clocks you ever
expect then there's no need for additionalItems

> > 
> >> +    description: An I2C interface if the internal DDC I2C driver is not to be used
> >> +  ports: true
> > 
> > If there's a single port, you don't need ports
> 
> There can be two ports - one for input from LCDC and one
> for output (HDMI connector). But explicitly defining an output
> port is optional to some extent (depending on driver structure).

This needs to be defined then (and port@0 made mandatory)

Maxime

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[H. Nikolaus Schaller]

> Am 28.09.2021 um 11:18 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>>> +      - const: ingenic,jz4780-dw-hdmi
>>> 
>>> This can just be a const, there's no need for the items
>> 
>> Maybe starting with an enum is better if more compatible strings are to be added.
> 
> it's still fairly easy to change if needed, there's no need to confuse
> anyone.
> 
>>> 
>>>> +  reg-io-width:
>>>> +    const: 4
>>> 
>>> If it's fixed, why do you need it in the first place?
>> 
>> There is a fixed default of 1 if not specified.
> 
> My point was more about why do you need to have that property at all?
> Can't you just drop it and assume that the register width is 32 bits if
> it's all you will ever run on?

No, please see bridge/synopsys,dw-hdmi.yaml where it is derived from:

  reg-io-width:
    description:
      Width (in bytes) of the registers specified by the reg property.
    allOf:
      - $ref: /schemas/types.yaml#/definitions/uint32
      - enum: [1, 4]
    default: 1

Other bindings define it explicitly to be 4, e.g.

Documentation//devicetree/bindings/display/intel,keembay-msscam.yaml:  reg-io-width:
Documentation//devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml:  reg-io-width:

Therefore I'd assume that a regmap is not properly set up
if we don't require the DTS to include it with const: 4.

> 
>>>> +  clocks:
>>>> +    maxItems: 2
>>>> +    description: Clock specifiers for isrf and iahb clocks
>>> 
>>> This can be defined as
>>> 
>>> clocks:
>>> items:
>>>   - description: isrf
>>>   - description: iahb
>>> 
>>> A better description about what these clocks are would be nice as well
>> 
>> Generally I see that this all is nowadays not independent of
>> 
>> Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml
>> 
>> where there is already a description.
> 
> Ok, good then
> 
>> On the other hand every SoC specialization runs its own copy. e.g.
>> 
>> Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml
>> Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam
>> 
>>> 
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: isfr
>>> 
>>> Is it isfr or isrf?
>> 
>> isfr. Seems to be a typo in the description. See
>> bridge/synopsys,dw-hdmi.yaml#
>> 
>> One question to the yaml specialists:
>> 
>> since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we
>> have to repeat? Or can we reduce to just the changes?
> 
> If you add the ref you mentionned above, you don't have to repeat

Nice. It defines:

  clocks:
    minItems: 2
    maxItems: 5
    items:
      - description: The bus clock for either AHB and APB
      - description: The internal register configuration clock
    additionalItems: true

> yourself indeed. You can just put clock-names: true

Or should we do

  clocks:
    maxItems: 2
    additionalItems: false

> 
>> [I am still not familiar enough with the yaml stuff to understand if
>> it has sort of inheritance like device tree include files, so that you
>> just have to change relevant properties]
> 
> Kind of, but not entirely. schemas are all applied separately, unlike DT
> includes that will just expand to one big DT. In practice, it means that
> your device must validate against all the schemas, not just the sum of
> them.
> 
> For example, if you have a generic schema that has:
> 
> properties:
>  compatible:
>    const: vendor,my-generic-compatible
> 
> 
> and your schema that extends the generic binding, with a ref to the
> generic one that has:
> 
> properties:
>  compatible:
>    items:
>      - const: other-vendor,my-device-compatible
>      - const: vendor,my-generic-compatible
> 
> 
> It will still fail since the generic schema expects only a single
> compatible, whereas your device would have two.

Ok, I see. it is not a simple "overwrite" rule.

> 
>>> 
>>>> +      - const: iahb
>> 
>> would it make sense to add additionalItems: false here?
>> 
>> In the jz4780 case there are just two clocks while other specializations
>> use more and synopsys,dw-hdmi.yaml# defines additionalItems: true.
> 
> If you want to refine the generic one, and it's all the clocks you ever
> expect then there's no need for additionalItems

Ok.

> 
>>> 
>>>> +    description: An I2C interface if the internal DDC I2C driver is not to be used
>>>> +  ports: true
>>> 
>>> If there's a single port, you don't need ports
>> 
>> There can be two ports - one for input from LCDC and one
>> for output (HDMI connector). But explicitly defining an output
>> port is optional to some extent (depending on driver structure).
> 
> This needs to be defined then (and port@0 made mandatory)

Ok. I'll try to make the best out of it for v5 series. Maybe
it is still not perfect by then, but close...

> 
> Maxime

BR and thanks,
Nikolaus

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic, jz4780-dw-hdmi DT Schema

[H. Nikolaus Schaller]

> Am 28.09.2021 um 11:18 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>>> +      - const: ingenic,jz4780-dw-hdmi
>>> 
>>> This can just be a const, there's no need for the items
>> 
>> Maybe starting with an enum is better if more compatible strings are to be added.
> 
> it's still fairly easy to change if needed, there's no need to confuse
> anyone.
> 
>>> 
>>>> +  reg-io-width:
>>>> +    const: 4
>>> 
>>> If it's fixed, why do you need it in the first place?
>> 
>> There is a fixed default of 1 if not specified.
> 
> My point was more about why do you need to have that property at all?
> Can't you just drop it and assume that the register width is 32 bits if
> it's all you will ever run on?

No, please see bridge/synopsys,dw-hdmi.yaml where it is derived from:

  reg-io-width:
    description:
      Width (in bytes) of the registers specified by the reg property.
    allOf:
      - $ref: /schemas/types.yaml#/definitions/uint32
      - enum: [1, 4]
    default: 1

Other bindings define it explicitly to be 4, e.g.

Documentation//devicetree/bindings/display/intel,keembay-msscam.yaml:  reg-io-width:
Documentation//devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml:  reg-io-width:

Therefore I'd assume that a regmap is not properly set up
if we don't require the DTS to include it with const: 4.

> 
>>>> +  clocks:
>>>> +    maxItems: 2
>>>> +    description: Clock specifiers for isrf and iahb clocks
>>> 
>>> This can be defined as
>>> 
>>> clocks:
>>> items:
>>>   - description: isrf
>>>   - description: iahb
>>> 
>>> A better description about what these clocks are would be nice as well
>> 
>> Generally I see that this all is nowadays not independent of
>> 
>> Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml
>> 
>> where there is already a description.
> 
> Ok, good then
> 
>> On the other hand every SoC specialization runs its own copy. e.g.
>> 
>> Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml
>> Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam
>> 
>>> 
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: isfr
>>> 
>>> Is it isfr or isrf?
>> 
>> isfr. Seems to be a typo in the description. See
>> bridge/synopsys,dw-hdmi.yaml#
>> 
>> One question to the yaml specialists:
>> 
>> since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we
>> have to repeat? Or can we reduce to just the changes?
> 
> If you add the ref you mentionned above, you don't have to repeat

Nice. It defines:

  clocks:
    minItems: 2
    maxItems: 5
    items:
      - description: The bus clock for either AHB and APB
      - description: The internal register configuration clock
    additionalItems: true

> yourself indeed. You can just put clock-names: true

Or should we do

  clocks:
    maxItems: 2
    additionalItems: false

> 
>> [I am still not familiar enough with the yaml stuff to understand if
>> it has sort of inheritance like device tree include files, so that you
>> just have to change relevant properties]
> 
> Kind of, but not entirely. schemas are all applied separately, unlike DT
> includes that will just expand to one big DT. In practice, it means that
> your device must validate against all the schemas, not just the sum of
> them.
> 
> For example, if you have a generic schema that has:
> 
> properties:
>  compatible:
>    const: vendor,my-generic-compatible
> 
> 
> and your schema that extends the generic binding, with a ref to the
> generic one that has:
> 
> properties:
>  compatible:
>    items:
>      - const: other-vendor,my-device-compatible
>      - const: vendor,my-generic-compatible
> 
> 
> It will still fail since the generic schema expects only a single
> compatible, whereas your device would have two.

Ok, I see. it is not a simple "overwrite" rule.

> 
>>> 
>>>> +      - const: iahb
>> 
>> would it make sense to add additionalItems: false here?
>> 
>> In the jz4780 case there are just two clocks while other specializations
>> use more and synopsys,dw-hdmi.yaml# defines additionalItems: true.
> 
> If you want to refine the generic one, and it's all the clocks you ever
> expect then there's no need for additionalItems

Ok.

> 
>>> 
>>>> +    description: An I2C interface if the internal DDC I2C driver is not to be used
>>>> +  ports: true
>>> 
>>> If there's a single port, you don't need ports
>> 
>> There can be two ports - one for input from LCDC and one
>> for output (HDMI connector). But explicitly defining an output
>> port is optional to some extent (depending on driver structure).
> 
> This needs to be defined then (and port@0 made mandatory)

Ok. I'll try to make the best out of it for v5 series. Maybe
it is still not perfect by then, but close...

> 
> Maxime

BR and thanks,
Nikolaus

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[Rob Herring]

On Mon, 27 Sep 2021 18:44:21 +0200, H. Nikolaus Schaller wrote:
> From: Sam Ravnborg <sam@ravnborg.org>
> 
> Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
> Based on .txt binding from Zubair Lutfullah Kakakhel
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml:45:16: [error] syntax error: mapping values are not allowed here (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 120, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: mapping values are not allowed in this context
  in "<unicode string>", line 45, column 16
make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml:  mapping values are not allowed in this context
  in "<unicode string>", line 45, column 16
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
make: *** [Makefile:1441: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1533471

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[Rob Herring]

On Mon, 27 Sep 2021 18:44:21 +0200, H. Nikolaus Schaller wrote:
> From: Sam Ravnborg <sam@ravnborg.org>
> 
> Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
> Based on .txt binding from Zubair Lutfullah Kakakhel
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml:45:16: [error] syntax error: mapping values are not allowed here (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 120, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: mapping values are not allowed in this context
  in "<unicode string>", line 45, column 16
make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml:  mapping values are not allowed in this context
  in "<unicode string>", line 45, column 16
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
make: *** [Makefile:1441: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1533471

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v4 04/10] drm/bridge: synopsis: Add mode_fixup and bridge timings support

[H. Nikolaus Schaller]

From: Paul Boddie <paul@boddie.org.uk>

The platform-specific configuration structure is augmented with
mode_fixup and timings members so that specialisations of the
Synopsys driver can introduce mode flags and bus flags.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 7 +++++++
 include/drm/bridge/dw_hdmi.h              | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index f08d0fded61f..f082e14320e1 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2767,6 +2767,11 @@ static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
 		bridge_state->input_bus_cfg.format,
 		bridge_state->output_bus_cfg.format);
 
+	if (hdmi->plat_data->mode_fixup)
+		if (!hdmi->plat_data->mode_fixup(bridge, &crtc_state->mode,
+						 &crtc_state->adjusted_mode))
+			return -EINVAL;
+
 	return 0;
 }
 
@@ -3416,6 +3421,8 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
 #ifdef CONFIG_OF
 	hdmi->bridge.of_node = pdev->dev.of_node;
 #endif
+	if (plat_data->timings)
+		hdmi->bridge.timings = plat_data->timings;
 
 	memset(&pdevinfo, 0, sizeof(pdevinfo));
 	pdevinfo.parent = dev;
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 2a1f85f9a8a3..743038200044 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -8,6 +8,7 @@
 
 #include <sound/hdmi-codec.h>
 
+struct drm_bridge;
 struct drm_display_info;
 struct drm_display_mode;
 struct drm_encoder;
@@ -142,6 +143,10 @@ struct dw_hdmi_plat_data {
 	enum drm_mode_status (*mode_valid)(struct dw_hdmi *hdmi, void *data,
 					   const struct drm_display_info *info,
 					   const struct drm_display_mode *mode);
+	bool (*mode_fixup)(struct drm_bridge *bridge,
+			   const struct drm_display_mode *mode,
+			   struct drm_display_mode *adjusted_mode);
+	const struct drm_bridge_timings *timings;
 
 	/* Vendor PHY support */
 	const struct dw_hdmi_phy_ops *phy_ops;
-- 
2.31.1

[PATCH v4 05/10] drm/bridge: synopsis: Fix to properly handle HPD

[H. Nikolaus Schaller]

It appears that dw-hdmi plugin detection is not properly
propagated unless we call drm_kms_helper_hotplug_event().

Maybe drm_bridge_hpd_notify should have been setup to
call this.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index f082e14320e1..edea04f80576 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -3018,6 +3018,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 		if (hdmi->bridge.dev) {
 			drm_helper_hpd_irq_event(hdmi->bridge.dev);
 			drm_bridge_hpd_notify(&hdmi->bridge, status);
+
+			drm_kms_helper_hotplug_event(hdmi->bridge.dev);
 		}
 	}
 
-- 
2.31.1

Re: [PATCH v4 05/10] drm/bridge: synopsis: Fix to properly handle HPD

[Maxime Ripard]

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

Hi,

On Mon, Sep 27, 2021 at 06:44:23PM +0200, H. Nikolaus Schaller wrote:
> It appears that dw-hdmi plugin detection is not properly
> propagated unless we call drm_kms_helper_hotplug_event().
> 
> Maybe drm_bridge_hpd_notify should have been setup to
> call this.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index f082e14320e1..edea04f80576 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3018,6 +3018,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>  		if (hdmi->bridge.dev) {
>  			drm_helper_hpd_irq_event(hdmi->bridge.dev);
>  			drm_bridge_hpd_notify(&hdmi->bridge, status);
> +
> +			drm_kms_helper_hotplug_event(hdmi->bridge.dev);

drm_kms_helper_hotplug_event is already called from drm_helper_hpd_irq_event

Maxime

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

Re: [PATCH v4 05/10] drm/bridge: synopsis: Fix to properly handle HPD

[Maxime Ripard]

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

Hi,

On Mon, Sep 27, 2021 at 06:44:23PM +0200, H. Nikolaus Schaller wrote:
> It appears that dw-hdmi plugin detection is not properly
> propagated unless we call drm_kms_helper_hotplug_event().
> 
> Maybe drm_bridge_hpd_notify should have been setup to
> call this.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index f082e14320e1..edea04f80576 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3018,6 +3018,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>  		if (hdmi->bridge.dev) {
>  			drm_helper_hpd_irq_event(hdmi->bridge.dev);
>  			drm_bridge_hpd_notify(&hdmi->bridge, status);
> +
> +			drm_kms_helper_hotplug_event(hdmi->bridge.dev);

drm_kms_helper_hotplug_event is already called from drm_helper_hpd_irq_event

Maxime

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

Re: [PATCH v4 05/10] drm/bridge: synopsis: Fix to properly handle HPD

[H. Nikolaus Schaller]

> Am 27.09.2021 um 19:00 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> Hi,
> 
> On Mon, Sep 27, 2021 at 06:44:23PM +0200, H. Nikolaus Schaller wrote:
>> It appears that dw-hdmi plugin detection is not properly
>> propagated unless we call drm_kms_helper_hotplug_event().
>> 
>> Maybe drm_bridge_hpd_notify should have been setup to
>> call this.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index f082e14320e1..edea04f80576 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -3018,6 +3018,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>> 		if (hdmi->bridge.dev) {
>> 			drm_helper_hpd_irq_event(hdmi->bridge.dev);
>> 			drm_bridge_hpd_notify(&hdmi->bridge, status);
>> +
>> +			drm_kms_helper_hotplug_event(hdmi->bridge.dev);
> 
> drm_kms_helper_hotplug_event is already called from drm_helper_hpd_irq_event

Ah, now I see. It should be called but is not for some unkown
condition (poll disabled? changed = false?).

It may also be a leftover from the attempt to make it work with
the builtin dw-hdmi connector.

Will check for v5.

BR and thanks,
Nikolaus

[PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780

[H. Nikolaus Schaller]

From: Paul Boddie <paul@boddie.org.uk>

A specialisation of the generic Synopsys HDMI driver is employed for JZ4780
HDMI support. This requires a new driver, plus device tree and configuration
modifications.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/ingenic/Kconfig           |   9 ++
 drivers/gpu/drm/ingenic/Makefile          |   1 +
 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 142 ++++++++++++++++++++++
 3 files changed, 152 insertions(+)
 create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c

diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
index 3b57f8be007c..4c7d311fbeff 100644
--- a/drivers/gpu/drm/ingenic/Kconfig
+++ b/drivers/gpu/drm/ingenic/Kconfig
@@ -25,4 +25,13 @@ config DRM_INGENIC_IPU
 
 	  The Image Processing Unit (IPU) will appear as a second primary plane.
 
+config DRM_INGENIC_DW_HDMI
+	bool "Ingenic specific support for Synopsys DW HDMI"
+	depends on MACH_JZ4780
+	select DRM_DW_HDMI
+	help
+	  Choose this option to enable Synopsys DesignWare HDMI based driver.
+	  If you want to enable HDMI on Ingenic JZ4780 based SoC, you should
+	  select this option..
+
 endif
diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile
index d313326bdddb..3db9888a6c04 100644
--- a/drivers/gpu/drm/ingenic/Makefile
+++ b/drivers/gpu/drm/ingenic/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
 ingenic-drm-y = ingenic-drm-drv.o
 ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o
+ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o
diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
new file mode 100644
index 000000000000..dd9c94ae842e
--- /dev/null
+++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
+ *
+ * Derived from dw_hdmi-imx.c with i.MX portions removed.
+ * Probe and remove operations derived from rcar_dw_hdmi.c.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <drm/bridge/dw_hdmi.h>
+#include <drm/drm_of.h>
+#include <drm/drm_print.h>
+
+static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
+	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
+	{ 92500000,  { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 } } },
+	{ 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a } } },
+	{ 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f } } },
+	{ ~0UL,      { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, 0x0000 } } }
+};
+
+static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = {
+	/*pixelclk     bpp8    bpp10   bpp12 */
+	{ 54000000,  { 0x091c, 0x091c, 0x06dc } },
+	{ 58400000,  { 0x091c, 0x06dc, 0x06dc } },
+	{ 72000000,  { 0x06dc, 0x06dc, 0x091c } },
+	{ 74250000,  { 0x06dc, 0x0b5c, 0x091c } },
+	{ 118800000, { 0x091c, 0x091c, 0x06dc } },
+	{ 216000000, { 0x06dc, 0x0b5c, 0x091c } },
+	{ ~0UL,      { 0x0000, 0x0000, 0x0000 } },
+};
+
+/*
+ * Resistance term 133Ohm Cfg
+ * PREEMP config 0.00
+ * TX/CK level 10
+ */
+static const struct dw_hdmi_phy_config ingenic_phy_config[] = {
+	/*pixelclk   symbol   term   vlev */
+	{ 216000000, 0x800d, 0x0005, 0x01ad},
+	{ ~0UL,      0x0000, 0x0000, 0x0000}
+};
+
+static enum drm_mode_status
+ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data,
+			   const struct drm_display_info *info,
+			   const struct drm_display_mode *mode)
+{
+	if (mode->clock < 13500)
+		return MODE_CLOCK_LOW;
+	/* FIXME: Hardware is capable of 270MHz, but setup data is missing. */
+	if (mode->clock > 216000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+static bool
+ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge,
+			   const struct drm_display_mode *mode,
+			   struct drm_display_mode *adjusted_mode)
+{
+	adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
+	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
+
+	return true;
+}
+
+static const struct drm_bridge_timings ingenic_dw_hdmi_timings = {
+	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
+};
+
+static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = {
+	.mpll_cfg   = ingenic_mpll_cfg,
+	.cur_ctr    = ingenic_cur_ctr,
+	.phy_config = ingenic_phy_config,
+	.mode_valid = ingenic_dw_hdmi_mode_valid,
+	.mode_fixup = ingenic_dw_hdmi_mode_fixup,
+	.timings    = &ingenic_dw_hdmi_timings,
+	.output_port	= 1,
+};
+
+static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
+	{ .compatible = "ingenic,jz4780-dw-hdmi" },
+	{ /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
+
+static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
+{
+	struct dw_hdmi *hdmi;
+	struct regulator *regulator;
+	int ret;
+
+	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
+	if (IS_ERR(hdmi))
+		return PTR_ERR(hdmi);
+
+	platform_set_drvdata(pdev, hdmi);
+
+	regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v");
+
+	if (IS_ERR(regulator)) {
+		ret = PTR_ERR(regulator);
+
+		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %s (%d)\n",
+			      "hdmi-5v", ret);
+		return ret;
+	}
+
+	ret = regulator_enable(regulator);
+	if (ret) {
+		DRM_DEV_ERROR(&pdev->dev, "Failed to enable hpd regulator: %d\n",
+			      ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
+{
+	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
+
+	dw_hdmi_remove(hdmi);
+
+	return 0;
+}
+
+static struct platform_driver ingenic_dw_hdmi_driver = {
+	.probe  = ingenic_dw_hdmi_probe,
+	.remove = ingenic_dw_hdmi_remove,
+	.driver = {
+		.name = "dw-hdmi-ingenic",
+		.of_match_table = ingenic_dw_hdmi_dt_ids,
+	},
+};
+
+struct platform_driver *ingenic_dw_hdmi_driver_ptr = &ingenic_dw_hdmi_driver;
-- 
2.31.1

Re: [PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780

[Maxime Ripard]

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

On Mon, Sep 27, 2021 at 06:44:24PM +0200, H. Nikolaus Schaller wrote:
> From: Paul Boddie <paul@boddie.org.uk>
> 
> A specialisation of the generic Synopsys HDMI driver is employed for JZ4780
> HDMI support. This requires a new driver, plus device tree and configuration
> modifications.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/ingenic/Kconfig           |   9 ++
>  drivers/gpu/drm/ingenic/Makefile          |   1 +
>  drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 142 ++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> 
> diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
> index 3b57f8be007c..4c7d311fbeff 100644
> --- a/drivers/gpu/drm/ingenic/Kconfig
> +++ b/drivers/gpu/drm/ingenic/Kconfig
> @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU
>  
>  	  The Image Processing Unit (IPU) will appear as a second primary plane.
>  
> +config DRM_INGENIC_DW_HDMI
> +	bool "Ingenic specific support for Synopsys DW HDMI"
> +	depends on MACH_JZ4780
> +	select DRM_DW_HDMI
> +	help
> +	  Choose this option to enable Synopsys DesignWare HDMI based driver.
> +	  If you want to enable HDMI on Ingenic JZ4780 based SoC, you should
> +	  select this option..
> +
>  endif
> diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile
> index d313326bdddb..3db9888a6c04 100644
> --- a/drivers/gpu/drm/ingenic/Makefile
> +++ b/drivers/gpu/drm/ingenic/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
>  ingenic-drm-y = ingenic-drm-drv.o
>  ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o
> +ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o
> diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> new file mode 100644
> index 000000000000..dd9c94ae842e
> --- /dev/null
> +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
> + * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
> + *
> + * Derived from dw_hdmi-imx.c with i.MX portions removed.
> + * Probe and remove operations derived from rcar_dw_hdmi.c.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/bridge/dw_hdmi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_print.h>
> +
> +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
> +	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
> +	{ 92500000,  { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 } } },
> +	{ 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a } } },
> +	{ 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f } } },
> +	{ ~0UL,      { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, 0x0000 } } }
> +};
> +
> +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = {
> +	/*pixelclk     bpp8    bpp10   bpp12 */
> +	{ 54000000,  { 0x091c, 0x091c, 0x06dc } },
> +	{ 58400000,  { 0x091c, 0x06dc, 0x06dc } },
> +	{ 72000000,  { 0x06dc, 0x06dc, 0x091c } },
> +	{ 74250000,  { 0x06dc, 0x0b5c, 0x091c } },
> +	{ 118800000, { 0x091c, 0x091c, 0x06dc } },
> +	{ 216000000, { 0x06dc, 0x0b5c, 0x091c } },
> +	{ ~0UL,      { 0x0000, 0x0000, 0x0000 } },
> +};
> +
> +/*
> + * Resistance term 133Ohm Cfg
> + * PREEMP config 0.00
> + * TX/CK level 10
> + */
> +static const struct dw_hdmi_phy_config ingenic_phy_config[] = {
> +	/*pixelclk   symbol   term   vlev */
> +	{ 216000000, 0x800d, 0x0005, 0x01ad},
> +	{ ~0UL,      0x0000, 0x0000, 0x0000}
> +};
> +
> +static enum drm_mode_status
> +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data,
> +			   const struct drm_display_info *info,
> +			   const struct drm_display_mode *mode)
> +{
> +	if (mode->clock < 13500)
> +		return MODE_CLOCK_LOW;
> +	/* FIXME: Hardware is capable of 270MHz, but setup data is missing. */
> +	if (mode->clock > 216000)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
> +static bool
> +ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge,
> +			   const struct drm_display_mode *mode,
> +			   struct drm_display_mode *adjusted_mode)
> +{
> +	adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> +	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> +
> +	return true;
> +}
> +
> +static const struct drm_bridge_timings ingenic_dw_hdmi_timings = {
> +	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
> +};
> +
> +static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = {
> +	.mpll_cfg   = ingenic_mpll_cfg,
> +	.cur_ctr    = ingenic_cur_ctr,
> +	.phy_config = ingenic_phy_config,
> +	.mode_valid = ingenic_dw_hdmi_mode_valid,
> +	.mode_fixup = ingenic_dw_hdmi_mode_fixup,
> +	.timings    = &ingenic_dw_hdmi_timings,
> +	.output_port	= 1,
> +};
> +
> +static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
> +	{ .compatible = "ingenic,jz4780-dw-hdmi" },
> +	{ /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
> +
> +static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
> +{
> +	struct dw_hdmi *hdmi;
> +	struct regulator *regulator;
> +	int ret;
> +
> +	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
> +	if (IS_ERR(hdmi))
> +		return PTR_ERR(hdmi);
> +
> +	platform_set_drvdata(pdev, hdmi);
> +
> +	regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v");
> +
> +	if (IS_ERR(regulator)) {
> +		ret = PTR_ERR(regulator);
> +
> +		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %s (%d)\n",
> +			      "hdmi-5v", ret);
> +		return ret;
> +	}

This doesn't match your binding

Maxime

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

Re: [PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780

[Maxime Ripard]

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

On Mon, Sep 27, 2021 at 06:44:24PM +0200, H. Nikolaus Schaller wrote:
> From: Paul Boddie <paul@boddie.org.uk>
> 
> A specialisation of the generic Synopsys HDMI driver is employed for JZ4780
> HDMI support. This requires a new driver, plus device tree and configuration
> modifications.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/ingenic/Kconfig           |   9 ++
>  drivers/gpu/drm/ingenic/Makefile          |   1 +
>  drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 142 ++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> 
> diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
> index 3b57f8be007c..4c7d311fbeff 100644
> --- a/drivers/gpu/drm/ingenic/Kconfig
> +++ b/drivers/gpu/drm/ingenic/Kconfig
> @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU
>  
>  	  The Image Processing Unit (IPU) will appear as a second primary plane.
>  
> +config DRM_INGENIC_DW_HDMI
> +	bool "Ingenic specific support for Synopsys DW HDMI"
> +	depends on MACH_JZ4780
> +	select DRM_DW_HDMI
> +	help
> +	  Choose this option to enable Synopsys DesignWare HDMI based driver.
> +	  If you want to enable HDMI on Ingenic JZ4780 based SoC, you should
> +	  select this option..
> +
>  endif
> diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile
> index d313326bdddb..3db9888a6c04 100644
> --- a/drivers/gpu/drm/ingenic/Makefile
> +++ b/drivers/gpu/drm/ingenic/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
>  ingenic-drm-y = ingenic-drm-drv.o
>  ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o
> +ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o
> diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> new file mode 100644
> index 000000000000..dd9c94ae842e
> --- /dev/null
> +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
> + * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
> + *
> + * Derived from dw_hdmi-imx.c with i.MX portions removed.
> + * Probe and remove operations derived from rcar_dw_hdmi.c.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/bridge/dw_hdmi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_print.h>
> +
> +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
> +	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
> +	{ 92500000,  { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 } } },
> +	{ 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a } } },
> +	{ 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f } } },
> +	{ ~0UL,      { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, 0x0000 } } }
> +};
> +
> +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = {
> +	/*pixelclk     bpp8    bpp10   bpp12 */
> +	{ 54000000,  { 0x091c, 0x091c, 0x06dc } },
> +	{ 58400000,  { 0x091c, 0x06dc, 0x06dc } },
> +	{ 72000000,  { 0x06dc, 0x06dc, 0x091c } },
> +	{ 74250000,  { 0x06dc, 0x0b5c, 0x091c } },
> +	{ 118800000, { 0x091c, 0x091c, 0x06dc } },
> +	{ 216000000, { 0x06dc, 0x0b5c, 0x091c } },
> +	{ ~0UL,      { 0x0000, 0x0000, 0x0000 } },
> +};
> +
> +/*
> + * Resistance term 133Ohm Cfg
> + * PREEMP config 0.00
> + * TX/CK level 10
> + */
> +static const struct dw_hdmi_phy_config ingenic_phy_config[] = {
> +	/*pixelclk   symbol   term   vlev */
> +	{ 216000000, 0x800d, 0x0005, 0x01ad},
> +	{ ~0UL,      0x0000, 0x0000, 0x0000}
> +};
> +
> +static enum drm_mode_status
> +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data,
> +			   const struct drm_display_info *info,
> +			   const struct drm_display_mode *mode)
> +{
> +	if (mode->clock < 13500)
> +		return MODE_CLOCK_LOW;
> +	/* FIXME: Hardware is capable of 270MHz, but setup data is missing. */
> +	if (mode->clock > 216000)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
> +static bool
> +ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge,
> +			   const struct drm_display_mode *mode,
> +			   struct drm_display_mode *adjusted_mode)
> +{
> +	adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> +	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> +
> +	return true;
> +}
> +
> +static const struct drm_bridge_timings ingenic_dw_hdmi_timings = {
> +	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
> +};
> +
> +static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = {
> +	.mpll_cfg   = ingenic_mpll_cfg,
> +	.cur_ctr    = ingenic_cur_ctr,
> +	.phy_config = ingenic_phy_config,
> +	.mode_valid = ingenic_dw_hdmi_mode_valid,
> +	.mode_fixup = ingenic_dw_hdmi_mode_fixup,
> +	.timings    = &ingenic_dw_hdmi_timings,
> +	.output_port	= 1,
> +};
> +
> +static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
> +	{ .compatible = "ingenic,jz4780-dw-hdmi" },
> +	{ /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
> +
> +static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
> +{
> +	struct dw_hdmi *hdmi;
> +	struct regulator *regulator;
> +	int ret;
> +
> +	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
> +	if (IS_ERR(hdmi))
> +		return PTR_ERR(hdmi);
> +
> +	platform_set_drvdata(pdev, hdmi);
> +
> +	regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v");
> +
> +	if (IS_ERR(regulator)) {
> +		ret = PTR_ERR(regulator);
> +
> +		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %s (%d)\n",
> +			      "hdmi-5v", ret);
> +		return ret;
> +	}

This doesn't match your binding

Maxime

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

Re: [PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780

[H. Nikolaus Schaller]

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



> Am 27.09.2021 um 19:08 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> On Mon, Sep 27, 2021 at 06:44:24PM +0200, H. Nikolaus Schaller wrote:
>> From: Paul Boddie <paul@boddie.org.uk>
>> 
>> A specialisation of the generic Synopsys HDMI driver is employed for JZ4780
>> HDMI support. This requires a new driver, plus device tree and configuration
>> modifications.
>> 
>> +	regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v");
>> +
>> +	if (IS_ERR(regulator)) {
>> +		ret = PTR_ERR(regulator);
>> +
>> +		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %s (%d)\n",
>> +			      "hdmi-5v", ret);
>> +		return ret;
>> +	}
> 
> This doesn't match your binding

or the binding not what we wanted to have...

BR and thanks,
Nikolaus

[-- Attachment #2: Type: text/html, Size: 4441 bytes --]

Re: [PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780

[Neil Armstrong]

On 27/09/2021 18:44, H. Nikolaus Schaller wrote:
> From: Paul Boddie <paul@boddie.org.uk>
> 
> A specialisation of the generic Synopsys HDMI driver is employed for JZ4780
> HDMI support. This requires a new driver, plus device tree and configuration
> modifications.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/ingenic/Kconfig           |   9 ++
>  drivers/gpu/drm/ingenic/Makefile          |   1 +
>  drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 142 ++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> 
> diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
> index 3b57f8be007c..4c7d311fbeff 100644
> --- a/drivers/gpu/drm/ingenic/Kconfig
> +++ b/drivers/gpu/drm/ingenic/Kconfig
> @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU
>  
>  	  The Image Processing Unit (IPU) will appear as a second primary plane.
>  
> +config DRM_INGENIC_DW_HDMI
> +	bool "Ingenic specific support for Synopsys DW HDMI"
> +	depends on MACH_JZ4780
> +	select DRM_DW_HDMI
> +	help
> +	  Choose this option to enable Synopsys DesignWare HDMI based driver.
> +	  If you want to enable HDMI on Ingenic JZ4780 based SoC, you should
> +	  select this option..
> +
>  endif
> diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile
> index d313326bdddb..3db9888a6c04 100644
> --- a/drivers/gpu/drm/ingenic/Makefile
> +++ b/drivers/gpu/drm/ingenic/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
>  ingenic-drm-y = ingenic-drm-drv.o
>  ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o
> +ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o
> diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> new file mode 100644
> index 000000000000..dd9c94ae842e
> --- /dev/null
> +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
> + * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
> + *
> + * Derived from dw_hdmi-imx.c with i.MX portions removed.
> + * Probe and remove operations derived from rcar_dw_hdmi.c.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/bridge/dw_hdmi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_print.h>
> +
> +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
> +	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
> +	{ 92500000,  { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 } } },
> +	{ 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a } } },
> +	{ 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f } } },
> +	{ ~0UL,      { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, 0x0000 } } }
> +};
> +
> +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = {
> +	/*pixelclk     bpp8    bpp10   bpp12 */
> +	{ 54000000,  { 0x091c, 0x091c, 0x06dc } },
> +	{ 58400000,  { 0x091c, 0x06dc, 0x06dc } },
> +	{ 72000000,  { 0x06dc, 0x06dc, 0x091c } },
> +	{ 74250000,  { 0x06dc, 0x0b5c, 0x091c } },
> +	{ 118800000, { 0x091c, 0x091c, 0x06dc } },
> +	{ 216000000, { 0x06dc, 0x0b5c, 0x091c } },
> +	{ ~0UL,      { 0x0000, 0x0000, 0x0000 } },
> +};
> +
> +/*
> + * Resistance term 133Ohm Cfg
> + * PREEMP config 0.00
> + * TX/CK level 10
> + */
> +static const struct dw_hdmi_phy_config ingenic_phy_config[] = {
> +	/*pixelclk   symbol   term   vlev */
> +	{ 216000000, 0x800d, 0x0005, 0x01ad},
> +	{ ~0UL,      0x0000, 0x0000, 0x0000}
> +};
> +
> +static enum drm_mode_status
> +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data,
> +			   const struct drm_display_info *info,
> +			   const struct drm_display_mode *mode)
> +{
> +	if (mode->clock < 13500)
> +		return MODE_CLOCK_LOW;
> +	/* FIXME: Hardware is capable of 270MHz, but setup data is missing. */
> +	if (mode->clock > 216000)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
> +static bool
> +ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge,
> +			   const struct drm_display_mode *mode,
> +			   struct drm_display_mode *adjusted_mode)
> +{
> +	adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> +	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> +
> +	return true;
> +}
> +
> +static const struct drm_bridge_timings ingenic_dw_hdmi_timings = {
> +	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
> +};

These should go in the intermediate encoder bridge callbacks Paul introduces in his patch at [1].

With that patch 4 can be dropped.

[1] https://lore.kernel.org/r/20210922205555.496871-7-paul@crapouillou.net

Neil

> +
> +static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = {
> +	.mpll_cfg   = ingenic_mpll_cfg,
> +	.cur_ctr    = ingenic_cur_ctr,
> +	.phy_config = ingenic_phy_config,
> +	.mode_valid = ingenic_dw_hdmi_mode_valid,
> +	.mode_fixup = ingenic_dw_hdmi_mode_fixup,
> +	.timings    = &ingenic_dw_hdmi_timings,
> +	.output_port	= 1,
> +};
> +
> +static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
> +	{ .compatible = "ingenic,jz4780-dw-hdmi" },
> +	{ /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
> +
> +static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
> +{
> +	struct dw_hdmi *hdmi;
> +	struct regulator *regulator;
> +	int ret;
> +
> +	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
> +	if (IS_ERR(hdmi))
> +		return PTR_ERR(hdmi);
> +
> +	platform_set_drvdata(pdev, hdmi);
> +
> +	regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v");
> +
> +	if (IS_ERR(regulator)) {
> +		ret = PTR_ERR(regulator);
> +
> +		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %s (%d)\n",
> +			      "hdmi-5v", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(regulator);
> +	if (ret) {
> +		DRM_DEV_ERROR(&pdev->dev, "Failed to enable hpd regulator: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
> +{
> +	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
> +
> +	dw_hdmi_remove(hdmi);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver ingenic_dw_hdmi_driver = {
> +	.probe  = ingenic_dw_hdmi_probe,
> +	.remove = ingenic_dw_hdmi_remove,
> +	.driver = {
> +		.name = "dw-hdmi-ingenic",
> +		.of_match_table = ingenic_dw_hdmi_dt_ids,
> +	},
> +};
> +
> +struct platform_driver *ingenic_dw_hdmi_driver_ptr = &ingenic_dw_hdmi_driver;
> 

Re: [PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780

[H. Nikolaus Schaller]

Hi Neil,


> Am 28.09.2021 um 15:02 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> 
> On 27/09/2021 18:44, H. Nikolaus Schaller wrote:
>> From: Paul Boddie <paul@boddie.org.uk>
>> 
>> A specialisation of the generic Synopsys HDMI driver is employed for JZ4780
>> HDMI support. This requires a new driver, plus device tree and configuration
>> modifications.
>> 
>> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/gpu/drm/ingenic/Kconfig           |   9 ++
>> drivers/gpu/drm/ingenic/Makefile          |   1 +
>> drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 142 ++++++++++++++++++++++
>> 3 files changed, 152 insertions(+)
>> create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
>> 
>> diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
>> index 3b57f8be007c..4c7d311fbeff 100644
>> --- a/drivers/gpu/drm/ingenic/Kconfig
>> +++ b/drivers/gpu/drm/ingenic/Kconfig
>> @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU
>> 
>> 	  The Image Processing Unit (IPU) will appear as a second primary plane.
>> 
>> +config DRM_INGENIC_DW_HDMI
>> +	bool "Ingenic specific support for Synopsys DW HDMI"
>> +	depends on MACH_JZ4780
>> +	select DRM_DW_HDMI
>> +	help
>> +	  Choose this option to enable Synopsys DesignWare HDMI based driver.
>> +	  If you want to enable HDMI on Ingenic JZ4780 based SoC, you should
>> +	  select this option..
>> +
>> endif
>> diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile
>> index d313326bdddb..3db9888a6c04 100644
>> --- a/drivers/gpu/drm/ingenic/Makefile
>> +++ b/drivers/gpu/drm/ingenic/Makefile
>> @@ -1,3 +1,4 @@
>> obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
>> ingenic-drm-y = ingenic-drm-drv.o
>> ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o
>> +ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
>> new file mode 100644
>> index 000000000000..dd9c94ae842e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
>> @@ -0,0 +1,142 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
>> + * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
>> + *
>> + * Derived from dw_hdmi-imx.c with i.MX portions removed.
>> + * Probe and remove operations derived from rcar_dw_hdmi.c.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <drm/bridge/dw_hdmi.h>
>> +#include <drm/drm_of.h>
>> +#include <drm/drm_print.h>
>> +
>> +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
>> +	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
>> +	{ 92500000,  { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 } } },
>> +	{ 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a } } },
>> +	{ 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f } } },
>> +	{ ~0UL,      { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, 0x0000 } } }
>> +};
>> +
>> +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = {
>> +	/*pixelclk     bpp8    bpp10   bpp12 */
>> +	{ 54000000,  { 0x091c, 0x091c, 0x06dc } },
>> +	{ 58400000,  { 0x091c, 0x06dc, 0x06dc } },
>> +	{ 72000000,  { 0x06dc, 0x06dc, 0x091c } },
>> +	{ 74250000,  { 0x06dc, 0x0b5c, 0x091c } },
>> +	{ 118800000, { 0x091c, 0x091c, 0x06dc } },
>> +	{ 216000000, { 0x06dc, 0x0b5c, 0x091c } },
>> +	{ ~0UL,      { 0x0000, 0x0000, 0x0000 } },
>> +};
>> +
>> +/*
>> + * Resistance term 133Ohm Cfg
>> + * PREEMP config 0.00
>> + * TX/CK level 10
>> + */
>> +static const struct dw_hdmi_phy_config ingenic_phy_config[] = {
>> +	/*pixelclk   symbol   term   vlev */
>> +	{ 216000000, 0x800d, 0x0005, 0x01ad},
>> +	{ ~0UL,      0x0000, 0x0000, 0x0000}
>> +};
>> +
>> +static enum drm_mode_status
>> +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data,
>> +			   const struct drm_display_info *info,
>> +			   const struct drm_display_mode *mode)
>> +{
>> +	if (mode->clock < 13500)
>> +		return MODE_CLOCK_LOW;
>> +	/* FIXME: Hardware is capable of 270MHz, but setup data is missing. */
>> +	if (mode->clock > 216000)
>> +		return MODE_CLOCK_HIGH;
>> +
>> +	return MODE_OK;
>> +}
>> +
>> +static bool
>> +ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge,
>> +			   const struct drm_display_mode *mode,
>> +			   struct drm_display_mode *adjusted_mode)
>> +{
>> +	adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>> +	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
>> +
>> +	return true;
>> +}
>> +
>> +static const struct drm_bridge_timings ingenic_dw_hdmi_timings = {
>> +	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
>> +};
> 
> These should go in the intermediate encoder bridge callbacks Paul introduces in his patch at [1].
> 
> With that patch 4 can be dropped.
> 
> [1] https://lore.kernel.org/r/20210922205555.496871-7-paul@crapouillou.net
> 
> Neil

Sorry, but I can't follow you here. Our patch set is on top of Paul's patch [1]
and requires Paul's work to be merged first.

And our 4/10 is needed to specialize dw-hdmi to the jz4780 like it is done for
other SoC integration. It addresses a different stage of the jz4780 HDMI chain
than [1].

BR,
Nikolaus

Re: [PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780

[Paul Cercueil]

Hi Nikolaus & Paul,

Le lun., sept. 27 2021 at 18:44:24 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> From: Paul Boddie <paul@boddie.org.uk>
> 
> A specialisation of the generic Synopsys HDMI driver is employed for 
> JZ4780
> HDMI support. This requires a new driver, plus device tree and 
> configuration
> modifications.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/ingenic/Kconfig           |   9 ++
>  drivers/gpu/drm/ingenic/Makefile          |   1 +
>  drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 142 
> ++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> 
> diff --git a/drivers/gpu/drm/ingenic/Kconfig 
> b/drivers/gpu/drm/ingenic/Kconfig
> index 3b57f8be007c..4c7d311fbeff 100644
> --- a/drivers/gpu/drm/ingenic/Kconfig
> +++ b/drivers/gpu/drm/ingenic/Kconfig
> @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU
> 
>  	  The Image Processing Unit (IPU) will appear as a second primary 
> plane.
> 
> +config DRM_INGENIC_DW_HDMI
> +	bool "Ingenic specific support for Synopsys DW HDMI"
> +	depends on MACH_JZ4780
> +	select DRM_DW_HDMI
> +	help
> +	  Choose this option to enable Synopsys DesignWare HDMI based 
> driver.
> +	  If you want to enable HDMI on Ingenic JZ4780 based SoC, you should
> +	  select this option..
> +
>  endif
> diff --git a/drivers/gpu/drm/ingenic/Makefile 
> b/drivers/gpu/drm/ingenic/Makefile
> index d313326bdddb..3db9888a6c04 100644
> --- a/drivers/gpu/drm/ingenic/Makefile
> +++ b/drivers/gpu/drm/ingenic/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
>  ingenic-drm-y = ingenic-drm-drv.o
>  ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o
> +ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o
> diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c 
> b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> new file mode 100644
> index 000000000000..dd9c94ae842e
> --- /dev/null
> +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
> + * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
> + *
> + * Derived from dw_hdmi-imx.c with i.MX portions removed.
> + * Probe and remove operations derived from rcar_dw_hdmi.c.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/bridge/dw_hdmi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_print.h>
> +
> +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
> +	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 
> 0x0000 } } },
> +	{ 92500000,  { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 
> 0x0005 } } },
> +	{ 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 
> 0x000a } } },
> +	{ 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 
> 0x000f } } },
> +	{ ~0UL,      { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, 
> 0x0000 } } }
> +};
> +
> +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = {
> +	/*pixelclk     bpp8    bpp10   bpp12 */
> +	{ 54000000,  { 0x091c, 0x091c, 0x06dc } },
> +	{ 58400000,  { 0x091c, 0x06dc, 0x06dc } },
> +	{ 72000000,  { 0x06dc, 0x06dc, 0x091c } },
> +	{ 74250000,  { 0x06dc, 0x0b5c, 0x091c } },
> +	{ 118800000, { 0x091c, 0x091c, 0x06dc } },
> +	{ 216000000, { 0x06dc, 0x0b5c, 0x091c } },
> +	{ ~0UL,      { 0x0000, 0x0000, 0x0000 } },
> +};
> +
> +/*
> + * Resistance term 133Ohm Cfg
> + * PREEMP config 0.00
> + * TX/CK level 10
> + */
> +static const struct dw_hdmi_phy_config ingenic_phy_config[] = {
> +	/*pixelclk   symbol   term   vlev */
> +	{ 216000000, 0x800d, 0x0005, 0x01ad},
> +	{ ~0UL,      0x0000, 0x0000, 0x0000}
> +};
> +
> +static enum drm_mode_status
> +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data,
> +			   const struct drm_display_info *info,
> +			   const struct drm_display_mode *mode)
> +{
> +	if (mode->clock < 13500)
> +		return MODE_CLOCK_LOW;
> +	/* FIXME: Hardware is capable of 270MHz, but setup data is missing. 
> */
> +	if (mode->clock > 216000)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
> +static bool
> +ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge,
> +			   const struct drm_display_mode *mode,
> +			   struct drm_display_mode *adjusted_mode)
> +{
> +	adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | 
> DRM_MODE_FLAG_PVSYNC);
> +	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | 
> DRM_MODE_FLAG_NVSYNC);

Why do these flags need to be cleared? The LCDC can work with negative 
v/h sync, does the HDMI core not work with that?

> +
> +	return true;
> +}
> +
> +static const struct drm_bridge_timings ingenic_dw_hdmi_timings = {
> +	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,

That info should be provided in the EDID, is this really needed here?

Cheers,
-Paul

> +};
> +
> +static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = {
> +	.mpll_cfg   = ingenic_mpll_cfg,
> +	.cur_ctr    = ingenic_cur_ctr,
> +	.phy_config = ingenic_phy_config,
> +	.mode_valid = ingenic_dw_hdmi_mode_valid,
> +	.mode_fixup = ingenic_dw_hdmi_mode_fixup,
> +	.timings    = &ingenic_dw_hdmi_timings,
> +	.output_port	= 1,
> +};
> +
> +static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
> +	{ .compatible = "ingenic,jz4780-dw-hdmi" },
> +	{ /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
> +
> +static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
> +{
> +	struct dw_hdmi *hdmi;
> +	struct regulator *regulator;
> +	int ret;
> +
> +	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
> +	if (IS_ERR(hdmi))
> +		return PTR_ERR(hdmi);
> +
> +	platform_set_drvdata(pdev, hdmi);
> +
> +	regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v");
> +
> +	if (IS_ERR(regulator)) {
> +		ret = PTR_ERR(regulator);
> +
> +		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %s (%d)\n",
> +			      "hdmi-5v", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(regulator);
> +	if (ret) {
> +		DRM_DEV_ERROR(&pdev->dev, "Failed to enable hpd regulator: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
> +{
> +	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
> +
> +	dw_hdmi_remove(hdmi);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver ingenic_dw_hdmi_driver = {
> +	.probe  = ingenic_dw_hdmi_probe,
> +	.remove = ingenic_dw_hdmi_remove,
> +	.driver = {
> +		.name = "dw-hdmi-ingenic",
> +		.of_match_table = ingenic_dw_hdmi_dt_ids,
> +	},
> +};
> +
> +struct platform_driver *ingenic_dw_hdmi_driver_ptr = 
> &ingenic_dw_hdmi_driver;
> --
> 2.31.1
> 

[PATCH v4 07/10] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[H. Nikolaus Schaller]

From: Paul Boddie <paul@boddie.org.uk>

A specialisation of the generic Synopsys HDMI driver is employed for JZ4780
HDMI support. This requires a new driver, plus device tree and configuration
modifications.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 45 ++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 9e34f433b9b5..4cbc6a4db6cd 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -424,6 +424,51 @@ i2c4: i2c@10054000 {
 		status = "disabled";
 	};
 
+	hdmi: hdmi@10180000 {
+		compatible = "ingenic,jz4780-dw-hdmi";
+		reg = <0x10180000 0x8000>;
+		reg-io-width = <4>;
+
+		clocks = <&cgu JZ4780_CLK_HDMI>, <&cgu JZ4780_CLK_AHB0>;
+		clock-names = "isfr" , "iahb";
+
+		assigned-clocks = <&cgu JZ4780_CLK_HDMI>;
+		assigned-clock-rates = <27000000>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <3>;
+
+		/* ddc-i2c-bus = <&i2c4>; */
+
+		status = "disabled";
+	};
+
+	lcdc0: lcdc0@13050000 {
+		compatible = "ingenic,jz4780-lcd";
+		reg = <0x13050000 0x1800>;
+
+		clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>;
+		clock-names = "lcd", "lcd_pclk";
+
+		interrupt-parent = <&intc>;
+		interrupts = <31>;
+
+		status = "disabled";
+	};
+
+	lcdc1: lcdc1@130a0000 {
+		compatible = "ingenic,jz4780-lcd";
+		reg = <0x130a0000 0x1800>;
+
+		clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>;
+		clock-names = "lcd", "lcd_pclk";
+
+		interrupt-parent = <&intc>;
+		interrupts = <31>;
+
+		status = "disabled";
+	};
+
 	nemc: nemc@13410000 {
 		compatible = "ingenic,jz4780-nemc", "simple-mfd";
 		reg = <0x13410000 0x10000>;
-- 
2.31.1

[PATCH v4 08/10] MIPS: DTS: CI20: Add DT nodes for HDMI setup

[H. Nikolaus Schaller]

From: Paul Boddie <paul@boddie.org.uk>

We need to hook up
* HDMI connector
* HDMI power regulator
* DDC pinmux
* HDMI and LCDC endpoint connections

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/mips/boot/dts/ingenic/ci20.dts | 67 +++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index a688809beebc..4776be35b14d 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -78,6 +78,18 @@ eth0_power: fixedregulator@0 {
 		enable-active-high;
 	};
 
+	hdmi_out: connector {
+		compatible = "hdmi-connector";
+		label = "HDMI OUT";
+		type = "a";
+
+		port {
+			hdmi_con: endpoint {
+				remote-endpoint = <&dw_hdmi_out>;
+			};
+		};
+	};
+
 	ir: ir {
 		compatible = "gpio-ir-receiver";
 		gpios = <&gpe 3 GPIO_ACTIVE_LOW>;
@@ -102,6 +114,17 @@ otg_power: fixedregulator@2 {
 		gpio = <&gpf 14 GPIO_ACTIVE_LOW>;
 		enable-active-high;
 	};
+
+	hdmi_power: fixedregulator@3 {
+		compatible = "regulator-fixed";
+
+		regulator-name = "hdmi_power";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+
+		gpio = <&gpa 25 GPIO_ACTIVE_LOW>;
+		enable-active-high;
+	};
 };
 
 &ext {
@@ -506,6 +529,12 @@ pins_i2c4: i2c4 {
 		bias-disable;
 	};
 
+	pins_hdmi_ddc: hdmi_ddc {
+		function = "hdmi-ddc";
+		groups = "hdmi-ddc";
+		bias-disable;
+	};
+
 	pins_nemc: nemc {
 		function = "nemc";
 		groups = "nemc-data", "nemc-cle-ale", "nemc-rd-we", "nemc-frd-fwe";
@@ -536,3 +565,41 @@ pins_mmc1: mmc1 {
 		bias-disable;
 	};
 };
+
+&hdmi {
+	status = "okay";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pins_hdmi_ddc>;
+
+	hdmi-5v-supply = <&hdmi_power>;
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			dw_hdmi_in: endpoint {
+				remote-endpoint = <&lcd_out>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+			dw_hdmi_out: endpoint {
+				remote-endpoint = <&hdmi_con>;
+			};
+		};
+	};
+};
+
+&lcdc0 {
+	status = "okay";
+
+	port {
+		lcd_out: endpoint {
+			remote-endpoint = <&dw_hdmi_in>;
+		};
+	};
+};
-- 
2.31.1

[PATCH v4 09/10] MIPS: defconfig: CI20: configure for DRM_DW_HDMI_JZ4780

[H. Nikolaus Schaller]

Enable CONFIG options as modules.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/mips/configs/ci20_defconfig | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig
index ab7ebb066834..9c9c649d385b 100644
--- a/arch/mips/configs/ci20_defconfig
+++ b/arch/mips/configs/ci20_defconfig
@@ -98,7 +98,13 @@ CONFIG_RC_DEVICES=y
 CONFIG_IR_GPIO_CIR=m
 CONFIG_IR_GPIO_TX=m
 CONFIG_MEDIA_SUPPORT=m
+CONFIG_DRM=m
+CONFIG_DRM_INGENIC=m
+CONFIG_DRM_INGENIC_DW_HDMI=y
+CONFIG_DRM_DISPLAY_CONNECTOR=m
 # CONFIG_VGA_CONSOLE is not set
+CONFIG_FB=y
+CONFIG_FRAMEBUFFER_CONSOLE=y
 # CONFIG_HID is not set
 CONFIG_USB=y
 CONFIG_USB_STORAGE=y
-- 
2.31.1

[PATCH v4 10/10] drm/ingenic: add some jz4780 specific features

[H. Nikolaus Schaller]

From: Paul Boddie <paul@boddie.org.uk>

The jz4780 has some features which need initialization
according to the vendor kernel.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 39 +++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index e2df4b085905..605549b316b5 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -66,6 +66,10 @@ struct jz_soc_info {
 	bool needs_dev_clk;
 	bool has_osd;
 	bool map_noncoherent;
+	bool has_alpha;
+	bool has_pcfg;
+	bool has_recover;
+	bool has_rgbc;
 	bool use_extended_hwdesc;
 	unsigned int max_width, max_height;
 	const u32 *formats_f0, *formats_f1;
@@ -732,6 +736,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
 	}
 
+	if (priv->soc_info->has_recover)
+		cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN;
+
 	/* set use of the 8-word descriptor and OSD foreground usage. */
 	if (priv->soc_info->use_extended_hwdesc)
 		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
@@ -1321,6 +1328,25 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	if (soc_info->has_osd)
 		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
 
+	if (soc_info->has_alpha)
+		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_ALPHAEN);
+
+	/* Magic values from the vendor kernel for the priority thresholds. */
+	if (soc_info->has_pcfg)
+		regmap_write(priv->map, JZ_REG_LCD_PCFG,
+			     JZ_LCD_PCFG_PRI_MODE |
+			     JZ_LCD_PCFG_HP_BST_16 |
+			     (511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) |
+			     (400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) |
+			     (256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET));
+
+	/* RGB output control may be superfluous. */
+	if (soc_info->has_rgbc)
+		regmap_write(priv->map, JZ_REG_LCD_RGBC,
+			     JZ_LCD_RGBC_RGB_FORMAT_ENABLE |
+			     JZ_LCD_RGBC_ODD_RGB |
+			     JZ_LCD_RGBC_EVEN_RGB);
+
 	mutex_init(&priv->clk_mutex);
 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
 
@@ -1484,6 +1510,9 @@ static const struct jz_soc_info jz4740_soc_info = {
 	.needs_dev_clk = true,
 	.has_osd = false,
 	.map_noncoherent = false,
+	.has_pcfg = false,
+	.has_recover = false,
+	.has_rgbc = false,
 	.max_width = 800,
 	.max_height = 600,
 	.formats_f1 = jz4740_formats,
@@ -1496,6 +1525,9 @@ static const struct jz_soc_info jz4725b_soc_info = {
 	.needs_dev_clk = false,
 	.has_osd = true,
 	.map_noncoherent = false,
+	.has_pcfg = false,
+	.has_recover = false,
+	.has_rgbc = false,
 	.max_width = 800,
 	.max_height = 600,
 	.formats_f1 = jz4725b_formats_f1,
@@ -1509,6 +1541,9 @@ static const struct jz_soc_info jz4770_soc_info = {
 	.needs_dev_clk = false,
 	.has_osd = true,
 	.map_noncoherent = true,
+	.has_pcfg = false,
+	.has_recover = false,
+	.has_rgbc = false,
 	.max_width = 1280,
 	.max_height = 720,
 	.formats_f1 = jz4770_formats_f1,
@@ -1521,6 +1556,10 @@ static const struct jz_soc_info jz4770_soc_info = {
 static const struct jz_soc_info jz4780_soc_info = {
 	.needs_dev_clk = true,
 	.has_osd = true,
+	.has_alpha = true,
+	.has_pcfg = true,
+	.has_recover = true,
+	.has_rgbc = true,
 	.use_extended_hwdesc = true,
 	.max_width = 4096,
 	.max_height = 2048,
-- 
2.31.1

Re: [PATCH v4 10/10] drm/ingenic: add some jz4780 specific features

[Paul Cercueil]

Hi,

Le lun., sept. 27 2021 at 18:44:28 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> From: Paul Boddie <paul@boddie.org.uk>
> 
> The jz4780 has some features which need initialization
> according to the vendor kernel.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 39 
> +++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index e2df4b085905..605549b316b5 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -66,6 +66,10 @@ struct jz_soc_info {
>  	bool needs_dev_clk;
>  	bool has_osd;
>  	bool map_noncoherent;
> +	bool has_alpha;
> +	bool has_pcfg;
> +	bool has_recover;
> +	bool has_rgbc;
>  	bool use_extended_hwdesc;
>  	unsigned int max_width, max_height;
>  	const u32 *formats_f0, *formats_f1;
> @@ -732,6 +736,9 @@ static void 
> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>  		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>  	}
> 
> +	if (priv->soc_info->has_recover)
> +		cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN;

Did you actually test this? I know that in theory it sounds like 
something we'd want, but unless there is a proven use for it, it's 
better to keep it disabled.

> +
>  	/* set use of the 8-word descriptor and OSD foreground usage. */
>  	if (priv->soc_info->use_extended_hwdesc)
>  		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
> @@ -1321,6 +1328,25 @@ static int ingenic_drm_bind(struct device 
> *dev, bool has_components)
>  	if (soc_info->has_osd)
>  		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
> 
> +	if (soc_info->has_alpha)
> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_ALPHAEN);

I remember you saying that OSD mode was not yet working on the JZ4780. 
So I can't see how you could have tested this.

> +
> +	/* Magic values from the vendor kernel for the priority thresholds. 
> */
> +	if (soc_info->has_pcfg)
> +		regmap_write(priv->map, JZ_REG_LCD_PCFG,
> +			     JZ_LCD_PCFG_PRI_MODE |
> +			     JZ_LCD_PCFG_HP_BST_16 |
> +			     (511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) |
> +			     (400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) |
> +			     (256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET));

Unless you add a big comment that explains what these values do and why 
we do want them, I don't want magic values in here. The fact that the 
kernel vendor sets this doesn't mean it's needed and/or wanted.

> +
> +	/* RGB output control may be superfluous. */
> +	if (soc_info->has_rgbc)
> +		regmap_write(priv->map, JZ_REG_LCD_RGBC,
> +			     JZ_LCD_RGBC_RGB_FORMAT_ENABLE |
> +			     JZ_LCD_RGBC_ODD_RGB |
> +			     JZ_LCD_RGBC_EVEN_RGB);

ingenic-drm only supports RGB output right now, so I guess the 
RGB_FORMAT_ENABLE bit needs to be set in patch [2/10], otherwise patch 
[2/10] cannot state that it adds support for the JZ4780, if it doesn't 
actually work.

The other two bits can be dropped, they are already set in 
ingenic_drm_encoder_atomic_mode_set().

> +
>  	mutex_init(&priv->clk_mutex);
>  	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
> 
> @@ -1484,6 +1510,9 @@ static const struct jz_soc_info jz4740_soc_info 
> = {
>  	.needs_dev_clk = true,
>  	.has_osd = false,
>  	.map_noncoherent = false,
> +	.has_pcfg = false,
> +	.has_recover = false,
> +	.has_rgbc = false,
>  	.max_width = 800,
>  	.max_height = 600,
>  	.formats_f1 = jz4740_formats,
> @@ -1496,6 +1525,9 @@ static const struct jz_soc_info 
> jz4725b_soc_info = {
>  	.needs_dev_clk = false,
>  	.has_osd = true,
>  	.map_noncoherent = false,
> +	.has_pcfg = false,
> +	.has_recover = false,
> +	.has_rgbc = false,

This is wrong, the JZ4725B and JZ4770 SoCs both have the RGBC register 
and the RECOVER bit.

Cheers,
-Paul

>  	.max_width = 800,
>  	.max_height = 600,
>  	.formats_f1 = jz4725b_formats_f1,
> @@ -1509,6 +1541,9 @@ static const struct jz_soc_info jz4770_soc_info 
> = {
>  	.needs_dev_clk = false,
>  	.has_osd = true,
>  	.map_noncoherent = true,
> +	.has_pcfg = false,
> +	.has_recover = false,
> +	.has_rgbc = false,
>  	.max_width = 1280,
>  	.max_height = 720,
>  	.formats_f1 = jz4770_formats_f1,
> @@ -1521,6 +1556,10 @@ static const struct jz_soc_info 
> jz4770_soc_info = {
>  static const struct jz_soc_info jz4780_soc_info = {
>  	.needs_dev_clk = true,
>  	.has_osd = true,
> +	.has_alpha = true,
> +	.has_pcfg = true,
> +	.has_recover = true,
> +	.has_rgbc = true,
>  	.use_extended_hwdesc = true,
>  	.max_width = 4096,
>  	.max_height = 2048,
> --
> 2.31.1
> 

Re: [PATCH v4 10/10] drm/ingenic: add some jz4780 specific features

[H. Nikolaus Schaller]

Hi Paul,

> Am 28.09.2021 um 11:58 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi,
> 
> Le lun., sept. 27 2021 at 18:44:28 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> From: Paul Boddie <paul@boddie.org.uk>
>> The jz4780 has some features which need initialization
>> according to the vendor kernel.
>> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 39 +++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> index e2df4b085905..605549b316b5 100644
>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> @@ -66,6 +66,10 @@ struct jz_soc_info {
>> 	bool needs_dev_clk;
>> 	bool has_osd;
>> 	bool map_noncoherent;
>> +	bool has_alpha;
>> +	bool has_pcfg;
>> +	bool has_recover;
>> +	bool has_rgbc;
>> 	bool use_extended_hwdesc;
>> 	unsigned int max_width, max_height;
>> 	const u32 *formats_f0, *formats_f1;
>> @@ -732,6 +736,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>> 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>> 	}
>> +	if (priv->soc_info->has_recover)
>> +		cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN;
> 
> Did you actually test this? I know that in theory it sounds like something we'd want, but unless there is a proven use for it, it's better to keep it disabled.
> 
>> +
>> 	/* set use of the 8-word descriptor and OSD foreground usage. */
>> 	if (priv->soc_info->use_extended_hwdesc)
>> 		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
>> @@ -1321,6 +1328,25 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	if (soc_info->has_osd)
>> 		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>> +	if (soc_info->has_alpha)
>> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_ALPHAEN);
> 
> I remember you saying that OSD mode was not yet working on the JZ4780. So I can't see how you could have tested this.

Basically this is all stuff from the vendor kernel under the assumption that they know better than everyone of us.
On the other hand this whole patch is sort of optional and we know that the basic milestone to get HDMI working
is reached without it. So if you prefer we can drop it for the moment in v5 and leave it for further analysis later.

> 
>> +
>> +	/* Magic values from the vendor kernel for the priority thresholds. */
>> +	if (soc_info->has_pcfg)
>> +		regmap_write(priv->map, JZ_REG_LCD_PCFG,
>> +			     JZ_LCD_PCFG_PRI_MODE |
>> +			     JZ_LCD_PCFG_HP_BST_16 |
>> +			     (511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) |
>> +			     (400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) |
>> +			     (256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET));
> 
> Unless you add a big comment that explains what these values do and why we do want them, I don't want magic values in here. The fact that the kernel vendor sets this doesn't mean it's needed and/or wanted.

Well, who has a contact within Ingenic?

> 
>> +
>> +	/* RGB output control may be superfluous. */
>> +	if (soc_info->has_rgbc)
>> +		regmap_write(priv->map, JZ_REG_LCD_RGBC,
>> +			     JZ_LCD_RGBC_RGB_FORMAT_ENABLE |
>> +			     JZ_LCD_RGBC_ODD_RGB |
>> +			     JZ_LCD_RGBC_EVEN_RGB);
> 
> ingenic-drm only supports RGB output right now, so I guess the RGB_FORMAT_ENABLE bit needs to be set in patch [2/10], otherwise patch [2/10] cannot state that it adds support for the JZ4780, if it doesn't actually work.
> 
> The other two bits can be dropped, they are already set in ingenic_drm_encoder_atomic_mode_set().

Ok.

> 
>> +
>> 	mutex_init(&priv->clk_mutex);
>> 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
>> @@ -1484,6 +1510,9 @@ static const struct jz_soc_info jz4740_soc_info = {
>> 	.needs_dev_clk = true,
>> 	.has_osd = false,
>> 	.map_noncoherent = false,
>> +	.has_pcfg = false,
>> +	.has_recover = false,
>> +	.has_rgbc = false,
>> 	.max_width = 800,
>> 	.max_height = 600,
>> 	.formats_f1 = jz4740_formats,
>> @@ -1496,6 +1525,9 @@ static const struct jz_soc_info jz4725b_soc_info = {
>> 	.needs_dev_clk = false,
>> 	.has_osd = true,
>> 	.map_noncoherent = false,
>> +	.has_pcfg = false,
>> +	.has_recover = false,
>> +	.has_rgbc = false,
> 
> This is wrong, the JZ4725B and JZ4770 SoCs both have the RGBC register and the RECOVER bit.

Ok, good to know! Will change.

BR and thanks,
Nikolaus

> 
> Cheers,
> -Paul
> 
>> 	.max_width = 800,
>> 	.max_height = 600,
>> 	.formats_f1 = jz4725b_formats_f1,
>> @@ -1509,6 +1541,9 @@ static const struct jz_soc_info jz4770_soc_info = {
>> 	.needs_dev_clk = false,
>> 	.has_osd = true,
>> 	.map_noncoherent = true,
>> +	.has_pcfg = false,
>> +	.has_recover = false,
>> +	.has_rgbc = false,
>> 	.max_width = 1280,
>> 	.max_height = 720,
>> 	.formats_f1 = jz4770_formats_f1,
>> @@ -1521,6 +1556,10 @@ static const struct jz_soc_info jz4770_soc_info = {
>> static const struct jz_soc_info jz4780_soc_info = {
>> 	.needs_dev_clk = true,
>> 	.has_osd = true,
>> +	.has_alpha = true,
>> +	.has_pcfg = true,
>> +	.has_recover = true,
>> +	.has_rgbc = true,
>> 	.use_extended_hwdesc = true,
>> 	.max_width = 4096,
>> 	.max_height = 2048,
>> --
>> 2.31.1

Re: [PATCH v4 10/10] drm/ingenic: add some jz4780 specific features

[H. Nikolaus Schaller]

> Am 28.09.2021 um 12:06 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
>>> 
>>> +
>>> +	/* RGB output control may be superfluous. */
>>> +	if (soc_info->has_rgbc)
>>> +		regmap_write(priv->map, JZ_REG_LCD_RGBC,
>>> +			     JZ_LCD_RGBC_RGB_FORMAT_ENABLE |
>>> +			     JZ_LCD_RGBC_ODD_RGB |
>>> +			     JZ_LCD_RGBC_EVEN_RGB);
>> 
>> ingenic-drm only supports RGB output right now, so I guess the RGB_FORMAT_ENABLE bit needs to be set in patch [2/10], otherwise patch [2/10] cannot state that it adds support for the JZ4780, if it doesn't actually work.

interestingly it works without setting anything in this register.

>> 
>> The other two bits can be dropped, they are already set in ingenic_drm_encoder_atomic_mode_set().
> 
> Ok.

Setting it manually doesn't change anything visible:

root@letux:~# devmem2 0x13050090
/dev/mem opened.
Memory mapped at address 0x77e14000.
Value at address 0x13050090 (0x77e14090): 0x0
root@letux:~# devmem2 0x13050090 w 0x80
/dev/mem opened.
Memory mapped at address 0x77e38000.
Value at address 0x13050090 (0x77e38090): 0x0
Written 0x80; readback 0x80
root@letux:~# 

Same for 0x130A0090. Maybe this lcdc register
is not used at all - at least for HDMI?

So I'd suggest to drop this whole patch from v5.

BR and thanks,
Nikolaus