MIPS: JZ4780 and CI20 HDMI

From: Paul Boddie <[email protected]>

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 <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 
+++++++++++++++++++++--
 drivers/gpu/drm/ingenic/ingenic-drm.h     | 42 +++++++++++
 2 files changed, 122 insertions(+), 5 deletions(-)

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +44 -2

index f73522bdacaa..e2df4b085905 100644
@@ -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;

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +1 -0

 	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;

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +0 -0

 	unsigned int i, clone_mask = 0;

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +1 -0

 	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 = {

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +1 -0

@@ -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 = {

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +16 -0

@@ -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);

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +8 -1

+
 	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);
 }

drivers/gpu/drm/ingenic/ingenic-drm.h +42 -0

index 22654ac1dde1..13dbc5d25c3b 100644
@@ -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

Am 28.09.2021 um 11:35 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus / Paul,

Quoted 111 lines · H. Nikolaus Schaller

From: Paul Boddie <[email protected]>
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 <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +++++++++++++++++++++--
drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++++++++++

2 files changed, 122 insertions(+), 5 deletions(-)

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +45 -2

index f73522bdacaa..e2df4b085905 100644
@@ -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);

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_bind(struct device *dev, bool has_components) · line 1112

Quoted 6 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +3 -1

 	}
+	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?

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_bind(struct device *dev, bool has_components) · line 1321

Quoted 9 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +1 -1

 	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?

Quoted 47 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +18 -0

 	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)
 {

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_init(void) · line 1562

Quoted 6 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +5 -0

 	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

Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 10 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_init(void) · line 1561

Quoted 8 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +5 -0

@@ -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.

Am 28.09.2021 um 14:06 schrieb H. Nikolaus Schaller <[email protected]>:

Hi Paul,

Quoted 19 lines

Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 10 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_init(void) · line 1561

Quoted 8 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +5 -0

@@ -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?

Quoted 1 line

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.

Hi Paul,

Quoted 23 lines

Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller
<[email protected]>:

Quoted 11 lines

Quoted 8 lines

@@ -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?

Quoted 3 lines

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

Am 29.09.2021 um 16:30 schrieb Paul Cercueil <[email protected]>:

Hi,

Quoted 11 lines · H. Nikolaus Schaller

Hi Paul,

Quoted 10 lines

Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 9 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_init(void) · line 1561

Quoted 8 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +5 -0

@@ -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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

From: Sam Ravnborg <[email protected]>

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 <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

Cc Rob Herring <[email protected]>

Cc [email protected]

---
 .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +7 -0

new file mode 100644
index 000000000000..5e60cdac4f63
@@ -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

Am 27.09.2021 um 19:07 schrieb [email protected]:

Hi,

Quoted 31 lines · H. Nikolaus Schaller

From: Sam Ravnborg <[email protected]>

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 <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

Cc Rob Herring <[email protected]>

Cc [email protected]

---
.../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++

1 file changed, 85 insertions(+)

create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +11 -0

new file mode 100644
index 000000000000..5e60cdac4f63
@@ -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 <[email protected]>
+

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 17

Quoted 6 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +6 -0

+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?

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 22

Quoted 5 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +4 -0

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

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

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 26

Quoted 5 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +4 -0

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

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

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 29

Quoted 3 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +2 -0

+  reg-io-width:
+    const: 4

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

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 33

Quoted 4 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +3 -0

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

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

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 37

Quoted 4 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +3 -0

+  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

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 41

Quoted 4 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +3 -0

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

Is it isfr or isrf?

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 42

Quoted 2 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +1 -0

+      - const: iahb

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 45

Quoted 3 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +3 -0

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

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 46

Quoted 1 line

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +1 -0

+  ddc-i2c-bus: true

ditto

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 48

Quoted 3 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +2 -0

+    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

Quoted 6 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 22

Quoted 4 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +4 -0

+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.

Quoted 5 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 29

Quoted 3 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +2 -0

+  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.

Quoted 11 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 37

Quoted 3 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +3 -0

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

This can be defined as

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

Quoted 1 line

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

Quoted 6 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 41

Quoted 4 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +3 -0

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

Quoted 2 lines

Quoted 2 lines

+ - 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.

Quoted 5 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 48

Quoted 3 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +2 -0

+    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).

Am 28.09.2021 um 11:18 schrieb Maxime Ripard <[email protected]>:

Quoted 8 lines · H. Nikolaus Schaller

Quoted 6 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 22

Quoted 4 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +4 -0

+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.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +2 -0

+  reg-io-width:
+    const: 4

Quoted 3 lines

Quoted 1 line

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?

Quoted 12 lines

Quoted 12 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 37

Quoted 4 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +3 -0

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

This can be defined as

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

Quoted 7 lines

Quoted 1 line

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

Quoted 15 lines

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

Quoted 6 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml line 41

Quoted 4 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +3 -0

+  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#

Quoted 4 lines

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

Quoted 4 lines

[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.

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

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

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file +2 -0

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

Quoted 5 lines

Quoted 1 line

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

From: Sam Ravnborg <[email protected]>

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 <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

Cc Rob Herring <[email protected]>

Cc [email protected]

---
 .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

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 <[email protected]>

---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +0 -0

index f082e14320e1..edea04f80576 100644

Am 27.09.2021 um 19:00 schrieb Maxime Ripard <[email protected]>:

Hi,

Quoted 11 lines · 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 <[email protected]>

---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 ++

1 file changed, 2 insertions(+)

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) · line 3022

Quoted 10 lines

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +2 -0

