All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] drm: rcar-du: A few cosmetic changes
@ 2018-08-22  7:21 ` Jacopo Mondi
  0 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-08-22  7:21 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS

Hi Laurent,
   there are a few things currently in-flight for DU on my side:
- non-DPLL channel clock source selction
- ESCR/OTAR handling for non-DPLL channel

Before taking those two functional changes series into account,
could you include these three cosmetic changes in your tree?

Patches based on your drm/du/next branch, with the following patch applied on
top:
'01449a9779b8 (" drm: rcar-du: Rework clock configuration based on hardware limits")'

Tested on Salvator-X M3-W with kms-modes-tests.py: no functional changes

Thanks
  j

Jacopo Mondi (3):
  drm: rcar-du: Rename and document dpll_ch field
  drm: rcar-du: Write ESCR register per channel
  drm: rcar-du: Write OTAR register per channel

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 +++----
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  | 3 ++-
 drivers/gpu/drm/rcar-du/rcar_du_regs.h | 8 ++++----
 4 files changed, 12 insertions(+), 12 deletions(-)

--
2.7.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 0/3] drm: rcar-du: A few cosmetic changes
@ 2018-08-22  7:21 ` Jacopo Mondi
  0 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-08-22  7:21 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: open list:DRM DRIVERS FOR RENESAS, Jacopo Mondi,
	open list:DRM DRIVERS FOR RENESAS

Hi Laurent,
   there are a few things currently in-flight for DU on my side:
- non-DPLL channel clock source selction
- ESCR/OTAR handling for non-DPLL channel

Before taking those two functional changes series into account,
could you include these three cosmetic changes in your tree?

Patches based on your drm/du/next branch, with the following patch applied on
top:
'01449a9779b8 (" drm: rcar-du: Rework clock configuration based on hardware limits")'

Tested on Salvator-X M3-W with kms-modes-tests.py: no functional changes

Thanks
  j

Jacopo Mondi (3):
  drm: rcar-du: Rename and document dpll_ch field
  drm: rcar-du: Write ESCR register per channel
  drm: rcar-du: Write OTAR register per channel

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 +++----
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  | 3 ++-
 drivers/gpu/drm/rcar-du/rcar_du_regs.h | 8 ++++----
 4 files changed, 12 insertions(+), 12 deletions(-)

--
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/3] drm: rcar-du: Rename and document dpll_ch field
  2018-08-22  7:21 ` Jacopo Mondi
@ 2018-08-22  7:21   ` Jacopo Mondi
  -1 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-08-22  7:21 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS

Document and re-name the 'dpll_ch' field to a more precise 'dpll_mask' for
consistency with the 'channels_mask' field defined in 'struct
rcar_du_device_info'.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  | 3 ++-
 3 files changed, 6 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 2664336..5454884 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -211,7 +211,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	u32 dsmr;
 	u32 escr;
 
-	if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
+	if (rcdu->info->dpll_mask & (1 << rcrtc->index)) {
 		unsigned long target = mode_clock;
 		struct dpll_info dpll = { 0 };
 		unsigned long extclk;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 02aee6c..b42145c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -215,7 +215,7 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
 		},
 	},
 	.num_lvds = 1,
-	.dpll_ch =  BIT(2) | BIT(1),
+	.dpll_mask =  BIT(2) | BIT(1),
 };
 
 static const struct rcar_du_device_info rcar_du_r8a7796_info = {
@@ -243,7 +243,7 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = {
 		},
 	},
 	.num_lvds = 1,
-	.dpll_ch =  BIT(1),
+	.dpll_mask =  BIT(1),
 };
 
 static const struct rcar_du_device_info rcar_du_r8a77965_info = {
@@ -271,7 +271,7 @@ static const struct rcar_du_device_info rcar_du_r8a77965_info = {
 		},
 	},
 	.num_lvds = 1,
