[PATCH v5 0/7] MIPS: JZ4780 and CI20 HDMI

[PATCH v5 0/7] MIPS: JZ4780 and CI20 HDMI

[H. Nikolaus Schaller]

PATCH V5  2021-10-05 14:28:44:
- dropped mode_fixup and timings support in dw-hdmi as it is no longer needed in this V5 (by hns@goldelico.com)
- dropped "drm/ingenic: add some jz4780 specific features" (stimulated by paul@crapouillou.net)
- fixed typo in commit subject: "synopsis" -> "synopsys" (by hns@goldelico.com)
- swapped clocks in jz4780.dtsi to match synopsys,dw-hdmi.yaml (by hns@goldelico.com)
- improved, simplified, fixed, dtbschecked ingenic-jz4780-hdmi.yaml and made dependent of bridge/synopsys,dw-hdmi.yaml (based on suggestions by maxime@cerno.tech)
- fixed binding vs. driver&DTS use of hdmi-5v regulator (suggested by maxime@cerno.tech)
- dropped "drm/bridge: synopsis: Fix to properly handle HPD" - was a no longer needed workaround for a previous version
  (suggested by maxime@cerno.tech)

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

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

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

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


H. Nikolaus Schaller (1):
  MIPS: defconfig: CI20: configure for DRM_DW_HDMI_JZ4780

Paul Boddie (5):
  drm/ingenic: Fix drm_init error path if IPU was registered
  drm/ingenic: Add support for JZ4780 and HDMI output
  drm/ingenic: Add dw-hdmi driver for jz4780
  MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD
    controllers
  MIPS: DTS: CI20: Add DT nodes for HDMI setup

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

 .../bindings/display/ingenic-jz4780-hdmi.yaml |  79 +++++++++++
 arch/mips/boot/dts/ingenic/ci20.dts           |  67 ++++++++++
 arch/mips/boot/dts/ingenic/jz4780.dtsi        |  45 +++++++
 arch/mips/configs/ci20_defconfig              |   6 +
 drivers/gpu/drm/ingenic/Kconfig               |   9 ++
 drivers/gpu/drm/ingenic/Makefile              |   1 +
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c     |  96 +++++++++++++-
 drivers/gpu/drm/ingenic/ingenic-drm.h         |  42 ++++++
 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c     | 125 ++++++++++++++++++
 9 files changed, 464 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
 create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c

-- 
2.33.0

[PATCH v5 1/7] drm/ingenic: Fix drm_init error path if IPU was registered

[H. Nikolaus Schaller]

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

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

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

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

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

[PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

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

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

Tested on MIPS Creator CI20 board.

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

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

[PATCH v5 3/7] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[H. Nikolaus Schaller]

From: Sam Ravnborg <sam@ravnborg.org>

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

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
---
 .../bindings/display/ingenic-jz4780-hdmi.yaml | 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.
+
+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
+
+  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

[PATCH v5 4/7] drm/ingenic: Add dw-hdmi driver for jz4780

[H. Nikolaus Schaller]

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

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

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

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

[PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[H. Nikolaus Schaller]

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

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

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

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 9e34f433b9b5..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>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <3>;
+
+		/* ddc-i2c-bus = <&i2c4>; */
+
+		status = "disabled";
+	};
+
+	lcdc0: lcdc0@13050000 {
+		compatible = "ingenic,jz4780-lcd";
+		reg = <0x13050000 0x1800>;
+
+		clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>;
+		clock-names = "lcd", "lcd_pclk";
+
+		interrupt-parent = <&intc>;
+		interrupts = <31>;
+
+		status = "disabled";
+	};
+
+	lcdc1: lcdc1@130a0000 {
+		compatible = "ingenic,jz4780-lcd";
+		reg = <0x130a0000 0x1800>;
+
+		clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>;
+		clock-names = "lcd", "lcd_pclk";
+
+		interrupt-parent = <&intc>;
+		interrupts = <31>;
+
+		status = "disabled";
+	};
+
 	nemc: nemc@13410000 {
 		compatible = "ingenic,jz4780-nemc", "simple-mfd";
 		reg = <0x13410000 0x10000>;
-- 
2.33.0

[PATCH v5 6/7] MIPS: DTS: CI20: Add DT nodes for HDMI setup

[H. Nikolaus Schaller]

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

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

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

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

[PATCH v5 7/7] MIPS: defconfig: CI20: configure for DRM_DW_HDMI_JZ4780

[H. Nikolaus Schaller]

Enable CONFIG options as modules.

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

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

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
> 

Re: [PATCH v5 3/7] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[Paul Cercueil]

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)

Or am I comparing apples to oranges?

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

Cheers,
-Paul

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

Re: [PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[Paul Cercueil]

Hi Nikolaus & Paul,

Le mar., oct. 5 2021 at 14:29:17 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> From: Paul Boddie <paul@boddie.org.uk>
> 
> A specialisation of the generic Synopsys HDMI driver is employed for 
> JZ4780
> HDMI support. This requires a new driver, plus device tree and 
> configuration
> modifications.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  arch/mips/boot/dts/ingenic/jz4780.dtsi | 45 
> ++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
> b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index 9e34f433b9b5..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.

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

Cheers,
-Paul

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

Re: [PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[Paul Boddie]

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

Re: [PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[Paul Cercueil]

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.

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

Cheers,
-Paul

Re: [PATCH v5 3/7] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[Rob Herring]

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

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml:39:5: [warning] wrong indentation: expected 2 but found 4 (indentation)

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.example.dt.yaml: hdmi@10180000: 'clock-names', 'interrupt-parent', 'interrupts', 'reg' do not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

doc reference errors (make refcheckdocs):

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

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

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

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v5 3/7] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[H. Nikolaus Schaller]

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

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

Re: [PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[H. Nikolaus Schaller]

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

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
> 

Re: [PATCH v5 3/7] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[Paul Cercueil]

Hi Nikolaus,

Le dim., nov. 7 2021 at 14:43:33 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> 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-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.

Fair enough. Let's keep "HDMI 1.4" until proven otherwise.

Cheers,
-Paul

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

Re: [PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[Paul Cercueil]

Hi,

Le dim., nov. 7 2021 at 14:45:37 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> 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. "

Ok, that's the important information that was missing.

So 27 MHz is OK.

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

Cheers,
-Paul

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

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

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

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.

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

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

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

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

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

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

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?

BR and thanks,
Nikolaus

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

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.

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

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

Code execution time? ...

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

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

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

Cheers,
-Paul

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

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.

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

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

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,

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

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

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

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

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

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

Hi,

Le lun., nov. 8 2021 at 18:22:58 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 08.11.2021 um 17:30 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  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,
>>>>>  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.
> 
> Hm. Then we are changing the .max_register initialization to a much 
> bigger value.
> 
>> 
>>>>>  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.
> 
> 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).
> 
>> 
>>>  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.
> 
> 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.

-Paul

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,

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

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

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

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

BR and thanks,
Nikolaus

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

Hi Nikolaus,

Le lun., nov. 8 2021 at 19:33:48 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 08.11.2021 um 18:49 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>>>  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.
> 
>>  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...
> 
>>  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).

Sure, if you want. Send the DTSI patch(es) separate from this patchset 
then.

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

Great. Waiting for your v6 then.

Cheers,
-Paul

Re: [PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[H. Nikolaus Schaller]

Hi Paul,

> Am 07.11.2021 um 20:05 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
>> 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.

BR and thanks,
Nikolaus

Re: [PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[Paul Cercueil]

Hi Nikolaus,

Le mar., nov. 9 2021 at 21:19:17 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 07.11.2021 um 20:05 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>>  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.

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

Cheers,
-Paul

Re: [PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[H. Nikolaus Schaller]

> Am 09.11.2021 um 21:36 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le mar., nov. 9 2021 at 21:19:17 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>>> Am 07.11.2021 um 20:05 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>> 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.
> 
> These properties are already set for the CGU node in ci20.dts:

Ah, I didn't look into that. Maybe because I thought adding this should stay in jz4780.dtsi to be available for any board making use of it.

So it gets overwritten and is then completely missing.

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

Will give it a try.

I would prefer if it could sit in jz4780.dtsi and ci20.dts would just extend it but IMHO this is beyond DTS capabilities.
So we likely have to live with that.

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers

[H. Nikolaus Schaller]

> Am 09.11.2021 um 21:42 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
>> 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.

Yes, works. So v6 is not far away.

BR,
Nikolaus

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,
sorry to go back here...

DDC power control is working now (and I now understand that a typo in my work-in-progress
ci20.dts had switched the DDC SDA to active "1" without powering DDC and that may be beyond
what the monitor wanted to see and therefore DDC edid isn't reliable any more... Other
HDMI devices are more probably robust).

I have also tested HPD and could make events passed to user-space by setting poll_mode.

There is one subtle thing in ingenic-drm-drv I do not yet understand:

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

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 with JZ_LCD_OSDC_OSDEN breaks it.

I didn't notice before because my test setup had some additional private patches
+ reverts and it did not properly revert to regmap_write.

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.

BR and thanks,
Nikolaus

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

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

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

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.

Cheers,
-Paul

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

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

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,

> Am 18.01.2022 um 23:59 schrieb Paul Boddie <paul@boddie.org.uk>:
> 
> 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.
> 
>> 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.

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.

BR,
Nikolaus

Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Boddie]

On Wednesday, 19 January 2022 07:40:22 CET H. Nikolaus Schaller wrote:
> Hi Paul,
> 
> > Am 18.01.2022 um 23:59 schrieb Paul Boddie <paul@boddie.org.uk>:
> > 
> > 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.
> > 
> > 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?

The descriptors will cause it to be set when the peripheral is enabled, as far 
as I can tell.

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

It may be set in various versions of the Linux driver, but my observation was 
that in a non-Linux environment where nothing else is setting anything in the 
register concerned, initialising the descriptors seems to enable OSD and the 
OSD alpha enable bit.

Yesterday, I did consider what might be done to avoid the alpha bit being set, 
but I didn't immediately see anything in the descriptor fields that would 
offer such an alternative. The bit in question seems to be a global alpha 
enable setting, and so choosing per-pixel alpha would probably also result in 
it being set, although I didn't fire up the CI20 to check.

Paul