index f082e14320e1..edea04f80576 100644
@@ -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

From: Paul Boddie <[email protected]>

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 <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

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

drivers/gpu/drm/ingenic/Kconfig +9 -0

index 3b57f8be007c..4c7d311fbeff 100644
@@ -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

drivers/gpu/drm/ingenic/Makefile +1 -0

index d313326bdddb..3db9888a6c04 100644
@@ -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

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +102 -0

new file mode 100644
index 000000000000..dd9c94ae842e
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2019, 2020 Paul Boddie <[email protected]>
+ *
+ * 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);

Am 27.09.2021 um 19:08 schrieb Maxime Ripard <[email protected]>:

Quoted 6 lines · H. Nikolaus Schaller

From: Paul Boddie <[email protected]>

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.

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c line 112

Quoted 9 lines

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +9 -0

+	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

From: Paul Boddie <[email protected]>

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 <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

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

drivers/gpu/drm/ingenic/Kconfig +9 -0

index 3b57f8be007c..4c7d311fbeff 100644
@@ -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

drivers/gpu/drm/ingenic/Makefile +1 -0

index d313326bdddb..3db9888a6c04 100644
@@ -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

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +64 -0

new file mode 100644
index 000000000000..dd9c94ae842e
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2019, 2020 Paul Boddie <[email protected]>
+ *
+ * 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)

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +68 -0

+
+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;

Am 28.09.2021 um 15:02 schrieb Neil Armstrong <[email protected]>:

Quoted 113 lines · H. Nikolaus Schaller

From: Paul Boddie <[email protected]>

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 <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---

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

drivers/gpu/drm/ingenic/Kconfig +9 -0

index 3b57f8be007c..4c7d311fbeff 100644
@@ -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

drivers/gpu/drm/ingenic/Makefile +0 -0

index d313326bdddb..3db9888a6c04 100644
@@ -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

drivers/gpu/drm/ingenic/Makefile +1 -0

+ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +64 -0

new file mode 100644
index 000000000000..dd9c94ae842e
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2019, 2020 Paul Boddie <[email protected]>
+ *
+ * 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)

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c line 74

Quoted 10 lines

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +10 -0

+{
+	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/[email protected]

Neil

From: Paul Boddie <[email protected]>

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 <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

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

drivers/gpu/drm/ingenic/Kconfig +0 -0

index 3b57f8be007c..4c7d311fbeff 100644
@@ -25,4 +25,13 @@ config DRM_INGENIC_IPU
 
 	  The Image Processing Unit (IPU) will appear as a second primary 

plane.

drivers/gpu/drm/ingenic/Kconfig +6 -0

+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.

drivers/gpu/drm/ingenic/Kconfig +3 -0

+	  If you want to enable HDMI on Ingenic JZ4780 based SoC, you should
+	  select this option..
+
 endif

drivers/gpu/drm/ingenic/Makefile +1 -0

index d313326bdddb..3db9888a6c04 100644
@@ -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

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +18 -0

new file mode 100644
index 000000000000..dd9c94ae842e
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2019, 2020 Paul Boddie <[email protected]>
+ *
+ * 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 } } },

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +1 -0

+	{ 92500000,  { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 

0x0005 } } },

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +1 -0

+	{ 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 

0x000a } } },

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +1 -0

+	{ 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 

0x000f } } },

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +1 -0

+	{ ~0UL,      { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, 

0x0000 } } }

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +32 -0

+};
+
+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. 

*/

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +12 -0

+	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);

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +1 -0

+	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | 

DRM_MODE_FLAG_NVSYNC);

drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file +69 -0

+};
+
+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

From: Paul Boddie <[email protected]>

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

Signed-off-by Paul Boddie <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 39 
+++++++++++++++++++++++
 1 file changed, 39 insertions(+)

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +4 -0

index e2df4b085905..605549b316b5 100644
@@ -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;

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +1 -0

+
 	mutex_init(&priv->clk_mutex);

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +3 -0

 	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,

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +0 -0

 	.formats_f1 = jz4740_formats,

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +0 -0

 	.max_width = 800,
 	.max_height = 600,

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +3 -0

 	.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,

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +4 -0

 	.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

Am 28.09.2021 um 11:58 schrieb Paul Cercueil <[email protected]>:

Hi,

Quoted 24 lines · H. Nikolaus Schaller

From: Paul Boddie <[email protected]>
The jz4780 has some features which need initialization according to the vendor kernel.

Signed-off-by Paul Boddie <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 39 +++++++++++++++++++++++

1 file changed, 39 insertions(+)

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +4 -0

index e2df4b085905..605549b316b5 100644
@@ -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;

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, · line 739

Quoted 4 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +2 -0

@@ -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.

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_bind(struct device *dev, bool has_components) · line 1331

Quoted 9 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +3 -0

+
 	/* 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.

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_bind(struct device *dev, bool has_components) · line 1341

Quoted 10 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +9 -0

+
+	/* 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.

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_bind(struct device *dev, bool has_components) · line 1348

Quoted 8 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +7 -0

+
+	/* 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().

Quoted 15 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +4 -0

+
 	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 = {

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static const struct jz_soc_info jz4725b_soc_info = { · line 1530

Quoted 6 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +3 -0

 	.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

Quoted 26 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +7 -1

 	.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

Am 28.09.2021 um 12:06 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 10 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static int ingenic_drm_bind(struct device *dev, bool has_components) · line 1348

Quoted 8 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +7 -0

+
+	/* 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.

Quoted 2 lines

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

Ok.