-	.dpll_ch =  BIT(1),
+	.dpll_mask =  BIT(1),
 };
 
 static const struct rcar_du_device_info rcar_du_r8a77970_info = {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index b3a25e8..6453b33 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -55,6 +55,7 @@ struct rcar_du_output_routing {
  * @channels_mask: bit mask of available DU channels
  * @routes: array of CRTC to output routes, indexed by output (RCAR_DU_OUTPUT_*)
  * @num_lvds: number of internal LVDS encoders
+ * @dpll_mask: mask of DU channels equipped with a DPLL
  */
 struct rcar_du_device_info {
 	unsigned int gen;
@@ -63,7 +64,7 @@ struct rcar_du_device_info {
 	unsigned int channels_mask;
 	struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX];
 	unsigned int num_lvds;
-	unsigned int dpll_ch;
+	unsigned int dpll_mask;
 };
 
 #define RCAR_DU_MAX_CRTCS		4
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 1/3] drm: rcar-du: Rename and document dpll_ch field
@ 2018-08-22  7:21   ` Jacopo Mondi
  0 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-08-22  7:21 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: open list:DRM DRIVERS FOR RENESAS, Jacopo Mondi,
	open list:DRM DRIVERS FOR RENESAS

Document and re-name the 'dpll_ch' field to a more precise 'dpll_mask' for
consistency with the 'channels_mask' field defined in 'struct
rcar_du_device_info'.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  | 3 ++-
 3 files changed, 6 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 2664336..5454884 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -211,7 +211,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	u32 dsmr;
 	u32 escr;
 
-	if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
+	if (rcdu->info->dpll_mask & (1 << rcrtc->index)) {
 		unsigned long target = mode_clock;
 		struct dpll_info dpll = { 0 };
 		unsigned long extclk;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 02aee6c..b42145c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -215,7 +215,7 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
 		},
 	},
 	.num_lvds = 1,
-	.dpll_ch =  BIT(2) | BIT(1),
+	.dpll_mask =  BIT(2) | BIT(1),
 };
 
 static const struct rcar_du_device_info rcar_du_r8a7796_info = {
@@ -243,7 +243,7 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = {
 		},
 	},
 	.num_lvds = 1,
-	.dpll_ch =  BIT(1),
+	.dpll_mask =  BIT(1),
 };
 
 static const struct rcar_du_device_info rcar_du_r8a77965_info = {
@@ -271,7 +271,7 @@ static const struct rcar_du_device_info rcar_du_r8a77965_info = {
 		},
 	},
 	.num_lvds = 1,
-	.dpll_ch =  BIT(1),
+	.dpll_mask =  BIT(1),
 };
 
 static const struct rcar_du_device_info rcar_du_r8a77970_info = {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index b3a25e8..6453b33 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -55,6 +55,7 @@ struct rcar_du_output_routing {
  * @channels_mask: bit mask of available DU channels
  * @routes: array of CRTC to output routes, indexed by output (RCAR_DU_OUTPUT_*)
  * @num_lvds: number of internal LVDS encoders
+ * @dpll_mask: mask of DU channels equipped with a DPLL
  */
 struct rcar_du_device_info {
 	unsigned int gen;
@@ -63,7 +64,7 @@ struct rcar_du_device_info {
 	unsigned int channels_mask;
 	struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX];
 	unsigned int num_lvds;
-	unsigned int dpll_ch;
+	unsigned int dpll_mask;
 };
 
 #define RCAR_DU_MAX_CRTCS		4
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/3] drm: rcar-du: Write ESCR register per channel
  2018-08-22  7:21 ` Jacopo Mondi
@ 2018-08-22  7:21   ` Jacopo Mondi
  -1 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-08-22  7:21 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS

The ESCR registers offset definition is confusing, as each channel is
equipped with an ESCR register instance, but the names suggest only ESCR and
ESCR2 are taken into account.

Rename the offsets to a name that includes the channels they apply to, and
write them to each channel with 'rcar_du_crtc_write()'.

Cosmetic patch, no functional changes intended.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +--
 drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
 2 files changed, 3 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 5454884..714c1fc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 		}
 	}
 
-	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
-			    escr);
+	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
 
 	/* Signal polarities */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index 9dfd220..ebc4aea 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -492,8 +492,8 @@
  * External Synchronization Control Registers
  */
 
-#define ESCR			0x10000
-#define ESCR2			0x31000
+#define ESCR02			0x10000
+#define ESCR13			0x01000
 #define ESCR_DCLKOINV		(1 << 25)
 #define ESCR_DCLKSEL_DCLKIN	(0 << 20)
 #define ESCR_DCLKSEL_CLKS	(1 << 20)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/3] drm: rcar-du: Write ESCR register per channel
@ 2018-08-22  7:21   ` Jacopo Mondi
  0 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-08-22  7:21 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: open list:DRM DRIVERS FOR RENESAS, Jacopo Mondi,
	open list:DRM DRIVERS FOR RENESAS

The ESCR registers offset definition is confusing, as each channel is
equipped with an ESCR register instance, but the names suggest only ESCR and
ESCR2 are taken into account.

Rename the offsets to a name that includes the channels they apply to, and
write them to each channel with 'rcar_du_crtc_write()'.

Cosmetic patch, no functional changes intended.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +--
 drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
 2 files changed, 3 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 5454884..714c1fc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 		}
 	}
 
