[PATCH 0/6] drm: rcar-du: Misc patches

[PATCH 0/6] drm: rcar-du: Misc patches

[Tomi Valkeinen]

From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Hi,

Here are some small rcar-du patches based on commits in the Renesas BSP
tree.

 Tomi

Koji Matsuoka (2):
  drm: rcar-du: lvds: Add reset control
  drm: rcar-du: Fix LVDS stop sequence

Tomi Valkeinen (4):
  drm: rcar-du: dsi: add 'select RESET_CONTROLLER'
  drm: rcar-du: Add quirk for H3 ES1 pclk WA
  drm: rcar-du: Fix setting a reserved bit in DPLLCR
  drm: rcar-du: Stop accessing non-existant registers on gen4

 drivers/gpu/drm/rcar-du/Kconfig         |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 28 +++++++------
 drivers/gpu/drm/rcar-du/rcar_du_drv.c   | 52 ++++++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_group.c |  6 ++-
 drivers/gpu/drm/rcar-du/rcar_du_regs.h  |  3 +-
 drivers/gpu/drm/rcar-du/rcar_lvds.c     | 42 +++++++++++++++++++-
 7 files changed, 118 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH 1/6] drm: rcar-du: dsi: add 'select RESET_CONTROLLER'

[Tomi Valkeinen]

The RCAR DSI driver uses reset controller, so we should select it in the
Kconfig.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index fd2c2eaee26b..a8f862c68b4f 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -55,6 +55,7 @@ config DRM_RCAR_MIPI_DSI
 	def_tristate DRM_RCAR_DU
 	depends on DRM_RCAR_USE_MIPI_DSI
 	select DRM_MIPI_DSI
+	select RESET_CONTROLLER
 
 config DRM_RCAR_VSP
 	bool "R-Car DU VSP Compositor Support" if ARM
-- 
2.34.1

[PATCH 2/6] drm: rcar-du: lvds: Add reset control

[Tomi Valkeinen]

From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

Reset LVDS using the reset control as CPG reset/release is required in
H/W manual sequence.

Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com>
[tomi.valkeinen: Rewrite the patch description]
Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/Kconfig     |  1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index a8f862c68b4f..151e400b996d 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -43,6 +43,7 @@ config DRM_RCAR_LVDS
 	select DRM_PANEL
 	select OF_FLATTREE
 	select OF_OVERLAY
+	select RESET_CONTROLLER
 
 config DRM_RCAR_USE_MIPI_DSI
 	bool "R-Car DU MIPI DSI Encoder Support"
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 81a060c2fe3f..674b727cdaa2 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -16,6 +16,7 @@
 #include <linux/of_device.h>
 #include <linux/of_graph.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
 
@@ -60,6 +61,7 @@ struct rcar_lvds_device_info {
 struct rcar_lvds {
 	struct device *dev;
 	const struct rcar_lvds_device_info *info;
+	struct reset_control *rstc;
 
 	struct drm_bridge bridge;
 
@@ -316,6 +318,8 @@ int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq)
 
 	dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq);
 
+	reset_control_deassert(lvds->rstc);
+
 	ret = clk_prepare_enable(lvds->clocks.mod);
 	if (ret < 0)
 		return ret;
@@ -338,6 +342,8 @@ void rcar_lvds_pclk_disable(struct drm_bridge *bridge)
 	rcar_lvds_write(lvds, LVDPLLCR, 0);
 
 	clk_disable_unprepare(lvds->clocks.mod);
+
+	reset_control_assert(lvds->rstc);
 }
 EXPORT_SYMBOL_GPL(rcar_lvds_pclk_disable);
 
@@ -396,6 +402,8 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
 	u32 lvdcr0;
 	int ret;
 
+	reset_control_deassert(lvds->rstc);
+
 	ret = clk_prepare_enable(lvds->clocks.mod);
 	if (ret < 0)
 		return;
@@ -552,6 +560,7 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
 						       old_bridge_state);
 
 	clk_disable_unprepare(lvds->clocks.mod);
+	reset_control_assert(lvds->rstc);
 }
 
 static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
@@ -844,6 +853,12 @@ static int rcar_lvds_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	lvds->rstc = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(lvds->rstc)) {
+		dev_err(&pdev->dev, "failed to get cpg reset\n");
+		return PTR_ERR(lvds->rstc);
+	}
+
 	drm_bridge_add(&lvds->bridge);
 
 	return 0;
-- 
2.34.1

[PATCH 3/6] drm: rcar-du: Fix LVDS stop sequence

[Tomi Valkeinen]

From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

According to H/W manual, LVDCR0 register must be cleared bit by bit when
disabling LVDS.

Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com>
[tomi.valkeinen: simplified the code a bit]
Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 674b727cdaa2..01800cef1c33 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -87,6 +87,11 @@ static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
 	iowrite32(data, lvds->mmio + reg);
 }
 
+static u32 rcar_lvds_read(struct rcar_lvds *lvds, u32 reg)
+{
+	return ioread32(lvds->mmio + reg);
+}
+
 /* -----------------------------------------------------------------------------
  * PLL Setup
  */
@@ -549,8 +554,28 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
 				     struct drm_bridge_state *old_bridge_state)
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
+	u32 lvdcr0;
+
+	lvdcr0 = rcar_lvds_read(lvds, LVDCR0);
+
+	lvdcr0 &= ~LVDCR0_LVRES;
+	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
+
+	if (lvds->info->quirks & RCAR_LVDS_QUIRK_GEN3_LVEN) {
+		lvdcr0 &= ~LVDCR0_LVEN;
+		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
+	}
+
+	if (lvds->info->quirks & RCAR_LVDS_QUIRK_PWD) {
+		lvdcr0 &= ~LVDCR0_PWD;
+		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
+	}
+
+	if (!(lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)) {
+		lvdcr0 &= ~LVDCR0_PLLON;
+		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
+	}
 
-	rcar_lvds_write(lvds, LVDCR0, 0);
 	rcar_lvds_write(lvds, LVDCR1, 0);
 	rcar_lvds_write(lvds, LVDPLLCR, 0);
 
-- 
2.34.1

[PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA

[Tomi Valkeinen]

rcar_du_crtc.c does a soc_device_match() in
rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and
if so, apply a WA.

We will need another H3 ES1 check in the following patch, so rather than
adding more soc_device_match() calls, let's add a rcar_du_device_info
entry for the ES1, and a quirk flag,
RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  8 +---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 51 +++++++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
 3 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 3619e1ddeb62..f2d3266509cc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -10,7 +10,6 @@
 #include <linux/clk.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
-#include <linux/sys_soc.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -204,11 +203,6 @@ static void rcar_du_escr_divider(struct clk *clk, unsigned long target,
 	}
 }
 
-static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
-	{ .soc_id = "r8a7795", .revision = "ES1.*" },
-	{ /* sentinel */ }
-};
-
 static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 {
 	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
@@ -238,7 +232,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 		 * no post-divider when a display PLL is present (as shown by
 		 * the workaround breaking HDMI output on M3-W during testing).
 		 */
-		if (soc_device_match(rcar_du_r8a7795_es1)) {
+		if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY) {
 			target *= 2;
 			div = 1;
 		}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index c7c5217cfc1a..ba2e069fc0f7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
+#include <linux/sys_soc.h>
 #include <linux/wait.h>
 
 #include <drm/drm_atomic_helper.h>
@@ -386,6 +387,42 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
 	.dpll_mask =  BIT(2) | BIT(1),
 };
 
