Hi Nikolaus,

Le mar., oct. 5 2021 at 14:29:14 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :

From: Paul Boddie paul@boddie.org.uk

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

Tested on MIPS Creator CI20 board.

Signed-off-by: Paul Boddie paul@boddie.org.uk Signed-off-by: Ezequiel Garcia ezequiel@collabora.com Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

---

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index f73522bdacaa..e2df4b085905 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -6,6 +6,7 @@

#include "ingenic-drm.h"

+#include <linux/bitfield.h> #include <linux/component.h> #include <linux/clk.h> #include <linux/dma-mapping.h> @@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc { u32 addr; u32 id; u32 cmd;

• /* extended hw descriptor for jz4780 */
• u32 offsize;
• u32 pagewidth;
• u32 cpos;
• u32 dessize;

} __aligned(16);

struct ingenic_dma_hwdescs { @@ -60,9 +66,11 @@ struct jz_soc_info { bool needs_dev_clk; bool has_osd; bool map_noncoherent;

• bool use_extended_hwdesc; unsigned int max_width, max_height; const u32 *formats_f0, *formats_f1; unsigned int num_formats_f0, num_formats_f1;
• unsigned int max_reg;

};

struct ingenic_drm_private_state { @@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg) } }

-static const struct regmap_config ingenic_drm_regmap_config = { +static struct regmap_config ingenic_drm_regmap_config = { .reg_bits = 32, .val_bits = 32, .reg_stride = 4,

• .max_register = JZ_REG_LCD_SIZE1, .writeable_reg = ingenic_drm_writeable_reg,

};

@@ -663,6 +670,37 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4); hwdesc->next = dma_hwdesc_addr(priv, next_id);

• if (priv->soc_info->use_extended_hwdesc) {
  

• 	hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE;
  

• 	/* Extended 8-byte descriptor */
  

• 	hwdesc->cpos = 0;
  

• 	hwdesc->offsize = 0;
  

• 	hwdesc->pagewidth = 0;
  

• 	switch (newstate->fb->format->format) {
  

• 	case DRM_FORMAT_XRGB1555:
  

• 		hwdesc->cpos |= JZ_LCD_CPOS_RGB555;
  

• 		fallthrough;
  

• 	case DRM_FORMAT_RGB565:
  

• 		hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16;
  

• 		break;
  

• 	case DRM_FORMAT_XRGB8888:
  

• 		hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
  

• 		break;
  

• 	}
  

• 	hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
  

• 			    (JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 <<
  

• 			     JZ_LCD_CPOS_COEFFICIENT_OFFSET);
  

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

• 	hwdesc->dessize =
  

• 		(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
  

Same here.

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

• }
  

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

@@ -694,6 +732,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE; }

• /* set use of the 8-word descriptor and OSD foreground usage. */

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.

• if (priv->soc_info->use_extended_hwdesc)

• cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
  

• if (mode->flags & DRM_MODE_FLAG_NHSYNC) cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; if (mode->flags & DRM_MODE_FLAG_NVSYNC)

@@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) struct drm_encoder *encoder; struct ingenic_drm_bridge *ib; struct drm_device *drm;

• struct regmap_config regmap_config; void __iomem *base; long parent_rate; unsigned int i, clone_mask = 0;

@@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) return PTR_ERR(base); }

• regmap_config = ingenic_drm_regmap_config;
• regmap_config.max_register = soc_info->max_reg; priv->map = devm_regmap_init_mmio(dev, base,

• 			  &ingenic_drm_regmap_config);
  

• 			  &regmap_config);
  

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

if (IS_ERR(priv->map)) { dev_err(dev, "Failed to create regmap\n"); return PTR_ERR(priv->map); @@ -1274,7 +1319,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)

/* Enable OSD if available */ if (soc_info->has_osd)

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

• regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
  

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.

mutex_init(&priv->clk_mutex); priv->clock_nb.notifier_call = ingenic_drm_update_pixclk; @@ -1444,6 +1489,7 @@ static const struct jz_soc_info jz4740_soc_info = { .formats_f1 = jz4740_formats, .num_formats_f1 = ARRAY_SIZE(jz4740_formats), /* JZ4740 has only one plane */

• .max_reg = JZ_REG_LCD_SIZE1,

};