-	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
-			    escr);
+	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
 
 	/* Signal polarities */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index 9dfd220..ebc4aea 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -492,8 +492,8 @@
  * External Synchronization Control Registers
  */
 
-#define ESCR			0x10000
-#define ESCR2			0x31000
+#define ESCR02			0x10000
+#define ESCR13			0x01000
 #define ESCR_DCLKOINV		(1 << 25)
 #define ESCR_DCLKSEL_DCLKIN	(0 << 20)
 #define ESCR_DCLKSEL_CLKS	(1 << 20)
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/3] drm: rcar-du: Write OTAR register per channel
  2018-08-22  7:21 ` Jacopo Mondi
@ 2018-08-22  7:21   ` Jacopo Mondi
  -1 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-08-22  7:21 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS

The OTAR registers offset definition is confusing, as each channel is
equipped with an OTAR register instance, but the names suggest only OTAR and
OTAR2 are taken into account.

Rename the offsets to a name that includes the channels they apply to, and
write them to each channel with 'rcar_du_crtc_write()'.

Cosmetic patch, no functional changes intended.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 714c1fc..e2d9653 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -295,7 +295,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	}
 
 	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
-	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
+	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_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index ebc4aea..4144950 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -504,8 +504,8 @@
 #define ESCR_SYNCSEL_EXHSYNC	(3 << 8)
 #define ESCR_FRQSEL_MASK	(0x3f << 0)
 
