diff mbox series

[v2,09/16] drm: rcar-du: Cache DSYSR value to ensure known initial value

Message ID 20180914091046.483-10-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State Not Applicable
Delegated to: Geert Uytterhoeven
Headers show
Series R-Car D3/E3 display support (with LVDS PLL) | expand

Commit Message

Laurent Pinchart Sept. 14, 2018, 9:10 a.m. UTC 
DSYSR is a DU channel register that also contains group fields. It is
thus written to by both the group and CRTC code, using read-update-write
sequences. As the register isn't initialized explicitly at startup time,
this can lead to invalid or otherwise unexpected values being written to
some of the fields if they have been modified by the firmware or just
not reset properly.

To fix this we can write a fully known value to the DSYSR register when
turning a channel's functional clock on. However, the mix of group and
channel fields complicate this. A simpler solution is to cache the
register and initialize the cached value to the desired hardware
defaults.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 16 ++++++++--------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  5 +++++
 drivers/gpu/drm/rcar-du/rcar_du_group.c |  7 ++++---
 3 files changed, 17 insertions(+), 11 deletions(-)

Comments

Kieran Bingham Sept. 24, 2018, 11:18 a.m. UTC | #1 
Hi Laurent,

On 14/09/18 10:10, Laurent Pinchart wrote:
> DSYSR is a DU channel register that also contains group fields. It is
> thus written to by both the group and CRTC code, using read-update-write
> sequences. As the register isn't initialized explicitly at startup time,
> this can lead to invalid or otherwise unexpected values being written to
> some of the fields if they have been modified by the firmware or just
> not reset properly.
> 
> To fix this we can write a fully known value to the DSYSR register when
> turning a channel's functional clock on. However, the mix of group and
> channel fields complicate this. A simpler solution is to cache the
> register and initialize the cached value to the desired hardware
> defaults.