static const struct jz_soc_info jz4725b_soc_info = { @@ -1456,6 +1502,7 @@ static const struct jz_soc_info jz4725b_soc_info = { .num_formats_f1 = ARRAY_SIZE(jz4725b_formats_f1), .formats_f0 = jz4725b_formats_f0, .num_formats_f0 = ARRAY_SIZE(jz4725b_formats_f0),

• .max_reg = JZ_REG_LCD_SIZE1,

};

static const struct jz_soc_info jz4770_soc_info = { @@ -1468,12 +1515,28 @@ static const struct jz_soc_info jz4770_soc_info = { .num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1), .formats_f0 = jz4770_formats_f0, .num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),

• .max_reg = JZ_REG_LCD_SIZE1,

+};

+static const struct jz_soc_info jz4780_soc_info = {

• .needs_dev_clk = true,
• .has_osd = true,
• .use_extended_hwdesc = true,
• .max_width = 4096,
• .max_height = 2048,
• /* REVISIT: do we support formats different from jz4770? */

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

• .formats_f1 = jz4770_formats_f1,
• .num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
• .formats_f0 = jz4770_formats_f0,
• .num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
• .max_reg = JZ_REG_LCD_PCFG,

};

static const struct of_device_id ingenic_drm_of_match[] = { { .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info }, { .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info }, { .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },

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

}; MODULE_DEVICE_TABLE(of, ingenic_drm_of_match); @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void) { int err;

• if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {

• err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
  

• if (err)
  

• 	return err;
  

• }

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

• if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) { err = platform_driver_register(ingenic_ipu_driver_ptr); if (err)

• 	return err;
  

• 	goto err_hdmi_unreg;
  

  }

  err = platform_driver_register(&ingenic_drm_driver);

@@ -1507,6 +1576,10 @@ static int ingenic_drm_init(void) err_ipu_unreg: if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) platform_driver_unregister(ingenic_ipu_driver_ptr); +err_hdmi_unreg:

• if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))

• platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
  

• return err;

} module_init(ingenic_drm_init); @@ -1515,6 +1588,8 @@ static void ingenic_drm_exit(void) { platform_driver_unregister(&ingenic_drm_driver);

• if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))

• platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
  

  if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) platform_driver_unregister(ingenic_ipu_driver_ptr);

} diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h index 22654ac1dde1..13dbc5d25c3b 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm.h +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h @@ -44,8 +44,11 @@ #define JZ_REG_LCD_XYP1 0x124 #define JZ_REG_LCD_SIZE0 0x128 #define JZ_REG_LCD_SIZE1 0x12c +#define JZ_REG_LCD_PCFG 0x2c0

#define JZ_LCD_CFG_SLCD BIT(31) +#define JZ_LCD_CFG_DESCRIPTOR_8 BIT(28) +#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN BIT(25) #define JZ_LCD_CFG_PS_DISABLE BIT(23) #define JZ_LCD_CFG_CLS_DISABLE BIT(22) #define JZ_LCD_CFG_SPL_DISABLE BIT(21) @@ -63,6 +66,7 @@ #define JZ_LCD_CFG_DE_ACTIVE_LOW BIT(9) #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW BIT(8) #define JZ_LCD_CFG_18_BIT BIT(7) +#define JZ_LCD_CFG_24_BIT BIT(6) #define JZ_LCD_CFG_PDW (BIT(5) | BIT(4))

#define JZ_LCD_CFG_MODE_GENERIC_16BIT 0 @@ -132,6 +136,7 @@ #define JZ_LCD_CMD_SOF_IRQ BIT(31) #define JZ_LCD_CMD_EOF_IRQ BIT(30) #define JZ_LCD_CMD_ENABLE_PAL BIT(28) +#define JZ_LCD_CMD_FRM_ENABLE BIT(26)

#define JZ_LCD_SYNC_MASK 0x3ff

@@ -153,6 +158,7 @@ #define JZ_LCD_RGBC_EVEN_BGR (0x5 << 0)

#define JZ_LCD_OSDC_OSDEN BIT(0) +#define JZ_LCD_OSDC_ALPHAEN BIT(2) #define JZ_LCD_OSDC_F0EN BIT(3) #define JZ_LCD_OSDC_F1EN BIT(4)