-#define OTAR			0x10004
-#define OTAR2			0x31004
+#define OTAR02			0x10004
+#define OTAR13			0x01004
 
 /* -----------------------------------------------------------------------------
  * Dual Display Output Control Registers
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/3] drm: rcar-du: Write OTAR register per channel
@ 2018-08-22  7:21   ` Jacopo Mondi
  0 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-08-22  7:21 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: open list:DRM DRIVERS FOR RENESAS, Jacopo Mondi,
	open list:DRM DRIVERS FOR RENESAS

The OTAR registers offset definition is confusing, as each channel is
equipped with an OTAR register instance, but the names suggest only OTAR and
OTAR2 are taken into account.

Rename the offsets to a name that includes the channels they apply to, and
write them to each channel with 'rcar_du_crtc_write()'.

Cosmetic patch, no functional changes intended.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 714c1fc..e2d9653 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -295,7 +295,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	}
 
 	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
-	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
+	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_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index ebc4aea..4144950 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -504,8 +504,8 @@
 #define ESCR_SYNCSEL_EXHSYNC	(3 << 8)
 #define ESCR_FRQSEL_MASK	(0x3f << 0)
 
-#define OTAR			0x10004
-#define OTAR2			0x31004
+#define OTAR02			0x10004
+#define OTAR13			0x01004
 
 /* -----------------------------------------------------------------------------
  * Dual Display Output Control Registers
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] drm: rcar-du: Rename and document dpll_ch field
  2018-08-22  7:21   ` Jacopo Mondi
@ 2018-08-22  8:17     ` Laurent Pinchart
  -1 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2018-08-22  8:17 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: open list:DRM DRIVERS FOR RENESAS, open list:DRM DRIVERS FOR RENESAS

Hi Jacopo,

Thank you for the patch.

On Wednesday, 22 August 2018 10:21:47 EEST Jacopo Mondi wrote:
> Document and re-name the 'dpll_ch' field to a more precise 'dpll_mask' for
> consistency with the 'channels_mask' field defined in 'struct
> rcar_du_device_info'.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 6 +++---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h  | 3 ++-
>  3 files changed, 6 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 2664336..5454884 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -211,7 +211,7 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc) u32 dsmr;
>  	u32 escr;
> 
> -	if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
> +	if (rcdu->info->dpll_mask & (1 << rcrtc->index)) {
>  		unsigned long target = mode_clock;
>  		struct dpll_info dpll = { 0 };
>  		unsigned long extclk;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..b42145c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -215,7 +215,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7795_info = { },
>  	},
>  	.num_lvds = 1,
> -	.dpll_ch =  BIT(2) | BIT(1),
> +	.dpll_mask =  BIT(2) | BIT(1),
>  };
> 
>  static const struct rcar_du_device_info rcar_du_r8a7796_info = {
> @@ -243,7 +243,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7796_info = { },
>  	},
>  	.num_lvds = 1,
> -	.dpll_ch =  BIT(1),
> +	.dpll_mask =  BIT(1),
>  };
> 
>  static const struct rcar_du_device_info rcar_du_r8a77965_info = {
> @@ -271,7 +271,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a77965_info = { },
>  	},
>  	.num_lvds = 1,
> -	.dpll_ch =  BIT(1),
> +	.dpll_mask =  BIT(1),
>  };
> 
>  static const struct rcar_du_device_info rcar_du_r8a77970_info = {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..6453b33 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -55,6 +55,7 @@ struct rcar_du_output_routing {
>   * @channels_mask: bit mask of available DU channels
>   * @routes: array of CRTC to output routes, indexed by output
> (RCAR_DU_OUTPUT_*) * @num_lvds: number of internal LVDS encoders
> + * @dpll_mask: mask of DU channels equipped with a DPLL

I'd way "bit mask" instead of "mask" in the description. Apart from that,

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

and taken in my tree.

>   */
>  struct rcar_du_device_info {
>  	unsigned int gen;
> @@ -63,7 +64,7 @@ struct rcar_du_device_info {
>  	unsigned int channels_mask;
>  	struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX];
>  	unsigned int num_lvds;
> -	unsigned int dpll_ch;
> +	unsigned int dpll_mask;
>  };
> 
>  #define RCAR_DU_MAX_CRTCS		4

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] drm: rcar-du: Rename and document dpll_ch field
@ 2018-08-22  8:17     ` Laurent Pinchart
  0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2018-08-22  8:17 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: open list:DRM DRIVERS FOR RENESAS, open list:DRM DRIVERS FOR RENESAS

Hi Jacopo,

Thank you for the patch.

On Wednesday, 22 August 2018 10:21:47 EEST Jacopo Mondi wrote:
> Document and re-name the 'dpll_ch' field to a more precise 'dpll_mask' for
> consistency with the 'channels_mask' field defined in 'struct
> rcar_du_device_info'.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 6 +++---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h  | 3 ++-
>  3 files changed, 6 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 2664336..5454884 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -211,7 +211,7 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc) u32 dsmr;
>  	u32 escr;
> 
> -	if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
> +	if (rcdu->info->dpll_mask & (1 << rcrtc->index)) {
>  		unsigned long target = mode_clock;
>  		struct dpll_info dpll = { 0 };
>  		unsigned long extclk;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..b42145c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -215,7 +215,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7795_info = { },
>  	},
>  	.num_lvds = 1,
> -	.dpll_ch =  BIT(2) | BIT(1),
> +	.dpll_mask =  BIT(2) | BIT(1),
>  };
> 
>  static const struct rcar_du_device_info rcar_du_r8a7796_info = {
> @@ -243,7 +243,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7796_info = { },
>  	},
>  	.num_lvds = 1,
> -	.dpll_ch =  BIT(1),
> +	.dpll_mask =  BIT(1),
>  };
> 
>  static const struct rcar_du_device_info rcar_du_r8a77965_info = {
> @@ -271,7 +271,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a77965_info = { },
>  	},
>  	.num_lvds = 1,
> -	.dpll_ch =  BIT(1),
> +	.dpll_mask =  BIT(1),
>  };
> 
>  static const struct rcar_du_device_info rcar_du_r8a77970_info = {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..6453b33 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -55,6 +55,7 @@ struct rcar_du_output_routing {
>   * @channels_mask: bit mask of available DU channels
>   * @routes: array of CRTC to output routes, indexed by output
> (RCAR_DU_OUTPUT_*) * @num_lvds: number of internal LVDS encoders
> + * @dpll_mask: mask of DU channels equipped with a DPLL

I'd way "bit mask" instead of "mask" in the description. Apart from that,

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

and taken in my tree.

>   */
>  struct rcar_du_device_info {
>  	unsigned int gen;
> @@ -63,7 +64,7 @@ struct rcar_du_device_info {
>  	unsigned int channels_mask;
>  	struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX];
>  	unsigned int num_lvds;
> -	unsigned int dpll_ch;
> +	unsigned int dpll_mask;
>  };
> 
>  #define RCAR_DU_MAX_CRTCS		4

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] drm: rcar-du: Write ESCR register per channel
  2018-08-22  7:21   ` Jacopo Mondi
