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

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

+	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 +0 -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),

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

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

Am 05.10.2021 um 22:22 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 17 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 +0 -0

index f73522bdacaa..e2df4b085905 100644
@@ -6,6 +6,7 @@

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, · line 694

Quoted 7 lines

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

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

Knowing that OSD mode doesn't really work with this patch, I doubt you need to configure per-plane alpha blending.

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, · line 697

Quoted 4 lines

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

+
+			hwdesc->dessize =
+				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |

Same here.

drivers/gpu/drm/ingenic/ingenic-drm-drv.c static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, · line 701

Quoted 4 lines

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

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

Better pre-shift your *_MASK macros (and use GENMASK() in them) and remove the *_OFFSET macros.

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

Quoted 9 lines

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

+		}
+

		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
			fourcc = newstate->fb->format->format;

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

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

I think you can remove this comment - this code doesn't actually set OSD mode, but it does enable the use of the extended hardware descriptor format, and I think it is already self-explanatory.

Quoted 17 lines

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

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

I remember saying to split this change 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);

This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values.

Quoted 25 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +6 -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 = {

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

Quoted 6 lines

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

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

From a quick look at the PMs, it doesn't seem so.

Quoted 17 lines

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

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

As I said in your v4... You don't need to add this here. The ingenic-dw-hdmi.c should take care of registering its driver.

As for the overall change... I am a bit annoyed that this only supports the F1 plane at the screen's resolution. Anything else (F1 plane at specific coordinates, F0 plane alone, or F0+F1) does not work.

I think at least the F1 plane's position should be easy to do (just setting the cpos field in the hwdesc).

It is OK to leave the rest for later (I'm not asking you to do all that). However it would be a good idea to add a check in ingenic_drm_crtc_atomic_check(), which would return -EINVAL if anything else than the working configuration is attempted.

Cheers,
-Paul

Hi Paul,
sorry for the delay in getting back to this, but I was distracted by more urgent topics.

Quoted 34 lines

Am 05.10.2021 um 22:22 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 20 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(-)

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

Quoted 7 lines

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

Quoted 2 lines

Knowing that OSD mode doesn't really work with this patch, I doubt
you need to configure per-plane alpha blending.

Well, we can not omit setting some CPOS_COEFFICIENT different from 0.

This would mean to multiply all values with 0, i.e. gives a black screen.

So at least we have to apply JZ_LCD_CPOS_COEFFICIENT_1. JZ_LCD_CPOS_PREMULTIPLY_LCD is not relevant in the non-alpha case.

But then, why not do it right from the beginning?

Quoted 7 lines

Quoted 4 lines

+
+ hwdesc->dessize =
+ (0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |

Same here.

Quoted 7 lines

Quoted 4 lines

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

Better pre-shift your *_MASK macros (and use GENMASK() in them) and
remove the *_OFFSET macros.

Ok, I see. Nice. Makes code and definitions much cleaner. Changed for v6.

Quoted 11 lines

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

Quoted 10 lines

+ }
+

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

 		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,

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

 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
 	}
 +	/* set use of the 8-word descriptor and OSD foreground usage. */

Quoted 2 lines

I think you can remove this comment - this code doesn't actually
set OSD mode, but it does enable the use of the extended hardware descriptor format, and I think it is already self-explanatory.

Agreed and removed.

Quoted 26 lines

Quoted 18 lines

+ if (priv->soc_info->use_extended_hwdesc)
+ cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
+

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

 	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)

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

 	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)

Review

Quoted 7 lines

(quoted diff) +0 -0

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

Quoted 1 line

I remember saying to split this change into its own patch :)

Yes, I remember as well, but it does not make sense to me.

A first patch would introduce regmap_config. This needs soc_info->max_reg to be defined as a struct component.

This requires all soc_info to be updated for all SoC (w/o jz4780_soc_info in this first patch because it has not been added yet) to a constant (!) JZ_REG_LCD_SIZE1.

And the second patch would then add jz4780_soc_info and set its max_reg to a different value.

IMHO, such a separate first patch has no benefit independent from adding jz4780 support, as far as I can see.

If your fear issues with bisectability:
- code has been tested
- if this fails, bisect will still point to this patch, where it is easy to locate

So I leave it in v6 unsplitted.

Quoted 11 lines

Quoted 10 lines

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

 	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)

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

 	/* 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);

Quoted 2 lines

This change is unrelated to this patch, and I'm not even sure it's
a valid change. The driver shouldn't rely on previous register values.

I think I already commented that I think the driver should also not reset previous register values to zero.

If I counted correctly this register has 18 bits which seem to include some interrupt masks (which could be initialized somewhere else) and we write a constant 0x1.

Of course most other bits are clearly OSD related (alpha blending), i.e. they can have any value (incl. 0) if OSD is disabled. But here we enable it. I think there may be missing some setting for the other bits.

So are you sure, that we can unconditionally reset *all* bits except JZ_LCD_OSDC_OSDEN for the jz4780?

Well I have no experience with OSD being enabled at all. I.e. I have no test scenario.

So we can leave it out in v6.

Quoted 35 lines

Quoted 24 lines

drivers/gpu/drm/ingenic/ingenic-drm-drv.c +0 -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 = {

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

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

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

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

(quoted diff) +0 -0

 	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
 	.formats_f0 = jz4770_formats_f0,
 	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),

Review

Quoted 10 lines

(quoted diff) +0 -0

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

Quoted 1 line

From a quick look at the PMs, it doesn't seem so.

Fine. I'll remove the comment in v6.

Quoted 25 lines

Quoted 14 lines

+ .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[] = {

(quoted diff) +0 -0

 	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
 	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info 

},

(quoted diff) +0 -0

 	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
 +	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
 	{ /* sentinel */ },

Review

Quoted 10 lines

(quoted diff) +0 -0

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

Quoted 2 lines

As I said in your v4... You don't need to add this here. The
ingenic-dw-hdmi.c should take care of registering its driver.

Well, I can not identify any difference in code structure to the IPU code.

The Makefile (after our patch) looks like:

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

which means that ingenic-dw-hdmi.o is also compiled into ingenic-drm, like ingenic-drm-drv.o and ingenic-ipu.o - if CONFIGured. If not, there are these IS_ENABLED() tests to guard against compiler errors.

Is there any technical reason to request a driver structure and registration different from IPU here?

Why not having ingenic-ipu.c taking care of registering its driver as well?

As soon as this is clarified, I can post a v6.

Quoted 3 lines

As for the overall change... I am a bit annoyed that this only
supports the F1 plane at the screen's resolution. Anything else (F1 plane at specific coordinates, F0 plane alone, or F0+F1) does not work.

Yes, having two planes working would be interesting.

Quoted 3 lines

I think at least the F1 plane's position should be easy to do (just
setting the cpos field in the hwdesc).

Quoted 2 lines

It is OK to leave the rest for later (I'm not asking you to do all
that). However it would be a good idea to add a check in ingenic_drm_crtc_atomic_check(), which would return -EINVAL if anything else than the working configuration is attempted.

Hm. I am not familiar with how ingenic_drm_crtc_atomic_check() would be notified about planes. Which configuration parameters should be checked for?

Quoted 4 lines

Cheers,
-Paul

BR and thanks,
Nikolaus

Am 07.11.2021 um 20:01 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 30 lines · H. Nikolaus Schaller

Hi Paul,
sorry for the delay in getting back to this, but I was distracted by more urgent topics.

Quoted 28 lines

Am 05.10.2021 um 22:22 schrieb Paul Cercueil <[email protected]>:
Hi Nikolaus,

Quoted 17 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 +0 -0

index f73522bdacaa..e2df4b085905 100644

drivers/gpu/drm/ingenic/ingenic-drm-drv.c line 12

Quoted 7 lines

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

@@ -6,6 +6,7 @@
 			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);

Knowing that OSD mode doesn't really work with this patch, I doubt you need to configure per-plane alpha blending.

Quoted 4 lines

Well, we can not omit setting some CPOS_COEFFICIENT different from 0. This would mean to multiply all values with 0, i.e. gives a black screen. So at least we have to apply JZ_LCD_CPOS_COEFFICIENT_1. JZ_LCD_CPOS_PREMULTIPLY_LCD is not relevant in the non-alpha case.

hwdesc->cpos = JZ_LCD_CPOS_COEFFICIENT_1 << JZ_LCD_CPOS_COEFFICIENT_OFFSET;

That's enough to get an image.

Quoted 2 lines

But then, why not do it right from the beginning?

Because there's no way to test alpha blending without getting the overlay plane to work first.

Quoted 7 lines

Quoted 7 lines

	}

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

+	regmap_config = ingenic_drm_regmap_config;
+	regmap_config.max_register = soc_info->max_reg;

	priv->map = devm_regmap_init_mmio(dev, base,

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

-					  &ingenic_drm_regmap_config);
+					  &regmap_config);

I remember saying to split this change into its own patch :)

Quoted 8 lines

Yes, I remember as well, but it does not make sense to me.
A first patch would introduce regmap_config. This needs soc_info->max_reg to be defined as a struct component.
This requires all soc_info to be updated for all SoC (w/o jz4780_soc_info in this first patch because it has not been added yet) to a constant (!) JZ_REG_LCD_SIZE1.
And the second patch would then add jz4780_soc_info and set its max_reg to a different value.

Correct, that's how it should be.

Note that you can do even better, set the .max_register field according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this.

Quoted 6 lines

IMHO, such a separate first patch has no benefit independent from adding jz4780 support, as far as I can see.
If your fear issues with bisectability:
- code has been tested
- if this fails, bisect will still point to this patch, where it is easy to locate

It's not about bisectability. One functional change per patch, and patches should be as atomic as possible.

Quoted 11 lines

So I leave it in v6 unsplitted.

Quoted 9 lines

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