Great, this looks good to me, and is less overhead than pulling in a
full reg-cache when we only need a single register handled.

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 16 ++++++++--------
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  5 +++++
>  drivers/gpu/drm/rcar-du/rcar_du_group.c |  7 ++++---
>  3 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 2f8776c1ec8f..f827fccf6416 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -57,13 +57,12 @@ static void rcar_du_crtc_set(struct rcar_du_crtc *rcrtc, u32 reg, u32 set)
>  		      rcar_du_read(rcdu, rcrtc->mmio_offset + reg) | set);
>  }
>  
> -static void rcar_du_crtc_clr_set(struct rcar_du_crtc *rcrtc, u32 reg,
> -				 u32 clr, u32 set)
> +void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set)
>  {
>  	struct rcar_du_device *rcdu = rcrtc->group->dev;
> -	u32 value = rcar_du_read(rcdu, rcrtc->mmio_offset + reg);
>  
> -	rcar_du_write(rcdu, rcrtc->mmio_offset + reg, (value & ~clr) | set);
> +	rcrtc->dsysr = (rcrtc->dsysr & ~clr) | set;
> +	rcar_du_write(rcdu, rcrtc->mmio_offset + DSYSR, rcrtc->dsysr);
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -576,9 +575,9 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>  	 * actively driven).
>  	 */
>  	interlaced = rcrtc->crtc.mode.flags & DRM_MODE_FLAG_INTERLACE;
> -	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK | DSYSR_SCM_MASK,
> -			     (interlaced ? DSYSR_SCM_INT_VIDEO : 0) |
> -			     DSYSR_TVM_MASTER);
> +	rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_TVM_MASK | DSYSR_SCM_MASK,
> +				   (interlaced ? DSYSR_SCM_INT_VIDEO : 0) |
> +				   DSYSR_TVM_MASTER);
>  
>  	rcar_du_group_start_stop(rcrtc->group, true);
>  }
> @@ -645,7 +644,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>  	 * Select switch sync mode. This stops display operation and configures
>  	 * the HSYNC and VSYNC signals as inputs.
>  	 */
> -	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
> +	rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
>  
>  	rcar_du_group_start_stop(rcrtc->group, false);
>  }
> @@ -1121,6 +1120,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>  	rcrtc->group = rgrp;
>  	rcrtc->mmio_offset = mmio_offsets[hwindex];
>  	rcrtc->index = hwindex;
> +	rcrtc->dsysr = (rcrtc->index % 2 ? 0 : DSYSR_DRES) | DSYSR_TVM_TVSYNC;
>  
>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 4990bbe9ba26..59ac6e7d22c9 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -30,6 +30,7 @@ struct rcar_du_vsp;
>   * @mmio_offset: offset of the CRTC registers in the DU MMIO block
>   * @index: CRTC software and hardware index
>   * @initialized: whether the CRTC has been initialized and clocks enabled
> + * @dsysr: cached value of the DSYSR register
>   * @vblank_enable: whether vblank events are enabled on this CRTC
>   * @event: event to post when the pending page flip completes
>   * @flip_wait: wait queue used to signal page flip completion
> @@ -50,6 +51,8 @@ struct rcar_du_crtc {
>  	unsigned int index;
>  	bool initialized;
>  
> +	u32 dsysr;
> +
>  	bool vblank_enable;
>  	struct drm_pending_vblank_event *event;
>  	wait_queue_head_t flip_wait;
> @@ -103,4 +106,6 @@ void rcar_du_crtc_route_output(struct drm_crtc *crtc,
>  			       enum rcar_du_output output);
>  void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
>  
> +void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set);
> +
>  #endif /* __RCAR_DU_CRTC_H__ */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index f38703e7a10d..d85f0a1c1581 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -202,9 +202,10 @@ void rcar_du_group_put(struct rcar_du_group *rgrp)
>  
>  static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
>  {
> -	rcar_du_group_write(rgrp, DSYSR,
> -		(rcar_du_group_read(rgrp, DSYSR) & ~(DSYSR_DRES | DSYSR_DEN)) |
> -		(start ? DSYSR_DEN : DSYSR_DRES));
> +	struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
> +
> +	rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
> +				   start ? DSYSR_DEN : DSYSR_DRES);
>  }
>  
>  void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
>
Ulrich Hecht Sept. 26, 2018, 3:55 p.m. UTC | #2 
> On September 14, 2018 at 11:10 AM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> DSYSR is a DU channel register that also contains group fields. It is
> thus written to by both the group and CRTC code, using read-update-write
> sequences. As the register isn't initialized explicitly at startup time,
> this can lead to invalid or otherwise unexpected values being written to
> some of the fields if they have been modified by the firmware or just
> not reset properly.
> 
> To fix this we can write a fully known value to the DSYSR register when
> turning a channel's functional clock on. However, the mix of group and
> channel fields complicate this. A simpler solution is to cache the
> register and initialize the cached value to the desired hardware
> defaults.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 16 ++++++++--------
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  5 +++++
>  drivers/gpu/drm/rcar-du/rcar_du_group.c |  7 ++++---
>  3 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 2f8776c1ec8f..f827fccf6416 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -57,13 +57,12 @@ static void rcar_du_crtc_set(struct rcar_du_crtc *rcrtc, u32 reg, u32 set)
>  		      rcar_du_read(rcdu, rcrtc->mmio_offset + reg) | set);
>  }
>  
> -static void rcar_du_crtc_clr_set(struct rcar_du_crtc *rcrtc, u32 reg,
> -				 u32 clr, u32 set)
> +void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set)
>  {
>  	struct rcar_du_device *rcdu = rcrtc->group->dev;
> -	u32 value = rcar_du_read(rcdu, rcrtc->mmio_offset + reg);
>  
> -	rcar_du_write(rcdu, rcrtc->mmio_offset + reg, (value & ~clr) | set);
> +	rcrtc->dsysr = (rcrtc->dsysr & ~clr) | set;
> +	rcar_du_write(rcdu, rcrtc->mmio_offset + DSYSR, rcrtc->dsysr);
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -576,9 +575,9 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>  	 * actively driven).
>  	 */
>  	interlaced = rcrtc->crtc.mode.flags & DRM_MODE_FLAG_INTERLACE;
> -	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK | DSYSR_SCM_MASK,
> -			     (interlaced ? DSYSR_SCM_INT_VIDEO : 0) |
> -			     DSYSR_TVM_MASTER);
> +	rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_TVM_MASK | DSYSR_SCM_MASK,
> +				   (interlaced ? DSYSR_SCM_INT_VIDEO : 0) |
> +				   DSYSR_TVM_MASTER);
>  
>  	rcar_du_group_start_stop(rcrtc->group, true);
>  }
> @@ -645,7 +644,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>  	 * Select switch sync mode. This stops display operation and configures
>  	 * the HSYNC and VSYNC signals as inputs.
>  	 */
> -	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
> +	rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
>  
>  	rcar_du_group_start_stop(rcrtc->group, false);
>  }
> @@ -1121,6 +1120,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>  	rcrtc->group = rgrp;
>  	rcrtc->mmio_offset = mmio_offsets[hwindex];
>  	rcrtc->index = hwindex;
> +	rcrtc->dsysr = (rcrtc->index % 2 ? 0 : DSYSR_DRES) | DSYSR_TVM_TVSYNC;
>  
>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 4990bbe9ba26..59ac6e7d22c9 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -30,6 +30,7 @@ struct rcar_du_vsp;
>   * @mmio_offset: offset of the CRTC registers in the DU MMIO block
>   * @index: CRTC software and hardware index
>   * @initialized: whether the CRTC has been initialized and clocks enabled
> + * @dsysr: cached value of the DSYSR register
>   * @vblank_enable: whether vblank events are enabled on this CRTC
>   * @event: event to post when the pending page flip completes
>   * @flip_wait: wait queue used to signal page flip completion
> @@ -50,6 +51,8 @@ struct rcar_du_crtc {
>  	unsigned int index;
>  	bool initialized;
>  
> +	u32 dsysr;
> +
>  	bool vblank_enable;
>  	struct drm_pending_vblank_event *event;
>  	wait_queue_head_t flip_wait;
> @@ -103,4 +106,6 @@ void rcar_du_crtc_route_output(struct drm_crtc *crtc,
>  			       enum rcar_du_output output);
>  void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
>  
> +void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set);
> +
>  #endif /* __RCAR_DU_CRTC_H__ */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index f38703e7a10d..d85f0a1c1581 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -202,9 +202,10 @@ void rcar_du_group_put(struct rcar_du_group *rgrp)
>  
>  static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
>  {
> -	rcar_du_group_write(rgrp, DSYSR,
> -		(rcar_du_group_read(rgrp, DSYSR) & ~(DSYSR_DRES | DSYSR_DEN)) |
> -		(start ? DSYSR_DEN : DSYSR_DRES));
> +	struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
> +
> +	rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
> +				   start ? DSYSR_DEN : DSYSR_DRES);
>  }
>  
>  void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
> -- 

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 2f8776c1ec8f..f827fccf6416 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -57,13 +57,12 @@  static void rcar_du_crtc_set(struct rcar_du_crtc *rcrtc, u32 reg, u32 set)
 		      rcar_du_read(rcdu, rcrtc->mmio_offset + reg) | set);
 }
 