@ 2018-08-22 12:17     ` Laurent Pinchart
  -1 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2018-08-22 12:17 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: open list:DRM DRIVERS FOR RENESAS, open list:DRM DRIVERS FOR RENESAS

Hi Jacopo,

Thank you for the patch.

On Wednesday, 22 August 2018 10:21:48 EEST Jacopo Mondi wrote:
> The ESCR registers offset definition is confusing, as each channel is
> equipped with an ESCR register instance, but the names suggest only ESCR and
> ESCR2 are taken into account.
> 
> Rename the offsets to a name that includes the channels they apply to, and
> write them to each channel with 'rcar_du_crtc_write()'.
> 
> Cosmetic patch, no functional changes intended.

I think patches 2/3 and 3/3 can be squashed together, there's no real reason 
to keep them separate. I propose updating the commit message to

"drm: rcar-du: Write ESCR and OTAR as CRTC registers 
  
The ESCR and OTAR registers exist in each DU channel, but at different 
offsets for odd and even channels. This led to usage of the group 
register access API to write them, with offsets macros named ESCR/OTAR 
and ESCR2/OTAR2 for the first and second ESCR/OTAR register in the group 
respectively.

The names are confusing as it suggests that the ESCR/OTAR registers for 
DU0 and DU2 are taken into account, especially with writes performed to
the group register access API.

Rename the offsets to ESCR/OTAR02 and ESCR/OTAR13, and use the CRTC 
register access API to clarify the code. The offsets values are updated 
accordingly.

Cosmetic patch, no functional changes intended."

Otherwise the patches look good to me, so

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

and applied the squashed version to my tree.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +--
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
>  2 files changed, 3 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 5454884..714c1fc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc) }
>  	}
> 
> -	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> -			    escr);
> +	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
> 
>  	/* Signal polarities */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index 9dfd220..ebc4aea 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -492,8 +492,8 @@
>   * External Synchronization Control Registers
>   */
> 
> -#define ESCR			0x10000
> -#define ESCR2			0x31000
> +#define ESCR02			0x10000
> +#define ESCR13			0x01000
>  #define ESCR_DCLKOINV		(1 << 25)
>  #define ESCR_DCLKSEL_DCLKIN	(0 << 20)
>  #define ESCR_DCLKSEL_CLKS	(1 << 20)

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] drm: rcar-du: Write ESCR register per channel
@ 2018-08-22 12:17     ` Laurent Pinchart
  0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2018-08-22 12:17 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: open list:DRM DRIVERS FOR RENESAS, open list:DRM DRIVERS FOR RENESAS

