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+rene...@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+rene...@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