Quoted 8 lines

	if (IS_ERR(priv->map)) {
		dev_err(dev, "Failed to create regmap\n");
		return PTR_ERR(priv->map);

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

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

This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values.

Quoted 2 lines

I think I already commented that I think the driver should also not reset previous register values to zero.

You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before.

Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch.

Quoted 12 lines

If I counted correctly this register has 18 bits which seem to include some interrupt masks (which could be initialized somewhere else) and we write a constant 0x1.
Of course most other bits are clearly OSD related (alpha blending), i.e. they can have any value (incl. 0) if OSD is disabled. But here we enable it. I think there may be missing some setting for the other bits. So are you sure, that we can unconditionally reset *all* bits except JZ_LCD_OSDC_OSDEN for the jz4780?
Well I have no experience with OSD being enabled at all. I.e. I have no test scenario.
So we can leave it out in v6.

Quoted 14 lines

Quoted 3 lines

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

Quoted 2 lines

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

+	}

As I said in your v4... You don't need to add this here. The ingenic-dw-hdmi.c should take care of registering its driver.

Well, I can not identify any difference in code structure to the IPU code. The Makefile (after our patch) looks like:
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 which means that ingenic-dw-hdmi.o is also compiled into ingenic-drm, like ingenic-drm-drv.o and ingenic-ipu.o - if CONFIGured. If not, there are these IS_ENABLED() tests to guard against compiler errors. Is there any technical reason to request a driver structure and registration different from IPU here?

There is no reason to have ingenic-dw-hdmi built into the ingenic-drm module. It should be a separate module.

Quoted 1 line

Why not having ingenic-ipu.c taking care of registering its driver as well?

IIRC ingenic-ipu.c was built into the ingenic-drm at the beginning because of circular dependencies between the IPU and main DRM driver. I think ingenic-ipu.c could be its own module now. That's something I will test soon.

Quoted 5 lines

As soon as this is clarified, I can post a v6.
Hm. I am not familiar with how ingenic_drm_crtc_atomic_check() would be notified about planes. Which configuration parameters should be checked for?

You know that the &ingenic_drm->f0 plane cannot be used (right now), so in ingenic_drm_plane_atomic_check() just:

if (plane == &priv->f0 && crtc)
return -EINVAL;

Hi Paul,

Quoted 49 lines

Am 07.11.2021 um 20:01 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 34 lines · H. Nikolaus Schaller

Hi Paul,
sorry for the delay in getting back to this, but I was distracted
by more urgent topics.

Quoted 31 lines

Am 05.10.2021 um 22:22 schrieb Paul Cercueil
<[email protected]>:
Hi Nikolaus,

Quoted 26 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(-)

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

Quoted 8 lines

Quoted 2 lines

Knowing that OSD mode doesn't really work with this patch, I
doubt you need to configure per-plane alpha blending.

Well, we can not omit setting some CPOS_COEFFICIENT different from
0.
This would mean to multiply all values with 0, i.e. gives a black
screen.
So at least we have to apply JZ_LCD_CPOS_COEFFICIENT_1. JZ_LCD_CPOS_PREMULTIPLY_LCD is not relevant in the non-alpha case.

Quoted 2 lines

hwdesc->cpos = JZ_LCD_CPOS_COEFFICIENT_1 <<
JZ_LCD_CPOS_COEFFICIENT_OFFSET;

Exactly what I wrote and did test.

Quoted 2 lines

That's enough to get an image.

Fine that we can agree on that.

Quoted 26 lines

Quoted 2 lines

But then, why not do it right from the beginning?

Because there's no way to test alpha blending without getting the
overlay plane to work first.

Quoted 7 lines

Review

Quoted 7 lines

(quoted diff) +0 -0

 	}
 +	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);
 I remember saying to split this change into its own patch :)

Quoted 12 lines

Yes, I remember as well, but it does not make sense to me. A first patch would introduce regmap_config. This needs
soc_info->max_reg
to be defined as a struct component.
This requires all soc_info to be updated for all SoC (w/o
jz4780_soc_info
in this first patch because it has not been added yet) to a
constant (!)
JZ_REG_LCD_SIZE1.
And the second patch would then add jz4780_soc_info and set its
max_reg to
a different value.

Quoted 1 line

Correct, that's how it should be.

Well, if you prefer separating things that are deeply related into two commits...

Quoted 1 line

Quoted 2 lines

Note that you can do even better, set the .max_register field
according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this.

That is an interesting idea. Although I don't see where

https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171

does make use of the memory resource from DTS. It just reads two values from the ingenic_chip_info instead of one I do read from soc_info.

If you see it I'd prefer to leave this patch to you (as it is not jz4780 related except that jz4780 needs it to be in place) and then I can simply make use of it for adding jz4780+hdmi.

Quoted 9 lines

Quoted 8 lines

IMHO, such a separate first patch has no benefit independent from
adding
jz4780 support, as far as I can see.
If your fear issues with bisectability:
- code has been tested
- if this fails, bisect will still point to this patch, where it
is easy to locate

Quoted 2 lines

It's not about bisectability. One functional change per patch, and
patches should be as atomic as possible.

Well, it was atomic: "add jz4780+hdmi functionality" or not. Now we separate into "preparation for adding jz4780" and "really adding". Yes, you can split atoms into quarks...

BTW: without adding jz4780_soc_info there is not even a functional change. Just the constant is made dependent on the .compatible entry. And since it is initialized to the same constant value in all cases, it is still a constant. A very very clever compiler could find out that regmap_config.max_register = soc_info->max_reg is a NOOP and produce the same code as before by avoiding the copy operation of regmap_config = ingenic_drm_regmap_config.

Quoted 18 lines

Quoted 12 lines

So I leave it in v6 unsplitted.

Quoted 10 lines