Hi Jacopo,

Thank you for the patch.

On Wednesday, 22 August 2018 10:21:48 EEST Jacopo Mondi wrote:
> The ESCR registers offset definition is confusing, as each channel is
> equipped with an ESCR register instance, but the names suggest only ESCR and
> ESCR2 are taken into account.
> 
> Rename the offsets to a name that includes the channels they apply to, and
> write them to each channel with 'rcar_du_crtc_write()'.
> 
> Cosmetic patch, no functional changes intended.

I think patches 2/3 and 3/3 can be squashed together, there's no real reason 
to keep them separate. I propose updating the commit message to

"drm: rcar-du: Write ESCR and OTAR as CRTC registers 
  
The ESCR and OTAR registers exist in each DU channel, but at different 
offsets for odd and even channels. This led to usage of the group 
register access API to write them, with offsets macros named ESCR/OTAR 
and ESCR2/OTAR2 for the first and second ESCR/OTAR register in the group 
respectively.

The names are confusing as it suggests that the ESCR/OTAR registers for 
DU0 and DU2 are taken into account, especially with writes performed to
the group register access API.

Rename the offsets to ESCR/OTAR02 and ESCR/OTAR13, and use the CRTC 
register access API to clarify the code. The offsets values are updated 
accordingly.

Cosmetic patch, no functional changes intended."

Otherwise the patches look good to me, so

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

and applied the squashed version to my tree.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +--
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
>  2 files changed, 3 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 5454884..714c1fc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc) }
>  	}
> 
> -	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> -			    escr);
> +	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
> 
>  	/* Signal polarities */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index 9dfd220..ebc4aea 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -492,8 +492,8 @@
>   * External Synchronization Control Registers
>   */
> 
> -#define ESCR			0x10000
> -#define ESCR2			0x31000
> +#define ESCR02			0x10000
> +#define ESCR13			0x01000
>  #define ESCR_DCLKOINV		(1 << 25)
>  #define ESCR_DCLKSEL_DCLKIN	(0 << 20)
>  #define ESCR_DCLKSEL_CLKS	(1 << 20)

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] drm: rcar-du: Write ESCR register per channel
  2018-08-22  7:21   ` Jacopo Mondi
@ 2018-08-30 16:00     ` Kieran Bingham
  -1 siblings, 0 replies; 16+ messages in thread
From: Kieran Bingham @ 2018-08-30 16:00 UTC (permalink / raw)
  To: Jacopo Mondi, Laurent Pinchart
  Cc: open list:DRM DRIVERS FOR RENESAS, open list:DRM DRIVERS FOR RENESAS

Hi Jacopo,

On 22/08/18 08:21, Jacopo Mondi wrote:
> The ESCR registers offset definition is confusing, as each channel is
> equipped with an ESCR register instance, but the names suggest only ESCR and
> ESCR2 are taken into account.
> 
> Rename the offsets to a name that includes the channels they apply to, and
> write them to each channel with 'rcar_du_crtc_write()'.
> 
> Cosmetic patch, no functional changes intended.

Noted, ...

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +--
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
>  2 files changed, 3 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 5454884..714c1fc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  		}
>  	}
>  
> -	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> -			    escr);
> +	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
>  
>  	/* Signal polarities */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> index 9dfd220..ebc4aea 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -492,8 +492,8 @@
>   * External Synchronization Control Registers
>   */
>  
> -#define ESCR			0x10000
> -#define ESCR2			0x31000
> +#define ESCR02			0x10000
> +#define ESCR13			0x01000

Assertion failed at:
 ASSERT(ESCR2 == ESCR13)