+static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
+	.gen = 3,
+	.features = RCAR_DU_FEATURE_CRTC_IRQ
+		  | RCAR_DU_FEATURE_CRTC_CLOCK
+		  | RCAR_DU_FEATURE_VSP1_SOURCE
+		  | RCAR_DU_FEATURE_INTERLACED
+		  | RCAR_DU_FEATURE_TVM_SYNC,
+	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
+	.channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
+	.routes = {
+		/*
+		 * R8A7795 has one RGB output, two HDMI outputs and one
+		 * LVDS output.
+		 */
+		[RCAR_DU_OUTPUT_DPAD0] = {
+			.possible_crtcs = BIT(3),
+			.port = 0,
+		},
+		[RCAR_DU_OUTPUT_HDMI0] = {
+			.possible_crtcs = BIT(1),
+			.port = 1,
+		},
+		[RCAR_DU_OUTPUT_HDMI1] = {
+			.possible_crtcs = BIT(2),
+			.port = 2,
+		},
+		[RCAR_DU_OUTPUT_LVDS0] = {
+			.possible_crtcs = BIT(0),
+			.port = 3,
+		},
+	},
+	.num_lvds = 1,
+	.num_rpf = 5,
+	.dpll_mask =  BIT(2) | BIT(1),
+};
+
 static const struct rcar_du_device_info rcar_du_r8a7796_info = {
 	.gen = 3,
 	.features = RCAR_DU_FEATURE_CRTC_IRQ
@@ -576,6 +613,11 @@ static const struct of_device_id rcar_du_of_table[] = {
 
 MODULE_DEVICE_TABLE(of, rcar_du_of_table);
 
+static const struct soc_device_attribute rcar_du_soc_table[] = {
+	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &rcar_du_r8a7795_es1_info },
+	{ /* sentinel */ }
+};
+
 const char *rcar_du_output_name(enum rcar_du_output output)
 {
 	static const char * const names[] = {
@@ -670,6 +712,7 @@ static int rcar_du_probe(struct platform_device *pdev)
 	struct rcar_du_device *rcdu;
 	unsigned int mask;
 	int ret;
+	const struct soc_device_attribute *soc_attr;
 
 	if (drm_firmware_drivers_only())
 		return -ENODEV;
@@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev)
 		return PTR_ERR(rcdu);
 
 	rcdu->dev = &pdev->dev;
-	rcdu->info = of_device_get_match_data(rcdu->dev);
+
+	soc_attr = soc_device_match(rcar_du_soc_table);
+	if (soc_attr)
+		rcdu->info = soc_attr->data;
+
+	if (!rcdu->info)
+		rcdu->info = of_device_get_match_data(rcdu->dev);
 
 	platform_set_drvdata(pdev, rcdu);
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index 5cfa2bb7ad93..df87ccab146f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -34,6 +34,7 @@ struct rcar_du_device;
 #define RCAR_DU_FEATURE_NO_BLENDING	BIT(5)	/* PnMR.SPIM does not have ALP nor EOR bits */
 
 #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
+#define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1)	/* H3 ES1 has pclk stability issue */
 
 enum rcar_du_output {
 	RCAR_DU_OUTPUT_DPAD0,
-- 
2.34.1

[PATCH 5/6] drm: rcar-du: Fix setting a reserved bit in DPLLCR

[Tomi Valkeinen]

On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock
source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On
non-ES1, only the higher bits are used (bits 21 and 23), and the lower
bits are reserved and should be set to 0 (or not set at all).

The current code always sets the lower bits, even on non-ES1.

For both DU1 and DU2, on all SoC versions, when writing zeroes to those
bits the input clock is DCLKIN, and thus there's no difference between
ES1 and non-ES1.

For DU1, writing 0b10 to the bits (or only writing the higher bit)
results in using PLL0 as the input clock, so in this case there's also
no difference between ES1 and non-ES1.

However, for DU2, writing 0b10 to the bits results in using PLL0 as the
input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1
you need to write 0b11 to select PLL1.

The current code always writes 0b11 to PLCS0 field to select PLL1 on all
SoC versions, which works but causes an illegal (in the sense of not
allowed by the documentation) write to a reserved bit field.

To remove the illegal bit write on PLSC0 we need to handle the input dot
clock selection differently for ES1 and non-ES1.

Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new
rcar_du_device_info entry for the ES1 SoC. Using these, we can always
set the bit 21 on PLSC0 when choosing the PLL as the source clock, and
additionally set the bit 20 when on ES1.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++--
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  3 ++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
 drivers/gpu/drm/rcar-du/rcar_du_regs.h |  3 ++-
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index f2d3266509cc..8d660a6141bf 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 		       | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
 		       | DPLLCR_STBY;
 
-		if (rcrtc->index == 1)
+		if (rcrtc->index == 1) {
 			dpllcr |= DPLLCR_PLCS1
 			       |  DPLLCR_INCS_DOTCLKIN1;
-		else
+		} else {
 			dpllcr |= DPLLCR_PLCS0
 			       |  DPLLCR_INCS_DOTCLKIN0;
+			/*
+			 * On H3 ES1.x, in addition to setting bit 21 (PLCS0),
+			 * also bit 20 has to be set to select PLL1 as the
+			 * clock source.
+			 */
+			if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PLL)
+				dpllcr |= DPLLCR_PLCS0_H3ES1X_PLL1_SEL;
+		}
 
 		rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr);
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index ba2e069fc0f7..d689f2510081 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -394,7 +394,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
 		  | RCAR_DU_FEATURE_VSP1_SOURCE
 		  | RCAR_DU_FEATURE_INTERLACED
 		  | RCAR_DU_FEATURE_TVM_SYNC,
-	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
+	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY
+		| RCAR_DU_QUIRK_H3_ES1_PLL,
 	.channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
 	.routes = {
 		/*
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index df87ccab146f..acc3673fefe1 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -35,6 +35,7 @@ struct rcar_du_device;
 
 #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
 #define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1)	/* H3 ES1 has pclk stability issue */
+#define RCAR_DU_QUIRK_H3_ES1_PLL	BIT(2)	/* H3 ES1 PLL setup differs from non-ES1 */
 
 enum rcar_du_output {
 	RCAR_DU_OUTPUT_DPAD0,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index c1bcb0e8b5b4..94d913f66c8f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -288,7 +288,8 @@
  * isn't implemented by other SoC in the Gen3 family it can safely be set
  * unconditionally.
  */
-#define DPLLCR_PLCS0		(3 << 20)
+#define DPLLCR_PLCS0		(1 << 21)
+#define DPLLCR_PLCS0_H3ES1X_PLL1_SEL	(1 << 20)
 #define DPLLCR_CLKE		(1 << 18)
 #define DPLLCR_FDPLL(n)		((n) << 12)
 #define DPLLCR_N(n)		((n) << 5)
-- 
2.34.1

[PATCH 6/6] drm: rcar-du: Stop accessing non-existant registers on gen4

[Tomi Valkeinen]

The following registers do not exist on gen4, so we should not write
them: DEF6Rm, DEF8Rm, ESCRn, OTARn.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 8 +++++---
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 6 ++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 8d660a6141bf..56b23333993c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -289,10 +289,12 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 		escr = params.escr;
 	}
 
-	dev_dbg(rcrtc->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
+	if (rcdu->info->gen < 4) {
+		dev_dbg(rcrtc->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
 
-	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
-	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? OTAR13 : OTAR02, 0);
+		rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
+		rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? OTAR13 : OTAR02, 0);
+	}
 
 	/* Signal polarities */
 	dsmr = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 6da01760ede5..c236e2aa8a01 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -148,7 +148,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
 	}
 	rcar_du_group_write(rgrp, DEFR5, DEFR5_CODE | DEFR5_DEFE5);
 
-	rcar_du_group_setup_pins(rgrp);
+	if (rcdu->info->gen < 4)
+		rcar_du_group_setup_pins(rgrp);
 
 	/*
 	 * TODO: Handle routing of the DU output to CMM dynamically, as we
@@ -160,7 +161,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
 	rcar_du_group_write(rgrp, DEFR7, defr7);
 
 	if (rcdu->info->gen >= 2) {
-		rcar_du_group_setup_defr8(rgrp);
+		if (rcdu->info->gen < 4)
+			rcar_du_group_setup_defr8(rgrp);
 		rcar_du_group_setup_didsr(rgrp);
 	}
 
-- 
2.34.1

Re: [PATCH 2/6] drm: rcar-du: lvds: Add reset control

[Geert Uytterhoeven]

Hi Tomi,

On Tue, Jan 17, 2023 at 2:54 PM Tomi Valkeinen
<tomi.valkeinen+renesas@ideasonboard.com> wrote:
> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>
> Reset LVDS using the reset control as CPG reset/release is required in
> H/W manual sequence.
>
> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com>
> [tomi.valkeinen: Rewrite the patch description]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Thanks for your patch!

> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c

> @@ -844,6 +853,12 @@ static int rcar_lvds_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 return ret;
>
> +       lvds->rstc = devm_reset_control_get(&pdev->dev, NULL);

devm_reset_control_get_exclusive()

> +       if (IS_ERR(lvds->rstc)) {
> +               dev_err(&pdev->dev, "failed to get cpg reset\n");
> +               return PTR_ERR(lvds->rstc);

return dev_err_probe(...);

> +       }
> +
>         drm_bridge_add(&lvds->bridge);
>
>         return 0;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA

[Geert Uytterhoeven]

Hi Tomi,

On Tue, Jan 17, 2023 at 2:54 PM Tomi Valkeinen
<tomi.valkeinen+renesas@ideasonboard.com> wrote:
> rcar_du_crtc.c does a soc_device_match() in
> rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and
> if so, apply a WA.
>
> We will need another H3 ES1 check in the following patch, so rather than
> adding more soc_device_match() calls, let's add a rcar_du_device_info
> entry for the ES1, and a quirk flag,
> RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Thanks for your patch!

> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c

> @@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev)
>                 return PTR_ERR(rcdu);
>
>         rcdu->dev = &pdev->dev;
> -       rcdu->info = of_device_get_match_data(rcdu->dev);

No need to remove this line...

> +
> +       soc_attr = soc_device_match(rcar_du_soc_table);
> +       if (soc_attr)
> +               rcdu->info = soc_attr->data;
> +
> +       if (!rcdu->info)
> +               rcdu->info = of_device_get_match_data(rcdu->dev);

... and no need to add these two lines.
The idiom is to set rcdu->info based on of_device_get_match_data()
first, and override based on of_device_get_match_data() when needed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] drm: rcar-du: dsi: add 'select RESET_CONTROLLER'

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Jan 17, 2023 at 03:51:49PM +0200, Tomi Valkeinen wrote:
> The RCAR DSI driver uses reset controller, so we should select it in the
> Kconfig.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index fd2c2eaee26b..a8f862c68b4f 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -55,6 +55,7 @@ config DRM_RCAR_MIPI_DSI
>  	def_tristate DRM_RCAR_DU
>  	depends on DRM_RCAR_USE_MIPI_DSI
>  	select DRM_MIPI_DSI
> +	select RESET_CONTROLLER
>  
>  config DRM_RCAR_VSP
>  	bool "R-Car DU VSP Compositor Support" if ARM

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] drm: rcar-du: lvds: Add reset control

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Jan 17, 2023 at 03:51:50PM +0200, Tomi Valkeinen wrote:
> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> 
> Reset LVDS using the reset control as CPG reset/release is required in
> H/W manual sequence.

s@H/W@the hardware@

> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com>
> [tomi.valkeinen: Rewrite the patch description]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/Kconfig     |  1 +
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 15 +++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index a8f862c68b4f..151e400b996d 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -43,6 +43,7 @@ config DRM_RCAR_LVDS
>  	select DRM_PANEL
>  	select OF_FLATTREE
>  	select OF_OVERLAY
> +	select RESET_CONTROLLER
>  
>  config DRM_RCAR_USE_MIPI_DSI
>  	bool "R-Car DU MIPI DSI Encoder Support"
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 81a060c2fe3f..674b727cdaa2 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/sys_soc.h>
>  
> @@ -60,6 +61,7 @@ struct rcar_lvds_device_info {
>  struct rcar_lvds {
>  	struct device *dev;
>  	const struct rcar_lvds_device_info *info;
> +	struct reset_control *rstc;
>  
>  	struct drm_bridge bridge;
>  
> @@ -316,6 +318,8 @@ int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq)
>  
>  	dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq);
>  
> +	reset_control_deassert(lvds->rstc);

Can this fail ? Same in __rcar_lvds_atomic_enable().

> +
>  	ret = clk_prepare_enable(lvds->clocks.mod);

It would be nice to switch this driver to runtime PM and move reset and
clock handling to the suspend/resume handlers. A candidate for a future
patch.

>  	if (ret < 0)

Missing reset_control_assert(). Same in other error paths if applicable,
here and in __rcar_lvds_atomic_enable().

>  		return ret;
> @@ -338,6 +342,8 @@ void rcar_lvds_pclk_disable(struct drm_bridge *bridge)
>  	rcar_lvds_write(lvds, LVDPLLCR, 0);
>  
>  	clk_disable_unprepare(lvds->clocks.mod);
> +
> +	reset_control_assert(lvds->rstc);
>  }
>  EXPORT_SYMBOL_GPL(rcar_lvds_pclk_disable);
>  
> @@ -396,6 +402,8 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
>  	u32 lvdcr0;
>  	int ret;
>  
> +	reset_control_deassert(lvds->rstc);
> +
>  	ret = clk_prepare_enable(lvds->clocks.mod);
>  	if (ret < 0)
>  		return;
> @@ -552,6 +560,7 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
>  						       old_bridge_state);
>  
>  	clk_disable_unprepare(lvds->clocks.mod);
> +	reset_control_assert(lvds->rstc);
>  }
>  
>  static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
> @@ -844,6 +853,12 @@ static int rcar_lvds_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	lvds->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(lvds->rstc)) {
> +		dev_err(&pdev->dev, "failed to get cpg reset\n");
> +		return PTR_ERR(lvds->rstc);
> +	}
> +
>  	drm_bridge_add(&lvds->bridge);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] drm: rcar-du: Fix LVDS stop sequence

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Jan 17, 2023 at 03:51:51PM +0200, Tomi Valkeinen wrote:
> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> 
> According to H/W manual, LVDCR0 register must be cleared bit by bit when

s@H/W@the hardware/

> disabling LVDS.

I don't like this much, but I think I'll stop fighting :-)

> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com>
> [tomi.valkeinen: simplified the code a bit]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 674b727cdaa2..01800cef1c33 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -87,6 +87,11 @@ static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
>  	iowrite32(data, lvds->mmio + reg);
>  }
>  
> +static u32 rcar_lvds_read(struct rcar_lvds *lvds, u32 reg)
> +{
> +	return ioread32(lvds->mmio + reg);
> +}

Could you please move read before write ?

> +
>  /* -----------------------------------------------------------------------------
>   * PLL Setup
>   */
> @@ -549,8 +554,28 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
>  				     struct drm_bridge_state *old_bridge_state)
>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> +	u32 lvdcr0;
> +
> +	lvdcr0 = rcar_lvds_read(lvds, LVDCR0);
> +
> +	lvdcr0 &= ~LVDCR0_LVRES;
> +	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> +
> +	if (lvds->info->quirks & RCAR_LVDS_QUIRK_GEN3_LVEN) {
> +		lvdcr0 &= ~LVDCR0_LVEN;
> +		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> +	}
> +
> +	if (lvds->info->quirks & RCAR_LVDS_QUIRK_PWD) {
> +		lvdcr0 &= ~LVDCR0_PWD;
> +		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> +	}
> +
> +	if (!(lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)) {
> +		lvdcr0 &= ~LVDCR0_PLLON;
> +		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> +	}