@@ -176,6 +182,41 @@ #define JZ_LCD_SIZE01_WIDTH_LSB 0 #define JZ_LCD_SIZE01_HEIGHT_LSB 16

+#define JZ_LCD_DESSIZE_ALPHA_OFFSET 24 +#define JZ_LCD_DESSIZE_HEIGHT_OFFSET 12 +#define JZ_LCD_DESSIZE_WIDTH_OFFSET 0 +#define JZ_LCD_DESSIZE_HEIGHT_MASK 0xfff +#define JZ_LCD_DESSIZE_WIDTH_MASK 0xfff

+/* TODO: 4,5 and 7 match the above BPP */ +#define JZ_LCD_CPOS_BPP_15_16 (4 << 27) +#define JZ_LCD_CPOS_BPP_18_24 (5 << 27) +#define JZ_LCD_CPOS_BPP_30 (7 << 27) +#define JZ_LCD_CPOS_RGB555 BIT(30) +#define JZ_LCD_CPOS_PREMULTIPLY_LCD BIT(26) +#define JZ_LCD_CPOS_COEFFICIENT_OFFSET 24 +#define JZ_LCD_CPOS_COEFFICIENT_0 0 +#define JZ_LCD_CPOS_COEFFICIENT_1 1 +#define JZ_LCD_CPOS_COEFFICIENT_ALPHA1 2 +#define JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 3

+#define JZ_LCD_RGBC_RGB_PADDING BIT(15) +#define JZ_LCD_RGBC_RGB_PADDING_FIRST BIT(14) +#define JZ_LCD_RGBC_422 BIT(8) +#define JZ_LCD_RGBC_RGB_FORMAT_ENABLE BIT(7)

+#define JZ_LCD_PCFG_PRI_MODE BIT(31) +#define JZ_LCD_PCFG_HP_BST_4 (0 << 28) +#define JZ_LCD_PCFG_HP_BST_8 (1 << 28) +#define JZ_LCD_PCFG_HP_BST_16 (2 << 28) +#define JZ_LCD_PCFG_HP_BST_32 (3 << 28) +#define JZ_LCD_PCFG_HP_BST_64 (4 << 28) +#define JZ_LCD_PCFG_HP_BST_16_CONT (5 << 28) +#define JZ_LCD_PCFG_HP_BST_DISABLE (7 << 28) +#define JZ_LCD_PCFG_THRESHOLD2_OFFSET 18 +#define JZ_LCD_PCFG_THRESHOLD1_OFFSET 9 +#define JZ_LCD_PCFG_THRESHOLD0_OFFSET 0

struct device; struct drm_plane; struct drm_plane_state; @@ -187,5 +228,6 @@ void ingenic_drm_plane_disable(struct device *dev, struct drm_plane *plane); bool ingenic_drm_map_noncoherent(const struct device *dev);

extern struct platform_driver *ingenic_ipu_driver_ptr; +extern struct platform_driver *ingenic_dw_hdmi_driver_ptr;

#endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */

2.33.0

Hi Nikolaus,

Le dim., nov. 7 2021 at 14:41:18 +0100, H. Nikolaus Schaller hns@goldelico.com a écrit :

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

...

Am 05.10.2021 um 22:22 schrieb Paul Cercueil paul@crapouillou.net:

Hi Nikolaus,

Le mar., oct. 5 2021 at 14:29:14 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

From: Paul Boddie paul@boddie.org.uk Add support for the LCD controller present on JZ4780 SoCs. This SoC uses 8-byte descriptors which extend the current 4-byte descriptors used for other Ingenic SoCs. Tested on MIPS Creator CI20 board. Signed-off-by: Paul Boddie paul@boddie.org.uk Signed-off-by: Ezequiel Garcia ezequiel@collabora.com Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

---

drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +++++++++++++++++++++-- drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++++++++++ 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index f73522bdacaa..e2df4b085905 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -6,6 +6,7 @@

...

...

	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.

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.

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.

...

...

• 	hwdesc->dessize =
  

• 		(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
  

Same here.

...

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

...

...

• }
  

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

@@ -694,6 +732,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE; }

• /* set use of the 8-word descriptor and OSD foreground usage. */

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.