This looks intentional ?
but that makes this a bit more than a cosmetic change?




>  #define ESCR_DCLKOINV		(1 << 25)
>  #define ESCR_DCLKSEL_DCLKIN	(0 << 20)
>  #define ESCR_DCLKSEL_CLKS	(1 << 20)
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] drm: rcar-du: Write ESCR register per channel
@ 2018-08-30 16:00     ` Kieran Bingham
  0 siblings, 0 replies; 16+ messages in thread
From: Kieran Bingham @ 2018-08-30 16:00 UTC (permalink / raw)
  To: Jacopo Mondi, Laurent Pinchart
  Cc: open list:DRM DRIVERS FOR RENESAS, open list:DRM DRIVERS FOR RENESAS

Hi Jacopo,

On 22/08/18 08:21, Jacopo Mondi wrote:
> The ESCR registers offset definition is confusing, as each channel is
> equipped with an ESCR register instance, but the names suggest only ESCR and
> ESCR2 are taken into account.
> 
> Rename the offsets to a name that includes the channels they apply to, and
> write them to each channel with 'rcar_du_crtc_write()'.
> 
> Cosmetic patch, no functional changes intended.

Noted, ...

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +--
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
>  2 files changed, 3 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 5454884..714c1fc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  		}
>  	}
>  
> -	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> -			    escr);
> +	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
>  
>  	/* Signal polarities */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> index 9dfd220..ebc4aea 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -492,8 +492,8 @@
>   * External Synchronization Control Registers
>   */
>  
> -#define ESCR			0x10000
> -#define ESCR2			0x31000
> +#define ESCR02			0x10000
> +#define ESCR13			0x01000

Assertion failed at:
 ASSERT(ESCR2 == ESCR13)

This looks intentional ?
but that makes this a bit more than a cosmetic change?




>  #define ESCR_DCLKOINV		(1 << 25)
>  #define ESCR_DCLKSEL_DCLKIN	(0 << 20)
>  #define ESCR_DCLKSEL_CLKS	(1 << 20)
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] drm: rcar-du: Write ESCR register per channel
  2018-08-30 16:00     ` Kieran Bingham
@ 2018-08-30 16:12       ` Kieran Bingham
  -1 siblings, 0 replies; 16+ messages in thread
From: Kieran Bingham @ 2018-08-30 16:12 UTC (permalink / raw)
  To: Jacopo Mondi, Laurent Pinchart
  Cc: open list:DRM DRIVERS FOR RENESAS, open list:DRM DRIVERS FOR RENESAS

Hi Jacopo,

On 30/08/18 17:00, Kieran Bingham wrote:
> Hi Jacopo,
> 
> On 22/08/18 08:21, Jacopo Mondi wrote:
>> The ESCR registers offset definition is confusing, as each channel is
>> equipped with an ESCR register instance, but the names suggest only ESCR and
>> ESCR2 are taken into account.
>>
>> Rename the offsets to a name that includes the channels they apply to, and
>> write them to each channel with 'rcar_du_crtc_write()'.
>>
>> Cosmetic patch, no functional changes intended.
> 
> Noted, ...
> 
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +--
>>  drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
>>  2 files changed, 3 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 5454884..714c1fc 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>>  		}
>>  	}
>>  
>> -	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
>> -			    escr);
>> +	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);

>>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
>>  
>>  	/* Signal polarities */
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>> index 9dfd220..ebc4aea 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>> @@ -492,8 +492,8 @@
>>   * External Synchronization Control Registers
>>   */
>>  
>> -#define ESCR			0x10000
>> -#define ESCR2			0x31000
>> +#define ESCR02			0x10000
>> +#define ESCR13			0x01000
> 
> Assertion failed at:
>  ASSERT(ESCR2 == ESCR13)
> 
> This looks intentional ?
> but that makes this a bit more than a cosmetic change?

Aha - sorry - I see.

It looks like the '0x3' from '0x31000' was providing the offset into the
group I assume.


I'm surprised an equivalent offset wasn't removed from the ESCR02.

But I assume this is handled by the change of "rcar_du_crtc_write() ->
rcar_du_group_write()"?

Regards

Kieran



> 
> 
> 
> 
>>  #define ESCR_DCLKOINV		(1 << 25)
>>  #define ESCR_DCLKSEL_DCLKIN	(0 << 20)
>>  #define ESCR_DCLKSEL_CLKS	(1 << 20)
>>
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] drm: rcar-du: Write ESCR register per channel
@ 2018-08-30 16:12       ` Kieran Bingham
  0 siblings, 0 replies; 16+ messages in thread