This will leave LVDCR0_BEN and LVDCR0_LVEN on Gen2. Is that fine ?

>  
> -	rcar_lvds_write(lvds, LVDCR0, 0);
>  	rcar_lvds_write(lvds, LVDCR1, 0);
>  	rcar_lvds_write(lvds, LVDPLLCR, 0);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Jan 17, 2023 at 03:51:52PM +0200, Tomi Valkeinen wrote:
> rcar_du_crtc.c does a soc_device_match() in
> rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and

s/ES1/ES1.x/

Same below.

> if so, apply a WA.

s/WA/workaround/

Same below.

> We will need another H3 ES1 check in the following patch, so rather than
> adding more soc_device_match() calls, let's add a rcar_du_device_info
> entry for the ES1, and a quirk flag,
> RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  8 +---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 51 +++++++++++++++++++++++++-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
>  3 files changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 3619e1ddeb62..f2d3266509cc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -10,7 +10,6 @@
>  #include <linux/clk.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> -#include <linux/sys_soc.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -204,11 +203,6 @@ static void rcar_du_escr_divider(struct clk *clk, unsigned long target,
>  	}
>  }
>  
> -static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> -	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> -	{ /* sentinel */ }
> -};
> -
>  static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  {
>  	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
> @@ -238,7 +232,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  		 * no post-divider when a display PLL is present (as shown by
>  		 * the workaround breaking HDMI output on M3-W during testing).
>  		 */
> -		if (soc_device_match(rcar_du_r8a7795_es1)) {
> +		if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY) {
>  			target *= 2;
>  			div = 1;
>  		}
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index c7c5217cfc1a..ba2e069fc0f7 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -16,6 +16,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> +#include <linux/sys_soc.h>
>  #include <linux/wait.h>
>  
>  #include <drm/drm_atomic_helper.h>
> @@ -386,6 +387,42 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
>  	.dpll_mask =  BIT(2) | BIT(1),
>  };
>  
> +static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
> +	.gen = 3,
> +	.features = RCAR_DU_FEATURE_CRTC_IRQ
> +		  | RCAR_DU_FEATURE_CRTC_CLOCK
> +		  | RCAR_DU_FEATURE_VSP1_SOURCE
> +		  | RCAR_DU_FEATURE_INTERLACED
> +		  | RCAR_DU_FEATURE_TVM_SYNC,
> +	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
> +	.channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
> +	.routes = {
> +		/*
> +		 * R8A7795 has one RGB output, two HDMI outputs and one
> +		 * LVDS output.
> +		 */
> +		[RCAR_DU_OUTPUT_DPAD0] = {
> +			.possible_crtcs = BIT(3),
> +			.port = 0,
> +		},
> +		[RCAR_DU_OUTPUT_HDMI0] = {
> +			.possible_crtcs = BIT(1),
> +			.port = 1,
> +		},
> +		[RCAR_DU_OUTPUT_HDMI1] = {
> +			.possible_crtcs = BIT(2),
> +			.port = 2,
> +		},
> +		[RCAR_DU_OUTPUT_LVDS0] = {
> +			.possible_crtcs = BIT(0),
> +			.port = 3,
> +		},
> +	},
> +	.num_lvds = 1,
> +	.num_rpf = 5,
> +	.dpll_mask =  BIT(2) | BIT(1),
> +};
> +
>  static const struct rcar_du_device_info rcar_du_r8a7796_info = {
>  	.gen = 3,
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ
> @@ -576,6 +613,11 @@ static const struct of_device_id rcar_du_of_table[] = {
>  
>  MODULE_DEVICE_TABLE(of, rcar_du_of_table);
>  
> +static const struct soc_device_attribute rcar_du_soc_table[] = {
> +	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &rcar_du_r8a7795_es1_info },
> +	{ /* sentinel */ }
> +};
> +
>  const char *rcar_du_output_name(enum rcar_du_output output)
>  {
>  	static const char * const names[] = {
> @@ -670,6 +712,7 @@ static int rcar_du_probe(struct platform_device *pdev)
>  	struct rcar_du_device *rcdu;
>  	unsigned int mask;
>  	int ret;
> +	const struct soc_device_attribute *soc_attr;

Please move this up before rcdu.

>  
>  	if (drm_firmware_drivers_only())
>  		return -ENODEV;
> @@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev)
>  		return PTR_ERR(rcdu);
>  
>  	rcdu->dev = &pdev->dev;
> -	rcdu->info = of_device_get_match_data(rcdu->dev);
> +
> +	soc_attr = soc_device_match(rcar_du_soc_table);
> +	if (soc_attr)
> +		rcdu->info = soc_attr->data;
> +
> +	if (!rcdu->info)
> +		rcdu->info = of_device_get_match_data(rcdu->dev);

As Geert mentioned,

	rcdu->info = of_device_get_match_data(rcdu->dev);

	soc_attr = soc_device_match(rcar_du_soc_table);
	if (soc_attr)
		rcdu->info = soc_attr->data;

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

>  
>  	platform_set_drvdata(pdev, rcdu);
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index 5cfa2bb7ad93..df87ccab146f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -34,6 +34,7 @@ struct rcar_du_device;
>  #define RCAR_DU_FEATURE_NO_BLENDING	BIT(5)	/* PnMR.SPIM does not have ALP nor EOR bits */
>  
>  #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
> +#define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1)	/* H3 ES1 has pclk stability issue */
>  
>  enum rcar_du_output {
>  	RCAR_DU_OUTPUT_DPAD0,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] drm: rcar-du: Fix setting a reserved bit in DPLLCR

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote:
> On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock

s/ES1/ES1.x/

Same below.

> source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On
> non-ES1, only the higher bits are used (bits 21 and 23), and the lower
> bits are reserved and should be set to 0 (or not set at all).

How do you not set a bit ? :-)

> The current code always sets the lower bits, even on non-ES1.

I think that's harmless, and not worth making the driver more complex,
but I'll stop fighting.

> For both DU1 and DU2, on all SoC versions, when writing zeroes to those
> bits the input clock is DCLKIN, and thus there's no difference between
> ES1 and non-ES1.
> 
> For DU1, writing 0b10 to the bits (or only writing the higher bit)
> results in using PLL0 as the input clock, so in this case there's also
> no difference between ES1 and non-ES1.
> 
> However, for DU2, writing 0b10 to the bits results in using PLL0 as the
> input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1
> you need to write 0b11 to select PLL1.
> 
> The current code always writes 0b11 to PLCS0 field to select PLL1 on all
> SoC versions, which works but causes an illegal (in the sense of not
> allowed by the documentation) write to a reserved bit field.
> 
> To remove the illegal bit write on PLSC0 we need to handle the input dot
> clock selection differently for ES1 and non-ES1.
> 
> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new
> rcar_du_device_info entry for the ES1 SoC. Using these, we can always

The new entry was added in the previous patch already.

> set the bit 21 on PLSC0 when choosing the PLL as the source clock, and
> additionally set the bit 20 when on ES1.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++--
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  3 ++-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h |  3 ++-
>  4 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index f2d3266509cc..8d660a6141bf 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  		       | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
>  		       | DPLLCR_STBY;
>  
> -		if (rcrtc->index == 1)
> +		if (rcrtc->index == 1) {
>  			dpllcr |= DPLLCR_PLCS1
>  			       |  DPLLCR_INCS_DOTCLKIN1;
> -		else
> +		} else {
>  			dpllcr |= DPLLCR_PLCS0
>  			       |  DPLLCR_INCS_DOTCLKIN0;
> +			/*
> +			 * On H3 ES1.x, in addition to setting bit 21 (PLCS0),
> +			 * also bit 20 has to be set to select PLL1 as the
> +			 * clock source.

I'd add "On ES2 and newer, PLL1 is selected unconditionally.".

> +			 */
> +			if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PLL)
> +				dpllcr |= DPLLCR_PLCS0_H3ES1X_PLL1_SEL;
> +		}
>  
>  		rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr);
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index ba2e069fc0f7..d689f2510081 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -394,7 +394,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
>  		  | RCAR_DU_FEATURE_VSP1_SOURCE
>  		  | RCAR_DU_FEATURE_INTERLACED
>  		  | RCAR_DU_FEATURE_TVM_SYNC,
> -	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
> +	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY
> +		| RCAR_DU_QUIRK_H3_ES1_PLL,
>  	.channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
>  	.routes = {
>  		/*
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index df87ccab146f..acc3673fefe1 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -35,6 +35,7 @@ struct rcar_du_device;
>  
>  #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
>  #define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1)	/* H3 ES1 has pclk stability issue */
> +#define RCAR_DU_QUIRK_H3_ES1_PLL	BIT(2)	/* H3 ES1 PLL setup differs from non-ES1 */
>  
>  enum rcar_du_output {
>  	RCAR_DU_OUTPUT_DPAD0,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> index c1bcb0e8b5b4..94d913f66c8f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -288,7 +288,8 @@
>   * isn't implemented by other SoC in the Gen3 family it can safely be set
>   * unconditionally.
>   */

This comment should be dropped.

> -#define DPLLCR_PLCS0		(3 << 20)
> +#define DPLLCR_PLCS0		(1 << 21)
> +#define DPLLCR_PLCS0_H3ES1X_PLL1_SEL	(1 << 20)

Bit 21 selects between DCLKIN (when 0) and PLL (when 1). On ES1.x, bit
20 selects between PLL0 (when 0) and PLL1 (when 1), while on ES2 and
newer, PLL1 is selected unconditionally. Could we name the two bits
accodingly ? Maybe

#define DPLLCR_PLCS0_PLL	(1 << 21)
#define DPLLCR_PLCS0_PLL1	(1 << 20)

>  #define DPLLCR_CLKE		(1 << 18)
>  #define DPLLCR_FDPLL(n)		((n) << 12)
>  #define DPLLCR_N(n)		((n) << 5)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/6] drm: rcar-du: Stop accessing non-existant registers on gen4

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Jan 17, 2023 at 03:51:54PM +0200, Tomi Valkeinen wrote:
> The following registers do not exist on gen4, so we should not write
> them: DEF6Rm, DEF8Rm, ESCRn, OTARn.

I think DEF7Rm should also be skipped. With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 8 +++++---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 6 ++++--
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 8d660a6141bf..56b23333993c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -289,10 +289,12 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  		escr = params.escr;
>  	}
>  
> -	dev_dbg(rcrtc->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> +	if (rcdu->info->gen < 4) {
> +		dev_dbg(rcrtc->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
>  
> -	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
> -	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? OTAR13 : OTAR02, 0);
> +		rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
> +		rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? OTAR13 : OTAR02, 0);
> +	}
>  
>  	/* Signal polarities */
>  	dsmr = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0)
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 6da01760ede5..c236e2aa8a01 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -148,7 +148,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>  	}
>  	rcar_du_group_write(rgrp, DEFR5, DEFR5_CODE | DEFR5_DEFE5);
>  
> -	rcar_du_group_setup_pins(rgrp);
> +	if (rcdu->info->gen < 4)
> +		rcar_du_group_setup_pins(rgrp);
>  
>  	/*
>  	 * TODO: Handle routing of the DU output to CMM dynamically, as we
> @@ -160,7 +161,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>  	rcar_du_group_write(rgrp, DEFR7, defr7);
>  
>  	if (rcdu->info->gen >= 2) {
> -		rcar_du_group_setup_defr8(rgrp);
> +		if (rcdu->info->gen < 4)
> +			rcar_du_group_setup_defr8(rgrp);
>  		rcar_du_group_setup_didsr(rgrp);
>  	}
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] drm: rcar-du: lvds: Add reset control

[Tomi Valkeinen]

On 17/01/2023 18:04, Geert Uytterhoeven wrote:
> Hi Tomi,
> 
> On Tue, Jan 17, 2023 at 2:54 PM Tomi Valkeinen
> <tomi.valkeinen+renesas@ideasonboard.com> wrote:
>> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>>
>> Reset LVDS using the reset control as CPG reset/release is required in
>> H/W manual sequence.
>>
>> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>> Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com>
>> [tomi.valkeinen: Rewrite the patch description]
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> 
>> @@ -844,6 +853,12 @@ static int rcar_lvds_probe(struct platform_device *pdev)
>>          if (ret < 0)
>>                  return ret;
>>
>> +       lvds->rstc = devm_reset_control_get(&pdev->dev, NULL);
> 
> devm_reset_control_get_exclusive()

Ok.

>> +       if (IS_ERR(lvds->rstc)) {
>> +               dev_err(&pdev->dev, "failed to get cpg reset\n");
>> +               return PTR_ERR(lvds->rstc);
> 
> return dev_err_probe(...);

Right.

Thanks!

  Tomi

Re: [PATCH 2/6] drm: rcar-du: lvds: Add reset control

[Tomi Valkeinen]

On 18/01/2023 23:06, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Jan 17, 2023 at 03:51:50PM +0200, Tomi Valkeinen wrote:
>> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>>
>> Reset LVDS using the reset control as CPG reset/release is required in
>> H/W manual sequence.
> 
> s@H/W@the hardware@
> 
>> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>> Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com>
>> [tomi.valkeinen: Rewrite the patch description]
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/rcar-du/Kconfig     |  1 +
>>   drivers/gpu/drm/rcar-du/rcar_lvds.c | 15 +++++++++++++++
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
>> index a8f862c68b4f..151e400b996d 100644
>> --- a/drivers/gpu/drm/rcar-du/Kconfig
>> +++ b/drivers/gpu/drm/rcar-du/Kconfig
>> @@ -43,6 +43,7 @@ config DRM_RCAR_LVDS
>>   	select DRM_PANEL
>>   	select OF_FLATTREE
>>   	select OF_OVERLAY
>> +	select RESET_CONTROLLER
>>   
>>   config DRM_RCAR_USE_MIPI_DSI
>>   	bool "R-Car DU MIPI DSI Encoder Support"
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>> index 81a060c2fe3f..674b727cdaa2 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/of_device.h>
>>   #include <linux/of_graph.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/reset.h>
>>   #include <linux/slab.h>
>>   #include <linux/sys_soc.h>
>>   
>> @@ -60,6 +61,7 @@ struct rcar_lvds_device_info {
>>   struct rcar_lvds {
>>   	struct device *dev;
>>   	const struct rcar_lvds_device_info *info;
>> +	struct reset_control *rstc;
>>   
>>   	struct drm_bridge bridge;
>>   
>> @@ -316,6 +318,8 @@ int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq)
>>   
>>   	dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq);
>>   
>> +	reset_control_deassert(lvds->rstc);
> 
> Can this fail ? Same in __rcar_lvds_atomic_enable().

Yes. Too hasty in picking a patch from the BSP =).

>> +
>>   	ret = clk_prepare_enable(lvds->clocks.mod);
> 
> It would be nice to switch this driver to runtime PM and move reset and
> clock handling to the suspend/resume handlers. A candidate for a future
> patch.

I have the runtime PM patch in my work branch, but on top. I'll pick 
that into this series, before adding the reset control. Makes error 
handling a bit simpler.

>>   	if (ret < 0)
> 
> Missing reset_control_assert(). Same in other error paths if applicable,
> here and in __rcar_lvds_atomic_enable().

Yep.

  Tomi

Re: [PATCH 3/6] drm: rcar-du: Fix LVDS stop sequence

[Tomi Valkeinen]

On 18/01/2023 23:12, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Jan 17, 2023 at 03:51:51PM +0200, Tomi Valkeinen wrote:
>> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>>
>> According to H/W manual, LVDCR0 register must be cleared bit by bit when
> 
> s@H/W@the hardware/
> 
>> disabling LVDS.
> 
> I don't like this much, but I think I'll stop fighting :-)
> 
>> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>> Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com>
>> [tomi.valkeinen: simplified the code a bit]
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/rcar-du/rcar_lvds.c | 27 ++++++++++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>> index 674b727cdaa2..01800cef1c33 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>> @@ -87,6 +87,11 @@ static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
>>   	iowrite32(data, lvds->mmio + reg);
>>   }
>>   
>> +static u32 rcar_lvds_read(struct rcar_lvds *lvds, u32 reg)
>> +{
>> +	return ioread32(lvds->mmio + reg);
>> +}
> 
> Could you please move read before write ?

Sure.

>> +
>>   /* -----------------------------------------------------------------------------
>>    * PLL Setup
>>    */
>> @@ -549,8 +554,28 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
>>   				     struct drm_bridge_state *old_bridge_state)
>>   {
>>   	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>> +	u32 lvdcr0;
>> +
>> +	lvdcr0 = rcar_lvds_read(lvds, LVDCR0);
>> +
>> +	lvdcr0 &= ~LVDCR0_LVRES;
>> +	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
>> +
>> +	if (lvds->info->quirks & RCAR_LVDS_QUIRK_GEN3_LVEN) {
>> +		lvdcr0 &= ~LVDCR0_LVEN;
>> +		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
>> +	}
>> +
>> +	if (lvds->info->quirks & RCAR_LVDS_QUIRK_PWD) {
>> +		lvdcr0 &= ~LVDCR0_PWD;
>> +		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
>> +	}
>> +
>> +	if (!(lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)) {
>> +		lvdcr0 &= ~LVDCR0_PLLON;
>> +		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
>> +	}
> 
> This will leave LVDCR0_BEN and LVDCR0_LVEN on Gen2. Is that fine ?

I don't know, I don't have the manuals or HW. But your point is a bit 
worrying.

I think we can just do a rcar_lvds_write(lvds, LVDCR0, 0) after the 
above shenanigans, to make sure everything is disabled. The HW manual 
doesn't tell us to do that, though, on gen3. Do you think that will be a 
problem?

And I'm not fully serious with the last sentence...

  Tomi

Re: [PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA

[Tomi Valkeinen]

On 17/01/2023 18:11, Geert Uytterhoeven wrote:
> Hi Tomi,
> 
> On Tue, Jan 17, 2023 at 2:54 PM Tomi Valkeinen
> <tomi.valkeinen+renesas@ideasonboard.com> wrote:
>> rcar_du_crtc.c does a soc_device_match() in
>> rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and
>> if so, apply a WA.
>>
>> We will need another H3 ES1 check in the following patch, so rather than
>> adding more soc_device_match() calls, let's add a rcar_du_device_info
>> entry for the ES1, and a quirk flag,
>> RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> 
>> @@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev)
>>                  return PTR_ERR(rcdu);
>>
>>          rcdu->dev = &pdev->dev;
>> -       rcdu->info = of_device_get_match_data(rcdu->dev);
> 
> No need to remove this line...
> 
>> +
>> +       soc_attr = soc_device_match(rcar_du_soc_table);
>> +       if (soc_attr)
>> +               rcdu->info = soc_attr->data;
>> +
>> +       if (!rcdu->info)
>> +               rcdu->info = of_device_get_match_data(rcdu->dev);
> 
> ... and no need to add these two lines.
> The idiom is to set rcdu->info based on of_device_get_match_data()
> first, and override based on of_device_get_match_data() when needed.

Ok.

  Tomi

Re: [PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA

[Tomi Valkeinen]

On 18/01/2023 23:19, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Jan 17, 2023 at 03:51:52PM +0200, Tomi Valkeinen wrote:
>> rcar_du_crtc.c does a soc_device_match() in
>> rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and
> 
> s/ES1/ES1.x/
> 
> Same below.
> 
>> if so, apply a WA.
> 
> s/WA/workaround/
> 
> Same below.

Ok.

>> We will need another H3 ES1 check in the following patch, so rather than
>> adding more soc_device_match() calls, let's add a rcar_du_device_info
>> entry for the ES1, and a quirk flag,
>> RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  8 +---
>>   drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 51 +++++++++++++++++++++++++-
>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
>>   3 files changed, 52 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> index 3619e1ddeb62..f2d3266509cc 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -10,7 +10,6 @@
>>   #include <linux/clk.h>
>>   #include <linux/mutex.h>
>>   #include <linux/platform_device.h>
>> -#include <linux/sys_soc.h>
>>   
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_atomic_helper.h>
>> @@ -204,11 +203,6 @@ static void rcar_du_escr_divider(struct clk *clk, unsigned long target,
>>   	}
>>   }
>>   
>> -static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
>> -	{ .soc_id = "r8a7795", .revision = "ES1.*" },
>> -	{ /* sentinel */ }
>> -};
>> -
>>   static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>>   {
>>   	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
>> @@ -238,7 +232,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>>   		 * no post-divider when a display PLL is present (as shown by
>>   		 * the workaround breaking HDMI output on M3-W during testing).
>>   		 */
>> -		if (soc_device_match(rcar_du_r8a7795_es1)) {
>> +		if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY) {
>>   			target *= 2;
>>   			div = 1;
>>   		}
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> index c7c5217cfc1a..ba2e069fc0f7 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/pm.h>
>>   #include <linux/slab.h>
>> +#include <linux/sys_soc.h>
>>   #include <linux/wait.h>
>>   
>>   #include <drm/drm_atomic_helper.h>
>> @@ -386,6 +387,42 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
>>   	.dpll_mask =  BIT(2) | BIT(1),
>>   };
>>   
>> +static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
>> +	.gen = 3,
>> +	.features = RCAR_DU_FEATURE_CRTC_IRQ
>> +		  | RCAR_DU_FEATURE_CRTC_CLOCK
>> +		  | RCAR_DU_FEATURE_VSP1_SOURCE
>> +		  | RCAR_DU_FEATURE_INTERLACED
>> +		  | RCAR_DU_FEATURE_TVM_SYNC,
>> +	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
>> +	.channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
>> +	.routes = {
>> +		/*
>> +		 * R8A7795 has one RGB output, two HDMI outputs and one
>> +		 * LVDS output.
>> +		 */
>> +		[RCAR_DU_OUTPUT_DPAD0] = {
>> +			.possible_crtcs = BIT(3),
>> +			.port = 0,
>> +		},
>> +		[RCAR_DU_OUTPUT_HDMI0] = {
>> +			.possible_crtcs = BIT(1),
>> +			.port = 1,
>> +		},
>> +		[RCAR_DU_OUTPUT_HDMI1] = {
>> +			.possible_crtcs = BIT(2),
>> +			.port = 2,
>> +		},
>> +		[RCAR_DU_OUTPUT_LVDS0] = {
>> +			.possible_crtcs = BIT(0),
>> +			.port = 3,
>> +		},
>> +	},
>> +	.num_lvds = 1,
>> +	.num_rpf = 5,
>> +	.dpll_mask =  BIT(2) | BIT(1),
>> +};
>> +
>>   static const struct rcar_du_device_info rcar_du_r8a7796_info = {
>>   	.gen = 3,
>>   	.features = RCAR_DU_FEATURE_CRTC_IRQ
>> @@ -576,6 +613,11 @@ static const struct of_device_id rcar_du_of_table[] = {
>>   
>>   MODULE_DEVICE_TABLE(of, rcar_du_of_table);
>>   
>> +static const struct soc_device_attribute rcar_du_soc_table[] = {
>> +	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &rcar_du_r8a7795_es1_info },
>> +	{ /* sentinel */ }
>> +};
>> +
>>   const char *rcar_du_output_name(enum rcar_du_output output)
>>   {
>>   	static const char * const names[] = {
>> @@ -670,6 +712,7 @@ static int rcar_du_probe(struct platform_device *pdev)
>>   	struct rcar_du_device *rcdu;
>>   	unsigned int mask;
>>   	int ret;
>> +	const struct soc_device_attribute *soc_attr;
> 
> Please move this up before rcdu.

Sure.

>>   
>>   	if (drm_firmware_drivers_only())
>>   		return -ENODEV;
>> @@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev)
>>   		return PTR_ERR(rcdu);
>>   
>>   	rcdu->dev = &pdev->dev;
>> -	rcdu->info = of_device_get_match_data(rcdu->dev);
>> +
>> +	soc_attr = soc_device_match(rcar_du_soc_table);
>> +	if (soc_attr)
>> +		rcdu->info = soc_attr->data;
>> +
>> +	if (!rcdu->info)
>> +		rcdu->info = of_device_get_match_data(rcdu->dev);
> 
> As Geert mentioned,
> 
> 	rcdu->info = of_device_get_match_data(rcdu->dev);
> 
> 	soc_attr = soc_device_match(rcar_du_soc_table);
> 	if (soc_attr)
> 		rcdu->info = soc_attr->data;
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Ok. Thanks!

  Tomi

Re: [PATCH 5/6] drm: rcar-du: Fix setting a reserved bit in DPLLCR

[Tomi Valkeinen]

On 18/01/2023 23:38, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote:
>> On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock
> 
> s/ES1/ES1.x/
> 
> Same below.

Ok. But I do wonder, is there a difference? What's the case when ES1 
could be mistaken to mean something else?

>> source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On
>> non-ES1, only the higher bits are used (bits 21 and 23), and the lower
>> bits are reserved and should be set to 0 (or not set at all).
> 
> How do you not set a bit ? :-)

By leaving it to the value the register already has. But as we don't 
read the register as a base value here, I guess that comment is a bit 
misleading.

>> The current code always sets the lower bits, even on non-ES1.
> 
> I think that's harmless, and not worth making the driver more complex,
> but I'll stop fighting.
> 
>> For both DU1 and DU2, on all SoC versions, when writing zeroes to those
>> bits the input clock is DCLKIN, and thus there's no difference between
>> ES1 and non-ES1.
>>
>> For DU1, writing 0b10 to the bits (or only writing the higher bit)
>> results in using PLL0 as the input clock, so in this case there's also
>> no difference between ES1 and non-ES1.
>>
>> However, for DU2, writing 0b10 to the bits results in using PLL0 as the
>> input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1
>> you need to write 0b11 to select PLL1.
>>
>> The current code always writes 0b11 to PLCS0 field to select PLL1 on all
>> SoC versions, which works but causes an illegal (in the sense of not
>> allowed by the documentation) write to a reserved bit field.
>>
>> To remove the illegal bit write on PLSC0 we need to handle the input dot
>> clock selection differently for ES1 and non-ES1.
>>
>> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new
>> rcar_du_device_info entry for the ES1 SoC. Using these, we can always
> 
> The new entry was added in the previous patch already.

Indeed.

>> set the bit 21 on PLSC0 when choosing the PLL as the source clock, and
>> additionally set the bit 20 when on ES1.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++--
>>   drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  3 ++-
>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
>>   drivers/gpu/drm/rcar-du/rcar_du_regs.h |  3 ++-
>>   4 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> index f2d3266509cc..8d660a6141bf 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>>   		       | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
>>   		       | DPLLCR_STBY;
>>   
>> -		if (rcrtc->index == 1)
>> +		if (rcrtc->index == 1) {
>>   			dpllcr |= DPLLCR_PLCS1
>>   			       |  DPLLCR_INCS_DOTCLKIN1;
>> -		else
>> +		} else {
>>   			dpllcr |= DPLLCR_PLCS0
>>   			       |  DPLLCR_INCS_DOTCLKIN0;
>> +			/*
>> +			 * On H3 ES1.x, in addition to setting bit 21 (PLCS0),
>> +			 * also bit 20 has to be set to select PLL1 as the
>> +			 * clock source.
> 
> I'd add "On ES2 and newer, PLL1 is selected unconditionally.".

It's not selected unconditionally, we need to set bit 21. And possibly 
we need to set bit 20 to 0, although it's not documented what bit 20 
would do when set to 1.

And is that "ES2.x"? =)

How about:

  * On ES2.x and newer, PLL1 is selected by setting bit
  * 21 (PLCS0) to 1 and keeping the (reserved) bit 20 as
  * 0. On H3 ES1.x, in addition to setting bit 21, also
  * bit 20 has to be set to select PLL1 as the clock source.

>> +			 */
>> +			if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PLL)
>> +				dpllcr |= DPLLCR_PLCS0_H3ES1X_PLL1_SEL;
>> +		}
>>   
>>   		rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr);
>>   
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> index ba2e069fc0f7..d689f2510081 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> @@ -394,7 +394,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
>>   		  | RCAR_DU_FEATURE_VSP1_SOURCE
>>   		  | RCAR_DU_FEATURE_INTERLACED
>>   		  | RCAR_DU_FEATURE_TVM_SYNC,
>> -	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
>> +	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY
>> +		| RCAR_DU_QUIRK_H3_ES1_PLL,
>>   	.channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
>>   	.routes = {
>>   		/*
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> index df87ccab146f..acc3673fefe1 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> @@ -35,6 +35,7 @@ struct rcar_du_device;
>>   
>>   #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
>>   #define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1)	/* H3 ES1 has pclk stability issue */
>> +#define RCAR_DU_QUIRK_H3_ES1_PLL	BIT(2)	/* H3 ES1 PLL setup differs from non-ES1 */
>>   
>>   enum rcar_du_output {
>>   	RCAR_DU_OUTPUT_DPAD0,
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>> index c1bcb0e8b5b4..94d913f66c8f 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>> @@ -288,7 +288,8 @@
>>    * isn't implemented by other SoC in the Gen3 family it can safely be set
>>    * unconditionally.
>>    */
> 
> This comment should be dropped.

Ah, true.

>> -#define DPLLCR_PLCS0		(3 << 20)
>> +#define DPLLCR_PLCS0		(1 << 21)
>> +#define DPLLCR_PLCS0_H3ES1X_PLL1_SEL	(1 << 20)
> 
> Bit 21 selects between DCLKIN (when 0) and PLL (when 1). On ES1.x, bit
> 20 selects between PLL0 (when 0) and PLL1 (when 1), while on ES2 and
> newer, PLL1 is selected unconditionally. Could we name the two bits
> accodingly ? Maybe
> 
> #define DPLLCR_PLCS0_PLL	(1 << 21)
> #define DPLLCR_PLCS0_PLL1	(1 << 20)

I'm fine with DPLLCR_PLCS0_PLL, but I do like to make it a bit more 
obvious that a bit is only for a particular ES/SoC version if it's 
simple to do. Especially here, as "DPLLCR_PLCS0_PLL1" sounds like you 
need to set it to use PLL1.

Would you be ok with "DPLLCR_PLCS0_H3ES1X_PLL1"?

  Tomi

Re: [PATCH 6/6] drm: rcar-du: Stop accessing non-existant registers on gen4

[Tomi Valkeinen]

On 18/01/2023 23:54, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Jan 17, 2023 at 03:51:54PM +0200, Tomi Valkeinen wrote:
>> The following registers do not exist on gen4, so we should not write
>> them: DEF6Rm, DEF8Rm, ESCRn, OTARn.
> 
> I think DEF7Rm should also be skipped. With that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Yep, makes sense.

  Tomi

Re: [PATCH 5/6] drm: rcar-du: Fix setting a reserved bit in DPLLCR

[Laurent Pinchart]

Hi Tomi,

On Thu, Jan 19, 2023 at 11:17:58AM +0200, Tomi Valkeinen wrote:
> On 18/01/2023 23:38, Laurent Pinchart wrote:
> > On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote:
> >> On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock
> > 
> > s/ES1/ES1.x/
> > 
> > Same below.
> 
> Ok. But I do wonder, is there a difference? What's the case when ES1 
> could be mistaken to mean something else?

It's just for consistency I suppose. No big deal.

> >> source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On
> >> non-ES1, only the higher bits are used (bits 21 and 23), and the lower
> >> bits are reserved and should be set to 0 (or not set at all).
> > 
> > How do you not set a bit ? :-)
> 
> By leaving it to the value the register already has. But as we don't 
> read the register as a base value here, I guess that comment is a bit 
> misleading.
> 
> >> The current code always sets the lower bits, even on non-ES1.
> > 
> > I think that's harmless, and not worth making the driver more complex,
> > but I'll stop fighting.
> > 
> >> For both DU1 and DU2, on all SoC versions, when writing zeroes to those
> >> bits the input clock is DCLKIN, and thus there's no difference between
> >> ES1 and non-ES1.
> >>
> >> For DU1, writing 0b10 to the bits (or only writing the higher bit)
> >> results in using PLL0 as the input clock, so in this case there's also
> >> no difference between ES1 and non-ES1.
> >>
> >> However, for DU2, writing 0b10 to the bits results in using PLL0 as the
> >> input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1
> >> you need to write 0b11 to select PLL1.
> >>
> >> The current code always writes 0b11 to PLCS0 field to select PLL1 on all
> >> SoC versions, which works but causes an illegal (in the sense of not
> >> allowed by the documentation) write to a reserved bit field.
> >>
> >> To remove the illegal bit write on PLSC0 we need to handle the input dot
> >> clock selection differently for ES1 and non-ES1.
> >>
> >> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new
> >> rcar_du_device_info entry for the ES1 SoC. Using these, we can always
> > 
> > The new entry was added in the previous patch already.
> 
> Indeed.
> 
> >> set the bit 21 on PLSC0 when choosing the PLL as the source clock, and
> >> additionally set the bit 20 when on ES1.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >> ---
> >>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++--
> >>   drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  3 ++-
> >>   drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
> >>   drivers/gpu/drm/rcar-du/rcar_du_regs.h |  3 ++-
> >>   4 files changed, 15 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> index f2d3266509cc..8d660a6141bf 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
> >>   		       | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
> >>   		       | DPLLCR_STBY;
> >>   
> >> -		if (rcrtc->index == 1)
> >> +		if (rcrtc->index == 1) {
> >>   			dpllcr |= DPLLCR_PLCS1
> >>   			       |  DPLLCR_INCS_DOTCLKIN1;
> >> -		else
> >> +		} else {
> >>   			dpllcr |= DPLLCR_PLCS0
> >>   			       |  DPLLCR_INCS_DOTCLKIN0;
> >> +			/*
> >> +			 * On H3 ES1.x, in addition to setting bit 21 (PLCS0),
> >> +			 * also bit 20 has to be set to select PLL1 as the
> >> +			 * clock source.
> > 
> > I'd add "On ES2 and newer, PLL1 is selected unconditionally.".
> 
> It's not selected unconditionally, we need to set bit 21. And possibly 
> we need to set bit 20 to 0, although it's not documented what bit 20 
> would do when set to 1.

We currently set bit 20 to 1 and it works, so I concluded that bit 20 is
ignored. That's what I meant by PLL1 being selected automatically,
between PLL0 and PLL1. We still need to select PLL instead of DCLKIN
with bit 21.

> And is that "ES2.x"? =)

Good point :-)

> How about:
> 
>   * On ES2.x and newer, PLL1 is selected by setting bit
>   * 21 (PLCS0) to 1 and keeping the (reserved) bit 20 as
>   * 0. On H3 ES1.x, in addition to setting bit 21, also
>   * bit 20 has to be set to select PLL1 as the clock source.

What I'd like to capture in the comment is that the clock topology is

        bit 20
          |     bit 21
          v       |
         |\       v
PLL0 --> |0|     |\
PLL1 --> |1| --> |1| -->
         |/  /-> |0|
             |   |/
DCLKIN ------/

on H3 ES1.x, while on newer revisions, bit 20 is ignored and the first
mux is hardcoded to PLL1.

> >> +			 */
> >> +			if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PLL)
> >> +				dpllcr |= DPLLCR_PLCS0_H3ES1X_PLL1_SEL;
> >> +		}
> >>   
> >>   		rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr);
> >>   
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> index ba2e069fc0f7..d689f2510081 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> @@ -394,7 +394,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
> >>   		  | RCAR_DU_FEATURE_VSP1_SOURCE
> >>   		  | RCAR_DU_FEATURE_INTERLACED
> >>   		  | RCAR_DU_FEATURE_TVM_SYNC,
> >> -	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
> >> +	.quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY
> >> +		| RCAR_DU_QUIRK_H3_ES1_PLL,
> >>   	.channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
> >>   	.routes = {
> >>   		/*
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >> index df87ccab146f..acc3673fefe1 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >> @@ -35,6 +35,7 @@ struct rcar_du_device;
> >>   
> >>   #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
> >>   #define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1)	/* H3 ES1 has pclk stability issue */
> >> +#define RCAR_DU_QUIRK_H3_ES1_PLL	BIT(2)	/* H3 ES1 PLL setup differs from non-ES1 */
> >>   
> >>   enum rcar_du_output {
> >>   	RCAR_DU_OUTPUT_DPAD0,
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> >> index c1bcb0e8b5b4..94d913f66c8f 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> >> @@ -288,7 +288,8 @@
> >>    * isn't implemented by other SoC in the Gen3 family it can safely be set
> >>    * unconditionally.
> >>    */
> > 
> > This comment should be dropped.
> 
> Ah, true.
> 
> >> -#define DPLLCR_PLCS0		(3 << 20)
> >> +#define DPLLCR_PLCS0		(1 << 21)
> >> +#define DPLLCR_PLCS0_H3ES1X_PLL1_SEL	(1 << 20)
> > 
> > Bit 21 selects between DCLKIN (when 0) and PLL (when 1). On ES1.x, bit
> > 20 selects between PLL0 (when 0) and PLL1 (when 1), while on ES2 and
> > newer, PLL1 is selected unconditionally. Could we name the two bits
> > accodingly ? Maybe
> > 
> > #define DPLLCR_PLCS0_PLL	(1 << 21)
> > #define DPLLCR_PLCS0_PLL1	(1 << 20)
> 
> I'm fine with DPLLCR_PLCS0_PLL, but I do like to make it a bit more 
> obvious that a bit is only for a particular ES/SoC version if it's 
> simple to do. Especially here, as "DPLLCR_PLCS0_PLL1" sounds like you 
> need to set it to use PLL1.
> 
> Would you be ok with "DPLLCR_PLCS0_H3ES1X_PLL1"?

A bit long but I suppose I can live with that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] drm: rcar-du: Fix setting a reserved bit in DPLLCR

[Tomi Valkeinen]

On 19/01/2023 11:40, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Thu, Jan 19, 2023 at 11:17:58AM +0200, Tomi Valkeinen wrote:
>> On 18/01/2023 23:38, Laurent Pinchart wrote:
>>> On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote:
>>>> On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock
>>>
>>> s/ES1/ES1.x/
>>>
>>> Same below.
>>
>> Ok. But I do wonder, is there a difference? What's the case when ES1
>> could be mistaken to mean something else?
> 
> It's just for consistency I suppose. No big deal.
> 
>>>> source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On
>>>> non-ES1, only the higher bits are used (bits 21 and 23), and the lower
>>>> bits are reserved and should be set to 0 (or not set at all).
>>>
>>> How do you not set a bit ? :-)
>>
>> By leaving it to the value the register already has. But as we don't
>> read the register as a base value here, I guess that comment is a bit
>> misleading.
>>
>>>> The current code always sets the lower bits, even on non-ES1.
>>>
>>> I think that's harmless, and not worth making the driver more complex,
>>> but I'll stop fighting.
>>>
>>>> For both DU1 and DU2, on all SoC versions, when writing zeroes to those
>>>> bits the input clock is DCLKIN, and thus there's no difference between
>>>> ES1 and non-ES1.
>>>>
>>>> For DU1, writing 0b10 to the bits (or only writing the higher bit)
>>>> results in using PLL0 as the input clock, so in this case there's also
>>>> no difference between ES1 and non-ES1.
>>>>
>>>> However, for DU2, writing 0b10 to the bits results in using PLL0 as the
>>>> input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1
>>>> you need to write 0b11 to select PLL1.
>>>>
>>>> The current code always writes 0b11 to PLCS0 field to select PLL1 on all
>>>> SoC versions, which works but causes an illegal (in the sense of not
>>>> allowed by the documentation) write to a reserved bit field.
>>>>
>>>> To remove the illegal bit write on PLSC0 we need to handle the input dot
>>>> clock selection differently for ES1 and non-ES1.
>>>>
>>>> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new
>>>> rcar_du_device_info entry for the ES1 SoC. Using these, we can always
>>>
>>> The new entry was added in the previous patch already.
>>
>> Indeed.
>>
>>>> set the bit 21 on PLSC0 when choosing the PLL as the source clock, and
>>>> additionally set the bit 20 when on ES1.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>>> ---
>>>>    drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++--
>>>>    drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  3 ++-
>>>>    drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
>>>>    drivers/gpu/drm/rcar-du/rcar_du_regs.h |  3 ++-
>>>>    4 files changed, 15 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> index f2d3266509cc..8d660a6141bf 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>>>>    		       | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
>>>>    		       | DPLLCR_STBY;
>>>>    
>>>> -		if (rcrtc->index == 1)
>>>> +		if (rcrtc->index == 1) {
>>>>    			dpllcr |= DPLLCR_PLCS1
>>>>    			       |  DPLLCR_INCS_DOTCLKIN1;
>>>> -		else
>>>> +		} else {
>>>>    			dpllcr |= DPLLCR_PLCS0
>>>>    			       |  DPLLCR_INCS_DOTCLKIN0;
>>>> +			/*
>>>> +			 * On H3 ES1.x, in addition to setting bit 21 (PLCS0),
>>>> +			 * also bit 20 has to be set to select PLL1 as the
>>>> +			 * clock source.
>>>
>>> I'd add "On ES2 and newer, PLL1 is selected unconditionally.".
>>
>> It's not selected unconditionally, we need to set bit 21. And possibly
>> we need to set bit 20 to 0, although it's not documented what bit 20
>> would do when set to 1.
> 
> We currently set bit 20 to 1 and it works, so I concluded that bit 20 is

Ah, right, we do set it to 1.

> ignored. That's what I meant by PLL1 being selected automatically,
> between PLL0 and PLL1. We still need to select PLL instead of DCLKIN
> with bit 21.
> 
>> And is that "ES2.x"? =)
> 
> Good point :-)
> 
>> How about:
>>
>>    * On ES2.x and newer, PLL1 is selected by setting bit
>>    * 21 (PLCS0) to 1 and keeping the (reserved) bit 20 as
>>    * 0. On H3 ES1.x, in addition to setting bit 21, also
>>    * bit 20 has to be set to select PLL1 as the clock source.
> 
> What I'd like to capture in the comment is that the clock topology is
> 
>          bit 20
>            |     bit 21
>            v       |
>           |\       v
> PLL0 --> |0|     |\
> PLL1 --> |1| --> |1| -->
>           |/  /-> |0|
>               |   |/
> DCLKIN ------/
> 
> on H3 ES1.x, while on newer revisions, bit 20 is ignored and the first
> mux is hardcoded to PLL1.

Isn't that the stuff that's supposed to be found by reading the manual? 
I could, of course, copy the above picture, and draw a similar one for 
ES2+, but that feels like just useless copying of the manual...

Doesn't the above comment already describe why we set the bits as we do? 
I could change the comment to talk about muxes, if you think that's a 
better approach. But, if I recall right, the manuals don't talk about 
muxes, just "set this to X to use PLL1", so my comment above approaches 
from that direction.

Maybe something in this direction, then:

On ES2.x we have a single mux controlled via bit 21, which selects 
between DCLKIN source (bit 21 = 0) and a PLL source (bit 21 = 1), where 
the PLL is always PLL1. On ES1.x we have an additional mux, controlled 
via bit 20, for choosing between PLL0 (bit 20 = 0) and PLL1 (bit 20 = 
1). We always want to use PLL1, so on ES1.x, in addition to setting bit 
21, we need to set the bit 20.

  Tomi

Re: [PATCH 3/6] drm: rcar-du: Fix LVDS stop sequence

[Laurent Pinchart]

Hi Tomi,

On Thu, Jan 19, 2023 at 10:49:28AM +0200, Tomi Valkeinen wrote:
> On 18/01/2023 23:12, Laurent Pinchart wrote:
> > On Tue, Jan 17, 2023 at 03:51:51PM +0200, Tomi Valkeinen wrote:
> >> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> >>
> >> According to H/W manual, LVDCR0 register must be cleared bit by bit when
> > 
> > s@H/W@the hardware/
> > 
> >> disabling LVDS.
> > 
> > I don't like this much, but I think I'll stop fighting :-)
> > 
> >> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> >> Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com>
> >> [tomi.valkeinen: simplified the code a bit]
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >> ---
> >>   drivers/gpu/drm/rcar-du/rcar_lvds.c | 27 ++++++++++++++++++++++++++-
> >>   1 file changed, 26 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >> index 674b727cdaa2..01800cef1c33 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >> @@ -87,6 +87,11 @@ static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
> >>   	iowrite32(data, lvds->mmio + reg);
> >>   }
> >>   
> >> +static u32 rcar_lvds_read(struct rcar_lvds *lvds, u32 reg)
> >> +{
> >> +	return ioread32(lvds->mmio + reg);
> >> +}
> > 
> > Could you please move read before write ?
> 
> Sure.
> 
> >> +
> >>   /* -----------------------------------------------------------------------------
> >>    * PLL Setup
> >>    */
> >> @@ -549,8 +554,28 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
> >>   				     struct drm_bridge_state *old_bridge_state)
> >>   {
> >>   	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> >> +	u32 lvdcr0;
> >> +
> >> +	lvdcr0 = rcar_lvds_read(lvds, LVDCR0);
> >> +
> >> +	lvdcr0 &= ~LVDCR0_LVRES;
> >> +	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> >> +
> >> +	if (lvds->info->quirks & RCAR_LVDS_QUIRK_GEN3_LVEN) {
> >> +		lvdcr0 &= ~LVDCR0_LVEN;
> >> +		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> >> +	}
> >> +
> >> +	if (lvds->info->quirks & RCAR_LVDS_QUIRK_PWD) {
> >> +		lvdcr0 &= ~LVDCR0_PWD;
> >> +		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> >> +	}
> >> +
> >> +	if (!(lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)) {
> >> +		lvdcr0 &= ~LVDCR0_PLLON;
> >> +		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> >> +	}
> > 
> > This will leave LVDCR0_BEN and LVDCR0_LVEN on Gen2. Is that fine ?
> 
> I don't know, I don't have the manuals or HW. But your point is a bit 
> worrying.
> 
> I think we can just do a rcar_lvds_write(lvds, LVDCR0, 0) after the 
> above shenanigans, to make sure everything is disabled. The HW manual 
> doesn't tell us to do that, though, on gen3. Do you think that will be a 
> problem?
> 
> And I'm not fully serious with the last sentence...

:-)