Quoted 10 lines

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

 	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)

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

 	/* 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);

Quoted 5 lines

Quoted 2 lines

This change is unrelated to this patch, and I'm not even sure
it's a valid change. The driver shouldn't rely on previous register values.

I think I already commented that I think the driver should also
not reset
previous register values to zero.

Quoted 5 lines

You did comment this, yes, but I don't agree. The driver *should*
reset the registers to zero. It should *not* have to rely on whatever was configured before.

Even if I did agree, this is a functional change unrelated to
JZ4780 support, so it would have to be splitted to its own patch.

Well it is in preparation of setting more bits that are only available for the jz4780.

But it will be splitted into its own patch for other reasons - if we ever make osd working...

Quoted 17 lines

Quoted 17 lines

If I counted correctly this register has 18 bits which seem to
include
some interrupt masks (which could be initialized somewhere else)
and we
write a constant 0x1.
Of course most other bits are clearly OSD related (alpha blending), i.e. they can have any value (incl. 0) if OSD is disabled. But
here we
enable it. I think there may be missing some setting for the other
bits.
So are you sure, that we can unconditionally reset *all* bits except JZ_LCD_OSDC_OSDEN for the jz4780?
Well I have no experience with OSD being enabled at all. I.e. I
have no
test scenario.
So we can leave it out in v6.

So we agree as here well.

Quoted 26 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

+ }

As I said in your v4... You don't need to add this here. The
ingenic-dw-hdmi.c should take care of registering its driver.

Quoted 15 lines

Well, I can not identify any difference in code structure to the
IPU code.
The Makefile (after our patch) looks like:
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 which means that ingenic-dw-hdmi.o is also compiled into
ingenic-drm,
like ingenic-drm-drv.o and ingenic-ipu.o - if CONFIGured. If not,
there
are these IS_ENABLED() tests to guard against compiler errors. Is there any technical reason to request a driver structure and
registration
different from IPU here?

There is no reason to have ingenic-dw-hdmi built into the
ingenic-drm module. It should be a separate module.

Quoted 2 lines

Why not having ingenic-ipu.c taking care of registering its driver
as well?

Quoted 2 lines

IIRC ingenic-ipu.c was built into the ingenic-drm at the beginning
because of circular dependencies between the IPU and main DRM driver. I think ingenic-ipu.c could be its own module now. That's something I will test soon.

Ok, that was the piece of information I was missing. I always thought that the way IPU is integrated is the best one and there is some special requirement. And it shows how we should do it.

So I'll wait until I see your proposal for IPU.

Quoted 11 lines

Quoted 5 lines

As soon as this is clarified, I can post a v6.
Hm. I am not familiar with how ingenic_drm_crtc_atomic_check() would be notified about planes. Which configuration parameters should be checked for?

You know that the &ingenic_drm->f0 plane cannot be used (right
now), so in ingenic_drm_plane_atomic_check() just:

if (plane == &priv->f0 && crtc)
return -EINVAL;

Ok, that is simple to add. Prepared for v6.

So v6 is to be postponed by the patch for setting up regmap_config.max_register and the separation of the IPU driver so that it does not interfere.

BR and thanks for all comments,
Nikolaus

Quoted 3 lines

Am 08.11.2021 um 10:37 schrieb Paul Cercueil <[email protected]>:

Well, it was atomic: "add jz4780+hdmi functionality" or not. Now we separate into "preparation for adding jz4780" and "really adding". Yes, you can split atoms into quarks...

And that's how it should be done. Lots of small atomic patches are much easier to review than a few big patches.

Quoted 4 lines

Quoted 1 line

Note that you can do even better, set the .max_register field according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this.

That is an interesting idea. Although I don't see where
https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171
does make use of the memory resource from DTS. It just reads two values from the ingenic_chip_info instead of one I do read from soc_info.

It overrides the .max_register from a static regmap_config instance.

You can do the same,

calculating the .max_register from the memory resource you get from DT.

I'm sure you guys can figure it out.

Hi Paul,

Quoted 3 lines

Quoted 3 lines

Am 08.11.2021 um 10:37 schrieb Paul Cercueil
<[email protected]>:

Quoted 5 lines

Quoted 2 lines

Well, it was atomic: "add jz4780+hdmi functionality" or not. Now
we separate into "preparation for adding jz4780" and "really adding". Yes, you can split atoms into quarks...

And that's how it should be done. Lots of small atomic patches are
much easier to review than a few big patches.

I doubt that in this case especially as it has nothing to do with jz4780...

But I have a proposal for a better solution at the end of this mail.

Quoted 9 lines

Quoted 7 lines

Quoted 2 lines

Note that you can do even better, set the .max_register field
according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this.

That is an interesting idea. Although I don't see where

https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171
does make use of the memory resource from DTS. It just reads two
values from the ingenic_chip_info instead of one I do read from soc_info.

It overrides the .max_register from a static regmap_config instance.

To be precise: it overrides .max_register of a copy of a static regmap_config instance (which has .max_register = 0).

Quoted 1 line

You can do the same,

We already do the same...

Quoted 2 lines

calculating the .max_register from the memory resource you get from
DT.

I can't see any code in pinctrl-ingenic.c getting the memory resource that from DT. It calculates it from the ingenic_chip_info tables inside the driver code but not DT.

Quoted 1 line

I'm sure you guys can figure it out.

Ah, we have to figure out. You are not sure yourself how to do it? And it is *not* exactly like the pinctrl driver (already) does? Please give precise directions in reviews and not vague research homework. Our time is also valuable. Sorry if I review your reviews...

Anyways I think you roughly intend (untested):

	struct resource *r;

	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	regmap_config.max_register = r.end - r.start;

But I wonder how that could work at all (despite adding code execution time) with:

e.g. jz4770.dtsi:

	lcd: lcd-controller@13050000 {
		compatible = "ingenic,jz4770-lcd";
		reg = <0x13050000 0x300>;

or jz4725b.dtsi:

	lcd: lcd-controller@13050000 {
		compatible = "ingenic,jz4725b-lcd";
		reg = <0x13050000 0x1000>;

So max_register becomes 0x300 or 0x1000 but not

#define JZ_REG_LCD_SIZE1	0x12c
	.max_reg = JZ_REG_LCD_SIZE1,

And therefore wastes a lot of regmap memory.

Do you want this? DTS should not be reduced (DTS should be kept as stable as possible), since the reg property describes address mapping
- not how many bytes are really used by registers or how big a cache
should be allocated (cache allocation size requirements are not hardware description).

But here are good news:

I have a simpler and less invasive proposal. We keep the devm_regmap_init_mmio code as is and just increase its .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. This wastes a handful bytes for all non-jz4780 chips but less than using the DTS memory region size. And is less code (no entry in soc_info tables, no modifyable copy) and faster code execution than all other proposals.

This is then just a single-line change when introducing the jz4780. And no "preparation for adding jz4780" patch is needed at all. No patch to split out for separate review.

Let's go this way to get it eventually finalized. Ok?

Am 08.11.2021 um 13:20 schrieb Paul Cercueil <[email protected]>:

Hi,

Quoted 5 lines · H. Nikolaus Schaller

Hi Paul,

Quoted 3 lines

Quoted 2 lines

Am 08.11.2021 um 10:37 schrieb Paul Cercueil <[email protected]>:
Well, it was atomic: "add jz4780+hdmi functionality" or not. Now we separate into "preparation for adding jz4780" and "really adding". Yes, you can split atoms into quarks...

And that's how it should be done. Lots of small atomic patches are much easier to review than a few big patches.

I doubt that in this case especially as it has nothing to do with jz4780...

It has nothing to do with JZ4780 and that's exactly why it should be a separate patch.

Quoted 10 lines

But I have a proposal for a better solution at the end of this mail.

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

Note that you can do even better, set the .max_register field according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this.

That is an interesting idea. Although I don't see where
https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171
does make use of the memory resource from DTS. It just reads two values from the ingenic_chip_info instead of one I do read from soc_info.

It overrides the .max_register from a static regmap_config instance.

To be precise: it overrides .max_register of a copy of a static regmap_config instance (which has .max_register = 0).

Quoted 1 line

You can do the same,

We already do the same...

Quoted 8 lines

Quoted 1 line

calculating the .max_register from the memory resource you get from DT.

I can't see any code in pinctrl-ingenic.c getting the memory resource that from DT. It calculates it from the ingenic_chip_info tables inside the driver code but not DT.

Quoted 1 line

I'm sure you guys can figure it out.

Ah, we have to figure out. You are not sure yourself how to do it? And it is *not* exactly like the pinctrl driver (already) does? Please give precise directions in reviews and not vague research homework. Our time is also valuable. Sorry if I review your reviews... Anyways I think you roughly intend (untested):

	struct resource *r;
	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	regmap_config.max_register = r.end - r.start;

Replace the "devm_platform_ioremap_resource" with "devm_platform_get_and_ioremap_resource" to get a pointer to the resource.

Then the .max_register should be (r.end - r.start - 4) I think.

And lose the aggressivity. It's not going to get you anywhere, especially since I'm the one who decides whether or not I should merge your patches. You want your code upstream, that's great, but it's your responsability to get it to shape so that it's eventually accepted.

Quoted 2 lines

But I wonder how that could work at all (despite adding code execution time) with:

Code execution time? ...

Quoted 12 lines

e.g. jz4770.dtsi:

	lcd: lcd-controller@13050000 {
		compatible = "ingenic,jz4770-lcd";
		reg = <0x13050000 0x300>;
or jz4725b.dtsi:
	lcd: lcd-controller@13050000 {
		compatible = "ingenic,jz4725b-lcd";
		reg = <0x13050000 0x1000>;
So max_register becomes 0x300 or 0x1000 but not
#define JZ_REG_LCD_SIZE1	0x12c
	.max_reg = JZ_REG_LCD_SIZE1,

Quoted 1 line

And therefore wastes a lot of regmap memory.

"regmap memory"? ...

Quoted 2 lines

Do you want this? DTS should not be reduced (DTS should be kept as stable as possible), since the reg property describes address mapping - not how many bytes are really used by registers or how big a cache should be allocated (cache allocation size requirements are not hardware description).

The DTS should list the address and size of the register area. If your last register is at address 0x12c and there's nothing above, then the size in DTS should be 0x130.

Quoted 5 lines

But here are good news:
I have a simpler and less invasive proposal. We keep the devm_regmap_init_mmio code as is and just increase its .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. This wastes a handful bytes for all non-jz4780 chips but less than using the DTS memory region size. And is less code (no entry in soc_info tables, no modifyable copy) and faster code execution than all other proposals. This is then just a single-line change when introducing the jz4780. And no "preparation for adding jz4780" patch is needed at all. No patch to split out for separate review.
Let's go this way to get it eventually finalized. Ok?

No.

Bnjour Paul,

Quoted 16 lines

Am 08.11.2021 um 13:20 schrieb Paul Cercueil <[email protected]>:

Hi,

Quoted 9 lines · H. Nikolaus Schaller

Hi Paul,

Quoted 6 lines

Quoted 4 lines

Am 08.11.2021 um 10:37 schrieb Paul Cercueil
<[email protected]>:
Well, it was atomic: "add jz4780+hdmi functionality" or not. Now
we separate into "preparation for adding jz4780" and "really adding". Yes, you can split atoms into quarks...

And that's how it should be done. Lots of small atomic patches
are much easier to review than a few big patches.

I doubt that in this case especially as it has nothing to do with
jz4780...

Quoted 2 lines

It has nothing to do with JZ4780 and that's exactly why it should
be a separate patch.

Question is why *I* should then make this patch and not someone else...

I am not necessarily a volunteer to contribute to non-jz4780 code just because I want to upstream jz4780 code.

Quoted 28 lines

Quoted 24 lines

But I have a proposal for a better solution at the end of this
mail.

Quoted 9 lines

Quoted 7 lines

Quoted 2 lines

Note that you can do even better, set the .max_register field
according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this.

That is an interesting idea. Although I don't see where

https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171
does make use of the memory resource from DTS. It just reads two
values from the ingenic_chip_info instead of one I do read from soc_info.

It overrides the .max_register from a static regmap_config
instance.

To be precise: it overrides .max_register of a copy of a static
regmap_config instance (which has .max_register = 0).

Quoted 1 line

You can do the same,

We already do the same...

Quoted 2 lines

calculating the .max_register from the memory resource you get
from DT.

I can't see any code in pinctrl-ingenic.c getting the memory
resource that from DT. It calculates it from the ingenic_chip_info tables inside the driver code but not DT.

Quoted 1 line

I'm sure you guys can figure it out.

Ah, we have to figure out. You are not sure yourself how to do it?
And it is *not* exactly like the pinctrl driver (already) does? Please give precise directions in reviews and not vague research homework. Our time is also valuable. Sorry if I review your reviews...
Anyways I think you roughly intend (untested):

Review

Quoted 3 lines

(quoted diff) +0 -0

 	struct resource *r;
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	regmap_config.max_register = r.end - r.start;

Quoted 7 lines

Replace the "devm_platform_ioremap_resource" with
"devm_platform_get_and_ioremap_resource" to get a pointer to the resource.

Then the .max_register should be (r.end - r.start - 4) I think.

And lose the aggressivity. It's not going to get you anywhere,
especially since I'm the one who decides whether or not I should merge your patches. You want your code upstream, that's great, but it's your responsability to get it to shape so that it's eventually accepted.

Well on the other side of the fence it is maintainers responsibility to give clear and understandable rules and guidance about what will be accepted and to enable us to bring it into the shape he/she has in mind.

But a fundamental problem is: "good shape" is very subjective. What would you recommend me to do, if I feel that my proposed code is in better shape than what the maintainer thinks or recommends?

Quoted 5 lines

Quoted 3 lines

But I wonder how that could work at all (despite adding code
execution time) with:

Code execution time? ...

I reasoned about doing an additional platform_get_resource() call and doing a subtraction. This is additional execution time. Maybe not worth thinking about because it is in the probe function. And using devm_platform_get_and_ioremap_resource() is of course potentially better.

Quoted 14 lines

Quoted 13 lines

e.g. jz4770.dtsi:

(quoted diff) +0 -0

 	lcd: lcd-controller@13050000 {
 		compatible = "ingenic,jz4770-lcd";
 		reg = <0x13050000 0x300>;
 or jz4725b.dtsi:
 	lcd: lcd-controller@13050000 {
 		compatible = "ingenic,jz4725b-lcd";
 		reg = <0x13050000 0x1000>;
 So max_register becomes 0x300 or 0x1000 but not
 #define JZ_REG_LCD_SIZE1	0x12c
 	.max_reg = JZ_REG_LCD_SIZE1,
 And therefore wastes a lot of regmap memory.

Quoted 1 line

"regmap memory"? ...

regmap allocates memory for its cache. Usually the total amount specified in the reg property.

Quoted 6 lines

Quoted 3 lines

Do you want this? DTS should not be reduced (DTS should be kept as
stable as possible), since the reg property describes address mapping - not how many bytes are really used by registers or how big a cache should be allocated (cache allocation size requirements are not hardware description).

The DTS should list the address and size of the register area. If
your last register is at address 0x12c and there's nothing above, then the size in DTS should be 0x130.

If I look into some .dtsi it is sometimes that way but sometimes not. There seems to be no consistent rule.

So does this mean you allow me to modify jz4740.dtsi, jz4770.dtsi and jz4725b.dtsi as well (as mentioned above: this is beyond the scope of my project)?

Quoted 9 lines

Quoted 7 lines

But here are good news:
I have a simpler and less invasive proposal. We keep the
devm_regmap_init_mmio code as is and just increase its .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. This wastes a handful bytes for all non-jz4780 chips but less than using the DTS memory region size. And is less code (no entry in soc_info tables, no modifyable copy) and faster code execution than all other proposals.
This is then just a single-line change when introducing the
jz4780. And no "preparation for adding jz4780" patch is needed at all. No patch to split out for separate review.
Let's go this way to get it eventually finalized. Ok?

No.

Look friend, if you explain your "no" and what is wrong with my arguments, it helps to understand your decisions and learn something from them. A plain "no" does not help anyone.

So to summarize: if you prefer something which I consider worse, it is ok for me... In the end you are right - you are the maintainer, not me. So you have to live with your proposals.

Therefore, I have prepared new variants so you can choose which one is easier to maintain for you.

Note that they are both preparing for full jz4780-lcdc/hdmi support but in very different ways:

Variant 1 already adds some jz4780 stuff while Variant 2 just prepares for it.

Variant 2 is not tested (except to compile). So it needs some Tested-by: from someone with access to hardware. IMHO it is more invasive.

And don't forget: DTB could be in ROM or be provided by a separate bootloader... So we should not change it too often (I had such discussions some years ago with maintainers when I thought it is easier to change DTS instead of code).

Variant 3 would be to not separate this. As proposed in [PATCH v5 2/7]. (Finally, a Variant 3b would be to combine the simple change from Variant 1 with Variant 3).

So what is your choice?

BR and thanks,
Nikolaus

#### VARIANT 0001 ####

From c7afa925f6b53bb6cafa080b7dd582788c8eb2eb Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Mon, 8 Nov 2021 15:38:21 +0100
Subject: [PATCH] RFC: drm/ingenic: Add register definitions for JZ4780 lcdc

JZ4780 has additional registers compared to the other SoC of the JZ47xx series. So we add the constants for registers and bits we make use of (there are even more which can be added later).

And we increase the regmap max_register accordingly.

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

---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c |  2 +-
 drivers/gpu/drm/ingenic/ingenic-drm.h     | 39 
+++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

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

index a5df1c8d34cde..1903e897d2299 100644
@@ -122,7 +122,7 @@ static const struct regmap_config 
 ingenic_drm_regmap_config = {
 	.val_bits = 32,
 	.reg_stride = 4,
 
-	.max_register = JZ_REG_LCD_SIZE1,
+	.max_register = JZ_REG_LCD_PCFG,
 	.writeable_reg = ingenic_drm_writeable_reg,
 };

drivers/gpu/drm/ingenic/ingenic-drm.h +39 -1

index 22654ac1dde1c..e7430c4af41f6 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,39 @@
 #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_MASK		GENMASK(23, 12)
+#define JZ_LCD_DESSIZE_WIDTH_MASK		GENMASK(11, 0)
+
+/* 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;
--

2.33.0

#### VARIANT 0002 ####

From c4b5cfa2789493f02da91e385719bc97aefb6c6c Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Mon, 8 Nov 2021 14:40:58 +0100
Subject: [PATCH] RFC: drm/ingenic: prepare ingenic drm for later addition of
JZ4780

This changes the way the regmap is allocated to prepare for the later addition of the JZ4780 which has more registers and bits than the others.

To make this work we have to change the device tree records of all devices so that the reg property is limited to the physically available registers.

The magic value 0x130 in the device tree is JZ_REG_LCD_SIZE1 + 4.

Note that it is not tested since I have no access to the hardware.

Suggested-by Paul Cercueil <[email protected]>

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

---
 arch/mips/boot/dts/ingenic/jz4725b.dtsi   | 2 +-
 arch/mips/boot/dts/ingenic/jz4740.dtsi    | 2 +-
 arch/mips/boot/dts/ingenic/jz4770.dtsi    | 2 +-
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 9 ++++++---
 4 files changed, 9 insertions(+), 6 deletions(-)

arch/mips/boot/dts/ingenic/jz4725b.dtsi +1 -1

index a1f0b71c92237..c017b087c0e52 100644
@@ -321,7 +321,7 @@ udc: usb@13040000 {
 
 	lcd: lcd-controller@13050000 {
 		compatible = "ingenic,jz4725b-lcd";
-		reg = <0x13050000 0x1000>;
+		reg = <0x13050000 0x130>;
 
 		interrupt-parent = <&intc>;
 		interrupts = <31>;

arch/mips/boot/dts/ingenic/jz4740.dtsi +1 -1

index c1afdfdaa8a38..ce3689e5015b5 100644
@@ -323,7 +323,7 @@ udc: usb@13040000 {
 
 	lcd: lcd-controller@13050000 {
 		compatible = "ingenic,jz4740-lcd";
-		reg = <0x13050000 0x1000>;
+		reg = <0x13050000 0x130>;
 
 		interrupt-parent = <&intc>;
 		interrupts = <30>;

arch/mips/boot/dts/ingenic/jz4770.dtsi +1 -1

index 05c00b93088e9..0d1ee58d6c40b 100644
@@ -399,7 +399,7 @@ gpu: gpu@13040000 {
 
 	lcd: lcd-controller@13050000 {
 		compatible = "ingenic,jz4770-lcd";
-		reg = <0x13050000 0x300>;
+		reg = <0x13050000 0x130>;
 
 		interrupt-parent = <&intc>;
 		interrupts = <31>;

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

index a5df1c8d34cde..3c8e3c5a447bb 100644
@@ -122,7 +122,6 @@ static const struct regmap_config 
 ingenic_drm_regmap_config = {
 	.val_bits = 32,
 	.reg_stride = 4,
 
-	.max_register = JZ_REG_LCD_SIZE1,
 	.writeable_reg = ingenic_drm_writeable_reg,
 };

@@ -858,6 +857,8 @@ static int ingenic_drm_bind(struct device *dev, 
 bool has_components)
 	struct drm_encoder *encoder;
 	struct drm_device *drm;
 	void __iomem *base;
+	struct resource *res;
+	struct regmap_config regmap_config;
 	long parent_rate;
 	unsigned int i, clone_mask = 0;

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

 	dma_addr_t dma_hwdesc_phys_f0, dma_hwdesc_phys_f1;

@@ -904,14 +905,16 @@ static int ingenic_drm_bind(struct device *dev, 
 bool has_components)
 	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
 	drm->mode_config.helper_private = &ingenic_drm_mode_config_helpers;
 
-	base = devm_platform_ioremap_resource(pdev, 0);
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(base)) {
 		dev_err(dev, "Failed to get memory resource\n");
 		return PTR_ERR(base);
 	}
 
+	regmap_config = ingenic_drm_regmap_config;
+	regmap_config.max_register = res->end - res->start - 4;
 	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);
--
2.33.0

Am 08.11.2021 um 17:30 schrieb Paul Cercueil <[email protected]>:

Hi,

Quoted 15 lines · H. Nikolaus Schaller

Bnjour Paul,

Quoted 14 lines

Am 08.11.2021 um 13:20 schrieb Paul Cercueil <[email protected]>:
Hi,

Quoted 12 lines

e.g. jz4770.dtsi:

	lcd: lcd-controller@13050000 {
		compatible = "ingenic,jz4770-lcd";
		reg = <0x13050000 0x300>;
or jz4725b.dtsi:
	lcd: lcd-controller@13050000 {
		compatible = "ingenic,jz4725b-lcd";
		reg = <0x13050000 0x1000>;
So max_register becomes 0x300 or 0x1000 but not
#define JZ_REG_LCD_SIZE1	0x12c
	.max_reg = JZ_REG_LCD_SIZE1,

And therefore wastes a lot of regmap memory.

Quoted 2 lines

Quoted 1 line

"regmap memory"? ...

regmap allocates memory for its cache. Usually the total amount specified in the reg property.

We are not using any register cache here.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

Do you want this? DTS should not be reduced (DTS should be kept as stable as possible), since the reg property describes address mapping - not how many bytes are really used by registers or how big a cache should be allocated (cache allocation size requirements are not hardware description).

The DTS should list the address and size of the register area. If your last register is at address 0x12c and there's nothing above, then the size in DTS should be 0x130.

If I look into some .dtsi it is sometimes that way but sometimes not. There seems to be no consistent rule.
So does this mean you allow me to modify jz4740.dtsi, jz4770.dtsi and jz4725b.dtsi as well (as mentioned above: this is beyond the scope of my project)?

You could update them if you wanted to, but there is no need to do it here.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

But here are good news:
I have a simpler and less invasive proposal. We keep the devm_regmap_init_mmio code as is and just increase its .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. This wastes a handful bytes for all non-jz4780 chips but less than using the DTS memory region size. And is less code (no entry in soc_info tables, no modifyable copy) and faster code execution than all other proposals. This is then just a single-line change when introducing the jz4780. And no "preparation for adding jz4780" patch is needed at all. No patch to split out for separate review.
Let's go this way to get it eventually finalized. Ok?

Quoted 2 lines

Quoted 1 line

No.

Look friend, if you explain your "no" and what is wrong with my arguments, it helps to understand your decisions and learn something from them. A plain "no" does not help anyone.

I answered just "no" because I felt like I explained already what I wanted to see in the previous email.

By using a huge number as the .max_register, we do *not* waste additional memory. Computing the value of the .max_register field does not add any overhead, either.

The .max_register is only used for boundary checking. To make sure that you're not calling regmap_write() with an invalid register. That's all there is to it.

Quoted 10 lines

So to summarize: if you prefer something which I consider worse, it is ok for me... In the end you are right - you are the maintainer, not me. So you have to live with your proposals.
Therefore, I have prepared new variants so you can choose which one is easier to maintain for you.
Note that they are both preparing for full jz4780-lcdc/hdmi support but in very different ways:
Variant 1 already adds some jz4780 stuff while Variant 2 just prepares for it.
Variant 2 is not tested (except to compile). So it needs some Tested-by: from someone with access to hardware. IMHO it is more invasive.
And don't forget: DTB could be in ROM or be provided by a separate bootloader... So we should not change it too often (I had such discussions some years ago with maintainers when I thought it is easier to change DTS instead of code). Variant 3 would be to not separate this. As proposed in [PATCH v5 2/7].
(Finally, a Variant 3b would be to combine the simple change from Variant 1 with Variant 3).
So what is your choice?

Variant 4: the variant #2 without the changes to the DTSI files.

Hi Paul,

Quoted 32 lines

Am 08.11.2021 um 17:30 schrieb Paul Cercueil <[email protected]>:

Hi,

Quoted 16 lines · H. Nikolaus Schaller

Bnjour Paul,

Quoted 15 lines

Am 08.11.2021 um 13:20 schrieb Paul Cercueil
<[email protected]>:
Hi,

Quoted 12 lines

e.g. jz4770.dtsi:

(quoted diff) +0 -0

 	lcd: lcd-controller@13050000 {
 		compatible = "ingenic,jz4770-lcd";
 		reg = <0x13050000 0x300>;
 or jz4725b.dtsi:
 	lcd: lcd-controller@13050000 {
 		compatible = "ingenic,jz4725b-lcd";
 		reg = <0x13050000 0x1000>;
 So max_register becomes 0x300 or 0x1000 but not
 #define JZ_REG_LCD_SIZE1	0x12c
 	.max_reg = JZ_REG_LCD_SIZE1,
 And therefore wastes a lot of regmap memory.

Quoted 3 lines

Quoted 1 line

"regmap memory"? ...

regmap allocates memory for its cache. Usually the total amount
specified in the reg property.

We are not using any register cache here.

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Do you want this? DTS should not be reduced (DTS should be kept
as stable as possible), since the reg property describes address mapping - not how many bytes are really used by registers or how big a cache should be allocated (cache allocation size requirements are not hardware description).

The DTS should list the address and size of the register area. If
your last register is at address 0x12c and there's nothing above, then the size in DTS should be 0x130.

Quoted 7 lines

Quoted 4 lines

If I look into some .dtsi it is sometimes that way but sometimes
not. There seems to be no consistent rule.
So does this mean you allow me to modify jz4740.dtsi, jz4770.dtsi
and jz4725b.dtsi as well (as mentioned above: this is beyond the scope of my project)?

You could update them if you wanted to, but there is no need to do
it here.

Hm. Then we are changing the .max_register initialization to a much bigger value.

Quoted 11 lines

Quoted 10 lines

Quoted 8 lines

Quoted 7 lines

But here are good news:
I have a simpler and less invasive proposal. We keep the
devm_regmap_init_mmio code as is and just increase its .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. This wastes a handful bytes for all non-jz4780 chips but less than using the DTS memory region size. And is less code (no entry in soc_info tables, no modifyable copy) and faster code execution than all other proposals.
This is then just a single-line change when introducing the
jz4780. And no "preparation for adding jz4780" patch is needed at all. No patch to split out for separate review.
Let's go this way to get it eventually finalized. Ok?

No.

Look friend, if you explain your "no" and what is wrong with my
arguments, it helps to understand your decisions and learn something from them. A plain "no" does not help anyone.

Quoted 8 lines

I answered just "no" because I felt like I explained already what I
wanted to see in the previous email.

By using a huge number as the .max_register, we do *not* waste
additional memory. Computing the value of the .max_register field does not add any overhead, either.

The .max_register is only used for boundary checking. To make sure
that you're not calling regmap_write() with an invalid register. That's all there is to it.

Ah, now I understand our disconnect. So far I have used regmaps mainly for i2c devices and there is caching to avoid redundant i2c traffic...

So I just assumed wrongly that the regmap driver also allocates some buffer/cache here. Although it does not initialize .cache_type (default: REGCACHE_NONE).

Quoted 19 lines

Quoted 18 lines

So to summarize: if you prefer something which I consider worse,
it is ok for me... In the end you are right - you are the maintainer, not me. So you have to live with your proposals.
Therefore, I have prepared new variants so you can choose which
one is easier to maintain for you.
Note that they are both preparing for full jz4780-lcdc/hdmi
support but in very different ways:
Variant 1 already adds some jz4780 stuff while Variant 2 just
prepares for it.
Variant 2 is not tested (except to compile). So it needs some
Tested-by: from someone with access to hardware. IMHO it is more invasive.
And don't forget: DTB could be in ROM or be provided by a separate
bootloader... So we should not change it too often (I had such discussions some years ago with maintainers when I thought it is easier to change DTS instead of code).
Variant 3 would be to not separate this. As proposed in [PATCH v5
2/7].
(Finally, a Variant 3b would be to combine the simple change from
Variant 1 with Variant 3).
So what is your choice?

Quoted 1 line

Variant 4: the variant #2 without the changes to the DTSI files.

Hm. If there is no cache and we can safely remove tight boundary checking (by JZ_REG_LCD_SIZE1) for jz4725/40/70 (by not fixing DTSI) why do we still need the max_register calculation from DTSI specifically for jz4780 and at all?

So what about:

Variant 5: set .max_register = 0x1800, i.e. "big enough for everyone" (includes z4780 gamma and vee registers) + no DTSI changes (+ no jz4780 register constants like Variant 1)

+ no DTSI changes
+ no calculation from DTSI needed
+ single separate patch to prepare for jz4780 but not included in jz4780 patch

BR and thanks,
Nikolaus

Am 08.11.2021 um 18:49 schrieb Paul Cercueil <[email protected]>:

Quoted 2 lines

Quoted 1 line

Variant 4: the variant #2 without the changes to the DTSI files.

Hm. If there is no cache and we can safely remove tight boundary checking (by JZ_REG_LCD_SIZE1) for jz4725/40/70 (by not fixing DTSI) why do we still need the max_register calculation from DTSI specifically for jz4780 and at all?

It's better to have the .max_register actually set to the proper value. Then reading the registers from debugfs (/sys/kernel/debug/regmap/) will print the actual list of registers without bogus values. If .max_register is set too high, it will end up reading outside the registers area.

On Ingenic SoCs such reads just return 0, but on some other SoCs it can lock up the system.

So the best way forward is to have .max_register computed from the register area's size, and fix the DTSI with the proper sizes. Since your JZ4780 code needs to update .max_register anyway it's a good moment to add this patch, and the DTSI files can be fixed later (by me or whoever is up to the task).

Fixing the DTS is not a problem in any way, btw. We just need to ensure that the drivers still work with old DTB files, which will be the case here.

Hi Paul,

Quoted 8 lines

Am 08.11.2021 um 18:49 schrieb Paul Cercueil <[email protected]>:

Quoted 3 lines

Quoted 1 line

Variant 4: the variant #2 without the changes to the DTSI files.

Hm. If there is no cache and we can safely remove tight boundary
checking (by JZ_REG_LCD_SIZE1) for jz4725/40/70 (by not fixing DTSI) why do we still need the max_register calculation from DTSI specifically for jz4780 and at all?

It's better to have the .max_register actually set to the proper
value. Then reading the registers from debugfs (/sys/kernel/debug/regmap/) will print the actual list of registers without bogus values. If .max_register is set too high, it will end up reading outside the registers area.

Ok, that is a good reason to convince me.

Quoted 2 lines

On Ingenic SoCs such reads just return 0, but on some other SoCs it
can lock up the system.

Yes, I know some of these...

Quoted 2 lines

So the best way forward is to have .max_register computed from the
register area's size, and fix the DTSI with the proper sizes. Since your JZ4780 code needs to update .max_register anyway it's a good moment to add this patch, and the DTSI files can be fixed later (by me or whoever is up to the task).

Well, it would already be part of my Variant #2 (untested). So I could simply split it up further and you can test the pure dtsi changes and apply them later or modify if that makes problems. Saves you a little work. BTW: the jz4740 seems to have even less registers (last register seems to be LCDCMD1 @ 0x1305005C).

Quoted 3 lines

Fixing the DTS is not a problem in any way, btw. We just need to
ensure that the drivers still work with old DTB files, which will be the case here.

Yes, that is right since the new values are smaller than the originals.

Ok, then let's do it that way.

Am 08.11.2021 um 10:37 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 24 lines · H. Nikolaus Schaller

Hi Paul,

Quoted 11 lines

Quoted 9 lines

Quoted 7 lines

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

Quoted 6 lines

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

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

This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values.

I think I already commented that I think the driver should also not reset previous register values to zero.

You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before. Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch.

Well it is in preparation of setting more bits that are only available for the jz4780. But it will be splitted into its own patch for other reasons - if we ever make osd working...

Quoted 10 lines

Quoted 10 lines

If I counted correctly this register has 18 bits which seem to include some interrupt masks (which could be initialized somewhere else) and we write a constant 0x1.
Of course most other bits are clearly OSD related (alpha blending), i.e. they can have any value (incl. 0) if OSD is disabled. But here we enable it. I think there may be missing some setting for the other bits. So are you sure, that we can unconditionally reset *all* bits except JZ_LCD_OSDC_OSDEN for the jz4780?
Well I have no experience with OSD being enabled at all. I.e. I have no test scenario.

Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 29 lines

Am 08.11.2021 um 10:37 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 24 lines · H. Nikolaus Schaller

Hi Paul,

Quoted 11 lines

Quoted 9 lines

Quoted 7 lines

Review static int ingenic_drm_bind(struct device *dev, bool has_components) · line 1321

Quoted 6 lines

(quoted diff) +1 -1

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

This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values.

I think I already commented that I think the driver should also not reset previous register values to zero.

You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before. Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch.

Well it is in preparation of setting more bits that are only available for the jz4780. But it will be splitted into its own patch for other reasons - if we ever make osd working...

Quoted 10 lines

Quoted 10 lines

If I counted correctly this register has 18 bits which seem to include some interrupt masks (which could be initialized somewhere else) and we write a constant 0x1.
Of course most other bits are clearly OSD related (alpha blending), i.e. they can have any value (incl. 0) if OSD is disabled. But here we enable it. I think there may be missing some setting for the other bits. So are you sure, that we can unconditionally reset *all* bits except JZ_LCD_OSDC_OSDEN for the jz4780?
Well I have no experience with OSD being enabled at all. I.e. I have no test scenario.

It turns out that the line

		ret = clk_prepare_enable(priv->lcd_clk);

initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0 before).

and writing

		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);

overwrites it with 0x00000001.

This makes the HDMI monitor go/stay black until I write manually 0x5 to JZ_REG_LCD_OSDC.

This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as well. Hence overwriting just with JZ_LCD_OSDC_OSDEN breaks it.

Now the questions:
a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why is it needed? Is this a not well documented difference between jz4740/60/70/80?
b) how can clk_prepare_enable() write 0x00010005 to JZ_REG_LCD_OSDC? Bug or feature? Something in cgu driver going wrong?

c) what do your SoC or panels do if you write 0x5 to JZ_REG_LCD_OSDC?
d) 0x00010005 also has bit 16 set which is undocumented... But this is a don't care according to jz4780 PM.

Hi Paul,
any insights on the JZ_REG_LCD_OSDC issue below?

There is a second, unrelated issue with the introduction of

"drm/bridge: display-connector: implement bus fmts callbacks"

which breaks bus format negotiations.

These are the two last unsolved issues to submit a fully working driver.

Quoted 50 lines

Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller
<[email protected]>:

Quoted 46 lines

Am 08.11.2021 um 10:37 schrieb Paul Cercueil
<[email protected]>:

Hi Nikolaus,

Quoted 39 lines · H. Nikolaus Schaller

Hi Paul,

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 9 lines

@@ -1274,7 +1319,7 @@ static int ingenic_drm_bind(struct
device *dev, bool has_components)

(quoted diff) +0 -0

 	/* 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);

This change is unrelated to this patch, and I'm not even sure
it's a valid change. The driver shouldn't rely on previous register values.

Quoted 7 lines

Quoted 3 lines

I think I already commented that I think the driver should also
not reset
previous register values to zero.

You did comment this, yes, but I don't agree. The driver
*should* reset the registers to zero. It should *not* have to rely on whatever was configured before.
Even if I did agree, this is a functional change unrelated to
JZ4780 support, so it would have to be splitted to its own patch.

Well it is in preparation of setting more bits that are only
available for the jz4780.
But it will be splitted into its own patch for other reasons - if
we ever make osd working...

Quoted 16 lines

Quoted 16 lines

If I counted correctly this register has 18 bits which seem to
include
some interrupt masks (which could be initialized somewhere
else) and we
write a constant 0x1.
Of course most other bits are clearly OSD related (alpha
blending),
i.e. they can have any value (incl. 0) if OSD is disabled. But
here we
enable it. I think there may be missing some setting for the
other bits.
So are you sure, that we can unconditionally reset *all* bits except JZ_LCD_OSDC_OSDEN for the jz4780?
Well I have no experience with OSD being enabled at all. I.e. I
have no
test scenario.

Quoted 6 lines

It turns out that the line

ret = clk_prepare_enable(priv->lcd_clk);

initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0
before).

more detailled test shows that it is the underlying

	clk_enable(priv->lcd_clk)

(i.e. not the prepare).

Quoted 15 lines

and writing

regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);

overwrites it with 0x00000001.

This makes the HDMI monitor go/stay black until I write manually
0x5 to
JZ_REG_LCD_OSDC.

This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as
well.
Hence overwriting just with JZ_LCD_OSDC_OSDEN breaks it.

Quoted 7 lines

Now the questions:
a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why
is it needed?
Is this a not well documented difference between jz4740/60/70/80?
b) how can clk_prepare_enable() write 0x00010005 to
JZ_REG_LCD_OSDC? Bug or feature?
Something in cgu driver going wrong?

I now suspect that it is an undocumented SoC feature.

Quoted 6 lines

c) what do your SoC or panels do if you write 0x5 to
JZ_REG_LCD_OSDC?
d) 0x00010005 also has bit 16 set which is undocumented... But this
is a don't care
according to jz4780 PM.

BR and thanks,
Nikolaus

Am 18.01.2022 um 17:58 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 2 lines · H. Nikolaus Schaller

Hi Paul,
any insights on the JZ_REG_LCD_OSDC issue below?

Sorry, I missed your previous email. I blame the holidays ;)

Quoted 50 lines

There is a second, unrelated issue with the introduction of "drm/bridge: display-connector: implement bus fmts callbacks" which breaks bus format negotiations.
These are the two last unsolved issues to submit a fully working driver.

Quoted 27 lines

Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 26 lines

Am 08.11.2021 um 10:37 schrieb Paul Cercueil <[email protected]>:
Hi Nikolaus,

Quoted 23 lines · H. Nikolaus Schaller

Hi Paul,

Quoted 10 lines

Quoted 8 lines

Quoted 6 lines

Review static int ingenic_drm_bind(struct device *dev, bool has_components) · line 1321

Quoted 5 lines

(quoted diff) +1 -1

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

This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values.

I think I already commented that I think the driver should also not reset previous register values to zero.

You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before. Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch.

Well it is in preparation of setting more bits that are only available for the jz4780. But it will be splitted into its own patch for other reasons - if we ever make osd working...

Quoted 10 lines

Quoted 10 lines

If I counted correctly this register has 18 bits which seem to include some interrupt masks (which could be initialized somewhere else) and we write a constant 0x1.
Of course most other bits are clearly OSD related (alpha blending), i.e. they can have any value (incl. 0) if OSD is disabled. But here we enable it. I think there may be missing some setting for the other bits. So are you sure, that we can unconditionally reset *all* bits except JZ_LCD_OSDC_OSDEN for the jz4780?
Well I have no experience with OSD being enabled at all. I.e. I have no test scenario.

Quoted 3 lines

It turns out that the line

		ret = clk_prepare_enable(priv->lcd_clk);

initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0 before).

more detailled test shows that it is the underlying

	clk_enable(priv->lcd_clk)

(i.e. not the prepare).

Quoted 12 lines

and writing

		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);

overwrites it with 0x00000001.
This makes the HDMI monitor go/stay black until I write manually 0x5 to JZ_REG_LCD_OSDC.
This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as well. Hence overwriting just with JZ_LCD_OSDC_OSDEN breaks it.
Now the questions:
a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why is it needed? Is this a not well documented difference between jz4740/60/70/80?
b) how can clk_prepare_enable() write 0x00010005 to JZ_REG_LCD_OSDC? Bug or feature? Something in cgu driver going wrong?

Quoted 1 line

I now suspect that it is an undocumented SoC feature.

Not at all. If the clock is disabled, the LCD controller is disabled, so all the registers read zero, this makes sense. You can only read the registers when the clock is enabled. On some SoCs, reading disabled registers can even cause a complete lockup.

Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working fine last time I tried, and now I indeed get a black screen unless this bit is set. The PM doesn't make it obvious that the bit is required,

but that wouldn't be surprising.

Anyway, feel free to send a patch to fix it (with a Fixes: tag). Ideally something like this:

u32 osdc = 0;
...
if (soc_info->has_osd)
osdc |= JZ_LCD_OSDC_OSDEN;
if (soc_info->has_alpha)
osdc |= JZ_LCD_OSDC_ALPHAEN;
regmap_write(priv->map, JZ_REG_LCD_OSDC, osdc);

This also ensures that the OSDC register is properly initialized in the !has_osd case. The driver shouldn't rely on pre-boot values in the registers.

Not at all. If the clock is disabled, the LCD controller is disabled, so all the registers read zero, this makes sense. You can only read the registers when the clock is enabled. On some SoCs, reading disabled registers can even cause a complete lockup.

Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working fine last time I tried, and now I indeed get a black screen unless this bit is set. The PM doesn't make it obvious that the bit is required, but that wouldn't be surprising.

Am 18.01.2022 um 23:59 schrieb Paul Boddie <[email protected]>:

Quoted 5 lines · On Tuesday, 18 January 2022 17:58:58 CET Paul Cercueil wrote:

Not at all. If the clock is disabled, the LCD controller is disabled, so all the registers read zero, this makes sense. You can only read the registers when the clock is enabled. On some SoCs, reading disabled registers can even cause a complete lockup.

My concern was that something might be accessing the registers before the clock had been enabled. It seems unlikely, given that the clock is enabled in the bind function, and I would have thought that nothing would invoke the different driver operations ("funcs") until bind has been called, nor should anything called from within bind itself be accessing registers.

Quoted 4 lines

Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working fine last time I tried, and now I indeed get a black screen unless this bit is set. The PM doesn't make it obvious that the bit is required, but that wouldn't be surprising.

It isn't actually needed. If the DMA descriptors are set up appropriately, the OSD alpha bit seems to be set as a consequence. In my non-Linux testing environment I don't even set any OSD registers explicitly, but the OSD alpha and enable flags become set when the display is active.

Hi Paul,

Quoted 9 lines

Am 18.01.2022 um 23:59 schrieb Paul Boddie <[email protected]>:

Quoted 5 lines · On Tuesday, 18 January 2022 17:58:58 CET Paul Cercueil wrote:

Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working fine last time I tried, and now I indeed get a black screen unless this bit is set. The PM doesn't make it obvious that the bit is required, but that wouldn't be surprising.

Quoted 4 lines

It isn't actually needed. If the DMA descriptors are set up appropriately, the OSD alpha bit seems to be set as a consequence. In my non-Linux testing environment I don't even set any OSD registers explicitly, but the OSD alpha and enable flags become set when the display is active.

Is it set by DMA descriptors or by explicit code?

We did have an explicit setting of JZ_LCD_OSDC_ALPHAEN

https://www.spinics.net/lists/devicetree/msg438447.html

but that was postponed for further discussion. And now if we add it (from basic functionality) back, it is fine again.

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 | 79 
+++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 

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

(quoted diff) +0 -0

new file mode 100644
index 000000000000..5bcb342da86f

b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

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

@@ -0,0 +1,79 @@
+# 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]>
+
+description: |
+  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys 

DesignWare HDMI 1.4
+ TX controller IP with accompanying PHY IP.

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

+
+allOf:
+  - $ref: bridge/synopsys,dw-hdmi.yaml#
+
+properties:
+  compatible:
+    const: ingenic,jz4780-dw-hdmi
+
+  reg-io-width:
+    const: 4
+
+  clocks:
+    maxItems: 2
+
+  hdmi-5v-supply:
+    description: Optional regulator to provide +5V at the connector
+
+  ddc-i2c-bus:
+    description: An I2C interface if the internal DDC I2C driver is 

not to be used

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

+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+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_AHB0>, <&cgu JZ4780_CLK_HDMI>;
+        clock-names = "iahb", "isfr";
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            hdmi_in: port@0 {
+                reg = <0>;
+                dw_hdmi_in: endpoint {
+                    remote-endpoint = <&jz4780_lcd_out>;
+                };
+            };
+            hdmi_out: port@1 {
+                reg = <1>;
+                dw_hdmi_out: endpoint {
+                    remote-endpoint = <&hdmi_con>;
+                };
+            };
+        };
+    };
+
+...
--

2.33.0

Am 05.10.2021 um 22:43 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 26 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 | 79 +++++++++++++++++++

1 file changed, 79 insertions(+)

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

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

new file mode 100644
index 000000000000..5bcb342da86f
@@ -0,0 +1,79 @@
+# 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:

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

Quoted 5 lines

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

+  - H. Nikolaus Schaller <[email protected]>
+
+description: |
+  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
+  TX controller IP with accompanying PHY IP.

My dmesg disagrees:
dw-hdmi-ingenic 10180000.hdmi: Detected HDMI TX controller v1.31a with HDCP (DWC HDMI 3D TX PHY)

Or am I comparing apples to oranges?

Quoted 14 lines

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

+
+allOf:
+  - $ref: bridge/synopsys,dw-hdmi.yaml#
+
+properties:
+  compatible:
+    const: ingenic,jz4780-dw-hdmi
+
+  reg-io-width:
+    const: 4
+
+  clocks:
+    maxItems: 2

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

Quoted 6 lines

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

+
+  hdmi-5v-supply:
+    description: Optional regulator to provide +5V at the connector
+
+  ddc-i2c-bus:
+    description: An I2C interface if the internal DDC I2C driver is not to be used

This property is used within (drivers/gpu/drm/bridge/synopsys/dw-hdmi.c); I think it would make sense to move it to bridge/synopsys,dw-hdmi.yaml.

Hi,

Quoted 44 lines

Am 05.10.2021 um 22:43 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 29 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 | 79 
+++++++++++++++++++
 1 file changed, 79 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..5bcb342da86f
--- /dev/null
+++
b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
@@ -0,0 +1,79 @@
+# 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

Quoted 8 lines

+
+maintainers:
+ - H. Nikolaus Schaller <[email protected]>
+
+description: |
+ The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys
DesignWare HDMI 1.4
+ TX controller IP with accompanying PHY IP.

Quoted 3 lines

My dmesg disagrees:
dw-hdmi-ingenic 10180000.hdmi: Detected HDMI TX controller v1.31a
with HDCP (DWC HDMI 3D TX PHY)

mine as well.

Quoted 2 lines

Or am I comparing apples to oranges?

There is a document called "JZ4780 High Efficiency Engine for Mobile Device" (JZ4780_PB.pdf) which says

"24-bit parallel/serial TFT interface, HDMI 1.4a interface, LVDS interface"

And the data sheet ("JZ4780 Mobile Application Processor Data Sheet ") says: "Support HDMI 1.4a Interface"

Finally, the programming manual also says "Support HDMI 1.4a Interface".

So what is correct?

dmesg may return something else. E.g. silicon revision 1.31a while the interface is HDMI protocol revision 1.4a compatible?

Trying to find something about "hdmi 1.31a" did only lead to some "Synopsys' HAPS-51 eval platform" [1].

Looking at HDMI standards [2] I can only find HDMI 1.3 and 1.3a but no HDMI 1.31a.

[1] https://www.digital-cp.com/hdcp-products/haps51-hdmi-tx-platform-dwc-hdmi-tx-controller-131a-ea-hdmi-3d-tx-phy-tsmc40g-ip
[2] https://en.wikipedia.org/wiki/HDMI#Version_1.3

Well it may also be some Synopsys-internal designation 1.31a referring so something newer than HDMI 1.3a which became the HDMI 1.4 standard (released June 2009)...

Whom should we believe? What the chip tells or what the data sheet and programming manual says?

I tend to keep confusion low and stay with "HDMI 1.4" in the bindings because there is no offical "HDMI 1.31a" standard. And HDMI 1.4 was already some years old when the jz4780 was released. So it is likely that the chip identification just returns 1.31a (maybe Ingenic licenced an interim release VHDL) although the standard was later officially named 1.4a.

Quoted 23 lines

Quoted 14 lines

+
+allOf:
+ - $ref: bridge/synopsys,dw-hdmi.yaml#
+
+properties:
+ compatible:
+ const: ingenic,jz4780-dw-hdmi
+
+ reg-io-width:
+ const: 4
+
+ clocks:
+ maxItems: 2

Quoted 8 lines

+
+ hdmi-5v-supply:
+ description: Optional regulator to provide +5V at the
connector
+
+ ddc-i2c-bus:
+ description: An I2C interface if the internal DDC I2C driver
is not to be used

Quoted 2 lines

This property is used within
(drivers/gpu/drm/bridge/synopsys/dw-hdmi.c); I think it would make sense to move it to bridge/synopsys,dw-hdmi.yaml.

It is indeed more general and not jz4780 specific. I'll move it for v6.

BR and thanks,
Nikolaus

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 | 79 +++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

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 H. Nikolaus Schaller <[email protected]>

---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 45 
++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

arch/mips/boot/dts/ingenic/jz4780.dtsi +0 -0

index 9e34f433b9b5..c3c18a59c377 100644
@@ -424,6 +424,51 @@ i2c4: i2c@10054000 {
 		status = "disabled";
 	};
 

arch/mips/boot/dts/ingenic/jz4780.dtsi +10 -0

+
+		interrupt-parent = <&intc>;
+		interrupts = <3>;
+
+		/* ddc-i2c-bus = <&i2c4>; */
+
+		status = "disabled";
+	};
+
+	lcdc0: lcdc0@13050000 {

arch/mips/boot/dts/ingenic/jz4780.dtsi +15 -1

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

Hi Nikolaus & Paul,

Le mar., oct. 5 2021 at 14:29:17 +0200, H. Nikolaus Schaller

Quoted 12 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +0 -0

index 9e34f433b9b5..c3c18a59c377 100644
@@ -424,6 +424,51 @@ i2c4: i2c@10054000 {
 
 		status = "disabled";
 
 	};
 

arch/mips/boot/dts/ingenic/jz4780.dtsi i2c4: i2c@10054000 { · line 436

Quoted 10 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +10 -0

+	hdmi: hdmi@10180000 {
+		compatible = "ingenic,jz4780-dw-hdmi";
+		reg = <0x10180000 0x8000>;
+		reg-io-width = <4>;
+
+		clocks = <&cgu JZ4780_CLK_AHB0>, <&cgu JZ4780_CLK_HDMI>;
+		clock-names = "iahb", "isfr";
+
+		assigned-clocks = <&cgu JZ4780_CLK_HDMI>;
+		assigned-clock-rates = <27000000>;

Any reason why this is set to 27 MHz? Is it even required? Because with the current ci20.dts, it won't be clocked at anything but 48 MHz.

Quoted 15 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +15 -0

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

arch/mips/boot/dts/ingenic/jz4780.dtsi i2c4: i2c@10054000 { · line 456

Quoted 5 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +5 -0

+
+		interrupt-parent = <&intc>;
+		interrupts = <31>;
+
+		status = "disabled";

I think you can keep lcdc0 enabled by default (not lcdc1 though), since it is highly likely that you'd want that.

Quoted 3 lines · On Tuesday, 5 October 2021 22:50:12 CEST Paul Cercueil wrote:

Hi Nikolaus & Paul,

Le mar., oct. 5 2021 at 14:29:17 +0200, H. Nikolaus Schaller

Quoted 23 lines · <[email protected]> a écrit :

Quoted 22 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +10 -0

index 9e34f433b9b5..c3c18a59c377 100644
@@ -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_AHB0>, <&cgu JZ4780_CLK_HDMI>;
+		clock-names = "iahb", "isfr";
+
+		assigned-clocks = <&cgu JZ4780_CLK_HDMI>;
+		assigned-clock-rates = <27000000>;

Quoted 3 lines

Any reason why this is set to 27 MHz? Is it even required? Because
with
the current ci20.dts, it won't be clocked at anything but 48 MHz.

EXCLK will be 48MHz, but the aim is to set the HDMI peripheral clock to 27MHz, which is supposedly required. I vaguely recall a conversation about whether we were doing this right, but I don't recall any conclusion.

Quoted 21 lines

Quoted 20 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +20 -0

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

Quoted 3 lines

I think you can keep lcdc0 enabled by default (not lcdc1 though),
since
it is highly likely that you'd want that.

As far as I know, the clock gating for the LCD controllers acts like a series circuit, meaning that they both need to be enabled. Some testing seemed to confirm this. Indeed, I seem to remember only enabling one clock and not getting any output until I figured this weird arrangement out.

Am 05.10.2021 um 23:52 schrieb Paul Cercueil <[email protected]>:

Hi Paul,

Quoted 28 lines · Paul Boddie

Quoted 2 lines · On Tuesday, 5 October 2021 22:50:12 CEST Paul Cercueil wrote:

Hi Nikolaus & Paul,
Le mar., oct. 5 2021 at 14:29:17 +0200, H. Nikolaus Schaller

Quoted 24 lines · <[email protected]> a écrit :

Quoted 12 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +0 -0

index 9e34f433b9b5..c3c18a59c377 100644
@@ -424,6 +424,51 @@ i2c4: i2c@10054000 {
 
 		status = "disabled";
 
 	};
 

arch/mips/boot/dts/ingenic/jz4780.dtsi i2c4: i2c@10054000 { · line 438

Quoted 10 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +10 -0

+	hdmi: hdmi@10180000 {
+		compatible = "ingenic,jz4780-dw-hdmi";
+		reg = <0x10180000 0x8000>;
+		reg-io-width = <4>;
+
+		clocks = <&cgu JZ4780_CLK_AHB0>, <&cgu JZ4780_CLK_HDMI>;
+		clock-names = "iahb", "isfr";
+
+		assigned-clocks = <&cgu JZ4780_CLK_HDMI>;
+		assigned-clock-rates = <27000000>;

Any reason why this is set to 27 MHz? Is it even required? Because with the current ci20.dts, it won't be clocked at anything but 48 MHz.

Quoted 3 lines

EXCLK will be 48MHz, but the aim is to set the HDMI peripheral clock to 27MHz, which is supposedly required. I vaguely recall a conversation about whether we were doing this right, but I don't recall any conclusion.

But right now your HDMI clock is 48 MHz and HDMI works.

Quoted 23 lines

Quoted 23 lines

Quoted 11 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +10 -0

+
+		interrupt-parent = <&intc>;
+		interrupts = <3>;
+
+		/* ddc-i2c-bus = <&i2c4>; */
+
+		status = "disabled";
+	};
+
+	lcdc0: lcdc0@13050000 {

arch/mips/boot/dts/ingenic/jz4780.dtsi i2c4: i2c@10054000 { · line 456

Quoted 10 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +10 -0

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

I think you can keep lcdc0 enabled by default (not lcdc1 though), since it is highly likely that you'd want that.

Quoted 4 lines

As far as I know, the clock gating for the LCD controllers acts like a series circuit, meaning that they both need to be enabled. Some testing seemed to confirm this. Indeed, I seem to remember only enabling one clock and not getting any output until I figured this weird arrangement out.

I'm not talking about clocks though, but about LCDC0 and LCDC1.

Hi Paul,

Quoted 35 lines

Am 05.10.2021 um 23:52 schrieb Paul Cercueil <[email protected]>:

Hi Paul,

Quoted 29 lines · Paul Boddie

Quoted 2 lines · On Tuesday, 5 October 2021 22:50:12 CEST Paul Cercueil wrote:

Hi Nikolaus & Paul,
Le mar., oct. 5 2021 at 14:29:17 +0200, H. Nikolaus Schaller

Quoted 25 lines · <[email protected]> a écrit :

Quoted 12 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +0 -0

index 9e34f433b9b5..c3c18a59c377 100644
@@ -424,6 +424,51 @@ i2c4: i2c@10054000 {
 
 		status = "disabled";
 
 	};
 

arch/mips/boot/dts/ingenic/jz4780.dtsi i2c4: i2c@10054000 { · line 438

Quoted 10 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +10 -0

+	hdmi: hdmi@10180000 {
+		compatible = "ingenic,jz4780-dw-hdmi";
+		reg = <0x10180000 0x8000>;
+		reg-io-width = <4>;
+
+		clocks = <&cgu JZ4780_CLK_AHB0>, <&cgu JZ4780_CLK_HDMI>;
+		clock-names = "iahb", "isfr";
+
+		assigned-clocks = <&cgu JZ4780_CLK_HDMI>;
+		assigned-clock-rates = <27000000>;

Any reason why this is set to 27 MHz? Is it even required?
Because with
the current ci20.dts, it won't be clocked at anything but 48 MHz.

Quoted 7 lines

Quoted 5 lines

EXCLK will be 48MHz, but the aim is to set the HDMI peripheral
clock to 27MHz,
which is supposedly required. I vaguely recall a conversation
about whether we
were doing this right, but I don't recall any conclusion.

But right now your HDMI clock is 48 MHz and HDMI works.

Is it? How did you find out?

And have you tried to remove assigned-clocks from jz4780.dtsi?

1. I read back:

root@letux:~# cat /sys/kernel/debug/clk/hdmi/clk_rate
26909090
root@letux:~#

So for me it seems to be running at ~27 MHz.

2. If I remove the assigned-clocks or assigned-clock-rates from DT the boot process hangs shortly after initializing drm.

3. If I set assigned-clock-rates = <48000000>, HDMI also works.

I get it read back from /sys/kernel/debug/clk/hdmi/clk_rate of 46736842.

4. Conclusions:
* assigned-clocks are required
* it does not matter if 27 or 48 MHz
* I have no idea which value is more correct
* so I'd stay on the safe side of 27 MHz

5. But despite that found, please look into the programming manual section 18.1.2.16. There is an

"Import Note: The clock must be between 18M and 27M, it occurs fatal error if exceeding the range. "

6. Therefore I think it *may* work overclocked with 48MHz but is not guaranteed or reliable above 27 MHz.

So everything is ok here.

Quoted 33 lines

Quoted 25 lines

Quoted 17 lines

Quoted 17 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +15 -0

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

Quoted 8 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi i2c4: i2c@10054000 { · line 456

Quoted 5 lines

arch/mips/boot/dts/ingenic/jz4780.dtsi +5 -0

+
+		interrupt-parent = <&intc>;
+		interrupts = <31>;
+
+		status = "disabled";

I think you can keep lcdc0 enabled by default (not lcdc1 though),
since
it is highly likely that you'd want that.

Quoted 7 lines

As far as I know, the clock gating for the LCD controllers acts
like a series
circuit, meaning that they both need to be enabled. Some testing
seemed to
confirm this. Indeed, I seem to remember only enabling one clock
and not
getting any output until I figured this weird arrangement out.

Quoted 1 line

I'm not talking about clocks though, but about LCDC0 and LCDC1.

Ah, you mean status = "okay"; vs. status = "disabled";

Well, IMHO it is common practise to keep SoC subsystems disabled by default (to save power and boot time) unless a board specific DTS explicitly requests the SoC feature to be active. See for example mmc0, mmc1 or i2c0..i2c4.

All these are disabled in jz4780.dtsi and partially enabled in ci20.dts.

Why should lcdc0 be an exception in jz4780.dtsi?

BR and thanks,
Nikolaus

Am 07.11.2021 um 20:05 schrieb Paul Cercueil <[email protected]>:

Quoted 3 lines

6. Therefore I think it *may* work overclocked with 48MHz but is not guaranteed or reliable above 27 MHz. So everything is ok here.

One thing though - the "assigned-clocks" and "assigned-clock-rates", while it works here, should be moved to the CGU node, to respect the YAML schemas.

Hi Paul,

Quoted 8 lines

Am 07.11.2021 um 20:05 schrieb Paul Cercueil <[email protected]>:

Quoted 3 lines

6. Therefore I think it *may* work overclocked with 48MHz but is not guaranteed or reliable above 27 MHz. So everything is ok here.

One thing though - the "assigned-clocks" and
"assigned-clock-rates", while it works here, should be moved to the CGU node, to respect the YAML schemas.

Trying to do this seems to break boot.

I can boot up to

[    8.312926] dw-hdmi-ingenic 10180000.hdmi: registered DesignWare HDMI I2C bus driver

and

[   11.366899] [drm] Initialized ingenic-drm 1.1.0 20200716 for 13050000.lcdc0 on minor 0

but then the boot process becomes slow and hangs. Last sign of activity is

[   19.347659] hub 1-0:1.0: USB hub found
[   19.353478] hub 1-0:1.0: 1 port detected
[   32.321760] wlan0_power: disabling

What I did was to just move

		assigned-clocks = <&cgu JZ4780_CLK_HDMI>;
		assigned-clock-rates = <27000000>;

from

	hdmi: hdmi@10180000 {

to

	cgu: jz4780-cgu@10000000 {

Does this mean the clock is assigned too early or too late?

Do you have any suggestions since I don't know the details of CGU.

Am 09.11.2021 um 21:36 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 22 lines · H. Nikolaus Schaller

Hi Paul,

Quoted 5 lines

Am 07.11.2021 um 20:05 schrieb Paul Cercueil <[email protected]>:

Quoted 3 lines

6. Therefore I think it *may* work overclocked with 48MHz but is not guaranteed or reliable above 27 MHz. So everything is ok here.

One thing though - the "assigned-clocks" and "assigned-clock-rates", while it works here, should be moved to the CGU node, to respect the YAML schemas.

Trying to do this seems to break boot.
I can boot up to

[    8.312926] dw-hdmi-ingenic 10180000.hdmi: registered DesignWare HDMI I2C bus driver
and
[   11.366899] [drm] Initialized ingenic-drm 1.1.0 20200716 for 13050000.lcdc0 on minor 0
but then the boot process becomes slow and hangs. Last sign of activity is
[   19.347659] hub 1-0:1.0: USB hub found
[   19.353478] hub 1-0:1.0: 1 port detected
[   32.321760] wlan0_power: disabling
What I did was to just move
		assigned-clocks = <&cgu JZ4780_CLK_HDMI>;
		assigned-clock-rates = <27000000>;
from
	hdmi: hdmi@10180000 {
to
	cgu: jz4780-cgu@10000000 {

Quoted 2 lines

Does this mean the clock is assigned too early or too late? Do you have any suggestions since I don't know the details of CGU.

These properties are already set for the CGU node in ci20.dts:

&cgu {
	/*
	 * Use the 32.768 kHz oscillator as the parent of the RTC for a higher
	 * precision.
	 */
	assigned-clocks = <&cgu JZ4780_CLK_OTGPHY>, <&cgu JZ4780_CLK_RTC>;
	assigned-clock-parents = <0>, <&cgu JZ4780_CLK_RTCLK>;
	assigned-clock-rates = <48000000>;
};

So you want to update these properties to add the HDMI clock setting, like this:

	assigned-clocks = <&cgu JZ4780_CLK_OTGPHY>, <&cgu JZ4780_CLK_RTC>, <&cgu JZ4780_CLK_HDMI>;
	assigned-clock-parents = <0>, <&cgu JZ4780_CLK_RTCLK>;
	assigned-clock-rates = <48000000>, <0>, <27000000>;

Am 09.11.2021 um 21:42 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 5 lines

So you want to update these properties to add the HDMI clock setting, like this:

	assigned-clocks = <&cgu JZ4780_CLK_OTGPHY>, <&cgu JZ4780_CLK_RTC>, <&cgu JZ4780_CLK_HDMI>;
	assigned-clock-parents = <0>, <&cgu JZ4780_CLK_RTCLK>;
	assigned-clock-rates = <48000000>, <0>, <27000000>;

Will give it a try.