From: Kieran Bingham @ 2018-08-30 16:12 UTC (permalink / raw)
  To: Jacopo Mondi, Laurent Pinchart
  Cc: open list:DRM DRIVERS FOR RENESAS, open list:DRM DRIVERS FOR RENESAS

Hi Jacopo,

On 30/08/18 17:00, Kieran Bingham wrote:
> Hi Jacopo,
> 
> On 22/08/18 08:21, Jacopo Mondi wrote:
>> The ESCR registers offset definition is confusing, as each channel is
>> equipped with an ESCR register instance, but the names suggest only ESCR and
>> ESCR2 are taken into account.
>>
>> Rename the offsets to a name that includes the channels they apply to, and
>> write them to each channel with 'rcar_du_crtc_write()'.
>>
>> Cosmetic patch, no functional changes intended.
> 
> Noted, ...
> 
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +--
>>  drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++--
>>  2 files changed, 3 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 5454884..714c1fc 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>>  		}
>>  	}
>>  
>> -	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
>> -			    escr);
>> +	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);

>>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
>>  
>>  	/* Signal polarities */
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>> index 9dfd220..ebc4aea 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>> @@ -492,8 +492,8 @@
>>   * External Synchronization Control Registers
>>   */
>>  
>> -#define ESCR			0x10000
>> -#define ESCR2			0x31000
>> +#define ESCR02			0x10000
>> +#define ESCR13			0x01000
> 
> Assertion failed at:
>  ASSERT(ESCR2 == ESCR13)
> 
> This looks intentional ?
> but that makes this a bit more than a cosmetic change?

Aha - sorry - I see.

It looks like the '0x3' from '0x31000' was providing the offset into the
group I assume.


I'm surprised an equivalent offset wasn't removed from the ESCR02.

But I assume this is handled by the change of "rcar_du_crtc_write() ->
rcar_du_group_write()"?

Regards

Kieran



> 
> 
> 
> 
>>  #define ESCR_DCLKOINV		(1 << 25)
>>  #define ESCR_DCLKSEL_DCLKIN	(0 << 20)
>>  #define ESCR_DCLKSEL_CLKS	(1 << 20)
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-08-30 20:15 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-22  7:21 [PATCH 0/3] drm: rcar-du: A few cosmetic changes Jacopo Mondi
2018-08-22  7:21 ` Jacopo Mondi
2018-08-22  7:21 ` [PATCH 1/3] drm: rcar-du: Rename and document dpll_ch field Jacopo Mondi
2018-08-22  7:21   ` Jacopo Mondi
2018-08-22  8:17   ` Laurent Pinchart
2018-08-22  8:17     ` Laurent Pinchart
2018-08-22  7:21 ` [PATCH 2/3] drm: rcar-du: Write ESCR register per channel Jacopo Mondi
2018-08-22  7:21   ` Jacopo Mondi
2018-08-22 12:17   ` Laurent Pinchart
2018-08-22 12:17     ` Laurent Pinchart
2018-08-30 16:00   ` Kieran Bingham
2018-08-30 16:00     ` Kieran Bingham
2018-08-30 16:12     ` Kieran Bingham
2018-08-30 16:12       ` Kieran Bingham
2018-08-22  7:21 ` [PATCH 3/3] drm: rcar-du: Write OTAR " Jacopo Mondi
2018-08-22  7:21   ` Jacopo Mondi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.