...

...

• if (priv->soc_info->use_extended_hwdesc)

• cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
  

• if (mode->flags & DRM_MODE_FLAG_NHSYNC) cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; if (mode->flags & DRM_MODE_FLAG_NVSYNC)

@@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) struct drm_encoder *encoder; struct ingenic_drm_bridge *ib; struct drm_device *drm;

• struct regmap_config regmap_config; void __iomem *base; long parent_rate; unsigned int i, clone_mask = 0;

@@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) return PTR_ERR(base); }

• regmap_config = ingenic_drm_regmap_config;
• regmap_config.max_register = soc_info->max_reg; priv->map = devm_regmap_init_mmio(dev, base,

• 			  &ingenic_drm_regmap_config);
  

• 			  &regmap_config);
  

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.

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.

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.

So I leave it in v6 unsplitted.

...

...

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.

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.

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.

...

...

mutex_init(&priv->clk_mutex); priv->clock_nb.notifier_call = ingenic_drm_update_pixclk; @@ -1444,6 +1489,7 @@ static const struct jz_soc_info jz4740_soc_info = { .formats_f1 = jz4740_formats, .num_formats_f1 = ARRAY_SIZE(jz4740_formats), /* JZ4740 has only one plane */

• .max_reg = JZ_REG_LCD_SIZE1,

}; static const struct jz_soc_info jz4725b_soc_info = { @@ -1456,6 +1502,7 @@ static const struct jz_soc_info jz4725b_soc_info = { .num_formats_f1 = ARRAY_SIZE(jz4725b_formats_f1), .formats_f0 = jz4725b_formats_f0, .num_formats_f0 = ARRAY_SIZE(jz4725b_formats_f0),

• .max_reg = JZ_REG_LCD_SIZE1,

}; static const struct jz_soc_info jz4770_soc_info = { @@ -1468,12 +1515,28 @@ static const struct jz_soc_info jz4770_soc_info = { .num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1), .formats_f0 = jz4770_formats_f0, .num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),

• .max_reg = JZ_REG_LCD_SIZE1,

+};

+static const struct jz_soc_info jz4780_soc_info = {

• .needs_dev_clk = true,
• .has_osd = true,
• .use_extended_hwdesc = true,
• .max_width = 4096,
• .max_height = 2048,
• /* REVISIT: do we support formats different from jz4770? */

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

Fine. I'll remove the comment in v6.

...

...

• .formats_f1 = jz4770_formats_f1,
• .num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
• .formats_f0 = jz4770_formats_f0,
• .num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
• .max_reg = JZ_REG_LCD_PCFG,

}; static const struct of_device_id ingenic_drm_of_match[] = { { .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info }, { .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info }, { .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },

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

}; MODULE_DEVICE_TABLE(of, ingenic_drm_of_match); @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void) { int err;

• if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {

• err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
  

• if (err)
  

• 	return err;
  

• }

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.

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.

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

...

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.

...

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.

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;

Cheers, -Paul

...

Cheers, -Paul

BR and thanks, Nikolaus

Hi Nikolaus,

Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller hns@goldelico.com a écrit :

Hi Paul,

...

Am 07.11.2021 um 20:01 schrieb Paul Cercueil paul@crapouillou.net:

Hi Nikolaus,

Le dim., nov. 7 2021 at 14:41:18 +0100, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

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

...

Am 05.10.2021 um 22:22 schrieb Paul Cercueil paul@crapouillou.net: Hi Nikolaus, Le mar., oct. 5 2021 at 14:29:14 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

From: Paul Boddie paul@boddie.org.uk Add support for the LCD controller present on JZ4780 SoCs. This SoC uses 8-byte descriptors which extend the current 4-byte descriptors used for other Ingenic SoCs. Tested on MIPS Creator CI20 board. Signed-off-by: Paul Boddie paul@boddie.org.uk Signed-off-by: Ezequiel Garcia ezequiel@collabora.com Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

---

drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +++++++++++++++++++++-- drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++++++++++ 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index f73522bdacaa..e2df4b085905 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -6,6 +6,7 @@ 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.

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;

Exactly what I wrote and did test.

...

That's enough to get an image.

Fine that we can agree on that.

...

...

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.

...

...

}

