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

[Kieran Bingham]

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 at ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo+renesas at jmondi.org>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas at 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)
> 

-- 
Regards
--
Kieran