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

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Jan 20, 2023 at 10:50:08AM +0200, Tomi Valkeinen wrote:
> On H3 ES1.x 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.x, only the higher bits are used (bits 21 and 23), and the lower
> bits are reserved and should be set to 0.
> 
> The current code always sets the lower bits, even on non-ES1.x.
> 
> 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.x and non-ES1.x.
> 
> 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.x and non-ES1.x.
> 
> However, for DU2, writing 0b10 to the bits results in using PLL0 as the
> input clock on ES1.x, whereas on non-ES1.x it results in using PLL1. On
> ES1.x 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.x and non-ES1.x.
> 
> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this. This way 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.x.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas at ideasonboard.com>

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

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 23 ++++++++++++++++++++---
>  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 |  8 ++------
>  4 files changed, 25 insertions(+), 10 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..b7dd59fe119e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -245,13 +245,30 @@ 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
> -			dpllcr |= DPLLCR_PLCS0
> +		} else {
> +			dpllcr |= DPLLCR_PLCS0_PLL
>  			       |  DPLLCR_INCS_DOTCLKIN0;
>  
> +			/*
> +			 * 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.
> +			 */
> +
> +			if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PLL)
> +				dpllcr |= DPLLCR_PLCS0_H3ES1X_PLL1;
> +		}
> +
>  		rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr);
>  
>  		escr = ESCR_DCLKSEL_DCLKIN | div;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index ea3a8cff74b7..82f9718ff500 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..789ae9285108 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -283,12 +283,8 @@
>  #define DPLLCR			0x20044
>  #define DPLLCR_CODE		(0x95 << 24)
>  #define DPLLCR_PLCS1		(1 << 23)
> -/*
> - * PLCS0 is bit 21, but H3 ES1.x requires bit 20 to be set as well. As bit 20
> - * isn't implemented by other SoC in the Gen3 family it can safely be set
> - * unconditionally.
> - */
> -#define DPLLCR_PLCS0		(3 << 20)
> +#define DPLLCR_PLCS0_PLL	(1 << 21)
> +#define DPLLCR_PLCS0_H3ES1X_PLL1	(1 << 20)
>  #define DPLLCR_CLKE		(1 << 18)
>  #define DPLLCR_FDPLL(n)		((n) << 12)
>  #define DPLLCR_N(n)		((n) << 5)

-- 
Regards,

Laurent Pinchart