• 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 :)

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.

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

...

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

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.

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.

...

...

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.

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.

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.

...

...

So I leave it in v6 unsplitted.

...

...

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.

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

...

...

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.

...

...

...

...

• }

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.

...

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.

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.

Don't need to wait for me - just create a standard platform_driver module for the HDMI code. Since it won't touch the ingenic-drm-drv.c file, if I later patch the IPU code to be its own module, it won't conflict.

Cheers, -Paul

...

...

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

Hi,

Le lun., nov. 8 2021 at 16:29:11 +0100, H. Nikolaus Schaller hns@goldelico.com a écrit :

Bnjour Paul,

...

Am 08.11.2021 um 13:20 schrieb Paul Cercueil paul@crapouillou.net:

Hi,

Le lun., nov. 8 2021 at 11:52:20 +0100, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

Hi Paul,

...

...

Am 08.11.2021 um 10:37 schrieb Paul Cercueil paul@crapouillou.net: 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.

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.

The JZ4780 patch lies on top of the other one, so they are still related. They just shouldn't be the same patch.

...

...

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

...

...

...

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

...

You can do the same,

We already do the same...

...

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.

...

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.

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?

...

...

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.

...

...

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.

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

...

...

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.

...

...

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.

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.

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.

Cheers, -Paul

BR and thanks, Nikolaus

#### VARIANT 0001 ####

From c7afa925f6b53bb6cafa080b7dd582788c8eb2eb Mon Sep 17 00:00:00 2001 From: "H. Nikolaus Schaller" hns@goldelico.com 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 hns@goldelico.com

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index a5df1c8d34cde..1903e897d2299 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -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,

};

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h index 22654ac1dde1c..e7430c4af41f6 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm.h +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h @@ -44,8 +44,11 @@ #define JZ_REG_LCD_XYP1 0x124 #define JZ_REG_LCD_SIZE0 0x128 #define JZ_REG_LCD_SIZE1 0x12c +#define JZ_REG_LCD_PCFG 0x2c0

#define JZ_LCD_CFG_SLCD BIT(31) +#define JZ_LCD_CFG_DESCRIPTOR_8 BIT(28) +#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN BIT(25) #define JZ_LCD_CFG_PS_DISABLE BIT(23) #define JZ_LCD_CFG_CLS_DISABLE BIT(22) #define JZ_LCD_CFG_SPL_DISABLE BIT(21) @@ -63,6 +66,7 @@ #define JZ_LCD_CFG_DE_ACTIVE_LOW BIT(9) #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW BIT(8) #define JZ_LCD_CFG_18_BIT BIT(7) +#define JZ_LCD_CFG_24_BIT BIT(6) #define JZ_LCD_CFG_PDW (BIT(5) | BIT(4))

#define JZ_LCD_CFG_MODE_GENERIC_16BIT 0 @@ -132,6 +136,7 @@ #define JZ_LCD_CMD_SOF_IRQ BIT(31) #define JZ_LCD_CMD_EOF_IRQ BIT(30) #define JZ_LCD_CMD_ENABLE_PAL BIT(28) +#define JZ_LCD_CMD_FRM_ENABLE BIT(26)

#define JZ_LCD_SYNC_MASK 0x3ff

@@ -153,6 +158,7 @@ #define JZ_LCD_RGBC_EVEN_BGR (0x5 << 0)

#define JZ_LCD_OSDC_OSDEN BIT(0) +#define JZ_LCD_OSDC_ALPHAEN BIT(2) #define JZ_LCD_OSDC_F0EN BIT(3) #define JZ_LCD_OSDC_F1EN BIT(4)

@@ -176,6 +182,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" hns@goldelico.com 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 paul@crapouillou.net Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

---

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

diff --git a/arch/mips/boot/dts/ingenic/jz4725b.dtsi b/arch/mips/boot/dts/ingenic/jz4725b.dtsi index a1f0b71c92237..c017b087c0e52 100644 --- a/arch/mips/boot/dts/ingenic/jz4725b.dtsi +++ b/arch/mips/boot/dts/ingenic/jz4725b.dtsi @@ -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>;

diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi index c1afdfdaa8a38..ce3689e5015b5 100644 --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi @@ -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>;