A write(lvds, LVDCR0, 0) should fix it, I'm fine with that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] drm: rcar-du: Fix setting a reserved bit in DPLLCR

[Laurent Pinchart]

Hi Tomi,

On Thu, Jan 19, 2023 at 12:24:48PM +0200, Tomi Valkeinen wrote:
> On 19/01/2023 11:40, Laurent Pinchart wrote:
> > On Thu, Jan 19, 2023 at 11:17:58AM +0200, Tomi Valkeinen wrote:
> >> On 18/01/2023 23:38, Laurent Pinchart wrote:
> >>> On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote:
> >>>> On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock
> >>>
> >>> s/ES1/ES1.x/
> >>>
> >>> Same below.
> >>
> >> Ok. But I do wonder, is there a difference? What's the case when ES1
> >> could be mistaken to mean something else?
> > 
> > It's just for consistency I suppose. No big deal.
> > 
> >>>> source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On
> >>>> non-ES1, only the higher bits are used (bits 21 and 23), and the lower
> >>>> bits are reserved and should be set to 0 (or not set at all).
> >>>
> >>> How do you not set a bit ? :-)
> >>
> >> By leaving it to the value the register already has. But as we don't
> >> read the register as a base value here, I guess that comment is a bit
> >> misleading.
> >>
> >>>> The current code always sets the lower bits, even on non-ES1.
> >>>
> >>> I think that's harmless, and not worth making the driver more complex,
> >>> but I'll stop fighting.
> >>>
> >>>> For both DU1 and DU2, on all SoC versions, when writing zeroes to those
> >>>> bits the input clock is DCLKIN, and thus there's no difference between
> >>>> ES1 and non-ES1.
> >>>>
> >>>> For DU1, writing 0b10 to the bits (or only writing the higher bit)
> >>>> results in using PLL0 as the input clock, so in this case there's also
> >>>> no difference between ES1 and non-ES1.
> >>>>
> >>>> However, for DU2, writing 0b10 to the bits results in using PLL0 as the
> >>>> input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1
> >>>> you need to write 0b11 to select PLL1.
> >>>>
> >>>> The current code always writes 0b11 to PLCS0 field to select PLL1 on all
> >>>> SoC versions, which works but causes an illegal (in the sense of not
> >>>> allowed by the documentation) write to a reserved bit field.
> >>>>
> >>>> To remove the illegal bit write on PLSC0 we need to handle the input dot
> >>>> clock selection differently for ES1 and non-ES1.
> >>>>
> >>>> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new
> >>>> rcar_du_device_info entry for the ES1 SoC. Using these, we can always
> >>>
> >>> The new entry was added in the previous patch already.
> >>
> >> Indeed.
> >>
> >>>> set the bit 21 on PLSC0 when choosing the PLL as the source clock, and
> >>>> additionally set the bit 20 when on ES1.
> >>>>
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >>>> ---
> >>>>    drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++--
> >>>>    drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  3 ++-
> >>>>    drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +
> >>>>    drivers/gpu/drm/rcar-du/rcar_du_regs.h |  3 ++-
> >>>>    4 files changed, 15 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>>> index f2d3266509cc..8d660a6141bf 100644
> >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>>> @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
> >>>>    		       | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
> >>>>    		       | DPLLCR_STBY;
> >>>>    
> >>>> -		if (rcrtc->index == 1)
> >>>> +		if (rcrtc->index == 1) {
> >>>>    			dpllcr |= DPLLCR_PLCS1
> >>>>    			       |  DPLLCR_INCS_DOTCLKIN1;
> >>>> -		else
> >>>> +		} else {
> >>>>    			dpllcr |= DPLLCR_PLCS0
> >>>>    			       |  DPLLCR_INCS_DOTCLKIN0;
> >>>> +			/*
> >>>> +			 * On H3 ES1.x, in addition to setting bit 21 (PLCS0),
> >>>> +			 * also bit 20 has to be set to select PLL1 as the
> >>>> +			 * clock source.
> >>>
> >>> I'd add "On ES2 and newer, PLL1 is selected unconditionally.".
> >>
> >> It's not selected unconditionally, we need to set bit 21. And possibly
> >> we need to set bit 20 to 0, although it's not documented what bit 20
> >> would do when set to 1.
> > 
> > We currently set bit 20 to 1 and it works, so I concluded that bit 20 is
> 
> Ah, right, we do set it to 1.
> 
> > ignored. That's what I meant by PLL1 being selected automatically,
> > between PLL0 and PLL1. We still need to select PLL instead of DCLKIN
> > with bit 21.
> > 
> >> And is that "ES2.x"? =)
> > 
> > Good point :-)
> > 
> >> How about:
> >>
> >>    * On ES2.x and newer, PLL1 is selected by setting bit
> >>    * 21 (PLCS0) to 1 and keeping the (reserved) bit 20 as
> >>    * 0. On H3 ES1.x, in addition to setting bit 21, also
> >>    * bit 20 has to be set to select PLL1 as the clock source.
> > 
> > What I'd like to capture in the comment is that the clock topology is
> > 
> >          bit 20
> >            |     bit 21
> >            v       |
> >           |\       v
> > PLL0 --> |0|     |\
> > PLL1 --> |1| --> |1| -->
> >           |/  /-> |0|
> >               |   |/
> > DCLKIN ------/
> > 
> > on H3 ES1.x, while on newer revisions, bit 20 is ignored and the first
> > mux is hardcoded to PLL1.
> 
> Isn't that the stuff that's supposed to be found by reading the manual? 
> I could, of course, copy the above picture, and draw a similar one for 
> ES2+, but that feels like just useless copying of the manual...

It would, if it was in the documentation :-) The above diagram is what I
deduced from the documentation, but it's certainly not explicit, and the
two bits 20 and 21 are not named separately. We don't need to capture
this in ascii art, but some kind of summary would be nice.

> Doesn't the above comment already describe why we set the bits as we do? 
> I could change the comment to talk about muxes, if you think that's a 
> better approach. But, if I recall right, the manuals don't talk about 
> muxes, just "set this to X to use PLL1", so my comment above approaches 
> from that direction.
> 
> Maybe something in this direction, then:
> 
> On ES2.x we have a single mux controlled via bit 21, which selects 
> between DCLKIN source (bit 21 = 0) and a PLL source (bit 21 = 1), where 
> the PLL is always PLL1. On ES1.x we have an additional mux, controlled 
> via bit 20, for choosing between PLL0 (bit 20 = 0) and PLL1 (bit 20 = 
> 1). We always want to use PLL1, so on ES1.x, in addition to setting bit 
> 21, we need to set the bit 20.

Seems good to me.

-- 
Regards,

Laurent Pinchart