-static void rcar_du_crtc_clr_set(struct rcar_du_crtc *rcrtc, u32 reg,
-				 u32 clr, u32 set)
+void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set)
 {
 	struct rcar_du_device *rcdu = rcrtc->group->dev;
-	u32 value = rcar_du_read(rcdu, rcrtc->mmio_offset + reg);
 
-	rcar_du_write(rcdu, rcrtc->mmio_offset + reg, (value & ~clr) | set);
+	rcrtc->dsysr = (rcrtc->dsysr & ~clr) | set;
+	rcar_du_write(rcdu, rcrtc->mmio_offset + DSYSR, rcrtc->dsysr);
 }
 
 /* -----------------------------------------------------------------------------
@@ -576,9 +575,9 @@  static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
 	 * actively driven).
 	 */
 	interlaced = rcrtc->crtc.mode.flags & DRM_MODE_FLAG_INTERLACE;
-	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK | DSYSR_SCM_MASK,
-			     (interlaced ? DSYSR_SCM_INT_VIDEO : 0) |
-			     DSYSR_TVM_MASTER);
+	rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_TVM_MASK | DSYSR_SCM_MASK,
+				   (interlaced ? DSYSR_SCM_INT_VIDEO : 0) |
+				   DSYSR_TVM_MASTER);
 
 	rcar_du_group_start_stop(rcrtc->group, true);
 }
@@ -645,7 +644,7 @@  static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 	 * Select switch sync mode. This stops display operation and configures
 	 * the HSYNC and VSYNC signals as inputs.
 	 */
-	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
+	rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
 
 	rcar_du_group_start_stop(rcrtc->group, false);
 }
@@ -1121,6 +1120,7 @@  int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
 	rcrtc->group = rgrp;
 	rcrtc->mmio_offset = mmio_offsets[hwindex];
 	rcrtc->index = hwindex;
+	rcrtc->dsysr = (rcrtc->index % 2 ? 0 : DSYSR_DRES) | DSYSR_TVM_TVSYNC;
 
 	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
 		primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 4990bbe9ba26..59ac6e7d22c9 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -30,6 +30,7 @@  struct rcar_du_vsp;
  * @mmio_offset: offset of the CRTC registers in the DU MMIO block
  * @index: CRTC software and hardware index
  * @initialized: whether the CRTC has been initialized and clocks enabled
+ * @dsysr: cached value of the DSYSR register
  * @vblank_enable: whether vblank events are enabled on this CRTC
  * @event: event to post when the pending page flip completes
  * @flip_wait: wait queue used to signal page flip completion
@@ -50,6 +51,8 @@  struct rcar_du_crtc {
 	unsigned int index;
 	bool initialized;
 
+	u32 dsysr;
+
 	bool vblank_enable;
 	struct drm_pending_vblank_event *event;
 	wait_queue_head_t flip_wait;
@@ -103,4 +106,6 @@  void rcar_du_crtc_route_output(struct drm_crtc *crtc,
 			       enum rcar_du_output output);
 void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
 
+void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set);
+
 #endif /* __RCAR_DU_CRTC_H__ */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index f38703e7a10d..d85f0a1c1581 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -202,9 +202,10 @@  void rcar_du_group_put(struct rcar_du_group *rgrp)
 
 static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
 {
-	rcar_du_group_write(rgrp, DSYSR,
-		(rcar_du_group_read(rgrp, DSYSR) & ~(DSYSR_DRES | DSYSR_DEN)) |
-		(start ? DSYSR_DEN : DSYSR_DRES));
+	struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
+
+	rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
+				   start ? DSYSR_DEN : DSYSR_DRES);
 }
 
 void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)