diff --git a/arch/mips/boot/dts/ingenic/jz4770.dtsi b/arch/mips/boot/dts/ingenic/jz4770.dtsi index 05c00b93088e9..0d1ee58d6c40b 100644 --- a/arch/mips/boot/dts/ingenic/jz4770.dtsi +++ b/arch/mips/boot/dts/ingenic/jz4770.dtsi @@ -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>;

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index a5df1c8d34cde..3c8e3c5a447bb 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -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; 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

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.

Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller hns@goldelico.com:

...

Am 08.11.2021 um 10:37 schrieb Paul Cercueil paul@crapouillou.net:

Hi Nikolaus,

Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

Hi Paul,

...

...

...

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

...

...

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

more detailled test shows that it is the underlying

clk_enable(priv->lcd_clk)

(i.e. not the prepare).

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?

I now suspect that it is an undocumented SoC feature.

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

Hi Paul,

Am 18.01.2022 um 17:58 schrieb Paul Cercueil paul@crapouillou.net:

Hi Nikolaus,

Le mar., janv. 18 2022 at 15:50:01 +0100, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

Hi Paul, any insights on the JZ_REG_LCD_OSDC issue below?

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

No problem... We all had deserved them.

...

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.

...

Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller hns@goldelico.com:

...

Am 08.11.2021 um 10:37 schrieb Paul Cercueil paul@crapouillou.net: Hi Nikolaus, Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

Hi Paul,

...

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

...

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

more detailled test shows that it is the underlying clk_enable(priv->lcd_clk) (i.e. not the prepare).

...

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?

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.

This is the right answer to the wrong question :) The question is why the register becomes 0x10005 as soon as the clock is enabled. Without writing to it. And contrary to the documented reset state. Or are you aware that u-boot initialized the lcdc and clocks get disabled when/during starting the kernel (I am using the good old v2013.10)? That could be an explanation that we read 0 before the clock is enabled and 0x10005 after.

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,

exactly.

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

Looks good to me. I'll give it a try.

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.

Ok. Not all SoC have osd.

BR and thanks, Nikolaus

Hi,

Am 05.10.2021 um 22:43 schrieb Paul Cercueil paul@crapouillou.net:

Hi Nikolaus,

Le mar., oct. 5 2021 at 14:29:15 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

From: Sam Ravnborg sam@ravnborg.org Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC. Based on .txt binding from Zubair Lutfullah Kakakhel Signed-off-by: Sam Ravnborg sam@ravnborg.org Signed-off-by: H. Nikolaus Schaller hns@goldelico.com Cc: Rob Herring robh@kernel.org Cc: devicetree@vger.kernel.org

---

.../bindings/display/ingenic-jz4780-hdmi.yaml | 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

+maintainers:

• • H. Nikolaus Schaller hns@goldelico.com

+description: |

• The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
• TX controller IP with accompanying PHY IP.

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

mine as well.

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

...

+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

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

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

hns@goldelico.com a écrit :

...

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi index 9e34f433b9b5..c3c18a59c377 100644 --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi @@ -424,6 +424,51 @@ i2c4: i2c@10054000 {

status = "disabled";

};

• hdmi: hdmi@10180000 {

• compatible = "ingenic,jz4780-dw-hdmi";
  

• reg = <0x10180000 0x8000>;
  

• reg-io-width = <4>;
  

• clocks = <&cgu JZ4780_CLK_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.

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.

...

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

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.

Paul

Hi Paul,

Am 05.10.2021 um 23:52 schrieb Paul Cercueil paul@crapouillou.net:

Hi Paul,

Le mar., oct. 5 2021 at 23:44:12 +0200, Paul Boddie paul@boddie.org.uk a écrit :

...

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

hns@goldelico.com a écrit :

...

...

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi index 9e34f433b9b5..c3c18a59c377 100644 --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi @@ -424,6 +424,51 @@ i2c4: i2c@10054000 {

status = "disabled";

};

• hdmi: hdmi@10180000 {

• compatible = "ingenic,jz4780-dw-hdmi";
  

• reg = <0x10180000 0x8000>;
  

• reg-io-width = <4>;
  

• clocks = <&cgu JZ4780_CLK_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.

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.

...

...

...

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

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.

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